Hi Simon, On Mon, Oct 2, 2023 at 9:42 AM Simon Glass <s...@chromium.org> wrote: > > Hi Bin, > > On Tue, 26 Sept 2023 at 02:54, Bin Meng <bm...@tinylab.org> wrote: > > > > Avoid using magic number 0/1 for the command result. > > > > Signed-off-by: Bin Meng <bm...@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