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

[Simon Glass]

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.

> 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.

> 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.

> 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()?

Regards,
Simon