[PATCH 1/1] cmd: fix tftpput command

[Simon Glass]

Hi Heinrich,

On Sat, 3 Sept 2022 at 06:21, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Calling tftpput with less than 2 arguments must lead to a failure.
>
> If tftpput is called with two arguments, these are the address and
> the size of the file to be transferred.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
>  cmd/net.c | 50 +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 13 deletions(-)

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

>
> diff --git a/cmd/net.c b/cmd/net.c
> index 3619c843d8..e1be7b431a 100644
> --- a/cmd/net.c
> +++ b/cmd/net.c
> @@ -189,6 +189,19 @@ static void netboot_update_env(void)
>  #endif
>  }
>
> +/**
> + * parse_addr_size() - parse address and size arguments
> + */
> +static int parse_addr_size(char * const argv[])
> +{
> +       if (strict_strtoul(argv[1], 16, &image_save_addr) < 0 ||
> +           strict_strtoul(argv[2], 16, &image_save_size) < 0) {
> +               printf("Invalid address/size\n");
> +               return CMD_RET_USAGE;
> +       }
> +       return 0;
> +}
> +
>  static int netboot_common(enum proto_t proto, struct cmd_tbl *cmdtp, int argc,
>                           char *const argv[])
>  {
> @@ -199,6 +212,7 @@ static int netboot_common(enum proto_t proto, struct cmd_tbl *cmdtp, int argc,
>         ulong addr;
>
>         net_boot_file_name_explicit = false;
> +       *net_boot_file_name = '\0';
>
>         /* pre-set image_load_addr */
>         s = env_get("loadaddr");
> @@ -207,12 +221,18 @@ static int netboot_common(enum proto_t proto, struct cmd_tbl *cmdtp, int argc,
>
>         switch (argc) {
>         case 1:
> +               if (CONFIG_IS_ENABLED(CMD_TFTPPUT) && proto == TFTPPUT)
> +                       goto err_usage;
> +
>                 /* refresh bootfile name from env */
>                 copy_filename(net_boot_file_name, env_get("bootfile"),
>                               sizeof(net_boot_file_name));
>                 break;
>
> -       case 2: /*
> +       case 2:
> +               if (CONFIG_IS_ENABLED(CMD_TFTPPUT) && proto == TFTPPUT)
> +                       goto err_usage;
> +               /*
>                  * Only one arg - accept two forms:
>                  * Just load address, or just boot file name. The latter
>                  * form must be written in a format which can not be
> @@ -232,28 +252,28 @@ static int netboot_common(enum proto_t proto, struct cmd_tbl *cmdtp, int argc,
>                 break;
>
>         case 3:
> -               image_load_addr = hextoul(argv[1], NULL);
> -               net_boot_file_name_explicit = true;
> -               copy_filename(net_boot_file_name, argv[2],
> -                             sizeof(net_boot_file_name));
> -
> +               if (CONFIG_IS_ENABLED(CMD_TFTPPUT) && proto == TFTPPUT) {
> +                       if (parse_addr_size(argv))
> +                               goto err_usage;
> +               } else {
> +                       image_load_addr = hextoul(argv[1], NULL);
> +                       net_boot_file_name_explicit = true;
> +                       copy_filename(net_boot_file_name, argv[2],
> +                                     sizeof(net_boot_file_name));
> +               }
>                 break;
>
>  #ifdef CONFIG_CMD_TFTPPUT
>         case 4:
> -               if (strict_strtoul(argv[1], 16, &image_save_addr) < 0 ||
> -                   strict_strtoul(argv[2], 16, &image_save_size) < 0) {
> -                       printf("Invalid address/size\n");
> -                       return CMD_RET_USAGE;
> -               }
> +               if (parse_addr_size(argv))
> +                       goto err_usage;
>                 net_boot_file_name_explicit = true;
>                 copy_filename(net_boot_file_name, argv[3],
>                               sizeof(net_boot_file_name));
>                 break;
>  #endif
>         default:
> -               bootstage_error(BOOTSTAGE_ID_NET_START);
> -               return CMD_RET_USAGE;
> +               goto err_usage;
>         }
>         bootstage_mark(BOOTSTAGE_ID_NET_START);
>
> @@ -282,6 +302,10 @@ static int netboot_common(enum proto_t proto, struct cmd_tbl *cmdtp, int argc,
>         else
>                 bootstage_error(BOOTSTAGE_ID_NET_DONE_ERR);
>         return rcode;
> +
> +err_usage:
> +       bootstage_error(BOOTSTAGE_ID_NET_START);
> +       return CMD_RET_USAGE;
>  }
>
>  #if defined(CONFIG_CMD_PING)
> --
> 2.30.2
>

To me this would be better if the arg parsing all moved to a separate
function which returns an error code. It would avoid the goto.

Also perhaps this should be in the same series as the tftpput docs?

Regards,
Simon