Hi Tom,
On Fri, 21 Mar 2025 at 03:15, Tom Rini <tr...@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 <tr...@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 <tr...@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 <s...@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 <holz...@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 <holz...@linux.vnet.ibm.com>
> > > > > Cc: Andre Goddard Rosa <andre.godd...@gmail.com>
> > > > > Cc: Martin Schwidefsky <schwidef...@de.ibm.com>
> > > > > Cc: Heiko Carstens <heiko.carst...@de.ibm.com>
> > > > > Signed-off-by: Andrew Morton <a...@linux-foundation.org>
> > > > > Signed-off-by: Linus Torvalds <torva...@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
