[PATCH] Fix strim() semantics for strings that have only blanks

[PATCH] Fix strim() semantics for strings that have only blanks

[Michael Holzheu]

Hello Andre,

With git commit 84c95c9acf088c99d8793d78036b67faa5d0b851 a patch from
you went upstream where you wanted to improve the performance of the
strim() function.

Unfortunately this changed the semantics of strim() and broke my code. Before
the patch it was possible to use strim() without using the return value for
removing trailing spaces from strings that had either only blanks or only
trailing blanks.

Now this does not work any longer for strings that *only* have blanks.

Before patch: "   " -> ""    (empty string)
After patch:  "   " -> "   " (no change)

I think we should remove your patch to restore the old behavior.

>From the description (lib/string.c):

 * Note that the first trailing whitespace is replaced with a %NUL-terminator

=> The first trailing whitespace of a string that only has whitespace
   characters is the first whitespace

Also strim() explicitly does not have "__must_check", in order to use it
without using the return value.

I attached a patch that restores the old strim() semantics.

Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
---
 lib/string.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/lib/string.c
+++ b/lib/string.c
@@ -360,7 +360,6 @@ char *strim(char *s)
 	size_t size;
 	char *end;
 
-	s = skip_spaces(s);
 	size = strlen(s);
 	if (!size)
 		return s;
@@ -370,7 +369,7 @@ char *strim(char *s)
 		end--;
 	*(end + 1) = '\0';
 
-	return s;
+	return skip_spaces(s);
 }
 EXPORT_SYMBOL(strim);
 

Re: [PATCH] Fix strim() semantics for strings that have only blanks

[Andrew Morton]

On Wed, 12 Oct 2011 10:31:57 +0200
Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:

> Hello Andre,
> 
> With git commit 84c95c9acf088c99d8793d78036b67faa5d0b851 a patch from
> you went upstream where you wanted to improve the performance of the
> strim() function.
> 
> Unfortunately this changed the semantics of strim() and broke my code. Before
> the patch it was possible to use strim() without using the return value for
> removing trailing spaces from strings that had either only blanks or only
> trailing blanks.
> 
> Now this does not work any longer for strings that *only* have blanks.
> 
> Before patch: "   " -> ""    (empty string)
> After patch:  "   " -> "   " (no change)
> 
> I think we should remove your patch to restore the old behavior.
> 
> >From the description (lib/string.c):
> 
>  * Note that the first trailing whitespace is replaced with a %NUL-terminator
> 
> => The first trailing whitespace of a string that only has whitespace
>    characters is the first whitespace

Yes, that change makes sense.

> Also strim() explicitly does not have "__must_check", in order to use it
> without using the return value.
> 

This observation seems to have nothing to do with the patch?

I think that strim() _should_ have __must_check annotation (which means
that strim() simply disappears, which is good).  It is only safe to
ignore the strim() return value if the caller knows that the string
started with a non-space.  The number of situations where this is
guaranteed are faily small, I suspect.  Whereas the chances of someone
screwing this up are pretty high :(

Re: [PATCH] Fix strim() semantics for strings that have only blanks

[Michael Holzheu]

Hello Andrew,

On Thu, 2011-10-13 at 16:54 -0700, Andrew Morton wrote:
> On Wed, 12 Oct 2011 10:31:57 +0200
> > Also strim() explicitly does not have "__must_check", in order to use it
> > without using the return value.
> > 
> 
> This observation seems to have nothing to do with the patch?

Well, it is just a hint that it was intentionally that the return value
of strim() can be ignored and therefore I legally can use it like I do
it ;-)

> I think that strim() _should_ have __must_check annotation (which means
> that strim() simply disappears, which is good).  It is only safe to
> ignore the strim() return value if the caller knows that the string
> started with a non-space.  The number of situations where this is
> guaranteed are faily small, I suspect.

Everywhere I use that function I know that my string has only trailing
blanks (or only blanks). Therefore I think we should have a function
that just removes trailing blanks. Perhaps we should change strim() to:

void strim(char *str);

And change the three users that need the return value to use the
strstrip() instead:

grep -r " = strim(" linux-2.6
security/apparmor/lib.c:	char *name = strim(fqname);
security/apparmor/lsm.c:	args = strim(args);
security/apparmor/policy.c:	hname = strim((char *)hname);

Michael

Re: [PATCH] Fix strim() semantics for strings that have only blanks

[André Goddard Rosa]

Hello Michael,

On Thu, Oct 13, 2011 at 8:54 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 12 Oct 2011 10:31:57 +0200
> Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
>
>> Hello Andre,
>>
>> With git commit 84c95c9acf088c99d8793d78036b67faa5d0b851 a patch from
>> you went upstream where you wanted to improve the performance of the
>> strim() function.
>>
>> Unfortunately this changed the semantics of strim() and broke my code. Before
>> the patch it was possible to use strim() without using the return value for
>> removing trailing spaces from strings that had either only blanks or only
>> trailing blanks.
>>
>> Now this does not work any longer for strings that *only* have blanks.
>>
>> Before patch: "   " -> ""    (empty string)
>> After patch:  "   " -> "   " (no change)
>>
>> I think we should remove your patch to restore the old behavior.
>>
>> >From the description (lib/string.c):
>>
>>  * Note that the first trailing whitespace is replaced with a %NUL-terminator
>>
>> => The first trailing whitespace of a string that only has whitespace
>>    characters is the first whitespace
>
> Yes, that change makes sense.

breaking your code was not good at all. Is it maintained off tree?
I believe It happened because you use strim() without using its return.
In any case, I'm OK on reverting it to fix your problem.

Thanks,
André