[PATCH 0/6] Test improvements

[PATCH 0/6] Test improvements

[Simon Glass]

This includes some small abuf and test improvements.


Simon Glass (6):
  abuf: Add a function to copy a buffer
  abuf: Add a way to printf() into a buffer
  test: Add a test for strim()
  membuf: Add an easy way to set up a buffer with data
  sandbox: Enable PHYS_64BIT for 64-bit builds
  lib: Provide a signed version of simple_itoa()

 Kconfig                       |  1 +
 arch/sandbox/include/asm/io.h |  4 +-
 include/abuf.h                | 32 +++++++++++++
 include/membuf.h              | 15 ++++++-
 include/vsprintf.h            | 14 ++++++
 lib/abuf.c                    | 49 ++++++++++++++++++++
 lib/membuf.c                  |  8 ++++
 lib/vsprintf.c                | 29 ++++++++++--
 test/lib/abuf.c               | 84 +++++++++++++++++++++++++++++++++++
 test/lib/str.c                | 17 ++++++-
 test/lib/string.c             | 31 +++++++++++++
 11 files changed, 276 insertions(+), 8 deletions(-)

-- 
2.43.0

base-commit: eac5568b56dc8d048931a0e0d8e0fe573176ae4e
branch: schb

[PATCH 1/6] abuf: Add a function to copy a buffer

[Simon Glass]

It is useful to be able to copy an abuf, to allow changes while
preserving the original. Add a function for this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/abuf.h  | 11 +++++++++++
 lib/abuf.c      | 14 ++++++++++++++
 test/lib/abuf.c | 23 +++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/include/abuf.h b/include/abuf.h
index 62ff6499a0c..64189a4e32c 100644
--- a/include/abuf.h
+++ b/include/abuf.h
@@ -111,6 +111,17 @@ bool abuf_realloc(struct abuf *abuf, size_t new_size);
  */
 bool abuf_realloc_inc(struct abuf *abuf, size_t inc);
 
+/**
+ * abuf_copy() - Make a copy of an abuf
+ *
+ * Creates an allocated copy of @old in @new
+ *
+ * @old: abuf to copy
+ * @new: new abuf to hold the copy (inited by this function)
+ * Return: true if OK, false if out of memory
+ */
+bool abuf_copy(const struct abuf *old, struct abuf *new);
+
 /**
  * abuf_uninit_move() - Return the allocated contents and uninit the abuf
  *
diff --git a/lib/abuf.c b/lib/abuf.c
index 61adf7fc6b1..503f5004ff8 100644
--- a/lib/abuf.c
+++ b/lib/abuf.c
@@ -119,6 +119,20 @@ void abuf_init_set(struct abuf *abuf, void *data, size_t size)
 	abuf_set(abuf, data, size);
 }
 
+bool abuf_copy(const struct abuf *old, struct abuf *copy)
+{
+	char *data;
+
+	data = malloc(old->size);
+	if (!data)
+		return false;
+	memcpy(data, old->data, old->size);
+	abuf_init_set(copy, data, old->size);
+	copy->alloced = true;
+
+	return true;
+}
+
 void abuf_init_const(struct abuf *abuf, const void *data, size_t size)
 {
 	/* for now there is no flag indicating that the abuf data is constant */
diff --git a/test/lib/abuf.c b/test/lib/abuf.c
index b38690fe1a9..fa6b007248c 100644
--- a/test/lib/abuf.c
+++ b/test/lib/abuf.c
@@ -419,3 +419,26 @@ static int lib_test_abuf_init(struct unit_test_state *uts)
 	return 0;
 }
 LIB_TEST(lib_test_abuf_init, 0);
+
+/* Test abuf_copy() */
+static int lib_test_abuf_copy(struct unit_test_state *uts)
+{
+	struct abuf buf, copy;
+	ulong start;
+
+	start = ut_check_free();
+
+	abuf_init_set(&buf, test_data, TEST_DATA_LEN);
+	ut_assert(abuf_copy(&buf, &copy));
+	ut_asserteq(buf.size, copy.size);
+	ut_assert(buf.data != copy.data);
+	ut_assert(copy.alloced);
+	abuf_uninit(&copy);
+	abuf_uninit(&buf);
+
+	/* Check for memory leaks */
+	ut_assertok(ut_check_delta(start));
+
+	return 0;
+}
+LIB_TEST(lib_test_abuf_copy, 0);
-- 
2.43.0

[PATCH 2/6] abuf: Add a way to printf() into a buffer

[Simon Glass]

It is useful to format a string into a buffer, with the sizing handled
automatically. Add a function for this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/abuf.h  | 21 +++++++++++++++++
 lib/abuf.c      | 35 ++++++++++++++++++++++++++++
 test/lib/abuf.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)

diff --git a/include/abuf.h b/include/abuf.h
index 64189a4e32c..e761e87b584 100644
--- a/include/abuf.h
+++ b/include/abuf.h
@@ -122,6 +122,27 @@ bool abuf_realloc_inc(struct abuf *abuf, size_t inc);
  */
 bool abuf_copy(const struct abuf *old, struct abuf *new);
 
+/**
+ * abuf_printf() - Format a string and place it in an abuf
+ *
+ * @buf: The buffer to place the result into
+ * @fmt: The format string to use
+ * @...: Arguments for the format string
+ * Return: the number of characters writtenwhich would be
+ * generated for the given input, excluding the trailing null,
+ * as per ISO C99.
+ *
+ * The abuf is expanded as necessary to fit the formated string
+ *
+ * See the vsprintf() documentation for format string extensions over C99.
+ *
+ * Returns: number of characters written (excluding trailing nul) on success,
+ * -E2BIG if the size exceeds 4K, -ENOMEM if out of memory, -EFAULT if there is
+ * an internal bug in the vsnprintf() implementation
+ */
+int abuf_printf(struct abuf *buf, const char *fmt, ...)
+		__attribute__ ((format (__printf__, 2, 3)));
+
 /**
  * abuf_uninit_move() - Return the allocated contents and uninit the abuf
  *
diff --git a/lib/abuf.c b/lib/abuf.c
index 503f5004ff8..816f92587eb 100644
--- a/lib/abuf.c
+++ b/lib/abuf.c
@@ -10,8 +10,11 @@
 #include <malloc.h>
 #include <mapmem.h>
 #include <string.h>
+#include <vsprintf.h>
 #endif
 
+#include <errno.h>
+#include <stdarg.h>
 #include <abuf.h>
 
 void abuf_set(struct abuf *abuf, void *data, size_t size)
@@ -133,6 +136,38 @@ bool abuf_copy(const struct abuf *old, struct abuf *copy)
 	return true;
 }
 
+int abuf_printf(struct abuf *buf, const char *fmt, ...)
+{
+	int maxlen = buf->size;
+	va_list args;
+	int len;
+
+	va_start(args, fmt);
+	len = vsnprintf(buf->data, buf->size, fmt, args);
+	va_end(args);
+
+	/* add the terminator */
+	len++;
+
+	if (len > 4096)
+		return -E2BIG;
+	if (len > maxlen) {
+		/* make more space and try again */
+		maxlen = len;
+		if (!abuf_realloc(buf, maxlen))
+			return -ENOMEM;
+		va_start(args, fmt);
+		len = vsnprintf(buf->data, maxlen, fmt, args);
+		va_end(args);
+
+		/* check there isn't anything strange going on */
+		if (len > maxlen)
+			return -EFAULT;
+	}
+
+	return len;
+}
+
 void abuf_init_const(struct abuf *abuf, const void *data, size_t size)
 {
 	/* for now there is no flag indicating that the abuf data is constant */
diff --git a/test/lib/abuf.c b/test/lib/abuf.c
index fa6b007248c..1b16d79cc03 100644
--- a/test/lib/abuf.c
+++ b/test/lib/abuf.c
@@ -442,3 +442,64 @@ static int lib_test_abuf_copy(struct unit_test_state *uts)
 	return 0;
 }
 LIB_TEST(lib_test_abuf_copy, 0);
+
+/* Test abuf_printf() */
+static int lib_test_abuf_printf(struct unit_test_state *uts)
+{
+	struct abuf buf, fmt;
+	ulong start;
+	char *ptr;
+
+	start = ut_check_free();
+
+	/* start with a fresh buffer */
+	abuf_init(&buf);
+
+	/* check handling of out-of-memory condition */
+	malloc_enable_testing(0);
+	ut_asserteq(-ENOMEM, abuf_printf(&buf, "%s", ""));
+	malloc_enable_testing(1);
+
+	ut_asserteq(0, abuf_printf(&buf, "%s", ""));
+	ut_asserteq(1, buf.size);
+	ut_asserteq(true, buf.alloced);
+	ut_asserteq_str("", buf.data);
+
+	/* check expanding it, initially failing */
+	ut_asserteq(-ENOMEM, abuf_printf(&buf, "%s", "testing"));
+	malloc_disable_testing();
+
+	ut_asserteq(7, abuf_printf(&buf, "%s", "testing"));
+	ut_asserteq(8, buf.size);
+	ut_asserteq_str("testing", buf.data);
+
+	ut_asserteq(11, abuf_printf(&buf, "testing %d", 123));
+	ut_asserteq(12, buf.size);
+	ut_asserteq_str("testing 123", buf.data);
+
+	/* make it smaller; buffer should not shrink */
+	ut_asserteq(9, abuf_printf(&buf, "test %d", 456));
+	ut_asserteq(12, buf.size);
+	ut_asserteq_str("test 456", buf.data);
+
+	/* test the maximum size */
+	abuf_init(&fmt);
+	ut_assert(abuf_realloc(&fmt, 4100));
+	memset(fmt.data, 'x', 4100);
+	ptr = fmt.data;
+	ptr[4096] = '\0';
+
+	/* we are allowed up to 4K including the terminator */
+	ut_asserteq(-E2BIG, abuf_printf(&buf, "%s", ptr));
+	ptr[4095] = '\0';
+	ut_asserteq(4095, abuf_printf(&buf, "%s", ptr));
+
+	abuf_uninit(&fmt);
+	abuf_uninit(&buf);
+
+	/* Check for memory leaks */
+	ut_assertok(ut_check_delta(start));
+
+	return 0;
+}
+LIB_TEST(lib_test_abuf_printf, 0);
-- 
2.43.0

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

[Simon Glass]

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(+)

diff --git a/test/lib/string.c b/test/lib/string.c
index 8d22f3fd68f..af84dd0715e 100644
--- a/test/lib/string.c
+++ b/test/lib/string.c
@@ -221,3 +221,34 @@ static int lib_memdup(struct unit_test_state *uts)
 	return 0;
 }
 LIB_TEST(lib_memdup, 0);
+
+static int lib_strim(struct unit_test_state *uts)
+{
+	char buf[BUFLEN];
+
+	strcpy(buf, "abc");
+	ut_asserteq_str("abc", strim(buf));
+
+	/* leading space */
+	strcpy(buf, " abc");
+	ut_asserteq_str("abc", strim(buf));
+
+	/* multiple leading spaces */
+	strcpy(buf, "  abc");
+	ut_asserteq_str("abc", strim(buf));
+
+	/* multiple internal spaces */
+	strcpy(buf, "  a   bc");
+	ut_asserteq_str("a   bc", strim(buf));
+
+	/* with trailing space */
+	strcpy(buf, "  a   bc ");
+	ut_asserteq_str("a   bc", strim(buf));
+
+	/* with multiple trailing spaces */
+	strcpy(buf, "  a   bc   ");
+	ut_asserteq_str("a   bc", strim(buf));
+
+	return 0;
+}
+LIB_TEST(lib_strim, 0);
-- 
2.43.0

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

[Tom Rini]

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

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\x7f 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

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

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

[Simon Glass]

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.

Regards,
Simon

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

[Tom Rini]

[-- 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 --]

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

[Simon Glass]

Hi Tom,

On Wed, 19 Mar 2025 at 16:38, Tom Rini <trini@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@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?

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

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

[Tom Rini]

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

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@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@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?
> 
> 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

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

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

[Simon Glass]

Hi Tom,

On Fri, 21 Mar 2025 at 03:15, Tom Rini <trini@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@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@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?
> >
> > 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

[PATCH 4/6] membuf: Add an easy way to set up a buffer with data

[Simon Glass]

In some cases it is useful to set up a buffer with some data to read,
e.g. when reading lines from a text file. Add a helper for this.

Tidy up the comment for membuf_init() while we are here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/membuf.h | 15 ++++++++++++++-
 lib/membuf.c     |  8 ++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/membuf.h b/include/membuf.h
index b495a72652b..f5c474cf730 100644
--- a/include/membuf.h
+++ b/include/membuf.h
@@ -222,7 +222,9 @@ int membuf_readline(struct membuf *mb, char *str, int maxlen, int minch,
 int membuf_extend_by(struct membuf *mb, int by, int max);
 
 /**
- * membuf_init() - set up a new membuff using an existing membuff
+ * membuf_init() - set up a new membuff using an existing buffer
+ *
+ * The buffer is initially empty
  *
  * @mb: membuff to set up
  * @buff: Address of buffer
@@ -230,6 +232,17 @@ int membuf_extend_by(struct membuf *mb, int by, int max);
  */
 void membuf_init(struct membuf *mb, char *buff, int size);
 
+/**
+ * membuf_init_with_data() - set up a new membuff using existing data
+ *
+ * The buffer is set up to contain the provided data
+ *
+ * @mb: membuff to set up
+ * @buff: Address of buffer
+ * @size: Size of buffer
+ */
+void membuf_init_with_data(struct membuf *mb, char *buff, int size);
+
 /**
  * membuf_uninit() - clear a membuff so it can no longer be used
  *
diff --git a/lib/membuf.c b/lib/membuf.c
index 016430ae988..347c92e3326 100644
--- a/lib/membuf.c
+++ b/lib/membuf.c
@@ -405,6 +405,14 @@ void membuf_init(struct membuf *mb, char *buff, int size)
 	membuf_purge(mb);
 }
 
+void membuf_init_with_data(struct membuf *mb, char *buff, int size)
+{
+	char *data;
+
+	membuf_init(mb, buff, size);
+	membuf_putraw(mb, size, true, &data);
+}
+
 int membuf_new(struct membuf *mb, int size)
 {
 	mb->start = malloc(size);
-- 
2.43.0

[PATCH 5/6] sandbox: Enable PHYS_64BIT for 64-bit builds

[Simon Glass]

Sandbox is special in that we use the bitness of the host. This should
extend to PHYS_64BIT as well, so enable this option when building on a
64-bit host.

Update the conditions in io.h so that 64-bit access is available.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 Kconfig                       | 1 +
 arch/sandbox/include/asm/io.h | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Kconfig b/Kconfig
index 2e63896c477..c80d6128260 100644
--- a/Kconfig
+++ b/Kconfig
@@ -448,6 +448,7 @@ endif # EXPERT
 config PHYS_64BIT
 	bool "64bit physical address support"
 	select FDT_64BIT
+	default y if HOST_64BIT
 	help
 	  Say Y here to support 64bit physical memory address.
 	  This can be used not only for 64bit SoCs, but also for
diff --git a/arch/sandbox/include/asm/io.h b/arch/sandbox/include/asm/io.h
index 3d09170063f..3c3545a2747 100644
--- a/arch/sandbox/include/asm/io.h
+++ b/arch/sandbox/include/asm/io.h
@@ -39,13 +39,13 @@ void sandbox_write(void *addr, unsigned int val, enum sandboxio_size_t size);
 #define readb(addr) sandbox_read((const void *)addr, SB_SIZE_8)
 #define readw(addr) sandbox_read((const void *)addr, SB_SIZE_16)
 #define readl(addr) sandbox_read((const void *)addr, SB_SIZE_32)
-#ifdef CONFIG_SANDBOX64
+#ifdef CONFIG_HOST_64BIT
 #define readq(addr) sandbox_read((const void *)addr, SB_SIZE_64)
 #endif
 #define writeb(v, addr) sandbox_write((void *)addr, v, SB_SIZE_8)
 #define writew(v, addr) sandbox_write((void *)addr, v, SB_SIZE_16)
 #define writel(v, addr) sandbox_write((void *)addr, v, SB_SIZE_32)
-#ifdef CONFIG_SANDBOX64
+#ifdef CONFIG_HOST_64BIT
 #define writeq(v, addr) sandbox_write((void *)addr, v, SB_SIZE_64)
 #endif
 
-- 
2.43.0

[PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Simon Glass]

In some cases we want to show a signed value to the user without needing
to use the full printf() implementation. Add a new version of the
simple_itoa() function to handle this.

Update the existing function to use ulong instead of long, to avoid
confusion about which it actually uses.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/vsprintf.h | 14 ++++++++++++++
 lib/vsprintf.c     | 29 +++++++++++++++++++++++++----
 test/lib/str.c     | 17 ++++++++++++++++-
 3 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/include/vsprintf.h b/include/vsprintf.h
index 9da6ce7cc4d..cd079684572 100644
--- a/include/vsprintf.h
+++ b/include/vsprintf.h
@@ -219,6 +219,20 @@ int vsprintf(char *buf, const char *fmt, va_list args);
  */
 char *simple_itoa(ulong val);
 
+/**
+ * simple_itoa_signed() - convert a signed integer to a string
+ *
+ * This returns a static string containing the decimal representation of the
+ * given value. The returned value may be overwritten by other calls to other
+ * simple... functions, so should be used immediately
+ *
+ * If the value is negative, a minus sign ('-') is prepended
+ *
+ * @val: Value to convert
+ * Return: string containing the signed decimal representation of @val
+ */
+char *simple_itoa_signed(long i);
+
 /**
  * simple_xtoa() - convert an unsigned integer to a hex string
  *
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c7340a047b2..478d445a569 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -845,21 +845,42 @@ int vprintf(const char *fmt, va_list args)
 }
 #endif
 
-static char local_toa[22];
+static char local_toa[23];
 
-char *simple_itoa(ulong i)
+char *simple_itoa_(ulong i, bool use_signed)
 {
-	/* 21 digits plus null terminator, good for 64-bit or smaller ints */
-	char *p = &local_toa[21];
+	/*
+	 * 21 digits plus sign and null terminator, good for 64-bit or smaller
+	 * ints
+	 */
+	char *p = &local_toa[22];
+	bool neg = false;
 
+	if (use_signed && (long)i < 0) {
+		i = -i;
+		neg = true;
+	}
 	*p-- = '\0';
 	do {
 		*p-- = '0' + i % 10;
 		i /= 10;
 	} while (i > 0);
+
+	if (neg)
+		*p-- = '-';
 	return p + 1;
 }
 
+char *simple_itoa(ulong i)
+{
+	return simple_itoa_(i, false);
+}
+
+char *simple_itoa_signed(long i)
+{
+	return simple_itoa_(i, true);
+}
+
 char *simple_xtoa(ulong num)
 {
 	/* 16 digits plus nul terminator, good for 64-bit or smaller ints */
diff --git a/test/lib/str.c b/test/lib/str.c
index e62045318c0..d6d29bbf3eb 100644
--- a/test/lib/str.c
+++ b/test/lib/str.c
@@ -221,14 +221,29 @@ static int str_itoa(struct unit_test_state *uts)
 	ut_asserteq_str("123", simple_itoa(123));
 	ut_asserteq_str("0", simple_itoa(0));
 	ut_asserteq_str("2147483647", simple_itoa(0x7fffffff));
-	ut_asserteq_str("4294967295", simple_itoa(0xffffffff));
+	ut_asserteq_str("2147483647", simple_itoa_signed(0x7fffffff));
+	ut_asserteq_str("2147483648", simple_itoa(0x80000000));
+	if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
+		ut_asserteq_str("2147483648", simple_itoa_signed(0x80000000));
+		ut_asserteq_str("4294967295", simple_itoa_signed(0xffffffff));
+	} else {
+		ut_asserteq_str("-2147483648", simple_itoa_signed(0x80000000));
+		ut_asserteq_str("-1", simple_itoa_signed(0xffffffff));
+	}
 
 	/* Use #ifdef here to avoid a compiler warning on 32-bit machines */
 #ifdef CONFIG_PHYS_64BIT
 	if (sizeof(ulong) == 8) {
 		ut_asserteq_str("9223372036854775807",
 				simple_itoa((1UL << 63) - 1));
+		ut_asserteq_str("9223372036854775807",
+				simple_itoa_signed((1UL << 63) - 1));
+		ut_asserteq_str("9223372036854775808",
+				simple_itoa((1UL << 63)));
+		ut_asserteq_str("-9223372036854775808",
+				simple_itoa_signed((1UL << 63)));
 		ut_asserteq_str("18446744073709551615", simple_itoa(-1));
+		ut_asserteq_str("-1", simple_itoa_signed(-1));
 	}
 #endif /* CONFIG_PHYS_64BIT */
 
-- 
2.43.0

Re: [PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Heinrich Schuchardt]

Am 19. März 2025 12:59:08 MEZ schrieb Simon Glass <sjg@chromium.org>:
>In some cases we want to show a signed value to the user without needing
>to use the full printf() implementation. Add a new version of the
>simple_itoa() function to handle this.

Where will this be used?
Why can't printf be used?
Why would this fit into this series which is about test improvements and not about functional changes?

Best regards

Heinrich 


>
>Update the existing function to use ulong instead of long, to avoid
>confusion about which it actually uses.
>
>Signed-off-by: Simon Glass <sjg@chromium.org>
>---
>
> include/vsprintf.h | 14 ++++++++++++++
> lib/vsprintf.c     | 29 +++++++++++++++++++++++++----
> test/lib/str.c     | 17 ++++++++++++++++-
> 3 files changed, 55 insertions(+), 5 deletions(-)
>
>diff --git a/include/vsprintf.h b/include/vsprintf.h
>index 9da6ce7cc4d..cd079684572 100644
>--- a/include/vsprintf.h
>+++ b/include/vsprintf.h
>@@ -219,6 +219,20 @@ int vsprintf(char *buf, const char *fmt, va_list args);
>  */
> char *simple_itoa(ulong val);
> 
>+/**
>+ * simple_itoa_signed() - convert a signed integer to a string
>+ *
>+ * This returns a static string containing the decimal representation of the
>+ * given value. The returned value may be overwritten by other calls to other
>+ * simple... functions, so should be used immediately
>+ *
>+ * If the value is negative, a minus sign ('-') is prepended
>+ *
>+ * @val: Value to convert
>+ * Return: string containing the signed decimal representation of @val
>+ */
>+char *simple_itoa_signed(long i);
>+
> /**
>  * simple_xtoa() - convert an unsigned integer to a hex string
>  *
>diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>index c7340a047b2..478d445a569 100644
>--- a/lib/vsprintf.c
>+++ b/lib/vsprintf.c
>@@ -845,21 +845,42 @@ int vprintf(const char *fmt, va_list args)
> }
> #endif
> 
>-static char local_toa[22];
>+static char local_toa[23];
> 
>-char *simple_itoa(ulong i)
>+char *simple_itoa_(ulong i, bool use_signed)
> {
>-	/* 21 digits plus null terminator, good for 64-bit or smaller ints */
>-	char *p = &local_toa[21];
>+	/*
>+	 * 21 digits plus sign and null terminator, good for 64-bit or smaller
>+	 * ints
>+	 */
>+	char *p = &local_toa[22];
>+	bool neg = false;
> 
>+	if (use_signed && (long)i < 0) {
>+		i = -i;
>+		neg = true;
>+	}
> 	*p-- = '\0';
> 	do {
> 		*p-- = '0' + i % 10;
> 		i /= 10;
> 	} while (i > 0);
>+
>+	if (neg)
>+		*p-- = '-';
> 	return p + 1;
> }
> 
>+char *simple_itoa(ulong i)
>+{
>+	return simple_itoa_(i, false);
>+}
>+
>+char *simple_itoa_signed(long i)
>+{
>+	return simple_itoa_(i, true);
>+}
>+
> char *simple_xtoa(ulong num)
> {
> 	/* 16 digits plus nul terminator, good for 64-bit or smaller ints */
>diff --git a/test/lib/str.c b/test/lib/str.c
>index e62045318c0..d6d29bbf3eb 100644
>--- a/test/lib/str.c
>+++ b/test/lib/str.c
>@@ -221,14 +221,29 @@ static int str_itoa(struct unit_test_state *uts)
> 	ut_asserteq_str("123", simple_itoa(123));
> 	ut_asserteq_str("0", simple_itoa(0));
> 	ut_asserteq_str("2147483647", simple_itoa(0x7fffffff));
>-	ut_asserteq_str("4294967295", simple_itoa(0xffffffff));
>+	ut_asserteq_str("2147483647", simple_itoa_signed(0x7fffffff));
>+	ut_asserteq_str("2147483648", simple_itoa(0x80000000));
>+	if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
>+		ut_asserteq_str("2147483648", simple_itoa_signed(0x80000000));
>+		ut_asserteq_str("4294967295", simple_itoa_signed(0xffffffff));
>+	} else {
>+		ut_asserteq_str("-2147483648", simple_itoa_signed(0x80000000));
>+		ut_asserteq_str("-1", simple_itoa_signed(0xffffffff));
>+	}
> 
> 	/* Use #ifdef here to avoid a compiler warning on 32-bit machines */
> #ifdef CONFIG_PHYS_64BIT
> 	if (sizeof(ulong) == 8) {
> 		ut_asserteq_str("9223372036854775807",
> 				simple_itoa((1UL << 63) - 1));
>+		ut_asserteq_str("9223372036854775807",
>+				simple_itoa_signed((1UL << 63) - 1));
>+		ut_asserteq_str("9223372036854775808",
>+				simple_itoa((1UL << 63)));
>+		ut_asserteq_str("-9223372036854775808",
>+				simple_itoa_signed((1UL << 63)));
> 		ut_asserteq_str("18446744073709551615", simple_itoa(-1));
>+		ut_asserteq_str("-1", simple_itoa_signed(-1));
> 	}
> #endif /* CONFIG_PHYS_64BIT */
> 

Re: [PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Tom Rini]

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

On Wed, Mar 19, 2025 at 01:12:30PM +0100, Heinrich Schuchardt wrote:
> Am 19. März 2025 12:59:08 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >In some cases we want to show a signed value to the user without needing
> >to use the full printf() implementation. Add a new version of the
> >simple_itoa() function to handle this.
> 
> Where will this be used?
> Why can't printf be used?
> Why would this fit into this series which is about test improvements and not about functional changes?

It would be more fair to say that only one patch in this series is about
testing and 5 are adding otherwise unused changes to functions that
presumably are used in some later as yet to be posted series.

-- 
Tom

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

Re: [PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Simon Glass]

Hi Tom,

On Wed, 19 Mar 2025 at 15:20, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 19, 2025 at 01:12:30PM +0100, Heinrich Schuchardt wrote:
> > Am 19. März 2025 12:59:08 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > >In some cases we want to show a signed value to the user without needing
> > >to use the full printf() implementation. Add a new version of the
> > >simple_itoa() function to handle this.
> >
> > Where will this be used?
> > Why can't printf be used?
> > Why would this fit into this series which is about test improvements and not about functional changes?
>
> It would be more fair to say that only one patch in this series is about
> testing and 5 are adding otherwise unused changes to functions that
> presumably are used in some later as yet to be posted series.

Sure, would you like me to change the cover letter?

Regards,
Simon

Re: [PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Tom Rini]

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

On Wed, Mar 19, 2025 at 03:04:00PM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 19 Mar 2025 at 15:20, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 01:12:30PM +0100, Heinrich Schuchardt wrote:
> > > Am 19. März 2025 12:59:08 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > > >In some cases we want to show a signed value to the user without needing
> > > >to use the full printf() implementation. Add a new version of the
> > > >simple_itoa() function to handle this.
> > >
> > > Where will this be used?
> > > Why can't printf be used?
> > > Why would this fit into this series which is about test improvements and not about functional changes?
> >
> > It would be more fair to say that only one patch in this series is about
> > testing and 5 are adding otherwise unused changes to functions that
> > presumably are used in some later as yet to be posted series.
> 
> Sure, would you like me to change the cover letter?

No, I'd like to see the test by itself and defer the unused additions
until there's something to review it alongside of.

-- 
Tom

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

Re: [PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Simon Glass]

Hi Tom,

On Wed, 19 Mar 2025 at 16:36, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 19, 2025 at 03:04:00PM +0000, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 19 Mar 2025 at 15:20, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Mar 19, 2025 at 01:12:30PM +0100, Heinrich Schuchardt wrote:
> > > > Am 19. März 2025 12:59:08 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > > > >In some cases we want to show a signed value to the user without needing
> > > > >to use the full printf() implementation. Add a new version of the
> > > > >simple_itoa() function to handle this.
> > > >
> > > > Where will this be used?
> > > > Why can't printf be used?
> > > > Why would this fit into this series which is about test improvements and not about functional changes?
> > >
> > > It would be more fair to say that only one patch in this series is about
> > > testing and 5 are adding otherwise unused changes to functions that
> > > presumably are used in some later as yet to be posted series.
> >
> > Sure, would you like me to change the cover letter?
>
> No, I'd like to see the test by itself and defer the unused additions
> until there's something to review it alongside of.

So you don't want simple_itoa_signed() or abuf_printf() until you see
the series which uses it? I'm struggling to square that with your
request to break things up into smaller chunks, though.

Regards,
Simon

Re: [PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Tom Rini]

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

On Thu, Mar 20, 2025 at 03:43:17AM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 19 Mar 2025 at 16:36, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 03:04:00PM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 19 Mar 2025 at 15:20, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Mar 19, 2025 at 01:12:30PM +0100, Heinrich Schuchardt wrote:
> > > > > Am 19. März 2025 12:59:08 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > > > > >In some cases we want to show a signed value to the user without needing
> > > > > >to use the full printf() implementation. Add a new version of the
> > > > > >simple_itoa() function to handle this.
> > > > >
> > > > > Where will this be used?
> > > > > Why can't printf be used?
> > > > > Why would this fit into this series which is about test improvements and not about functional changes?
> > > >
> > > > It would be more fair to say that only one patch in this series is about
> > > > testing and 5 are adding otherwise unused changes to functions that
> > > > presumably are used in some later as yet to be posted series.
> > >
> > > Sure, would you like me to change the cover letter?
> >
> > No, I'd like to see the test by itself and defer the unused additions
> > until there's something to review it alongside of.
> 
> So you don't want simple_itoa_signed() or abuf_printf() until you see
> the series which uses it? I'm struggling to square that with your
> request to break things up into smaller chunks, though.

I'm sorry you find this normal requirement difficult.

-- 
Tom

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

Re: [PATCH 6/6] lib: Provide a signed version of simple_itoa()

[Simon Glass]

Hi Heinrich,

On Wed, 19 Mar 2025 at 13:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 19. März 2025 12:59:08 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >In some cases we want to show a signed value to the user without needing
> >to use the full printf() implementation. Add a new version of the
> >simple_itoa() function to handle this.
>
> Where will this be used?

In a following series.

> Why can't printf be used?

It can, but it is clumsy to do that.

> Why would this fit into this series which is about test improvements and not about functional changes?

It seems to fit in here, since I added a test for the existing function.

[..]

Regards,
Simon