[PATCH 07/27] qemu-img: check: refresh options/--help

Michael Tokarev posted 27 patches 1 year, 4 months ago
[PATCH 07/27] qemu-img: check: refresh options/--help
Posted by Michael Tokarev 1 year, 4 months ago
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"
+"  -f, --format FMT\n"
+"     specifies format of the image 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\n"
+"     image cache mode (" 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"
+"     (image will be open in read-write mode, incompatible with --force-share)\n"
+"  --object OBJDEF\n"
+"     QEMU user-creatable object (eg encryption key)\n"
+"  FILENAME\n"
+"     the image file (or image specification) to operate on\n"
+);
             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) {
-- 
2.39.5
Re: [PATCH 07/27] qemu-img: check: refresh options/--help
Posted by Kevin Wolf 9 months ago
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
Re: [PATCH 07/27] qemu-img: check: refresh options/--help
Posted by Michael Tokarev 8 months, 2 weeks ago
On 13.05.2025 18:54, Kevin Wolf wrote:
> Am 27.09.2024 um 08:11 hat Michael Tokarev geschrieben:
>> Add missing long options and --help output.
...
>> +"  --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)

What I ended up now is:

   --image-opts
      treat FILE as an option string (key=value,..), not a file name
      (incompatible with -f|--format)

Note I used short "FILE" everywhere, while in the manpage we see
longer FILENAME - my plan is to get the manpage to use the same
FILE too, in the next step (I can probably do it together with
each individual commit in this series too).

Maybe it's better to use IMAGE instead of FILE, because:

   FILE
      name of the image file, or an option string (key=value,..)
      with --image-opts, to operate on

(this is another common idiom I used in all the changes).


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

it is also different from the manpage (where OBJECTDEF is used).
I ended up with just:

   --object OBJDEF
      defines QEMU user-creatable object

Since --object can be used with any command, maybe it's a good
idea to have it in common options string, before a subcommand.

Thanks,

/mjt
Re: [PATCH 07/27] qemu-img: check: refresh options/--help
Posted by Kevin Wolf 8 months, 1 week ago
Am 31.05.2025 um 18:51 hat Michael Tokarev geschrieben:
> On 13.05.2025 18:54, Kevin Wolf wrote:
> > Am 27.09.2024 um 08:11 hat Michael Tokarev geschrieben:
> > > Add missing long options and --help output.
> ...
> > > +"  --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)
> 
> What I ended up now is:
> 
>   --image-opts
>      treat FILE as an option string (key=value,..), not a file name
>      (incompatible with -f|--format)

That's better, I like it.

I'm not sure if we really need to have both short and long name for
incomatible options, but I don't mind either way as long as we're
consistent with it.

> Note I used short "FILE" everywhere, while in the manpage we see
> longer FILENAME - my plan is to get the manpage to use the same
> FILE too, in the next step (I can probably do it together with
> each individual commit in this series too).
> 
> Maybe it's better to use IMAGE instead of FILE, because:
> 
>   FILE
>      name of the image file, or an option string (key=value,..)
>      with --image-opts, to operate on
> 
> (this is another common idiom I used in all the changes).

I think FILE is clear enough, but I wouldn't object to IMAGE either.

> > > +"  --object OBJDEF\n"
> > > +"     QEMU user-creatable object (eg encryption key)\n"
> > 
> > "e.g."
> 
> it is also different from the manpage (where OBJECTDEF is used).
> I ended up with just:
> 
>   --object OBJDEF
>      defines QEMU user-creatable object
> 
> Since --object can be used with any command, maybe it's a good
> idea to have it in common options string, before a subcommand.

That is an interesting idea. But wouldn't we need to keep the existing
code anyway for compatibility? I don't think we have code so far for
options that can be used both before and after the subcommand name.

Kevin
Re: [PATCH 07/27] qemu-img: check: refresh options/--help
Posted by Michael Tokarev 9 months ago
On 13.05.2025 18:54, Kevin Wolf wrote:
> Am 27.09.2024 um 08:11 hat Michael Tokarev geschrieben:

>> +            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 haven't touched the man page for a reason, - I wasn't sure I
understood all the options correctly.  And the man pages were the
next planning step.  Unfortunately it's been quite some time ago
and I don't remember details anymore.  It can be done either way,
and I tried to make the whole thing as short as possible in the
--help output.

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

Ok.

>> +"  -f, --format FMT\n"
>> +"     specifies format of the image explicitly\n"
> 
> Maybe "format of the image (default: probing is used)"?

Yeah, makes sense.

Not wanting to risk going into an endless wording discussion,
let's do it this way here and elsewhere.

Thank you for the review!

/mjt
Re: [PATCH 07/27] qemu-img: check: refresh options/--help
Posted by Kevin Wolf 9 months ago
Am 15.05.2025 um 08:50 hat Michael Tokarev geschrieben:
> On 13.05.2025 18:54, Kevin Wolf wrote:
> > Am 27.09.2024 um 08:11 hat Michael Tokarev geschrieben:
> 
> > > +            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 haven't touched the man page for a reason, - I wasn't sure I
> understood all the options correctly.  And the man pages were the
> next planning step.  Unfortunately it's been quite some time ago
> and I don't remember details anymore.  It can be done either way,
> and I tried to make the whole thing as short as possible in the
> --help output.

Yes, we definitely need to shorten the descriptions from the man page
for --help. I think as long as it fits on a single line, it's okay, but
I understand if you want to keep it even shorter in some cases.

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

If this is too wordy for your liking, just "quiet mode" could be enough,
but I would keep the terminology the same (i.e. not "operations" here
and "mode" there").

> > > +"  -f, --format FMT\n"
> > > +"     specifies format of the image explicitly\n"
> > 
> > Maybe "format of the image (default: probing is used)"?
> 
> Yeah, makes sense.
> 
> Not wanting to risk going into an endless wording discussion,
> let's do it this way here and elsewhere.

The nature of your series is that a big part of the review is actually
the new help text, so it's unavoidable to have some wording discussions
here and there. Don't necessarily take my suggestions as the final word,
they are just suggestions on which we can iterate and find the best
wording for users. I think refining help texts is worth some thought and
back and forth between multiple people. So if you don't like my
suggestions, or you prefer a mix of yours and mine, or you have
additional improvements to them, feel free to say so.

Kevin