On Fri, 2026-05-15 at 06:56 -0600, Simon Glass wrote:
> Hi Weijie,
>
> On 2026-05-13T08:02:45, Weijie Gao <[email protected]> wrote:
> > cmd: ubi: change all positive error return value to negative
> >
> > Change all return value using errno codes to negative. This makes
> > it
> > consistent with the linux ubi layer.
> >
> > Also, to follow the standard definition of U-Boot command, in the
> > do_ubi()
> > command handler, the return value is converted to CMD_RET_FAILURE
> > for error
> > returning, and CMD_RET_USAGE for incorrect usage.
> >
> > Signed-off-by: Weijie Gao <[email protected]>
> >
> > cmd/ubi.c | 106 ++++++++++++++++++++++++++++++++++++---------------
> > -----------
> > 1 file changed, 62 insertions(+), 44 deletions(-)
>
> Reviewed-by: Simon Glass <[email protected]>
>
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -761,7 +762,7 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> > return ubi_check(argv[2]);
> >
> > printf("Error, no volume name passed\n");
> > - return 1;
> > + return CMD_RET_FAILURE;
>
> A missing argument is a usage error, not a runtime failure. Please
> use
> CMD_RET_USAGE here, and likewise for the 'Incorrect type' and 'no
> volume name passed' paths in the create handler, so the user sees the
> help text.
OK. I'll change the remaining two to CMD_RET_USAGE.
>
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -677,7 +674,7 @@ int ubi_part(const char *part_name, const char
> > *vid_header_offset)
> > mtd = get_mtd_device_nm(part_name);
> > if (IS_ERR(mtd)) {
> > printf("Partition %s not found!\n", part_name);
> > - return 1;
> > + return PTR_ERR(mtd);
> > }
>
> PTR_ERR() returns long but ubi_part() returns int. Please cast, or
> just return a specific errno like -ENODEV
Returning -ENODEV is a better choice.
>
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,34 +807,46 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> > }
> > /* E.g., create volume */
> > if (argc == 3) {
> > - return ubi_create_vol(argv[2], size, dynamic,
> > id,
> > - skipcheck);
> > + ret = ubi_create_vol(argv[2], size, dynamic,
> > id,
> > + skipcheck);
> > + if (ret)
> > + return CMD_RET_FAILURE;
> > + return 0;
> > }
>
> This three-line conversion is repeated about seven times in do_ubi().
> Since the called functions already print their own errors, a helper
> or
> just 'return ret ? CMD_RET_FAILURE : 0;' would cut the duplication.
> What do you think?
That's OK. I'll modify them.
>
> Regards,
> Simon
