Fix strim() semantics for strings that have only blanks

PATCH In review Michael Holzheu 3 participants 4 messages Oct 12, 2011 – Oct 17, 2011

Subsystems (2)

Library code Generic string library

lore

Share & tools

lore

Michael Holzheu

author

2011-10-12 08:32 UTC #0 lore raw

PATCH 1/1 Fix strim() semantics for strings that have only blanks

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.

Quoted 1 line

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 <[email protected]>

---
 lib/string.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

lib/string.c +1 -2

@@ -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);

Andrew Morton

maintainer

2011-10-13 23:54 UTC #1 lore raw

Quoted 16 lines · On Wed, 12 Oct 2011 10:31:57 +0200 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)

Quoted 8 lines

I think we should remove your patch to restore the old behavior.
Quoted 1 line
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.

Quoted 3 lines

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 :(

Michael Holzheu

author

2011-10-14 08:47 UTC #2 lore raw

Hello Andrew,

Quoted 6 lines · Andrew Morton

On Wed, 12 Oct 2011 10:31:57 +0200
Quoted 3 lines
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 ;-)

Quoted 5 lines

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

André Goddard Rosa

2011-10-17 11:25 UTC #3 lore raw

In reply to Andrew Morton

Hello Michael,

Quoted 21 lines · Andrew Morton

Quoted 18 lines · On Wed, 12 Oct 2011 10:31:57 +0200 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.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
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é