Hi Evgeny,
On Fri, 10 Mar 2023 at 10:55, Evgeny Bachinin <eabachi...@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 <eabachi...@sberdevices.ru>
> ---
> common/cli.c | 19 ++++++++++++++-----
> include/command.h | 13 ++++++++++---
> 2 files changed, 24 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass <s...@chromium.org>
People are more used to using CONFIG_SYS_CBSIZE for the max cmdline
size, but I suppose this is fine. You could perhaps do a follow-on to
adjust this to use console_buffer[] ?
nits below
>
> diff --git a/common/cli.c b/common/cli.c
> index 9451e6a142..3db4e0f0b7 100644
> --- a/common/cli.c
> +++ b/common/cli.c
> @@ -8,6 +8,8 @@
> * JinHua Luo, GuangDong Linux Center, <luo.jin...@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
> /*
> @@ -130,15 +133,21 @@ 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);
> + nbytes = vsnprintf(cmd, sizeof(cmd), fmt, args);
> va_end(args);
>
> - ret = run_command(cmd, 0);
> -
> - return ret;
> + if (nbytes < 0) {
> + pr_err("I/O internal error occurred.\n");
> + return -EIO;
This can't really happen, so to reduce code size, how about pr_debug() ?
> + } else if ((size_t)nbytes >= sizeof(cmd)) {
> + pr_err("'fmt' size:%d exceeds the limit(%zu)\n",
> + nbytes, sizeof(cmd));
> + return -EINVAL;
-ENOSPC perhaps? Then the caller can handle the message and this can
become pr_debug() as well.
> + }
> + return run_command(cmd, 0);
> }
>
>
> /****************************************************************************/
> diff --git a/include/command.h b/include/command.h
> index 0db4898062..6c0c97054c 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 127 characters
> + *
> + * Return:
> + * Returns 0 on success, -EIO if internal output error occurred, -EINVAL 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
>
