[PATCH 10/15] cmd: blk_common: Use macros for the return values

[Tom Rini]

On Tue, Oct 10, 2023 at 08:58:04AM -0600, Simon Glass wrote:
> Hi Bin,
> 
> On Tue, 10 Oct 2023 at 03:06, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Oct 2, 2023 at 9:42 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Tue, 26 Sept 2023 at 02:54, Bin Meng <bmeng at tinylab.org> wrote:
> > > >
> > > > Avoid using magic number 0/1 for the command result.
> > > >
> > > > Signed-off-by: Bin Meng <bmeng at 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.

To be clear, we have a large amount of code today that uses
CMD_RET_SUCCESS in the way this patch converts to.  We do have a few
cases even of the kind of code you worry about seeing, but I think in
context clearer (it's in security or OTP fuse contexts) and the compiler
will do the right thing regardless.

Reviewed-by: Tom Rini <trini at konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231010/e0b6d9a6/attachment.sig>