Re: [PATCH 02/23] qemu-img: refresh options/--help for "create" subcommand

[Daniel P . Berrangé]

On Sat, Feb 10, 2024 at 12:22:23AM +0300, Michael Tokarev wrote:
> Add missing long options (eg --format).
> 
> Create helper function cmd_help() to display command-specific
> help text, and use it to print --help for 'create' subcommand.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  qemu-img.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 05f80b6e5b..7edfc56572 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -126,6 +126,25 @@ void unrecognized_option(const img_cmd_t *ccmd, const 
> char *option)
>      error_exit(ccmd, "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 starting with \t
> + * desrciption is indented by one char
> + */
> +static G_NORETURN
> +void cmd_help(const img_cmd_t *ccmd,
> +              const char *syntax, const char *arguments)
> +{
> +    printf("qemu-img %s %s"

I think we want an extra "\n" before & after 'Arguments:'

> +           "Arguments:\n"
> +           " -h|--help - print this help and exit\n"
> +           "%s",
> +           ccmd->name, syntax, arguments);
> +    exit(EXIT_SUCCESS);
> +}
> +
>  /* Please keep in synch with docs/tools/qemu-img.rst */
>  static G_NORETURN
>  void help(void)
> @@ -524,7 +543,13 @@ 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",
> @@ -540,7 +565,25 @@ static int img_create(const img_cmd_t *ccmd, int argc, 
> char **argv)
>              unrecognized_option(ccmd, argv[optind - 1]);
>              break;
>          case 'h':
> -            help();
> +            cmd_help(ccmd,
> +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n"
> +"    [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n"
> +,
> +" -q|--quiet - quiet operations\n"
> +" -f|--format FMT - specifies format of the new image, default is raw\n"
> +" -o|--options FMT_OPTS - format-specific options ('-o list' for list)\n"
> +" -b|--backing BACKING_FILENAME - stack new image on top of 
> BACKING_FILENAME\n"
> +"  (for formats which support stacking)\n"
> +" -F|--backing-format BACKING_FMT - specify format of BACKING_FILENAME\n"
> +" -u|--backing-unsafe - do not fail if BACKING_FMT can not be read\n"
> +" --object OBJDEF - QEMU user-creatable object (eg encryption key)\n"
> +" FILENAME - image file to create.  It will be overriden if exists\n"
> +" SIZE - image size with optional suffix: 'b' (byte, default), 'k' or\n"
> +"  'K' (kilobyte, 1024b), 'M' (megabyte, 1024K), 'G' (gigabyte, 1024M),\n"
> +"  'T' (terabyte, 1024G), 'P' (petabyte, 1024T), or 'E' (exabyte, 1024P)\n"
> +"  SIZE is required unless BACKING_IMG is specified, in which case\n"
> +"  it will be the same as size of BACKING_IMG\n"

This comes out as a bit of a wall of dense text.

I think we should have 2 space indent for options, and a further
4 space for continuations, and also put the description on its
own line.

eg so instead of getting:

$ ./build/qemu-img create --help
qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]
        [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]
Arguments:
 -h|--help - print this help and exit
 -q|--quiet - quiet operations
 -f|--format FMT - specifies format of the new image, default is raw
 -o|--options FMT_OPTS - format-specific options ('-o list' for list)
 -b|--backing BACKING_FILENAME - stack new image on top of BACKING_FILENAME
  (for formats which support stacking)
 -F|--backing-format BACKING_FMT - specify format of BACKING_FILENAME
 -u|--backing-unsafe - do not fail if BACKING_FMT can not be read
 --object OBJDEF - QEMU user-creatable object (eg encryption key)
 FILENAME - image file to create.  It will be overriden if exists
 SIZE - image size with optional suffix: 'b' (byte, default), 'k' or
  'K' (kilobyte, 1024b), 'M' (megabyte, 1024K), 'G' (gigabyte, 1024M),
  'T' (terabyte, 1024G), 'P' (petabyte, 1024T), or 'E' (exabyte, 1024P)
  SIZE is required unless BACKING_IMG is specified, in which case
  it will be the same as size of BACKING_IMG


we would get:

$ ./build/qemu-img create --help
qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]
        [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]

Arguments:

  -h|--help
      print this help and exit

  -q|--quiet
      quiet operations

  -f|--format FMT
      specifies format of the new image, default is raw

  -o|--options FMT_OPTS
      format-specific options ('-o list' for list)

  -b|--backing BACKING_FILENAME
      stack new image on top of BACKING_FILENAME
      (for formats which support stacking)

  -F|--backing-format BACKING_FMT
      specify format of BACKING_FILENAME

  -u|--backing-unsafe
      do not fail if BACKING_FMT can not be read

  --object OBJDEF
      QEMU user-creatable object (eg encryption key)
      
  FILENAME
      image file to create.  It will be overriden if exists

  SIZE
      image size with optional suffix: 'b' (byte, default), 'k' or
      'K' (kilobyte, 1024b), 'M' (megabyte, 1024K), 'G' (gigabyte, 1024M),
      'T' (terabyte, 1024G), 'P' (petabyte, 1024T), or 'E' (exabyte, 1024P)
      SIZE is required unless BACKING_IMG is specified, in which case
      it will be the same as size of BACKING_IMG


> +);
>              break;
>          case 'F':
>              base_fmt = optarg;
> -- 
> 2.39.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|