Hi Bin,
On Tue, 10 Oct 2023 at 03:06, Bin Meng bmeng.cn@gmail.com wrote:
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.
I agree except for success, where I don't, sorry. It should always be 0 in U-Boot.
People then have to look up the value and also we get things like
if (ret != CMD_RET_SUCCESS)
It is a slippery and I would rather not start down it.
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,