| Message ID | dc051659416b3e20ef4a3801f1d02459514827a2.1529585912.git.michal.simek@xilinx.com |
|---|---|
| State | Accepted |
| Commit | 3723324042ec92739d802604558a3ebe3d5616dc |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,v2,1/2] common: command: Use command_ret_t enum values instead of values | expand |
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
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; }
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(-)