Re: [PATCH 05/27] qemu-img: create: refresh options/--help

[Kevin Wolf <kwolf@redhat.com>]

Am 27.09.2024 um 08:10 hat Michael Tokarev geschrieben:
> Create helper function cmd_help() to display command-specific
> help text, and use it to print --help for 'create' subcommand.
> 
> Add missing long options (eg --format) in img_create().
> 
> Remove usage of missing_argument()/unrecognized_option() in
> img_create().
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  qemu-img.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e8234104e5..7ed5e6d1a8 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -132,6 +132,32 @@ void unrecognized_option(const char *option)
>      error_exit("qemu-img", "unrecognized option '%s'", option);
>  }
>  
> +/*
> + * Print --help output for a command and exit.
> + * syntax and description are multi-line with trailing EOL
> + * (to allow easy extending of the text)
> + * syntax has each subsequent line indented by 8 chars.
> + * desrciption is indented by 2 chars for argument on each own line,

s/desrciption/description/

I would also suggest writing @syntax and @description as we do in other
places to indicate that we're talking about parameter names here rather
than normal English words.

> + * and with 5 chars for argument description (like -h arg below).
> + */
> +static G_NORETURN
> +void cmd_help(const img_cmd_t *ccmd,
> +              const char *syntax, const char *arguments)
> +{
> +    printf(
> +"Usage:\n"
> +"\n"
> +"  %s %s %s"
> +"\n"
> +"Arguments:\n"
> +"  -h, --help\n"
> +"     print this help and exit\n"
> +"%s\n",
> +           "qemu-img", ccmd->name,
> +           syntax, arguments);

The last two lines should easily fit on a single one.

> +    exit(EXIT_SUCCESS);
> +}
> +
>  /* Please keep in synch with docs/tools/qemu-img.rst */
>  static G_NORETURN
>  void help(void)
> @@ -530,23 +556,48 @@ static int img_create(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'},
> +            {"backing", required_argument, 0, 'b'},
> +            {"backing-format", required_argument, 0, 'F'},
> +            {"backing-unsafe", no_argument, 0, 'u'},
> +            {"options", required_argument, 0, 'o'},
>              {0, 0, 0, 0}
>          };
> -        c = getopt_long(argc, argv, ":F:b:f:ho:qu",
> +        c = getopt_long(argc, argv, "F:b:f:ho:qu",
>                          long_options, NULL);

Can we please keep the same order in long_options and in the options
string?

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

Is this new help text copied from somewhere or is it written from
scratch? I couldn't find the source if it was copied, so I'll treat it
as new and compare it only with the generic help text and the man page.

> +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n"
> +"        [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n"

This is yet another order. It also seems to miss -q.

The existing documentation has this:

create [--object OBJECTDEF] [-q] [-f FMT] [-b BACKING_FILE [-F BACKING_FMT]] [-u] [-o OPTIONS] FILENAME [SIZE]

Which is a fourth different order and description. It would probably be
good to consolidate on a single option and way to name parameter
placeholders.

> +,
> +"  -q, --quiet\n"
> +"     quiet operations\n"
> +"  -f, --format FMT\n"
> +"     specifies format of the new image, default is raw\n"

s/format/the format/

I would suggest "(default: raw)" as the format to use for defaults. It's
a bit easier to read when it's clearly not part of the sentence.

> +"  -o, --options FMT_OPTS\n"
> +"     format-specific options ('-o list' for list)\n"

Do you mean '-o help'?

> +"  -b, --backing BACKING_FILENAME\n"
> +"     stack new image on top of BACKING_FILENAME\n"
> +"     (for formats which support stacking)\n"

"stacking" isn't terminology we use elsewhere. Maybe "create image as a
copy-on-write overlay on top of BACKING_FILENAME (not supported by all
formats)"?

Not sure if it's worth having the parenthesis at all, many options of
qemu-img will only work with some formats.

> +"  -F, --backing-format BACKING_FMT\n"
> +"     specify format of BACKING_FILENAME\n"

-f said "specifies" (which sounds a bit better to me), let's stay
consistent. Same s/format/the format/, too.

> +"  -u, --backing-unsafe\n"
> +"     do not fail if BACKING_FMT can not be read\n"

BACKING_FILENAME?

Maybe "don't try to open BACKING_FILENAME for verification"?

> +"  --object OBJDEF\n"
> +"     QEMU user-creatable object (eg encryption key)\n"

s/eg/e.g./

Most other options start with a verb. Maybe "define a user-creatable
object"?

> +"  FILENAME\n"
> +"     image file to create.  It will be overridden if exists\n"

overwritten?

If you have one full stop in it, I think we should have one at the end,
too: "...if it exists.\n"

> +"  SIZE\n"
> +"     image size with optional suffix (multiplies in 1024)\n"

Powers of 1024?

The existing help is a bit more explicit about the supported suffixes:

"  'size' is the disk image size in bytes. Optional suffixes\n"
"    'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M),\n"
"    'T' (terabyte, 1024G), 'P' (petabyte, 1024T) and 'E' (exabyte, 1024P)  are\n"
"    supported. 'b' is ignored.\n"


> +"     SIZE is required unless BACKING_IMG is specified,\n"
> +"     in which case it will be the same as size of BACKING_IMG\n"

"it will default to the same size as BACKING_IMG"?

> +);
>              break;
>          case 'F':
>              base_fmt = optarg;
> @@ -571,6 +622,8 @@ static int img_create(const img_cmd_t *ccmd, int argc, char **argv)
>          case OPTION_OBJECT:
>              user_creatable_process_cmdline(optarg);
>              break;
> +        default:
> +            tryhelp(argv[0]);
>          }
>      }

Kevin