[U-Boot] [PATCH v2 1/2] common: command: Use command_ret_t enum values instead of values
Use enum command_ret_t types in cmd_process_error().
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
Changes in v2: - Move adding RET_USAGE to separate patch.
common/command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/command.c b/common/command.c index 52d47c133c3c..a4a8dc601acb 100644 --- a/common/command.c +++ b/common/command.c @@ -549,8 +549,8 @@ int cmd_process_error(cmd_tbl_t *cmdtp, int err) { if (err) { printf("Command '%s' failed: Error %d\n", cmdtp->name, err); - return 1; + return CMD_RET_FAILURE; }
- return 0; + return CMD_RET_SUCCESS; }
command_ret_t enum contains 3 return values but only two are handled now. Extend cmd_process_error() and handle CMD_RET_USAGE separately.
These commands are affected by this change. cmd/demo.c cmd/efi.c cmd/gpio.c cmd/qfw.c cmd/x86/fsp.c test/dm/cmd_dm.c
And scripts shouldn't be affected because return value is not 0. But every command implementation can choose what it is correct to pass. I would expect that RET_USAGE is called when parameters are not correctly passed (have incorrect value, missing parameters) and RET_FAILURE when correct parameters are passed but command fails.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
Changes in v2: - Separated patch from 1/2 - Extend description in command.h
common/command.c | 3 +++ include/command.h | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/common/command.c b/common/command.c index a4a8dc601acb..2433a89e0a8e 100644 --- a/common/command.c +++ b/common/command.c @@ -547,6 +547,9 @@ enum command_ret_t cmd_process(int flag, int argc, char * const argv[],
int cmd_process_error(cmd_tbl_t *cmdtp, int err) { + if (err == CMD_RET_USAGE) + return CMD_RET_USAGE; + if (err) { printf("Command '%s' failed: Error %d\n", cmdtp->name, err); return CMD_RET_FAILURE; diff --git a/include/command.h b/include/command.h index 04cd1e745cbf..5b1577f3b477 100644 --- a/include/command.h +++ b/include/command.h @@ -67,7 +67,9 @@ extern int cmd_auto_complete(const char *const prompt, char *buf, int *np, int * * * @cmdtp: Command which caused the error * @err: Error code (0 if none, -ve for error, like -EIO) - * @return 0 if there is not error, 1 (CMD_RET_FAILURE) if an error is found + * @return 0 (CMD_RET_SUCCESX) if there is not error, + * 1 (CMD_RET_FAILURE) if an error is found + * -1 (CMD_RET_USAGE) if 'usage' error is found */ int cmd_process_error(cmd_tbl_t *cmdtp, int err);
On 21 June 2018 at 06:58, Michal Simek michal.simek@xilinx.com wrote:
command_ret_t enum contains 3 return values but only two are handled now. Extend cmd_process_error() and handle CMD_RET_USAGE separately.
These commands are affected by this change. cmd/demo.c cmd/efi.c cmd/gpio.c cmd/qfw.c cmd/x86/fsp.c test/dm/cmd_dm.c
And scripts shouldn't be affected because return value is not 0. But every command implementation can choose what it is correct to pass. I would expect that RET_USAGE is called when parameters are not correctly passed (have incorrect value, missing parameters) and RET_FAILURE when correct parameters are passed but command fails.
Signed-off-by: Michal Simek michal.simek@xilinx.com
Changes in v2:
- Separated patch from 1/2
- Extend description in command.h
common/command.c | 3 +++ include/command.h | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromum.org
On 21 June 2018 at 06:58, Michal Simek michal.simek@xilinx.com wrote:
Use enum command_ret_t types in cmd_process_error().
Signed-off-by: Michal Simek michal.simek@xilinx.com
Changes in v2:
- Move adding RET_USAGE to separate patch.
common/command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromum.org
diff --git a/common/command.c b/common/command.c index 52d47c133c3c..a4a8dc601acb 100644 --- a/common/command.c +++ b/common/command.c @@ -549,8 +549,8 @@ int cmd_process_error(cmd_tbl_t *cmdtp, int err) { if (err) { printf("Command '%s' failed: Error %d\n", cmdtp->name, err);
return 1;
return CMD_RET_FAILURE; }
return 0;
return CMD_RET_SUCCESS;
I actually thing 0 is fine here. That is the definition of success.
}
1.9.1
Regards, Simon
On 21.6.2018 21:45, Simon Glass wrote:
On 21 June 2018 at 06:58, Michal Simek michal.simek@xilinx.com wrote:
Use enum command_ret_t types in cmd_process_error().
Signed-off-by: Michal Simek michal.simek@xilinx.com
Changes in v2:
- Move adding RET_USAGE to separate patch.
common/command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromum.org
diff --git a/common/command.c b/common/command.c index 52d47c133c3c..a4a8dc601acb 100644 --- a/common/command.c +++ b/common/command.c @@ -549,8 +549,8 @@ int cmd_process_error(cmd_tbl_t *cmdtp, int err) { if (err) { printf("Command '%s' failed: Error %d\n", cmdtp->name, err);
return 1;
return CMD_RET_FAILURE; }
return 0;
return CMD_RET_SUCCESS;I actually thing 0 is fine here. That is the definition of success.
and CMD_RET_SUCCESS has this 0 value too.
maybe would be worth to also change return type to enum command_ret_t as is done for cmd_process.
For example ubi_remove_vol() in case of failure returs +ENODEV and others. I thought that commands could return only 3 values convert by enum command_ret_t. Or is it ok to return also different values?
Thanks, Michal
Hi Michal,
On 22 June 2018 at 00:33, Michal Simek michal.simek@xilinx.com wrote:
On 21.6.2018 21:45, Simon Glass wrote:
On 21 June 2018 at 06:58, Michal Simek michal.simek@xilinx.com wrote:
Use enum command_ret_t types in cmd_process_error().
Signed-off-by: Michal Simek michal.simek@xilinx.com
Changes in v2:
- Move adding RET_USAGE to separate patch.
common/command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromum.org
diff --git a/common/command.c b/common/command.c index 52d47c133c3c..a4a8dc601acb 100644 --- a/common/command.c +++ b/common/command.c @@ -549,8 +549,8 @@ int cmd_process_error(cmd_tbl_t *cmdtp, int err) { if (err) { printf("Command '%s' failed: Error %d\n", cmdtp->name, err);
return 1;
return CMD_RET_FAILURE; }
return 0;
return CMD_RET_SUCCESS;I actually thing 0 is fine here. That is the definition of success.
and CMD_RET_SUCCESS has this 0 value too.
maybe would be worth to also change return type to enum command_ret_t as is done for cmd_process.
For example ubi_remove_vol() in case of failure returs +ENODEV and others. I thought that commands could return only 3 values convert by enum command_ret_t. Or is it ok to return also different values?
Commands should only return things from the enum. My point was that I find 'return CMD_RET_SUCCESS' to be a bit painful. We know the value is 0 and it is much shorter to read, so I prefer 'return 0' instead of 'return CMD_RET_SUCCESS'
Also I like this:
if (!xx) // we got an error
and don't like this:
if (xx != CMD_RET_SUCCESS) // we got an error
Regards, Simon
On 22.6.2018 21:28, Simon Glass wrote:
Hi Michal,
On 22 June 2018 at 00:33, Michal Simek michal.simek@xilinx.com wrote:
On 21.6.2018 21:45, Simon Glass wrote:
On 21 June 2018 at 06:58, Michal Simek michal.simek@xilinx.com wrote:
Use enum command_ret_t types in cmd_process_error().
Signed-off-by: Michal Simek michal.simek@xilinx.com
Changes in v2:
- Move adding RET_USAGE to separate patch.
common/command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromum.org
diff --git a/common/command.c b/common/command.c index 52d47c133c3c..a4a8dc601acb 100644 --- a/common/command.c +++ b/common/command.c @@ -549,8 +549,8 @@ int cmd_process_error(cmd_tbl_t *cmdtp, int err) { if (err) { printf("Command '%s' failed: Error %d\n", cmdtp->name, err);
return 1;
return CMD_RET_FAILURE; }
return 0;
return CMD_RET_SUCCESS;I actually thing 0 is fine here. That is the definition of success.
and CMD_RET_SUCCESS has this 0 value too.
maybe would be worth to also change return type to enum command_ret_t as is done for cmd_process.
For example ubi_remove_vol() in case of failure returs +ENODEV and others. I thought that commands could return only 3 values convert by enum command_ret_t. Or is it ok to return also different values?
Commands should only return things from the enum. My point was that I find 'return CMD_RET_SUCCESS' to be a bit painful. We know the value is 0 and it is much shorter to read, so I prefer 'return 0' instead of 'return CMD_RET_SUCCESS'
Also I like this:
if (!xx) // we got an error
and don't like this:
if (xx != CMD_RET_SUCCESS) // we got an error
ok. Got what you meant.
Thanks, Michal
participants (2)
-
Michal Simek -
Simon Glass