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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250320/f924276f/attachment.sig>