Re: [PATCH 07/27] qemu-img: check: 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 | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 08536553c7..1bd88fcf63 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -805,7 +805,9 @@ static int img_check(const img_cmd_t *ccmd, int argc, char **argv)
>          int option_index = 0;
>          static const struct option long_options[] = {
>              {"help", no_argument, 0, 'h'},
> +            {"quiet", no_argument, 0, 'q'},
>              {"format", required_argument, 0, 'f'},
> +            {"cache", required_argument, 0, 'T'},
>              {"repair", required_argument, 0, 'r'},
>              {"output", required_argument, 0, OPTION_OUTPUT},
>              {"object", required_argument, 0, OPTION_OBJECT},
> @@ -813,20 +815,38 @@ static int img_check(const img_cmd_t *ccmd, int argc, char **argv)
>              {"force-share", no_argument, 0, 'U'},
>              {0, 0, 0, 0}
>          };
> -        c = getopt_long(argc, argv, ":hf:r:T:qU",
> +        c = getopt_long(argc, argv, "hf:r:T:qU",
>                          long_options, &option_index);
>          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] [-r] [-u]\n"
> +"        [--output human|json] [--object OBJDEF] FILENAME\n"
> +,
> +"  -q, --quiet\n"
> +"     quiet operations\n"

Let's keep the help text more in line with the terminology used in the
man page, even if shorter at times.

I would use the first sentence from it here: "Quiet mode - do not print
any output (except errors)"

> +"  -f, --format FMT\n"
> +"     specifies format of the image explicitly\n"

Maybe "format of the image (default: probing is used)"?

> +"  --image-opts\n"
> +"     indicates that FILENAME is a complete image specification\n"
> +"     instead of a file name (incompatible with --format)\n"

The man page has:

  Indicates that the source *FILENAME* parameter is to be interpreted as a
  full option string, not a plain filename. This parameter is mutually
  exclusive with the *-f* parameter.

A possible adaptation for qemu-img check specially:

  indicates that FILENAME is a full option string, not a plain filename
  (incompatible with --format)

> +"  -T, --cache CACHE_MODE\n"
> +"     image cache mode (" BDRV_DEFAULT_CACHE ")\n"

+"     image cache mode (default: " BDRV_DEFAULT_CACHE ")\n"

> +"  -U, --force-share\n"
> +"     open image in shared mode for concurrent access\n"
> +"  --output human|json\n"
> +"     output format\n"
> +"  -r, --repair leaks|all\n"
> +"     repair particular aspect of the image\n"

"repair errors of the given category in the image"?

> +"     (image will be open in read-write mode, incompatible with --force-share)\n"
> +"  --object OBJDEF\n"
> +"     QEMU user-creatable object (eg encryption key)\n"

"e.g."

> +"  FILENAME\n"
> +"     the image file (or image specification) to operate on\n"

To keep consistency with the above suggestion: "(or option string)"

> +);
>              break;
>          case 'f':
>              fmt = optarg;
> @@ -861,6 +881,8 @@ static int img_check(const img_cmd_t *ccmd, int argc, char **argv)
>          case OPTION_IMAGE_OPTS:
>              image_opts = true;
>              break;
> +        default:
> +            tryhelp(argv[0]);
>          }
>      }
>      if (optind != argc - 1) {

Kevin