Re: [PATCH 1/1] cmd: fix tftpput command

Hi Heinrich,

On Sat, 3 Sept 2022 at 06:21, Heinrich Schuchardt xypron.glpk@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@gmx.de

cmd/net.c | 50 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass sjg@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