Re: [PATCH 10/15] cmd: blk_common: Use macros for the return values

Hi Simon,

On Mon, Oct 2, 2023 at 9:42 AM Simon Glass sjg@chromium.org wrote:

Hi Bin,

On Tue, 26 Sept 2023 at 02:54, Bin Meng bmeng@tinylab.org wrote:

...

Avoid using magic number 0/1 for the command result.

Signed-off-by: Bin Meng bmeng@tinylab.org

cmd/blk_common.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/cmd/blk_common.c b/cmd/blk_common.c index 9f9d4327a9..ad9b16dc09 100644 --- a/cmd/blk_common.c +++ b/cmd/blk_common.c @@ -25,18 +25,18 @@ int blk_common_cmd(int argc, char *const argv[], enum uclass_id uclass_id, case 2: if (strncmp(argv[1], "inf", 3) == 0) { blk_list_devices(uclass_id);

•                   return 0;
  

•                   return CMD_RET_SUCCESS;
  

I really don't like this...0 is success.

...

            } else if (strncmp(argv[1], "dev", 3) == 0) {
                    if (blk_print_device_num(uclass_id, *cur_devnump)) {
                            printf("\nno %s devices available\n", if_name);
                            return CMD_RET_FAILURE;
                    }

•                   return 0;
  

•                   return CMD_RET_SUCCESS;
            } else if (strncmp(argv[1], "part", 4) == 0) {
                    if (blk_list_part(uclass_id))
                            printf("\nno %s partition table available\n",
                                   if_name);
  

•                   return 0;
  

•                   return CMD_RET_SUCCESS;
            }
            return CMD_RET_USAGE;
    case 3:
  

@@ -49,7 +49,7 @@ int blk_common_cmd(int argc, char *const argv[], enum uclass_id uclass_id, } else { return CMD_RET_FAILURE; }

•                   return 0;
  

•                   return CMD_RET_SUCCESS;
            } else if (strncmp(argv[1], "part", 4) == 0) {
                    int dev = (int)dectoul(argv[2], NULL);
  
  

@@ -58,7 +58,7 @@ int blk_common_cmd(int argc, char *const argv[], enum uclass_id uclass_id, if_name, dev); return CMD_RET_FAILURE; }

•                   return 0;
  

•                   return CMD_RET_SUCCESS;
            }
            return CMD_RET_USAGE;
  
  

@@ -80,7 +80,7 @@ int blk_common_cmd(int argc, char *const argv[], enum uclass_id uclass_id,

                    printf("%ld blocks read: %s\n", n,
                           n == cnt ? "OK" : "ERROR");

•                   return n == cnt ? 0 : 1;
  

•                   return n == cnt ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
  

CMD_RET_FAILURE is OK, but I would prefer not to use CMD_RET_SUCCESS. It is 0 and always will be.

It encourages people to do things like:

if (ret == CMD_RET_SUCCESS)

instead of

if (!ret)

I see your concern. However we don't change the return value type to enum, so people can still use

if (!ret)

I would still defend that we should use CMD_RET_SUCCESS.

This is like EXIT_XXX defined in stdlib.h:

#define EXIT_FAILURE 1 /* Failing exit status. */ #define EXIT_SUCCESS 0 /* Successful exit status. */

One should use predefined macros whenever possible.

It would eventually creep into everything, including our clean error handling.

...

            } else if (strcmp(argv[1], "write") == 0) {
                    phys_addr_t paddr = hextoul(argv[2], NULL);
                    lbaint_t blk = hextoul(argv[3], NULL);

@@ -98,7 +98,7 @@ int blk_common_cmd(int argc, char *const argv[], enum uclass_id uclass_id,

                    printf("%ld blocks written: %s\n", n,
                           n == cnt ? "OK" : "ERROR");

•                   return n == cnt ? 0 : 1;
  

•                   return n == cnt ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
            } else {
                    return CMD_RET_USAGE;
            }
  

--

Regards, Bin