[U-Boot] [PATCH v2 1/2] common: command: Use command_ret_t enum values instead of values

[Michal Simek]

On 22.6.2018 21:28, Simon Glass wrote:
> Hi Michal,
> 
> On 22 June 2018 at 00:33, Michal Simek <michal.simek at xilinx.com> wrote:
>>
>> On 21.6.2018 21:45, Simon Glass wrote:
>>> On 21 June 2018 at 06:58, Michal Simek <michal.simek at xilinx.com> wrote:
>>>> Use enum command_ret_t types in cmd_process_error().
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek at 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 at 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