Re: [PATCH 3/6] test: Add a test for strim()

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3138 bytes --]

On Wed, Mar 19, 2025 at 03:04:16PM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 19 Mar 2025 at 15:24, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 12:59:05PM +0100, Simon Glass wrote:
> >
> > > This function trims whitespace from the start and end of a string. Add a
> > > test for it.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  test/lib/string.c | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> >
> > This got me to ask "What even is using strim and where did it come
> > from?". To which the answer is:
> > - A few places, but it's probably reasonable.
> > - Linux, pre-2011.
> >
> > I say the latter because we're missing a bug fix to the strim function
> > that's been there since 2011:
> >
> > commit 66f6958e69d8055277356d3cc2e7a1d734db1755
> > Author: Michael Holzheu <holzheu@linux.vnet.ibm.com>
> > Date:   Mon Oct 31 17:12:37 2011 -0700
> >
> >     lib/string.c: fix strim() semantics for strings that have only blanks
> >
> >     Commit 84c95c9acf0 ("string: on strstrip(), first remove leading spaces
> >     before running over str") improved  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.
> >
> >     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
> >
> >     The patch restores the old strim() semantics.
> >
> >     Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
> >     Cc: Andre Goddard Rosa <andre.goddard@gmail.com>
> >     Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> >     Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> >     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> >
> > --
> > Tom
> 
> Here is the comment for the function, of note given Linux's disdain
> for commenting code:
> 
> /**
>  * strim - Removes leading and trailing whitespace from @s.
>  * @s: The string to be stripped.
>  *
>  * Note that the first trailing whitespace is replaced with a %NUL-terminator
>  * in the given string @s. Returns a pointer to the first non-whitespace
>  * character in @s.
>  */
> 
> Given that comment, I don't see a bug here. But of course we could add
> a test for it and adjust the function too. PLMK.

Did your test add a testcase for the situation described above?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]