[PATCH 1/7] string: introduce helper to get base file name from given path

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 1/7] string: introduce helper to get base file name from given path
@ 2012-10-02 15:00 Andy Shevchenko
  2012-10-02 15:00 ` [PATCH 2/7] lib: dynamic_debug: reuse kbasename() Andy Shevchenko
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 15:00 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel, Joe Perches; +Cc: Andy Shevchenko, YAMANE Toshiaki

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 <andriy.shevchenko@linux.intel.com>
Cc: YAMANE Toshiaki <yamanetoshi@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/7] lib: dynamic_debug: reuse kbasename()
  2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
@ 2012-10-02 15:00 ` Andy Shevchenko
  2012-10-02 15:00 ` [PATCH 3/7] staging: rts_pstor: " Andy Shevchenko
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 15:00 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel, Joe Perches; +Cc: Andy Shevchenko, Jason Baron

Remove the custom implementation of the functionality similar to kbasename().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/7] staging: rts_pstor: reuse kbasename()
  2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
  2012-10-02 15:00 ` [PATCH 2/7] lib: dynamic_debug: reuse kbasename() Andy Shevchenko
@ 2012-10-02 15:00 ` Andy Shevchenko
  2012-10-03  0:30   ` Ryan Mallon
  2012-10-02 15:00 ` [PATCH 4/7] mm: reuse kbasename() functionality Andy Shevchenko
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 15:00 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel, Joe Perches
  Cc: Andy Shevchenko, YAMANE Toshiaki, Greg Kroah-Hartman

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 <andriy.shevchenko@linux.intel.com>
Cc: YAMANE Toshiaki <yamanetoshi@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] staging: rts_pstor: reuse kbasename()
  2012-10-02 15:00 ` [PATCH 3/7] staging: rts_pstor: " Andy Shevchenko
@ 2012-10-03  0:30   ` Ryan Mallon
  2012-10-03  0:33     ` Ryan Mallon
  0 siblings, 1 reply; 17+ messages in thread
From: Ryan Mallon @ 2012-10-03  0:30 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andrew Morton, linux-kernel, Joe Perches, YAMANE Toshiaki,
	Greg Kroah-Hartman

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 <andriy.shevchenko@linux.intel.com>
> Cc: YAMANE Toshiaki <yamanetoshi@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  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)   										\
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] staging: rts_pstor: reuse kbasename()
  2012-10-03  0:30   ` Ryan Mallon
@ 2012-10-03  0:33     ` Ryan Mallon
  0 siblings, 0 replies; 17+ messages in thread
From: Ryan Mallon @ 2012-10-03  0:33 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andrew Morton, linux-kernel, Joe Perches, YAMANE Toshiaki,
	Greg Kroah-Hartman

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 <andriy.shevchenko@linux.intel.com>
>> Cc: YAMANE Toshiaki <yamanetoshi@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>  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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 4/7] mm: reuse kbasename() functionality
  2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
  2012-10-02 15:00 ` [PATCH 2/7] lib: dynamic_debug: reuse kbasename() Andy Shevchenko
  2012-10-02 15:00 ` [PATCH 3/7] staging: rts_pstor: " Andy Shevchenko
@ 2012-10-02 15:00 ` Andy Shevchenko
  2012-10-02 15:00 ` [PATCH 5/7] procfs: " Andy Shevchenko
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 15:00 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel, Joe Perches; +Cc: Andy Shevchenko, linux-mm

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-mm@kvack.org
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/7] procfs: reuse kbasename() functionality
  2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
                   ` (2 preceding siblings ...)
  2012-10-02 15:00 ` [PATCH 4/7] mm: reuse kbasename() functionality Andy Shevchenko
@ 2012-10-02 15:00 ` Andy Shevchenko
  2012-10-02 15:00 ` [PATCH 6/7] usb: core: reuse kbasename() Andy Shevchenko
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 15:00 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel, Joe Perches; +Cc: Andy Shevchenko

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 6/7] usb: core: reuse kbasename()
  2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
                   ` (3 preceding siblings ...)
  2012-10-02 15:00 ` [PATCH 5/7] procfs: " Andy Shevchenko
@ 2012-10-02 15:00 ` Andy Shevchenko
  2012-10-03  8:27   ` Andy Shevchenko
  2012-10-02 15:01 ` [PATCH 7/7] trace: reuse kbasename() functionality Andy Shevchenko
  2012-10-02 17:34 ` [PATCH 1/7] string: introduce helper to get base file name from given path Greg KH
  6 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 15:00 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel, Joe Perches
  Cc: Andy Shevchenko, Greg Kroah-Hartman, linux-usb

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 6/7] usb: core: reuse kbasename()
  2012-10-02 15:00 ` [PATCH 6/7] usb: core: reuse kbasename() Andy Shevchenko
@ 2012-10-03  8:27   ` Andy Shevchenko
  2012-10-03 15:08     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-03  8:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andrew Morton, linux-kernel, Joe Perches, Greg Kroah-Hartman,
	linux-usb

On Tue, Oct 2, 2012 at 6:00 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 6/7] usb: core: reuse kbasename()
  2012-10-03  8:27   ` Andy Shevchenko
@ 2012-10-03 15:08     ` Greg Kroah-Hartman
  2012-10-17  7:42       ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Greg Kroah-Hartman @ 2012-10-03 15:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Andrew Morton, linux-kernel, Joe Perches,
	linux-usb

On Wed, Oct 03, 2012 at 11:27:27AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 2, 2012 at 6:00 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 6/7] usb: core: reuse kbasename()
  2012-10-03 15:08     ` Greg Kroah-Hartman
@ 2012-10-17  7:42       ` Andy Shevchenko
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-17  7:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Andy Shevchenko, Andrew Morton, linux-kernel, Joe Perches,
	linux-usb

On Wed, Oct 3, 2012 at 6:08 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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
>> <andriy.shevchenko@linux.intel.com> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 7/7] trace: reuse kbasename() functionality
  2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
                   ` (4 preceding siblings ...)
  2012-10-02 15:00 ` [PATCH 6/7] usb: core: reuse kbasename() Andy Shevchenko
@ 2012-10-02 15:01 ` Andy Shevchenko
  2012-10-02 17:34 ` [PATCH 1/7] string: introduce helper to get base file name from given path Greg KH
  6 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 15:01 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel, Joe Perches; +Cc: Andy Shevchenko

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
Cc: Frederic Weisbecker <fweisbec@gmail.com> (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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] string: introduce helper to get base file name from given path
  2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
                   ` (5 preceding siblings ...)
  2012-10-02 15:01 ` [PATCH 7/7] trace: reuse kbasename() functionality Andy Shevchenko
@ 2012-10-02 17:34 ` Greg KH
  2012-10-02 17:52   ` Andy Shevchenko
  6 siblings, 1 reply; 17+ messages in thread
From: Greg KH @ 2012-10-02 17:34 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Andrew Morton, linux-kernel, Joe Perches, YAMANE Toshiaki

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 <andriy.shevchenko@linux.intel.com>
> Cc: YAMANE Toshiaki <yamanetoshi@gmail.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] string: introduce helper to get base file name from given path
  2012-10-02 17:34 ` [PATCH 1/7] string: introduce helper to get base file name from given path Greg KH
@ 2012-10-02 17:52   ` Andy Shevchenko
  2012-10-02 18:12     ` Greg KH
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-02 17:52 UTC (permalink / raw)
  To: Greg KH
  Cc: Andy Shevchenko, Andrew Morton, linux-kernel, Joe Perches,
	YAMANE Toshiaki

On Tue, Oct 2, 2012 at 8:34 PM, Greg KH <gregkh@linuxfoundation.org> 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 <andriy.shevchenko@linux.intel.com>
>> Cc: YAMANE Toshiaki <yamanetoshi@gmail.com>
>> ---
>>  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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] string: introduce helper to get base file name from given path
  2012-10-02 17:52   ` Andy Shevchenko
@ 2012-10-02 18:12     ` Greg KH
  2012-10-03 18:39       ` Nick Bowler
  0 siblings, 1 reply; 17+ messages in thread
From: Greg KH @ 2012-10-02 18:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Andrew Morton, linux-kernel, Joe Perches,
	YAMANE Toshiaki

On Tue, Oct 02, 2012 at 08:52:05PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 2, 2012 at 8:34 PM, Greg KH <gregkh@linuxfoundation.org> 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 <andriy.shevchenko@linux.intel.com>
> >> Cc: YAMANE Toshiaki <yamanetoshi@gmail.com>
> >> ---
> >>  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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] string: introduce helper to get base file name from given path
  2012-10-02 18:12     ` Greg KH
@ 2012-10-03 18:39       ` Nick Bowler
  2012-10-04  8:19         ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Nick Bowler @ 2012-10-03 18:39 UTC (permalink / raw)
  To: Greg KH
  Cc: Andy Shevchenko, Andy Shevchenko, Andrew Morton, linux-kernel,
	Joe Perches, YAMANE Toshiaki

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 <gregkh@linuxfoundation.org> 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/)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/7] string: introduce helper to get base file name from given path
  2012-10-03 18:39       ` Nick Bowler
@ 2012-10-04  8:19         ` Andy Shevchenko
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2012-10-04  8:19 UTC (permalink / raw)
  To: Nick Bowler
  Cc: Greg KH, Andy Shevchenko, Andrew Morton, linux-kernel,
	Joe Perches, YAMANE Toshiaki

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 <gregkh@linuxfoundation.org> 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2012-10-17  7:42 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-02 15:00 [PATCH 1/7] string: introduce helper to get base file name from given path Andy Shevchenko
2012-10-02 15:00 ` [PATCH 2/7] lib: dynamic_debug: reuse kbasename() Andy Shevchenko
2012-10-02 15:00 ` [PATCH 3/7] staging: rts_pstor: " Andy Shevchenko
2012-10-03  0:30   ` Ryan Mallon
2012-10-03  0:33     ` Ryan Mallon
2012-10-02 15:00 ` [PATCH 4/7] mm: reuse kbasename() functionality Andy Shevchenko
2012-10-02 15:00 ` [PATCH 5/7] procfs: " Andy Shevchenko
2012-10-02 15:00 ` [PATCH 6/7] usb: core: reuse kbasename() Andy Shevchenko
2012-10-03  8:27   ` Andy Shevchenko
2012-10-03 15:08     ` Greg Kroah-Hartman
2012-10-17  7:42       ` Andy Shevchenko
2012-10-02 15:01 ` [PATCH 7/7] trace: reuse kbasename() functionality Andy Shevchenko
2012-10-02 17:34 ` [PATCH 1/7] string: introduce helper to get base file name from given path Greg KH
2012-10-02 17:52   ` Andy Shevchenko
2012-10-02 18:12     ` Greg KH
2012-10-03 18:39       ` Nick Bowler
2012-10-04  8:19         ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).