There are several places in kernel that use functionality like shell's basename
function. Let's do it common helper for them.
Signed-off-by: Andy Shevchenko <[email protected]>
Cc: YAMANE Toshiaki <[email protected]>
---
include/linux/string.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index b917881..b09a342 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -147,5 +147,16 @@ static inline bool strstarts(const char *str, const char *prefix)
extern size_t memweight(const void *ptr, size_t bytes);
+/**
+ * kbasename - return the last part of a pathname.
+ *
+ * @path: path to extract the filename from.
+ */
+static inline const char *kbasename(const char *path)
+{
+ const char *tail = strrchr(path, '/');
+ return tail ? tail + 1 : path;
+}
+
#endif /* __KERNEL__ */
#endif /* _LINUX_STRING_H_ */
--
1.7.10.4
Remove the custom implementation of the functionality similar to kbasename().
Signed-off-by: Andy Shevchenko <[email protected]>
Cc: Jason Baron <[email protected]>
---
lib/dynamic_debug.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index e7f7d99..1db1fc6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -62,13 +62,6 @@ static LIST_HEAD(ddebug_tables);
static int verbose = 0;
module_param(verbose, int, 0644);
-/* Return the last part of a pathname */
-static inline const char *basename(const char *path)
-{
- const char *tail = strrchr(path, '/');
- return tail ? tail+1 : path;
-}
-
/* Return the path relative to source root */
static inline const char *trim_prefix(const char *path)
{
@@ -154,7 +147,7 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the source filename */
if (query->filename &&
strcmp(query->filename, dp->filename) &&
- strcmp(query->filename, basename(dp->filename)) &&
+ strcmp(query->filename, kbasename(dp->filename)) &&
strcmp(query->filename, trim_prefix(dp->filename)))
continue;
--
1.7.10.4
The custom filename function mostly repeats the kernel's kbasename. This patch
simplifies it. The updated filename() will not check for the '\' in the
filenames. It seems redundant in Linux.
Signed-off-by: Andy Shevchenko <[email protected]>
Cc: YAMANE Toshiaki <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/staging/rts_pstor/trace.h | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/rts_pstor/trace.h b/drivers/staging/rts_pstor/trace.h
index cf60a1b..59c5686 100644
--- a/drivers/staging/rts_pstor/trace.h
+++ b/drivers/staging/rts_pstor/trace.h
@@ -24,26 +24,16 @@
#ifndef __REALTEK_RTSX_TRACE_H
#define __REALTEK_RTSX_TRACE_H
+#include <linux/string.h>
+
#define _MSG_TRACE
#ifdef _MSG_TRACE
static inline char *filename(char *path)
{
- char *ptr;
-
if (path == NULL)
return NULL;
-
- ptr = path;
-
- while (*ptr != '\0') {
- if ((*ptr == '\\') || (*ptr == '/'))
- path = ptr + 1;
-
- ptr++;
- }
-
- return path;
+ return kbasename(path);
}
#define TRACE_RET(chip, ret) \
--
1.7.10.4
Signed-off-by: Andy Shevchenko <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
drivers/usb/core/file.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index e5387a4..eff9af9 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -19,6 +19,7 @@
#include <linux/errno.h>
#include <linux/rwsem.h>
#include <linux/slab.h>
+#include <linux/string.h>
#include <linux/usb.h>
#include "usb.h"
@@ -163,7 +164,6 @@ int usb_register_dev(struct usb_interface *intf,
int minor_base = class_driver->minor_base;
int minor;
char name[20];
- char *temp;
#ifdef CONFIG_USB_DYNAMIC_MINORS
/*
@@ -200,14 +200,9 @@ int usb_register_dev(struct usb_interface *intf,
/* create a usb class device for this usb interface */
snprintf(name, sizeof(name), class_driver->name, minor - minor_base);
- temp = strrchr(name, '/');
- if (temp && (temp[1] != '\0'))
- ++temp;
- else
- temp = name;
intf->usb_dev = device_create(usb_class->class, &intf->dev,
MKDEV(USB_MAJOR, minor), class_driver,
- "%s", temp);
+ "%s", kbasename(name));
if (IS_ERR(intf->usb_dev)) {
down_write(&minor_rwsem);
usb_minors[minor] = NULL;
--
1.7.10.4
Signed-off-by: Andy Shevchenko <[email protected]>
Cc: Steven Rostedt <[email protected]> (maintainer:TRACING)
Cc: Frederic Weisbecker <[email protected]> (maintainer:TRACING)
---
kernel/trace/trace_uprobe.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 03003cd..a2b2fab 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -22,6 +22,7 @@
#include <linux/uaccess.h>
#include <linux/uprobes.h>
#include <linux/namei.h>
+#include <linux/string.h>
#include "trace_probe.h"
@@ -263,16 +264,15 @@ static int create_trace_uprobe(int argc, char **argv)
/* setup a probe */
if (!event) {
- char *tail = strrchr(filename, '/');
+ char *tail;
char *ptr;
- ptr = kstrdup((tail ? tail + 1 : filename), GFP_KERNEL);
+ tail = ptr = kstrdup(kbasename(filename), GFP_KERNEL);
if (!ptr) {
ret = -ENOMEM;
goto fail_address_parse;
}
- tail = ptr;
ptr = strpbrk(tail, ".-_");
if (ptr)
*ptr = '\0';
--
1.7.10.4
Signed-off-by: Andy Shevchenko <[email protected]>
---
fs/proc/proc_devtree.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index df7dd08..3d9fd66 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -13,6 +13,7 @@
#include <linux/of.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/string.h>
#include <asm/prom.h>
#include <asm/uaccess.h>
#include "internal.h"
@@ -195,11 +196,7 @@ void proc_device_tree_add_node(struct device_node *np,
set_node_proc_entry(np, de);
for (child = NULL; (child = of_get_next_child(np, child));) {
/* Use everything after the last slash, or the full name */
- p = strrchr(child->full_name, '/');
- if (!p)
- p = child->full_name;
- else
- ++p;
+ p = kbasename(child->full_name);
if (duplicate_name(de, p))
p = fixup_name(np, de, p);
--
1.7.10.4
Signed-off-by: Andy Shevchenko <[email protected]>
Cc: [email protected]
---
mm/memory.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index 0e3a516..6b101a2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -58,6 +58,7 @@
#include <linux/elf.h>
#include <linux/gfp.h>
#include <linux/migrate.h>
+#include <linux/string.h>
#include <asm/io.h>
#include <asm/pgalloc.h>
@@ -4034,15 +4035,12 @@ void print_vma_addr(char *prefix, unsigned long ip)
struct file *f = vma->vm_file;
char *buf = (char *)__get_free_page(GFP_KERNEL);
if (buf) {
- char *p, *s;
+ char *p;
p = d_path(&f->f_path, buf, PAGE_SIZE);
if (IS_ERR(p))
p = "?";
- s = strrchr(p, '/');
- if (s)
- p = s+1;
- printk("%s%s[%lx+%lx]", prefix, p,
+ printk("%s%s[%lx+%lx]", prefix, kbasename(p),
vma->vm_start,
vma->vm_end - vma->vm_start);
free_page((unsigned long)buf);
--
1.7.10.4
On Tue, Oct 02, 2012 at 06:00:54PM +0300, Andy Shevchenko wrote:
> There are several places in kernel that use functionality like shell's basename
> function. Let's do it common helper for them.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Cc: YAMANE Toshiaki <[email protected]>
> ---
> include/linux/string.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b917881..b09a342 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -147,5 +147,16 @@ static inline bool strstarts(const char *str, const char *prefix)
>
> extern size_t memweight(const void *ptr, size_t bytes);
>
> +/**
> + * kbasename - return the last part of a pathname.
> + *
> + * @path: path to extract the filename from.
> + */
> +static inline const char *kbasename(const char *path)
> +{
> + const char *tail = strrchr(path, '/');
> + return tail ? tail + 1 : path;
What happens if '/' is the last thing in the string? You will then
point to an empty string, which I don't think all callers of this
function is assuming going to work properly (hint, the USB caller will
not...)
thanks,
greg k-h
On Tue, Oct 2, 2012 at 8:34 PM, Greg KH <[email protected]> wrote:
> On Tue, Oct 02, 2012 at 06:00:54PM +0300, Andy Shevchenko wrote:
>> There are several places in kernel that use functionality like shell's basename
>> function. Let's do it common helper for them.
>>
>> Signed-off-by: Andy Shevchenko <[email protected]>
>> Cc: YAMANE Toshiaki <[email protected]>
>> ---
>> include/linux/string.h | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index b917881..b09a342 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -147,5 +147,16 @@ static inline bool strstarts(const char *str, const char *prefix)
>>
>> extern size_t memweight(const void *ptr, size_t bytes);
>>
>> +/**
>> + * kbasename - return the last part of a pathname.
>> + *
>> + * @path: path to extract the filename from.
>> + */
>> +static inline const char *kbasename(const char *path)
>> +{
>> + const char *tail = strrchr(path, '/');
>> + return tail ? tail + 1 : path;
>
> What happens if '/' is the last thing in the string? You will then
> point to an empty string, which I don't think all callers of this
> function is assuming going to work properly (hint, the USB caller will
> not...)
Thanks for pointing to that. I think it's a usb specific case, so, I
assume your comment related to that patch.
--
With Best Regards,
Andy Shevchenko
On Tue, Oct 02, 2012 at 08:52:05PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 2, 2012 at 8:34 PM, Greg KH <[email protected]> wrote:
> > On Tue, Oct 02, 2012 at 06:00:54PM +0300, Andy Shevchenko wrote:
> >> There are several places in kernel that use functionality like shell's basename
> >> function. Let's do it common helper for them.
> >>
> >> Signed-off-by: Andy Shevchenko <[email protected]>
> >> Cc: YAMANE Toshiaki <[email protected]>
> >> ---
> >> include/linux/string.h | 11 +++++++++++
> >> 1 file changed, 11 insertions(+)
> >>
> >> diff --git a/include/linux/string.h b/include/linux/string.h
> >> index b917881..b09a342 100644
> >> --- a/include/linux/string.h
> >> +++ b/include/linux/string.h
> >> @@ -147,5 +147,16 @@ static inline bool strstarts(const char *str, const char *prefix)
> >>
> >> extern size_t memweight(const void *ptr, size_t bytes);
> >>
> >> +/**
> >> + * kbasename - return the last part of a pathname.
> >> + *
> >> + * @path: path to extract the filename from.
> >> + */
> >> +static inline const char *kbasename(const char *path)
> >> +{
> >> + const char *tail = strrchr(path, '/');
> >> + return tail ? tail + 1 : path;
> >
> > What happens if '/' is the last thing in the string? You will then
> > point to an empty string, which I don't think all callers of this
> > function is assuming going to work properly (hint, the USB caller will
> > not...)
> Thanks for pointing to that. I think it's a usb specific case, so, I
> assume your comment related to that patch.
Well, if you want your kbasename() function to work like the basename(3)
function, you need to properly handle a trailing '/' character.
And yes, the USB code does check for this. I don't remember why, odds
are it was needed for something.
greg k-h
On 03/10/12 01:00, Andy Shevchenko wrote:
> The custom filename function mostly repeats the kernel's kbasename. This patch
> simplifies it. The updated filename() will not check for the '\' in the
> filenames. It seems redundant in Linux.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Cc: YAMANE Toshiaki <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
> drivers/staging/rts_pstor/trace.h | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/rts_pstor/trace.h b/drivers/staging/rts_pstor/trace.h
> index cf60a1b..59c5686 100644
> --- a/drivers/staging/rts_pstor/trace.h
> +++ b/drivers/staging/rts_pstor/trace.h
> @@ -24,26 +24,16 @@
> #ifndef __REALTEK_RTSX_TRACE_H
> #define __REALTEK_RTSX_TRACE_H
>
> +#include <linux/string.h>
> +
> #define _MSG_TRACE
>
> #ifdef _MSG_TRACE
> static inline char *filename(char *path)
> {
> - char *ptr;
> -
> if (path == NULL)
> return NULL;
> -
> - ptr = path;
> -
> - while (*ptr != '\0') {
> - if ((*ptr == '\\') || (*ptr == '/'))
> - path = ptr + 1;
The original version here returns the string after the last '/' or '\',
the new kbasename function only looks for '/'. Does that matter here, or
was the original code over eager?
~Ryan
> -
> - ptr++;
> - }
> -
> - return path;
> + return kbasename(path);
> }
>
> #define TRACE_RET(chip, ret) \
>
On 03/10/12 10:30, Ryan Mallon wrote:
> On 03/10/12 01:00, Andy Shevchenko wrote:
>> The custom filename function mostly repeats the kernel's kbasename. This patch
>> simplifies it. The updated filename() will not check for the '\' in the
>> filenames. It seems redundant in Linux.
>>
>> Signed-off-by: Andy Shevchenko <[email protected]>
>> Cc: YAMANE Toshiaki <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> ---
>> drivers/staging/rts_pstor/trace.h | 16 +++-------------
>> 1 file changed, 3 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/staging/rts_pstor/trace.h b/drivers/staging/rts_pstor/trace.h
>> index cf60a1b..59c5686 100644
>> --- a/drivers/staging/rts_pstor/trace.h
>> +++ b/drivers/staging/rts_pstor/trace.h
>> @@ -24,26 +24,16 @@
>> #ifndef __REALTEK_RTSX_TRACE_H
>> #define __REALTEK_RTSX_TRACE_H
>>
>> +#include <linux/string.h>
>> +
>> #define _MSG_TRACE
>>
>> #ifdef _MSG_TRACE
>> static inline char *filename(char *path)
>> {
>> - char *ptr;
>> -
>> if (path == NULL)
>> return NULL;
>> -
>> - ptr = path;
>> -
>> - while (*ptr != '\0') {
>> - if ((*ptr == '\\') || (*ptr == '/'))
>> - path = ptr + 1;
>
> The original version here returns the string after the last '/' or '\',
> the new kbasename function only looks for '/'. Does that matter here, or
> was the original code over eager?
Nevermind, I didn't read the changelog fully :-/.
~Ryan
On Tue, Oct 2, 2012 at 6:00 PM, Andy Shevchenko
<[email protected]> wrote:
> --- a/drivers/usb/core/file.c
> +++ b/drivers/usb/core/file.c
> @@ -200,14 +200,9 @@ int usb_register_dev(struct usb_interface *intf,
>
> /* create a usb class device for this usb interface */
> snprintf(name, sizeof(name), class_driver->name, minor - minor_base);
> - temp = strrchr(name, '/');
> - if (temp && (temp[1] != '\0'))
I have checked current linux-next, the drivers define .name in the
usb_class_driver structure as '...%d'.
So, what is the reason to check for trailing '/' here? Historical
reasons or there is a (broken/3rd party/etc) driver with it?
> - ++temp;
> - else
> - temp = name;
> intf->usb_dev = device_create(usb_class->class, &intf->dev,
> MKDEV(USB_MAJOR, minor), class_driver,
> - "%s", temp);
> + "%s", kbasename(name));
--
With Best Regards,
Andy Shevchenko
On Wed, Oct 03, 2012 at 11:27:27AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 2, 2012 at 6:00 PM, Andy Shevchenko
> <[email protected]> wrote:
>
> > --- a/drivers/usb/core/file.c
> > +++ b/drivers/usb/core/file.c
>
> > @@ -200,14 +200,9 @@ int usb_register_dev(struct usb_interface *intf,
> >
> > /* create a usb class device for this usb interface */
> > snprintf(name, sizeof(name), class_driver->name, minor - minor_base);
> > - temp = strrchr(name, '/');
> > - if (temp && (temp[1] != '\0'))
> I have checked current linux-next, the drivers define .name in the
> usb_class_driver structure as '...%d'.
> So, what is the reason to check for trailing '/' here? Historical
> reasons or there is a (broken/3rd party/etc) driver with it?
I really do not remember why it was done this way, sorry. I have no
problem not doing it anymore, as long as you are willing to fix any
potential bugs that might pop up :)
And no, I don't worry about 3rd party drivers, that shouldn't be an
issue at all here.
greg k-h
On 2012-10-02 11:12 -0700, Greg KH wrote:
> On Tue, Oct 02, 2012 at 08:52:05PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 2, 2012 at 8:34 PM, Greg KH <[email protected]> wrote:
> > > On Tue, Oct 02, 2012 at 06:00:54PM +0300, Andy Shevchenko wrote:
[...]
> > >> +/**
> > >> + * kbasename - return the last part of a pathname.
> > >> + *
> > >> + * @path: path to extract the filename from.
> > >> + */
> > >> +static inline const char *kbasename(const char *path)
> > >> +{
> > >> + const char *tail = strrchr(path, '/');
> > >> + return tail ? tail + 1 : path;
> > >
> > > What happens if '/' is the last thing in the string? You will then
> > > point to an empty string, which I don't think all callers of this
> > > function is assuming going to work properly (hint, the USB caller will
> > > not...)
> > Thanks for pointing to that. I think it's a usb specific case, so, I
> > assume your comment related to that patch.
>
> Well, if you want your kbasename() function to work like the basename(3)
> function, you need to properly handle a trailing '/' character.
Specifically, POSIX basename trims trailing '/' characters, so
char foo[] = "a/string/with/trailing/slashes///";
basename(foo);
results in a string that compares equal to "slashes". This implies that
it must either modify the provided string or copy it somewhere else
(POSIX admits either behaviour).
On the other hand, GNU basename does not trim trailing '/' characters
and returns the empty string in this case. It's truly unfortunate that
glibc contains two different functions called basename, but regardless,
the behaviour of the function in this proposal is certainly not
unprecedented.
Cheers,
--
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
On Wed, 2012-10-03 at 14:39 -0400, Nick Bowler wrote:
> On 2012-10-02 11:12 -0700, Greg KH wrote:
> > On Tue, Oct 02, 2012 at 08:52:05PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 2, 2012 at 8:34 PM, Greg KH <[email protected]> wrote:
[...]
> > Well, if you want your kbasename() function to work like the basename(3)
> > function, you need to properly handle a trailing '/' character.
>
> Specifically, POSIX basename trims trailing '/' characters, so
>
> char foo[] = "a/string/with/trailing/slashes///";
> basename(foo);
>
> results in a string that compares equal to "slashes". This implies that
> it must either modify the provided string or copy it somewhere else
> (POSIX admits either behaviour).
>
> On the other hand, GNU basename does not trim trailing '/' characters
> and returns the empty string in this case. It's truly unfortunate that
> glibc contains two different functions called basename, but regardless,
> the behaviour of the function in this proposal is certainly not
> unprecedented.
I fixed the description of the patch in v2.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Wed, Oct 3, 2012 at 6:08 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Wed, Oct 03, 2012 at 11:27:27AM +0300, Andy Shevchenko wrote:
>> On Tue, Oct 2, 2012 at 6:00 PM, Andy Shevchenko
>> <[email protected]> wrote:
>>
>> > --- a/drivers/usb/core/file.c
>> > +++ b/drivers/usb/core/file.c
>>
>> > @@ -200,14 +200,9 @@ int usb_register_dev(struct usb_interface *intf,
>> >
>> > /* create a usb class device for this usb interface */
>> > snprintf(name, sizeof(name), class_driver->name, minor - minor_base);
>> > - temp = strrchr(name, '/');
>> > - if (temp && (temp[1] != '\0'))
>> I have checked current linux-next, the drivers define .name in the
>> usb_class_driver structure as '...%d'.
>> So, what is the reason to check for trailing '/' here? Historical
>> reasons or there is a (broken/3rd party/etc) driver with it?
>
> I really do not remember why it was done this way, sorry. I have no
> problem not doing it anymore, as long as you are willing to fix any
> potential bugs that might pop up :)
Hmm... this series about cleaning up. The bugs might pop up in the
drivers that still using something like '/foo/bar/' for their names
here.
Anyway, I tried to dig into history and only what I found is the patch
http://www.kernel.org/pub/linux/kernel//people/akpm/patches/2.5/2.5.69/2.5.69-mm3/broken-out/linus.patch
that brings a piece of code. And it the same time it brings the same
piece to the tty layer. I suspect that this piece was copied and
pasted in few place.
Currently the device_create() call uses the name parameter as a
parameter of kobject. But kobject doesn't accept '/' in the names, it
changes it to '!'.
So, I think the way of treating a trailing slash in the usb code is redundant.
> And no, I don't worry about 3rd party drivers, that shouldn't be an
> issue at all here.
Fair enough
--
With Best Regards,
Andy Shevchenko