[PATCH v1 1/4] cli: run_commandf(): small fixups

[Simon Glass]

Hi Evgeny,

On Fri, 10 Mar 2023 at 10:55, Evgeny Bachinin <EABachinin at 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 at sberdevices.ru>
> ---
>  common/cli.c      | 19 ++++++++++++++-----
>  include/command.h | 13 ++++++++++---
>  2 files changed, 24 insertions(+), 8 deletions(-)

Reviewed-by: Simon Glass <sjg at 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.jinhua at 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
>