u-boot.lists.denx.de archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] cli: run_commandf() coverage and small fixups
@ 2023-03-20  8:23 Evgeny Bachinin
  2023-03-20  8:23 ` [PATCH v2 1/4] cli: run_commandf(): " Evgeny Bachinin
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Evgeny Bachinin @ 2023-03-20  8:23 UTC (permalink / raw)
  To: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping
  Cc: u-boot, kernel, evgen89bachinin, Evgeny Bachinin

Our company has been using custom variadic version of run_command()
since the beginning of the 2022Y. Recently, we had started upstreaming
activity and figured out, that similar functionality already exists.
Thanks, it's very helpful, because our code uses such variadic API.

Let me share test-cases for run_commandf() to improve coverage and
btw provide several fixups for run_commandf(), found during applying
this API to our custom sources.

Patchset has been tested on our set of devices and sandbox64
(appropriate unit-tests were checked: ut_cmd, ut fdt, ut exit).
Additionally, github CI loop has been passed successfully without
regression in context of github PR [1].

Changes v2 since v1 at [2]:
- rebase
- run_commandf: apply comments
- command_ut: re-write truncation test-case
- test/cmd/fdt.c: fix compilation after rebase

Links:
[1] https://github.com/u-boot/u-boot/pull/277/checks
[2] https://lore.kernel.org/u-boot/20230310185409.22254-1-EABachinin@sberdevices.ru/

Evgeny Bachinin (4):
  cli: run_commandf(): small fixups
  unit-test: cover run_commandf() by test-cases
  test: fdt: fix run_commandf() warnings
  test: exit: fix run_commandf() warnings

 common/cli.c      | 25 +++++++++++++++++++------
 include/command.h | 13 ++++++++++---
 test/cmd/exit.c   | 18 +++++++++---------
 test/cmd/fdt.c    | 16 ++++++++--------
 test/command_ut.c | 34 ++++++++++++++++++++++++++++++++++
 5 files changed, 80 insertions(+), 26 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/4] cli: run_commandf(): small fixups
  2023-03-20  8:23 [PATCH v2 0/4] cli: run_commandf() coverage and small fixups Evgeny Bachinin
@ 2023-03-20  8:23 ` Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  2023-03-20  8:23 ` [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases Evgeny Bachinin
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Evgeny Bachinin @ 2023-03-20  8:23 UTC (permalink / raw)
  To: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping
  Cc: u-boot, kernel, evgen89bachinin, Evgeny Bachinin

* vsnprintf() can truncate cmd, hence it makes no sense to launch such
command (it's broken). Moreover, it's better to signalize to the caller
about such case (for facilitating debugging or bug hunting).

* Fix kernel-doc warnings:
  include/command.h:264: info: Scanning doc for run_commandf
  include/command.h:268: warning: contents before sections
  include/command.h:271: warning: No description found for return value
                                  of 'run_commandf'

* Add printf-like format attribute to validate at compile-time the format
string against parameters's type.

* Fix compilation error in case of -Wall, -Werror, -Wextra:
error: variable ‘i’ set but not used [-Werror=unused-but-set-variable]

* Drop extra ret variable.

Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
---
Changes for v2:
- s/pr_err/pr_debug/ to reduce code size for rare errors
- s/EINVAL/ENOSPC/
- replace on-stack buffer with global console_buffer[]
- use not full (CONFIG_SYS_CBSIZE + 1) space of console_buffer, because
interpreters return no more than CONFIG_SYS_CBSIZE (including \0),
hence we use CONFIG_SYS_CBSIZE as a max command size for run_commandf()
- not apply Reviewed-by due to changes above

 common/cli.c      | 25 +++++++++++++++++++------
 include/command.h | 13 ++++++++++---
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/common/cli.c b/common/cli.c
index 9451e6a142..3916a7b10a 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -8,6 +8,8 @@
  * JinHua Luo, GuangDong Linux Center, <luo.jinhua@gd-linux.com>
  */
 
+#define pr_fmt(fmt) "cli: %s: " fmt, __func__
+
 #include <common.h>
 #include <bootstage.h>
 #include <cli.h>
@@ -20,6 +22,7 @@
 #include <malloc.h>
 #include <asm/global_data.h>
 #include <dm/ofnode.h>
+#include <linux/errno.h>
 
 #ifdef CONFIG_CMDLINE
 /*
@@ -129,16 +132,26 @@ int run_command_list(const char *cmd, int len, int flag)
 int run_commandf(const char *fmt, ...)
 {
 	va_list args;
-	char cmd[128];
-	int i, ret;
+	int nbytes;
 
 	va_start(args, fmt);
-	i = vsnprintf(cmd, sizeof(cmd), fmt, args);
+	/*
+	 * Limit the console_buffer space being used to CONFIG_SYS_CBSIZE,
+	 * because its last byte is used to fit the replacement of \0 by \n\0
+	 * in underlying hush parser
+	 */
+	nbytes = vsnprintf(console_buffer, CONFIG_SYS_CBSIZE, fmt, args);
 	va_end(args);
 
-	ret = run_command(cmd, 0);
-
-	return ret;
+	if (nbytes < 0) {
+		pr_debug("I/O internal error occurred.\n");
+		return -EIO;
+	} else if (nbytes >= CONFIG_SYS_CBSIZE) {
+		pr_debug("'fmt' size:%d exceeds the limit(%d)\n",
+			 nbytes, CONFIG_SYS_CBSIZE);
+		return -ENOSPC;
+	}
+	return run_command(console_buffer, 0);
 }
 
 /****************************************************************************/
diff --git a/include/command.h b/include/command.h
index 0db4898062..0e153c6046 100644
--- a/include/command.h
+++ b/include/command.h
@@ -13,6 +13,8 @@
 #include <env.h>
 #include <linker_lists.h>
 
+#include <linux/compiler_attributes.h>
+
 #ifndef NULL
 #define NULL	0
 #endif
@@ -260,12 +262,17 @@ int run_command_repeatable(const char *cmd, int flag);
 /**
  * run_commandf() - Run a command created by a format string
  *
- * The command cannot be larger than 127 characters
- *
  * @fmt: printf() format string
  * @...: Arguments to use (flag is always 0)
+ *
+ * The command cannot be larger than (CONFIG_SYS_CBSIZE - 1) characters.
+ *
+ * Return:
+ * Returns 0 on success, -EIO if internal output error occurred, -ENOSPC in
+ *	case of 'fmt' string truncation, or != 0 on error, specific for
+ *	run_command().
  */
-int run_commandf(const char *fmt, ...);
+int run_commandf(const char *fmt, ...) __printf(1, 2);
 
 /**
  * Run a list of commands separated by ; or even \0
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases
  2023-03-20  8:23 [PATCH v2 0/4] cli: run_commandf() coverage and small fixups Evgeny Bachinin
  2023-03-20  8:23 ` [PATCH v2 1/4] cli: run_commandf(): " Evgeny Bachinin
@ 2023-03-20  8:23 ` Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  2023-03-20  8:23 ` [PATCH v2 3/4] test: fdt: fix run_commandf() warnings Evgeny Bachinin
  2023-03-20  8:23 ` [PATCH v2 4/4] test: exit: " Evgeny Bachinin
  3 siblings, 2 replies; 13+ messages in thread
From: Evgeny Bachinin @ 2023-03-20  8:23 UTC (permalink / raw)
  To: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping
  Cc: u-boot, kernel, evgen89bachinin, Evgeny Bachinin

As run_commandf() is variadic version of run_command() and just a wrapper,
hence apply similar run_command's test-cases.

Let's avoid warning about empty string passing:
warning: zero-length gnu_printf format string [-Wformat-zero-length]
   assert(run_commandf("") == 0);

Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
---
Changes for v2:
- s/EINVAL/ENOSPC/
- rewrite test-case for truncation case
- not apply Reviewed-by due to changes above

 test/command_ut.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/test/command_ut.c b/test/command_ut.c
index 9837d10eb5..a74bd109e1 100644
--- a/test/command_ut.c
+++ b/test/command_ut.c
@@ -9,6 +9,8 @@
 #include <command.h>
 #include <env.h>
 #include <log.h>
+#include <string.h>
+#include <linux/errno.h>
 
 static const char test_cmd[] = "setenv list 1\n setenv list ${list}2; "
 		"setenv list ${list}3\0"
@@ -17,6 +19,8 @@ static const char test_cmd[] = "setenv list 1\n setenv list ${list}2; "
 static int do_ut_cmd(struct cmd_tbl *cmdtp, int flag, int argc,
 		     char *const argv[])
 {
+	char long_str[CONFIG_SYS_CBSIZE + 42];
+
 	printf("%s: Testing commands\n", __func__);
 	run_command("env default -f -a", 0);
 
@@ -60,6 +64,36 @@ static int do_ut_cmd(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	assert(run_command("'", 0) == 1);
 
+	/* Variadic function test-cases */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-zero-length"
+	assert(run_commandf("") == 0);
+#pragma GCC diagnostic pop
+	assert(run_commandf(" ") == 0);
+	assert(run_commandf("'") == 1);
+
+	assert(run_commandf("env %s %s", "delete -f", "list") == 0);
+	/* Expected: "Error: "list" not defined" */
+	assert(run_commandf("printenv list") == 1);
+
+	memset(long_str, 'x', sizeof(long_str));
+	assert(run_commandf("Truncation case: %s", long_str) == -ENOSPC);
+
+	if (IS_ENABLED(CONFIG_HUSH_PARSER)) {
+		assert(run_commandf("env %s %s %s %s", "delete -f", "adder",
+				    "black", "foo") == 0);
+		assert(run_commandf("setenv foo 'setenv %s 1\nsetenv %s 2'",
+				    "black", "adder") == 0);
+		run_command("run foo", 0);
+		assert(env_get("black"));
+		assert(!strcmp("1", env_get("black")));
+		assert(env_get("adder"));
+		assert(!strcmp("2", env_get("adder")));
+	}
+
+	/* Clean up before exit */
+	run_command("env default -f -a", 0);
+
 	printf("%s: Everything went swimmingly\n", __func__);
 	return 0;
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] test: fdt: fix run_commandf() warnings
  2023-03-20  8:23 [PATCH v2 0/4] cli: run_commandf() coverage and small fixups Evgeny Bachinin
  2023-03-20  8:23 ` [PATCH v2 1/4] cli: run_commandf(): " Evgeny Bachinin
  2023-03-20  8:23 ` [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases Evgeny Bachinin
@ 2023-03-20  8:23 ` Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  2023-03-20  8:23 ` [PATCH v2 4/4] test: exit: " Evgeny Bachinin
  3 siblings, 2 replies; 13+ messages in thread
From: Evgeny Bachinin @ 2023-03-20  8:23 UTC (permalink / raw)
  To: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping
  Cc: u-boot, kernel, evgen89bachinin, Evgeny Bachinin, Marek Vasut

Fix warnings both for 32bit and 64bit architecture after adding
printf-like attribute format for run_commandf():
warning: format ‘%x’ expects argument of type ‘unsigned int’, but
  argument 2 has type ‘ulong {aka long unsigned int}’ [-Wformat=]
  ret = run_commandf("fdt addr -c %08x", addr);
                     ^
Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
Cc: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Changes for v2:
- fdt_test_move(): due to addition of __printf(), fix compilation
after rebase atop 79bcd809f49 ("test: cmd: fdt: Test fdt move")
- not apply Reviewed-by due to changes above

 test/cmd/fdt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/test/cmd/fdt.c b/test/cmd/fdt.c
index cdbaf8c425..538a30b909 100644
--- a/test/cmd/fdt.c
+++ b/test/cmd/fdt.c
@@ -168,7 +168,7 @@ static int fdt_test_addr(struct unit_test_state *uts)
 	/* Set the working FDT */
 	set_working_fdt_addr(0);
 	ut_assert_nextline("Working FDT set to 0");
-	ut_assertok(run_commandf("fdt addr %08x", addr));
+	ut_assertok(run_commandf("fdt addr %08lx", addr));
 	ut_assert_nextline("Working FDT set to %lx", addr);
 	ut_asserteq(addr, map_to_sysmem(working_fdt));
 	ut_assertok(ut_check_console_end(uts));
@@ -178,7 +178,7 @@ static int fdt_test_addr(struct unit_test_state *uts)
 	/* Set the control FDT */
 	fdt_blob = gd->fdt_blob;
 	gd->fdt_blob = NULL;
-	ret = run_commandf("fdt addr -c %08x", addr);
+	ret = run_commandf("fdt addr -c %08lx", addr);
 	new_fdt = gd->fdt_blob;
 	gd->fdt_blob = fdt_blob;
 	ut_assertok(ret);
@@ -187,7 +187,7 @@ static int fdt_test_addr(struct unit_test_state *uts)
 
 	/* Test setting an invalid FDT */
 	fdt[0] = 123;
-	ut_asserteq(1, run_commandf("fdt addr %08x", addr));
+	ut_asserteq(1, run_commandf("fdt addr %08lx", addr));
 	ut_assert_nextline("libfdt fdt_check_header(): FDT_ERR_BADMAGIC");
 	ut_assertok(ut_check_console_end(uts));
 
@@ -216,19 +216,19 @@ static int fdt_test_addr_resize(struct unit_test_state *uts)
 
 	/* Test setting and resizing the working FDT to a larger size */
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("fdt addr %08x %x", addr, newsize));
+	ut_assertok(run_commandf("fdt addr %08lx %x", addr, newsize));
 	ut_assert_nextline("Working FDT set to %lx", addr);
 	ut_assertok(ut_check_console_end(uts));
 
 	/* Try shrinking it */
-	ut_assertok(run_commandf("fdt addr %08x %x", addr, sizeof(fdt) / 4));
+	ut_assertok(run_commandf("fdt addr %08lx %zx", addr, sizeof(fdt) / 4));
 	ut_assert_nextline("Working FDT set to %lx", addr);
 	ut_assert_nextline("New length %d < existing length %d, ignoring",
 			   (int)sizeof(fdt) / 4, newsize);
 	ut_assertok(ut_check_console_end(uts));
 
 	/* ...quietly */
-	ut_assertok(run_commandf("fdt addr -q %08x %x", addr, sizeof(fdt) / 4));
+	ut_assertok(run_commandf("fdt addr -q %08lx %zx", addr, sizeof(fdt) / 4));
 	ut_assert_nextline("Working FDT set to %lx", addr);
 	ut_assertok(ut_check_console_end(uts));
 
@@ -258,13 +258,13 @@ static int fdt_test_move(struct unit_test_state *uts)
 
 	/* Test moving the working FDT to a new location */
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("fdt move %08x %08x %x", addr, newaddr, ts));
+	ut_assertok(run_commandf("fdt move %08lx %08lx %x", addr, newaddr, ts));
 	ut_assert_nextline("Working FDT set to %lx", newaddr);
 	ut_assertok(ut_check_console_end(uts));
 
 	/* Compare the source and destination DTs */
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("cmp.b %08x %08x %x", addr, newaddr, ts));
+	ut_assertok(run_commandf("cmp.b %08lx %08lx %x", addr, newaddr, ts));
 	ut_assert_nextline("Total of %d byte(s) were the same", ts);
 	ut_assertok(ut_check_console_end(uts));
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] test: exit: fix run_commandf() warnings
  2023-03-20  8:23 [PATCH v2 0/4] cli: run_commandf() coverage and small fixups Evgeny Bachinin
                   ` (2 preceding siblings ...)
  2023-03-20  8:23 ` [PATCH v2 3/4] test: fdt: fix run_commandf() warnings Evgeny Bachinin
@ 2023-03-20  8:23 ` Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  3 siblings, 2 replies; 13+ messages in thread
From: Evgeny Bachinin @ 2023-03-20  8:23 UTC (permalink / raw)
  To: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping
  Cc: u-boot, kernel, evgen89bachinin, Evgeny Bachinin

Fix warnings after adding printf-like attribute format for
run_commandf():
warning: too many arguments for format [-Wformat-extra-args]

Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 test/cmd/exit.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/test/cmd/exit.c b/test/cmd/exit.c
index ca34abef89..7e160f7e4b 100644
--- a/test/cmd/exit.c
+++ b/test/cmd/exit.c
@@ -60,20 +60,20 @@ static int cmd_exit_test(struct unit_test_state *uts)
 
 	/* Validate that 'exit' behaves the same way as 'exit 0' */
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo ; echo $?"));
 	ut_assert_nextline("bar");
 	ut_assert_nextline("0");
 	ut_assertok(ut_check_console_end(uts));
 
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo && echo quux ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo && echo quux ; echo $?"));
 	ut_assert_nextline("bar");
 	ut_assert_nextline("quux");
 	ut_assert_nextline("0");
 	ut_assertok(ut_check_console_end(uts));
 
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo || echo quux ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo || echo quux ; echo $?"));
 	ut_assert_nextline("bar");
 	/* Either 'exit' returns 0, or 'echo quux' returns 0 */
 	ut_assert_nextline("0");
@@ -81,39 +81,39 @@ static int cmd_exit_test(struct unit_test_state *uts)
 
 	/* Validate that return value still propagates from 'run' command */
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo ; echo $?"));
 	ut_assert_nextline("bar");
 	ut_assert_nextline("0");
 	ut_assertok(ut_check_console_end(uts));
 
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo && echo quux ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo && echo quux ; echo $?"));
 	ut_assert_nextline("bar");
 	ut_assert_nextline("quux");
 	ut_assert_nextline("0");
 	ut_assertok(ut_check_console_end(uts));
 
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo || echo quux ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo || echo quux ; echo $?"));
 	ut_assert_nextline("bar");
 	/* The 'true' returns 0 */
 	ut_assert_nextline("0");
 	ut_assertok(ut_check_console_end(uts));
 
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo ; echo $?"));
 	ut_assert_nextline("bar");
 	ut_assert_nextline("1");
 	ut_assertok(ut_check_console_end(uts));
 
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo && echo quux ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo && echo quux ; echo $?"));
 	ut_assert_nextline("bar");
 	ut_assert_nextline("1");
 	ut_assertok(ut_check_console_end(uts));
 
 	ut_assertok(console_record_reset_enable());
-	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo || echo quux ; echo $?", i));
+	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo || echo quux ; echo $?"));
 	ut_assert_nextline("bar");
 	ut_assert_nextline("quux");
 	/* The 'echo quux' returns 0 */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] cli: run_commandf(): small fixups
  2023-03-20  8:23 ` [PATCH v2 1/4] cli: run_commandf(): " Evgeny Bachinin
@ 2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Simon Glass @ 2023-03-20 18:40 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Hector Palacios, Marek Vasut, Heinrich Schuchardt, John Keeping,
	u-boot, kernel, evgen89bachinin

On Mon, 20 Mar 2023 at 21:23, Evgeny Bachinin <EABachinin@sberdevices.ru> wrote:
>
> * vsnprintf() can truncate cmd, hence it makes no sense to launch such
> command (it's broken). Moreover, it's better to signalize to the caller
> about such case (for facilitating debugging or bug hunting).
>
> * Fix kernel-doc warnings:
>   include/command.h:264: info: Scanning doc for run_commandf
>   include/command.h:268: warning: contents before sections
>   include/command.h:271: warning: No description found for return value
>                                   of 'run_commandf'
>
> * Add printf-like format attribute to validate at compile-time the format
> string against parameters's type.
>
> * Fix compilation error in case of -Wall, -Werror, -Wextra:
> error: variable ‘i’ set but not used [-Werror=unused-but-set-variable]
>
> * Drop extra ret variable.
>
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> ---
> Changes for v2:
> - s/pr_err/pr_debug/ to reduce code size for rare errors
> - s/EINVAL/ENOSPC/
> - replace on-stack buffer with global console_buffer[]
> - use not full (CONFIG_SYS_CBSIZE + 1) space of console_buffer, because
> interpreters return no more than CONFIG_SYS_CBSIZE (including \0),
> hence we use CONFIG_SYS_CBSIZE as a max command size for run_commandf()
> - not apply Reviewed-by due to changes above
>
>  common/cli.c      | 25 +++++++++++++++++++------
>  include/command.h | 13 ++++++++++---
>  2 files changed, 29 insertions(+), 9 deletions(-)
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] test: fdt: fix run_commandf() warnings
  2023-03-20  8:23 ` [PATCH v2 3/4] test: fdt: fix run_commandf() warnings Evgeny Bachinin
@ 2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Simon Glass @ 2023-03-20 18:40 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Hector Palacios, Marek Vasut, Heinrich Schuchardt, John Keeping,
	u-boot, kernel, evgen89bachinin, Marek Vasut

On Mon, 20 Mar 2023 at 21:23, Evgeny Bachinin <EABachinin@sberdevices.ru> wrote:
>
> Fix warnings both for 32bit and 64bit architecture after adding
> printf-like attribute format for run_commandf():
> warning: format ‘%x’ expects argument of type ‘unsigned int’, but
>   argument 2 has type ‘ulong {aka long unsigned int}’ [-Wformat=]
>   ret = run_commandf("fdt addr -c %08x", addr);
>                      ^
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> Cc: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Changes for v2:
> - fdt_test_move(): due to addition of __printf(), fix compilation
> after rebase atop 79bcd809f49 ("test: cmd: fdt: Test fdt move")
> - not apply Reviewed-by due to changes above
>
>  test/cmd/fdt.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] test: exit: fix run_commandf() warnings
  2023-03-20  8:23 ` [PATCH v2 4/4] test: exit: " Evgeny Bachinin
@ 2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Simon Glass @ 2023-03-20 18:40 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Hector Palacios, Marek Vasut, Heinrich Schuchardt, John Keeping,
	u-boot, kernel, evgen89bachinin

On Mon, 20 Mar 2023 at 21:23, Evgeny Bachinin <EABachinin@sberdevices.ru> wrote:
>
> Fix warnings after adding printf-like attribute format for
> run_commandf():
> warning: too many arguments for format [-Wformat-extra-args]
>
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  test/cmd/exit.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases
  2023-03-20  8:23 ` [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases Evgeny Bachinin
@ 2023-03-20 18:40   ` Simon Glass
  2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Simon Glass @ 2023-03-20 18:40 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Hector Palacios, Marek Vasut, Heinrich Schuchardt, John Keeping,
	u-boot, kernel, evgen89bachinin

On Mon, 20 Mar 2023 at 21:23, Evgeny Bachinin <EABachinin@sberdevices.ru> wrote:
>
> As run_commandf() is variadic version of run_command() and just a wrapper,
> hence apply similar run_command's test-cases.
>
> Let's avoid warning about empty string passing:
> warning: zero-length gnu_printf format string [-Wformat-zero-length]
>    assert(run_commandf("") == 0);
>
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> ---
> Changes for v2:
> - s/EINVAL/ENOSPC/
> - rewrite test-case for truncation case
> - not apply Reviewed-by due to changes above
>
>  test/command_ut.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] cli: run_commandf(): small fixups
  2023-03-20  8:23 ` [PATCH v2 1/4] cli: run_commandf(): " Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
@ 2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Tom Rini @ 2023-03-31 14:15 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping, u-boot, kernel, evgen89bachinin

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

On Mon, Mar 20, 2023 at 11:23:11AM +0300, Evgeny Bachinin wrote:

> * vsnprintf() can truncate cmd, hence it makes no sense to launch such
> command (it's broken). Moreover, it's better to signalize to the caller
> about such case (for facilitating debugging or bug hunting).
> 
> * Fix kernel-doc warnings:
>   include/command.h:264: info: Scanning doc for run_commandf
>   include/command.h:268: warning: contents before sections
>   include/command.h:271: warning: No description found for return value
>                                   of 'run_commandf'
> 
> * Add printf-like format attribute to validate at compile-time the format
> string against parameters's type.
> 
> * Fix compilation error in case of -Wall, -Werror, -Wextra:
> error: variable ‘i’ set but not used [-Werror=unused-but-set-variable]
> 
> * Drop extra ret variable.
> 
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases
  2023-03-20  8:23 ` [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
@ 2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Tom Rini @ 2023-03-31 14:15 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping, u-boot, kernel, evgen89bachinin

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

On Mon, Mar 20, 2023 at 11:23:12AM +0300, Evgeny Bachinin wrote:

> As run_commandf() is variadic version of run_command() and just a wrapper,
> hence apply similar run_command's test-cases.
> 
> Let's avoid warning about empty string passing:
> warning: zero-length gnu_printf format string [-Wformat-zero-length]
>    assert(run_commandf("") == 0);
> 
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] test: fdt: fix run_commandf() warnings
  2023-03-20  8:23 ` [PATCH v2 3/4] test: fdt: fix run_commandf() warnings Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
@ 2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Tom Rini @ 2023-03-31 14:15 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping, u-boot, kernel, evgen89bachinin, Marek Vasut

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

On Mon, Mar 20, 2023 at 11:23:13AM +0300, Evgeny Bachinin wrote:

> Fix warnings both for 32bit and 64bit architecture after adding
> printf-like attribute format for run_commandf():
> warning: format ‘%x’ expects argument of type ‘unsigned int’, but
>   argument 2 has type ‘ulong {aka long unsigned int}’ [-Wformat=]
>   ret = run_commandf("fdt addr -c %08x", addr);
>                      ^
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> Cc: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] test: exit: fix run_commandf() warnings
  2023-03-20  8:23 ` [PATCH v2 4/4] test: exit: " Evgeny Bachinin
  2023-03-20 18:40   ` Simon Glass
@ 2023-03-31 14:15   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Tom Rini @ 2023-03-31 14:15 UTC (permalink / raw)
  To: Evgeny Bachinin
  Cc: Simon Glass, Hector Palacios, Marek Vasut, Heinrich Schuchardt,
	John Keeping, u-boot, kernel, evgen89bachinin

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

On Mon, Mar 20, 2023 at 11:23:14AM +0300, Evgeny Bachinin wrote:

> Fix warnings after adding printf-like attribute format for
> run_commandf():
> warning: too many arguments for format [-Wformat-extra-args]
> 
> Signed-off-by: Evgeny Bachinin <EABachinin@sberdevices.ru>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-03-31 14:19 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20  8:23 [PATCH v2 0/4] cli: run_commandf() coverage and small fixups Evgeny Bachinin
2023-03-20  8:23 ` [PATCH v2 1/4] cli: run_commandf(): " Evgeny Bachinin
2023-03-20 18:40   ` Simon Glass
2023-03-31 14:15   ` Tom Rini
2023-03-20  8:23 ` [PATCH v2 2/4] unit-test: cover run_commandf() by test-cases Evgeny Bachinin
2023-03-20 18:40   ` Simon Glass
2023-03-31 14:15   ` Tom Rini
2023-03-20  8:23 ` [PATCH v2 3/4] test: fdt: fix run_commandf() warnings Evgeny Bachinin
2023-03-20 18:40   ` Simon Glass
2023-03-31 14:15   ` Tom Rini
2023-03-20  8:23 ` [PATCH v2 4/4] test: exit: " Evgeny Bachinin
2023-03-20 18:40   ` Simon Glass
2023-03-31 14:15   ` Tom Rini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).