[PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[Rickard Strandqvist]

Using memset before strncpy just to ensure a trailing null
character is an unnecessary double writing of a string

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/staging/lustre/lustre/libcfs/debug.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/libcfs/debug.c b/drivers/staging/lustre/lustre/libcfs/debug.c
index 6b58469..98c7c3a 100644
--- a/drivers/staging/lustre/lustre/libcfs/debug.c
+++ b/drivers/staging/lustre/lustre/libcfs/debug.c
@@ -402,9 +402,9 @@ int libcfs_debug_init(unsigned long bufsize)
 	}
 
 	if (libcfs_debug_file_path != NULL) {
-		memset(libcfs_debug_file_path_arr, 0, PATH_MAX);
 		strncpy(libcfs_debug_file_path_arr,
 			libcfs_debug_file_path, PATH_MAX-1);
+		libcfs_debug_file_path_arr[PATH_MAX - 1] = '\0';
 	}
 
 	/* If libcfs_debug_mb is set to an invalid value or uninitialized
-- 
1.7.10.4

Re: [PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[Dan Carpenter]

On Sun, Sep 14, 2014 at 06:03:16PM +0200, Rickard Strandqvist wrote:
> Using memset before strncpy just to ensure a trailing null
> character is an unnecessary double writing of a string
> 

You really should make a function which pads and NUL terminates.

I've said this before, of course, but you haven't even tried.

I can't get excited about these cleanups which open code NUL termination
every where.  They are risky and have introduced bugs before.

regards,
dan carpenter

Re: [PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[Rickard Strandqvist]

2014-09-15 10:23 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Sun, Sep 14, 2014 at 06:03:16PM +0200, Rickard Strandqvist wrote:
>> Using memset before strncpy just to ensure a trailing null
>> character is an unnecessary double writing of a string
>>
>
> You really should make a function which pads and NUL terminates.
>
> I've said this before, of course, but you haven't even tried.
>
> I can't get excited about these cleanups which open code NUL termination
> every where.  They are risky and have introduced bugs before.
>
> regards,
> dan carpenter



Hi Dan

Ok, I have made two suggestions for strncpy function that also
guarantees a terminating null character.
1) retunerar number of characters to be copied, it can be good to
have, but was not really satisfied.

int strncpyz(char *dest, const char *src, size_t count)
{
  size_t len=0;

  if(0 == count)
    return 0;

  --count;
  while(len < count && src[len])
   *dest++ = src[len++];

  do {
    *dest++ = '\0';
  }
  while(len < count--);

  return len;
}


2) The next version is almost the same code as the regular strncpy,
but with two extra lines.

char *strncpyz(char *dest, const char *src, size_t count)
{
        char *tmp = dest;

        while (count) {
                if ((*tmp = *src) != 0)
                        src++;
                tmp++;
                count--;
        }

        if(tmp != dest)
          *--tmp = '\0';

        return dest;
}


Since I did not got any better solution to variant 1, I prefer variant 2.

Then the next question is of course what it should be called  :-)


Kind regards
Rickard Strandqvist

Re: [PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[Dan Carpenter]

On Thu, Sep 18, 2014 at 12:12:42AM +0200, Rickard Strandqvist wrote:
> Hi Dan
> 
> Ok, I have made two suggestions for strncpy function that also
> guarantees a terminating null character.
> 1) retunerar number of characters to be copied, it can be good to
> have, but was not really satisfied.

strlcpy() is more popular the strncpy() in the kernel.

No one uses the return value of strncpy() because what is the point?
There are around 15-20 places which use the return value of strlcpy().
Some of the place which use the return value assume that the copy fits.

I think we should return the number of bytes before the NUL or else
count.

> 
> int strncpyz(char *dest, const char *src, size_t count)
> {
>   size_t len=0;
> 
>   if(0 == count)
>     return 0;
> 
>   --count;
>   while(len < count && src[len])
>    *dest++ = src[len++];
> 
>   do {
>     *dest++ = '\0';
>   }
>   while(len < count--);
> 
>   return len;
> }
> 
> 
> 2) The next version is almost the same code as the regular strncpy,
> but with two extra lines.
> 
> char *strncpyz(char *dest, const char *src, size_t count)
> {
>         char *tmp = dest;
> 
>         while (count) {
>                 if ((*tmp = *src) != 0)
>                         src++;
>                 tmp++;
>                 count--;
>         }
> 
>         if(tmp != dest)
>           *--tmp = '\0';
> 
>         return dest;
> }
> 
> 
> Since I did not got any better solution to variant 1, I prefer variant 2.

I also prefer variant 2.

> 
> Then the next question is of course what it should be called  :-)

I think a lot of people call this function strzcpy().

This sort of patch would go through Andrew Morton.

regards,
dan carpenter

Re: [PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[Rickard Strandqvist]

2014-09-18 10:51 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Thu, Sep 18, 2014 at 12:12:42AM +0200, Rickard Strandqvist wrote:
>> Hi Dan
>>
>> Ok, I have made two suggestions for strncpy function that also
>> guarantees a terminating null character.
>> 1) retunerar number of characters to be copied, it can be good to
>> have, but was not really satisfied.
>
> strlcpy() is more popular the strncpy() in the kernel.
>
> No one uses the return value of strncpy() because what is the point?
> There are around 15-20 places which use the return value of strlcpy().
> Some of the place which use the return value assume that the copy fits.
>
> I think we should return the number of bytes before the NUL or else
> count.
>
>>
>> int strncpyz(char *dest, const char *src, size_t count)
>> {
>>   size_t len=0;
>>
>>   if(0 == count)
>>     return 0;
>>
>>   --count;
>>   while(len < count && src[len])
>>    *dest++ = src[len++];
>>
>>   do {
>>     *dest++ = '\0';
>>   }
>>   while(len < count--);
>>
>>   return len;
>> }
>>
>>
>> 2) The next version is almost the same code as the regular strncpy,
>> but with two extra lines.
>>
>> char *strncpyz(char *dest, const char *src, size_t count)
>> {
>>         char *tmp = dest;
>>
>>         while (count) {
>>                 if ((*tmp = *src) != 0)
>>                         src++;
>>                 tmp++;
>>                 count--;
>>         }
>>
>>         if(tmp != dest)
>>           *--tmp = '\0';
>>
>>         return dest;
>> }
>>
>>
>> Since I did not got any better solution to variant 1, I prefer variant 2.
>
> I also prefer variant 2.
>
>>
>> Then the next question is of course what it should be called  :-)
>
> I think a lot of people call this function strzcpy().
>
> This sort of patch would go through Andrew Morton.
>
> regards,
> dan carpenter


Hi Dan

Ok, strzcpy is it :)

Should I add this as a patch in lib/string.c or email him first.. What
is customary in these situations?


Kind regards
Rickard Strandqvist

Re: [PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[Dan Carpenter]

On Thu, Sep 18, 2014 at 09:57:17PM +0200, Rickard Strandqvist wrote:
> 
> Should I add this as a patch in lib/string.c or email him first.. What
> is customary in these situations?
> 

Just write up a normal patch and try to merge it through the normal
methods.

It's not that controversial to do:

	strncpy(dest, src, n);
	dest[n] = '\0';

That's what we have been open coding all over the place anyway already.

regards,
dan carpenter

Re: [PATCH] staging: lustre: lustre: libcfs: debug.c: Cleaning up unnecessary use of memset in conjunction with strncpy

[Rickard Strandqvist]

2014-09-19 0:39 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Thu, Sep 18, 2014 at 09:57:17PM +0200, Rickard Strandqvist wrote:
>>
>> Should I add this as a patch in lib/string.c or email him first.. What
>> is customary in these situations?
>>
>
> Just write up a normal patch and try to merge it through the normal
> methods.
>
> It's not that controversial to do:
>
>         strncpy(dest, src, n);
>         dest[n] = '\0';
>
> That's what we have been open coding all over the place anyway already.


Hi Dan!

I have tried the function strzcpy that we talk about, but it has not
happened anything:-(

Is there anything else I can do?


Kind regards
Rickard Strandqvist