[PATCH] lustre:libcfs: remove redundant code.

[PATCH] lustre:libcfs: remove redundant code.

[Alexandru Juncu]

Found using coccinelle. It suggested kmalloc/strcpy should be replaced
with kstrdup, but the entire function can be replaced by kstrdup.

Signed-off-by: Alexandru Juncu <alexj@rosedu.org>
---
 drivers/staging/lustre/lustre/libcfs/libcfs_string.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
index 9edccc9..4dba304 100644
--- a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
+++ b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
@@ -135,18 +135,7 @@ EXPORT_SYMBOL(cfs_str2mask);
 /* Duplicate a string in a platform-independent way */
 char *cfs_strdup(const char *str, u_int32_t flags)
 {
-	size_t lenz; /* length of str + zero byte */
-	char *dup_str;
-
-	lenz = strlen(str) + 1;
-
-	dup_str = kmalloc(lenz, flags);
-	if (dup_str == NULL)
-		return NULL;
-
-	memcpy(dup_str, str, lenz);
-
-	return dup_str;
+	return kstrdup(str, flags);
 }
 EXPORT_SYMBOL(cfs_strdup);
 
-- 
1.8.1.2

Re: [PATCH] lustre:libcfs: remove redundant code.

[Pekka Enberg]

On Fri, Jul 19, 2013 at 5:45 PM, Alexandru Juncu <alexj@rosedu.org> wrote:
> Found using coccinelle. It suggested kmalloc/strcpy should be replaced
> with kstrdup, but the entire function can be replaced by kstrdup.
>
> Signed-off-by: Alexandru Juncu <alexj@rosedu.org>
> ---
>  drivers/staging/lustre/lustre/libcfs/libcfs_string.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
> index 9edccc9..4dba304 100644
> --- a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
> +++ b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
> @@ -135,18 +135,7 @@ EXPORT_SYMBOL(cfs_str2mask);
>  /* Duplicate a string in a platform-independent way */
>  char *cfs_strdup(const char *str, u_int32_t flags)
>  {
> -       size_t lenz; /* length of str + zero byte */
> -       char *dup_str;
> -
> -       lenz = strlen(str) + 1;
> -
> -       dup_str = kmalloc(lenz, flags);
> -       if (dup_str == NULL)
> -               return NULL;
> -
> -       memcpy(dup_str, str, lenz);
> -
> -       return dup_str;
> +       return kstrdup(str, flags);
>  }
>  EXPORT_SYMBOL(cfs_strdup);

It would be better if you replaced the calls to cfs_strdup() with
kstrdup() and got rid of cfs_strdup() altogether.

Re: [PATCH] lustre:libcfs: remove redundant code.

[Alexandru Juncu]

On 19 July 2013 18:08, Pekka Enberg <penberg@kernel.org> wrote:
> On Fri, Jul 19, 2013 at 5:45 PM, Alexandru Juncu <alexj@rosedu.org> wrote:
>> Found using coccinelle. It suggested kmalloc/strcpy should be replaced
>> with kstrdup, but the entire function can be replaced by kstrdup.
>>
>> Signed-off-by: Alexandru Juncu <alexj@rosedu.org>
>> ---
>>  drivers/staging/lustre/lustre/libcfs/libcfs_string.c | 13 +------------
>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> index 9edccc9..4dba304 100644
>> --- a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> +++ b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> @@ -135,18 +135,7 @@ EXPORT_SYMBOL(cfs_str2mask);
>>  /* Duplicate a string in a platform-independent way */
>>  char *cfs_strdup(const char *str, u_int32_t flags)
>>  {
>> -       size_t lenz; /* length of str + zero byte */
>> -       char *dup_str;
>> -
>> -       lenz = strlen(str) + 1;
>> -
>> -       dup_str = kmalloc(lenz, flags);
>> -       if (dup_str == NULL)
>> -               return NULL;
>> -
>> -       memcpy(dup_str, str, lenz);
>> -
>> -       return dup_str;
>> +       return kstrdup(str, flags);
>>  }
>>  EXPORT_SYMBOL(cfs_strdup);
>
> It would be better if you replaced the calls to cfs_strdup() with
> kstrdup() and got rid of cfs_strdup() altogether.

I was thinking the same thing, but I hesitated because I didn't know
how used it was and I didn't want to break something.

Re: [PATCH] lustre:libcfs: remove redundant code.

[Pekka Enberg]

On Fri, Jul 19, 2013 at 6:13 PM, Alexandru Juncu <alexj@rosedu.org> wrote:
> I was thinking the same thing, but I hesitated because I didn't know
> how used it was and I didn't want to break something.

"git grep cfs_strdup" suggests that nobody uses it so you could just
remove it completely...

Re: [PATCH] lustre:libcfs: remove redundant code.

[Alexandru Juncu]

On 19 July 2013 18:21, Pekka Enberg <penberg@kernel.org> wrote:
> On Fri, Jul 19, 2013 at 6:13 PM, Alexandru Juncu <alexj@rosedu.org> wrote:
>> I was thinking the same thing, but I hesitated because I didn't know
>> how used it was and I didn't want to break something.
>
> "git grep cfs_strdup" suggests that nobody uses it so you could just
> remove it completely...

Did a grep on the tree and yes, it's not used. But It's in a staging
directory so maybe the rest of the code has not been pushed in yet.

Re: [PATCH] lustre:libcfs: remove redundant code.

[Greg Kroah-Hartman]

On Fri, Jul 19, 2013 at 06:29:34PM +0300, Alexandru Juncu wrote:
> On 19 July 2013 18:21, Pekka Enberg <penberg@kernel.org> wrote:
> > On Fri, Jul 19, 2013 at 6:13 PM, Alexandru Juncu <alexj@rosedu.org> wrote:
> >> I was thinking the same thing, but I hesitated because I didn't know
> >> how used it was and I didn't want to break something.
> >
> > "git grep cfs_strdup" suggests that nobody uses it so you could just
> > remove it completely...
> 
> Did a grep on the tree and yes, it's not used. But It's in a staging
> directory so maybe the rest of the code has not been pushed in yet.

Doesn't matter, if there are no users, please just remove it.

thanks,

greg k-h

Re: [PATCH] lustre:libcfs: remove redundant code.

[Paul Bolle]

On Fri, 2013-07-19 at 08:46 -0700, Greg Kroah-Hartman wrote:
> Doesn't matter, if there are no users, please just remove it.

Is that, basically, your approach to staging cleanups?

I ask because I noticed that "drivers/staging/lustre/lustre/ptlrpc/gss/"
is only built if CONFIG_PTLRPC_GSS is set. But the corresponding Kconfig
symbol is nowhere to be found.

Or do you prefer that I poke the people involved in (this case in)
Lustre before suggesting a drastic cleanup like that?


Paul Bolle

Re: [PATCH] lustre:libcfs: remove redundant code.

[Alexandru Juncu]

On 19 July 2013 19:07, Paul Bolle <pebolle@tiscali.nl> wrote:
> On Fri, 2013-07-19 at 08:46 -0700, Greg Kroah-Hartman wrote:
>> Doesn't matter, if there are no users, please just remove it.
>
> Is that, basically, your approach to staging cleanups?
>
> I ask because I noticed that "drivers/staging/lustre/lustre/ptlrpc/gss/"
> is only built if CONFIG_PTLRPC_GSS is set. But the corresponding Kconfig
> symbol is nowhere to be found.
>
> Or do you prefer that I poke the people involved in (this case in)
> Lustre before suggesting a drastic cleanup like that?



Thanks for the comments!

The comment for that function is "Duplicate a string in a
platform-independent way" so it might be because it's called by code
that also run on non-Linux systems.

RE: [PATCH] lustre:libcfs: remove redundant code.

[Peng, Tao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1063 bytes --]

> -----Original Message-----
> From: Paul Bolle [mailto:pebolle@tiscali.nl]
> Sent: Saturday, July 20, 2013 12:07 AM
> To: Greg Kroah-Hartman
> Cc: Alexandru Juncu; Pekka Enberg; andreas.dilger; Peng, Tao; driverdev; LKML
> Subject: Re: [PATCH] lustre:libcfs: remove redundant code.
> 
> On Fri, 2013-07-19 at 08:46 -0700, Greg Kroah-Hartman wrote:
> > Doesn't matter, if there are no users, please just remove it.
> 
> Is that, basically, your approach to staging cleanups?
> 
> I ask because I noticed that "drivers/staging/lustre/lustre/ptlrpc/gss/"
> is only built if CONFIG_PTLRPC_GSS is set. But the corresponding Kconfig
> symbol is nowhere to be found.
> 
That's my fault. We certainly _want_ the ptlrpc gss code. I'll get it fixed.

Thanks,
Tao

> Or do you prefer that I poke the people involved in (this case in)
> Lustre before suggesting a drastic cleanup like that?
> 
> 
> Paul Bolle
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] lustre:libcfs: remove redundant code.

[Paul Bolle]

On Mon, 2013-07-22 at 02:07 +0000, Peng, Tao wrote:
> Paul Bolle [mailto:pebolle@tiscali.nl]
> > I ask because I noticed that "drivers/staging/lustre/lustre/ptlrpc/gss/"
> > is only built if CONFIG_PTLRPC_GSS is set. But the corresponding Kconfig
> > symbol is nowhere to be found.
> > 
> That's my fault. We certainly _want_ the ptlrpc gss code. I'll get it fixed.

I'll stop worrying about it. But if little has changed by, say, early
2014, I might try to prod you again.

Thanks!


Paul Bolle

Re: [PATCH] lustre:libcfs: remove redundant code.

[Daniel Baluta]

On Fri, Jul 19, 2013 at 6:46 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Jul 19, 2013 at 06:29:34PM +0300, Alexandru Juncu wrote:
>> On 19 July 2013 18:21, Pekka Enberg <penberg@kernel.org> wrote:
>> > On Fri, Jul 19, 2013 at 6:13 PM, Alexandru Juncu <alexj@rosedu.org> wrote:
>> >> I was thinking the same thing, but I hesitated because I didn't know
>> >> how used it was and I didn't want to break something.
>> >
>> > "git grep cfs_strdup" suggests that nobody uses it so you could just
>> > remove it completely...
>>
>> Did a grep on the tree and yes, it's not used. But It's in a staging
>> directory so maybe the rest of the code has not been pushed in yet.
>
> Doesn't matter, if there are no users, please just remove it.

Hi Greg,

There is already a similar patch for this https://lkml.org/lkml/2013/7/17/742,
already reviewed by Andreas Dilger.

thanks,
Daniel.