Re: [PATCH 09/27] qemu-img: commit: refresh options/--help

[Kevin Wolf <kwolf@redhat.com>]

Am 27.09.2024 um 08:11 hat Michael Tokarev geschrieben:
> Add missing long options and --help output.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 9157a6b45d..7a111bce72 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1048,24 +1048,50 @@ static int img_commit(const img_cmd_t *ccmd, int argc, char **argv)
>      for(;;) {
>          static const struct option long_options[] = {
>              {"help", no_argument, 0, 'h'},
> +            {"quiet", no_argument, 0, 'q'},
>              {"object", required_argument, 0, OPTION_OBJECT},
> +            {"format", required_argument, 0, 'f'},
>              {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> +            {"cache", required_argument, 0, 't'},
> +            {"drop", no_argument, 0, 'd'},
> +            {"base", required_argument, 0, 'b'},
> +            {"progress", no_argument, 0, 'p'},
> +            {"rate", required_argument, 0, 'r'},

"rate-limit"?

>              {0, 0, 0, 0}
>          };
> -        c = getopt_long(argc, argv, ":f:ht:b:dpqr:",
> +        c = getopt_long(argc, argv, "f:ht:b:dpqr:",
>                          long_options, NULL);

Should we try to keep the order in long_options and in the getopt string
consistent? There doesn't seem to be any system behind the order we have
currently. Maybe keep common options (--help, --quiet) first, but then
order things alphabetically?

It doesn't really matter that much here, it would just improve
legibility of the code a bit. But I think in the help text, we should
definitely have a more obvious order so that users can find their option
without having to read everything.

Of course, this is a comment that applies not only to this patch, but to
all subcommands.

>          if (c == -1) {
>              break;
>          }
>          switch(c) {
> -        case ':':
> -            missing_argument(argv[optind - 1]);
> -            break;
> -        case '?':
> -            unrecognized_option(argv[optind - 1]);
> -            break;
>          case 'h':
> -            help();
> +            cmd_help(ccmd,
> +"[-f FMT | --image-opts] [-t CACHE_MODE] [-b BASE_IMG] [-d]\n"
> +"        [-r RATE] [--object OBJDEF] FILENAME\n"
> +,
> +"  -q, --quiet\n"
> +"     quiet operations\n"

Same as in the previous patch. After this one, I won't point out things
any more if I already commented on the same thing earlier in the series.
They should apply to all similar instances.

> +"  -p, --progress\n"
> +"     show operation progress\n"

The man page has "display progress bar" (even though it's not a bar).
Maybe make it "display progress information" in both places?

> +"  -f, --format FMT\n"
> +"     specify FILENAME image format explicitly\n"
> +"  --image-opts\n"
> +"     indicates that FILENAME is a complete image specification\n"
> +"     instead of a file name (incompatible with --format)\n"
> +"  -t, --cache CACHE_MODE image cache mode (" BDRV_DEFAULT_CACHE ")\n"
> +"  -d, --drop\n"
> +"     skip emptying FILENAME on completion\n"
> +"  -b, --base BASE_IMG\n"
> +"     image in the backing chain to which to commit changes\n"
> +"     instead of the previous one (implies --drop)\n"

"image in the backing chain to commit change to (default: immediate
backing file; implies --drop)"?

> +"  -r, --rate RATE\n"
> +"     I/O rate limit\n"

in bytes per second

> +"  --object OBJDEF\n"
> +"     QEMU user-creatable object (eg encryption key)\n"
> +"  FILENAME\n"
> +"     name of the image file to operate on\n"
> +);
>              break;
>          case 'f':
>              fmt = optarg;
> @@ -1099,6 +1125,8 @@ static int img_commit(const img_cmd_t *ccmd, int argc, char **argv)
>          case OPTION_IMAGE_OPTS:
>              image_opts = true;
>              break;
> +        default:
> +            tryhelp(argv[0]);
>          }
>      }

Kevin