[PATCH v3 06/10] cmd: ubi: reorganize command messages

[Weijie Gao]

On Tue, 2026-04-28 at 08:10 -0600, Simon Glass wrote:
> Hi Weijie,
> 
> On 2026-04-27T07:09:11, Weijie Gao <weijie.gao at mediatek.com> wrote:
> > cmd: ubi: reorganize command messages
> > 
> > This patch moves normal subcommand messages into the main command
> > function.
> > This will allow current and potential api functions being called
> > with clean
> > output.
> > 
> > A new function ubi_require_volume() is added for finding and
> > printing error
> > message if volume not found. The original ubi_find_volume() will be
> > silent
> > for being an api function.
> > 
> > Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
> > 
> > cmd/ubi.c | 109 +++++++++++++++++++++++++++++++++++++++++++++----
> > -------------
> >  1 file changed, 79 insertions(+), 30 deletions(-)
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> >       if (strncmp(argv[1], 'remove', 6) == 0) {
> >               /* E.g., remove volume */
> > -             if (argc == 3)
> > -                     return ubi_remove_vol(argv[2]);
> > +             if (argc == 3) {
> > +                     int vol_id;
> > +
> > +                     vol = ubi_find_volume(argv[2]);
> > +                     if (!vol)
> > +                             return 0;
> > +
> > +                     vol_id = vol->vol_id;
> > +
> > +                     ret = ubi_remove_vol(argv[2]);
> 
> This seems wrong - previously 'ubi remove nonexistent' printed an
> error via ubi_find_volume() and returned ENODEV; now it silently
> returns 0. I think you should use ubi_require_volume() here.

I'll fix this.

> 
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> >               if (argc == 3) {
> > +                     if (!size) {
> > +                             vol = ubi_require_volume(argv[3]);
> > +                             if (!vol)
> > +                                     return 1;
> > +
> > +                             printf("No size specified -> Using
> > max size (%lld)\n",
> > +                                    vol->used_bytes);
> > +                             size = vol->used_bytes;
> > +                     }
> > +
> > +                     ret = ubi_volume_read(argv[3], (void *)addr,
> > 0, size);
> 
> When size is 0 the volume is now looked up twice,. i.e. here and
> again
> inside ubi_volume_read() - can you drop the fallback in
> ubi_volume_read() or move the 'No size specified' print back there
> guarded by a verbose flag.

I think we can do some modifications to ubi_volume_read:

static int __ubi_volume_read(struct ubi_volume *vol, ...) { ... }

int ubi_volume_read(const char *volume, ...)
{
	struct ubi_volume *vol = ubi_require_volume(volume);
	...
	return __ubi_volume_read(vol, ...);
}

And in do_ubi() we always call ubi_require_volume() to get vol and call
__ubi_volume_read with vol.

> 
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> > -     if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1],
> > 'rename', 6))
> > -             return ubi_rename_vol(argv[2], argv[3]);
> > +     if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1],
> > 'rename', 6)) {
> > +             ret = ubi_rename_vol(argv[2], argv[3]);
> > +             if (!ret) {
> > +                     printf("UBI volume %s renamed to %s\n",
> > argv[2],
> > +                            argv[3]);
> > +             }
> > +
> > +             return ret;
> > +     }
> 
> While you are here, please add an 'argc == 4' check to fix a
> pre-existing bug - i.e. argv[2]/argv[3] are dereferenced
> unconditionally.

OK.

> 
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> > +                     else if (ret == -EEXIST)
> > +                             printf("Volume %s already exists!\n",
> > argv[2]);
> > +
> > +                     return ret;
> 
> Just to check: ubi_create_volume() returns -EEXIST, but
> verify_mkvol_req() returns positive errnos, so name-too-long or
> bad-alignment failures get no friendly message here. What do you
> think
> about handling the verify_mkvol_req() errors here too, or making that
> helper silent like ubi_find_volume()?

I found that the ubi layer will print error message in this case:
ubi0 error: ubi_create_volume: volume "test" exists (ID 2)
ubi0 error: ubi_create_volume: cannot create volume 11, error -17

So this modification is unnecessary and I'll remove this in next patch.

All error messages will be kept in verify_mkvol_req().

> 
> Regards,
> Simon