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

[Simon Glass]

Hi Tom,

On Fri, 21 Mar 2025 at 03:15, Tom Rini <trini at konsulko.com> wrote:
>
> On Thu, Mar 20, 2025 at 03:43:30AM +0000, Simon Glass wrote:
> > 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.
>
> Well, being a function we borrow from the linux kernel, we should follow
> how it works and backport the trivial change to match how it behaves,
> then add tests in 2/2.

OK.

Regards,
Simon