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

[Simon Glass]

Hi Tom,

On Wed, 19 Mar 2025 at 16:38, Tom Rini <trini at konsulko.com> wrote:
>
> 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 at 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 at 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 at 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 at linux.vnet.ibm.com>
> > >     Cc: Andre Goddard Rosa <andre.goddard at gmail.com>
> > >     Cc: Martin Schwidefsky <schwidefsky at de.ibm.com>
> > >     Cc: Heiko Carstens <heiko.carstens at de.ibm.com>
> > >     Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> > >     Signed-off-by: Linus Torvalds <torvalds at 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?

No. I didn't know about that case. The function comment does not
suggest that it handles a whitespace-only string in that way. In fact,
even the Linux commit which changes the behaviour doesn't update the
comment to mention that behaviour.

Anyway, let me know what you'd like to do.

Regards,
Simon