[RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working

[RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working

[Michael Tokarev]

This is an incomplete first attempt only, there's a lot left to do.

All the options in qemu-img is a complete mess, - no, inconsistent or
incomplete syntax in documentation, many undocumented options, option
names are used inconsistently and differently for different commands,
no long options exists for many short options, --help output is a huge
mess by its own, and the way how it all is generated is another story.
docs/tools/qemu-img.rst with qemu-img-opts.hx is yet another.

I hoped to fix just an option or two, but it ended up in a large task,
and I need some help and discussion, hence the RFC.

The idea is:

 - create more or less consistent set of options between different
   subcommands
 - provide long options which can be used without figuring out which
   -T/-t, -f|-F|-O etc to use for which of the two images given
 - have qemu-img --help provide just a list of subcommands
 - have qemu-img COMMAND --help to describe just this subcommand
 - get rid of qemu-img-opts.hx, instead finish documentation in
   qemu-img.rst based on the actual options implemented in
   qemu-img.c.

I started converting subcommands one by one, providing long options
and --help output.  And immediately faced some questions which needs
wider discussion.

 o We have --image-opts and --target-image-opts.  Do we really need both?
   In my view, --image-opts should be sort of global, turning *all*
   filenames on this command line into complete image specifications,
   there's no need to have separate image-opts and --target-image-opts.
   Don't know what to do wrt compatibility though.  It shouldn't be made
   this way from the beginning.  As a possible solution, introduce a new
   option and deprecate current set.

 o For conversion (convert, dd, etc), we've source and destination,
   so it's easy to distinguish using long options, like --source-format
   --target-cache etc (-t/-T -f/-F is a mess here already).  What to
   do with compare? --format1 --format2 is ugly, maybe --a-format and
   --b-format?  Maybe we can get off with --source (a) and --target (b)
   instead of filename1 & filename2?
   (--cache in this context is global for both).

 o qemu-img convert.  It's the most messy one, and it is not really
   documented (nor in qemu-img.rst , eg there's nothing in there about
   FILENAME2, -B is difficult to understand, etc).
   At the very least, I'd say the options should be
    --source-format, --source-cache etc
    --target-format, --target-options
    --target-image-opts - this shold go IMHO

 o check and commit - inconsistent cache flags?
   In convert, cache is backwards (source/target)?

At first, I tried to have more or less common option descriptions,
using various parameters, variables or #defines, but in different
commands the same options has slightly different wording, and in
some option names are different, so it looks like it's best to
have complete text in each subcommand.


Michael Tokarev (8):
  qemu-img: pass current cmdname into command handlers
  qemu-img: refresh options/--help for "create" subcommand
  qemu-img: factor out parse_output_format() and use it in the code
   (this one has been sent in a separate patch, here it is just for completness)
  qemu-img: refresh options/--help for "check" command
  qemu-img: simplify --repair error message
  qemu-img: refresh options/--help for "commit" command
  qemu-img: refresh options/--help for "compare" command
  qemu-img: refresh options/--help for "convert" command

 qemu-img.c | 352 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 226 insertions(+), 126 deletions(-)

-- 
2.39.2

[PATCH 1/8] qemu-img: pass current cmdname into command handlers

[Michael Tokarev]

In order to be able to give correct --help output, pass current cmdname
to command handlers and to common error reporting functions. After the
change, in case of command-line error, qemu-img will now print:

 Try 'qemu-img create --help' for more information.

Current cmdname will be useful in --help output as well.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 150 ++++++++++++++++++++++++++---------------------------
 1 file changed, 75 insertions(+), 75 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index bcbf51402d..a634747701 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -60,7 +60,7 @@
 
 typedef struct img_cmd_t {
     const char *name;
-    int (*handler)(int argc, char **argv);
+    int (*handler)(const char *cmdname, int argc, char **argv);
 } img_cmd_t;
 
 enum {
@@ -101,8 +101,8 @@ static void format_print(void *opaque, const char *name)
     printf(" %s", name);
 }
 
-static G_NORETURN G_GNUC_PRINTF(1, 2)
-void error_exit(const char *fmt, ...)
+static G_NORETURN G_GNUC_PRINTF(2, 3)
+void error_exit(const char *cmdname, const char *fmt, ...)
 {
     va_list ap;
 
@@ -110,20 +110,20 @@ void error_exit(const char *fmt, ...)
     error_vreport(fmt, ap);
     va_end(ap);
 
-    error_printf("Try 'qemu-img --help' for more information\n");
+    error_printf("Try 'qemu-img %s --help' for more information\n", cmdname ? cmdname : "");
     exit(EXIT_FAILURE);
 }
 
 static G_NORETURN
-void missing_argument(const char *option)
+void missing_argument(const char *cmdname, const char *option)
 {
-    error_exit("missing argument for option '%s'", option);
+    error_exit(cmdname, "missing argument for option '%s'", option);
 }
 
 static G_NORETURN
-void unrecognized_option(const char *option)
+void unrecognized_option(const char *cmdname, const char *option)
 {
-    error_exit("unrecognized option '%s'", option);
+    error_exit(cmdname, "unrecognized option '%s'", option);
 }
 
 /* Please keep in synch with docs/tools/qemu-img.rst */
@@ -508,7 +508,7 @@ static int64_t cvtnum(const char *name, const char *value)
     return cvtnum_full(name, value, 0, INT64_MAX);
 }
 
-static int img_create(int argc, char **argv)
+static int img_create(const char *cmdname, int argc, char **argv)
 {
     int c;
     uint64_t img_size = -1;
@@ -534,10 +534,10 @@ static int img_create(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -576,7 +576,7 @@ static int img_create(int argc, char **argv)
     }
 
     if (optind >= argc) {
-        error_exit("Expecting image file name");
+        error_exit(cmdname, "Expecting image file name");
     }
     optind++;
 
@@ -591,7 +591,7 @@ static int img_create(int argc, char **argv)
         img_size = (uint64_t)sval;
     }
     if (optind != argc) {
-        error_exit("Unexpected argument: %s", argv[optind]);
+        error_exit(cmdname, "Unexpected argument: %s", argv[optind]);
     }
 
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
@@ -716,7 +716,7 @@ static int collect_image_check(BlockDriverState *bs,
  *  3 - Check completed, image has leaked clusters, but is good otherwise
  * 63 - Checks are not supported by the image format
  */
-static int img_check(int argc, char **argv)
+static int img_check(const char *cmdname, int argc, char **argv)
 {
     int c, ret;
     OutputFormat output_format = OFORMAT_HUMAN;
@@ -754,10 +754,10 @@ static int img_check(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -773,7 +773,7 @@ static int img_check(int argc, char **argv)
             } else if (!strcmp(optarg, "all")) {
                 fix = BDRV_FIX_LEAKS | BDRV_FIX_ERRORS;
             } else {
-                error_exit("Unknown option value for -r "
+                error_exit(cmdname, "Unknown option value for -r "
                            "(expecting 'leaks' or 'all'): %s", optarg);
             }
             break;
@@ -798,7 +798,7 @@ static int img_check(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_exit(cmdname, "Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -948,7 +948,7 @@ static void run_block_job(BlockJob *job, Error **errp)
     }
 }
 
-static int img_commit(int argc, char **argv)
+static int img_commit(const char *cmdname, int argc, char **argv)
 {
     int c, ret, flags;
     const char *filename, *fmt, *cache, *base;
@@ -979,10 +979,10 @@ static int img_commit(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -1028,7 +1028,7 @@ static int img_commit(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_exit(cmdname, "Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -1355,7 +1355,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t offset,
  * 1 - Images differ
  * >1 - Error occurred
  */
-static int img_compare(int argc, char **argv)
+static int img_compare(const char *cmdname, int argc, char **argv)
 {
     const char *fmt1 = NULL, *fmt2 = NULL, *cache, *filename1, *filename2;
     BlockBackend *blk1, *blk2;
@@ -1392,10 +1392,10 @@ static int img_compare(int argc, char **argv)
         }
         switch (c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -1449,7 +1449,7 @@ static int img_compare(int argc, char **argv)
 
 
     if (optind != argc - 2) {
-        error_exit("Expecting two image file names");
+        error_exit(cmdname, "Expecting two image file names");
     }
     filename1 = argv[optind++];
     filename2 = argv[optind++];
@@ -2231,7 +2231,7 @@ static void set_rate_limit(BlockBackend *blk, int64_t rate_limit)
     blk_set_io_limits(blk, &cfg);
 }
 
-static int img_convert(int argc, char **argv)
+static int img_convert(const char *cmdname, int argc, char **argv)
 {
     int c, bs_i, flags, src_flags = BDRV_O_NO_SHARE;
     const char *fmt = NULL, *out_fmt = NULL, *cache = "unsafe",
@@ -2284,10 +2284,10 @@ static int img_convert(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -2999,7 +2999,7 @@ err:
     return NULL;
 }
 
-static int img_info(int argc, char **argv)
+static int img_info(const char *cmdname, int argc, char **argv)
 {
     int c;
     OutputFormat output_format = OFORMAT_HUMAN;
@@ -3030,10 +3030,10 @@ static int img_info(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -3059,7 +3059,7 @@ static int img_info(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_exit(cmdname, "Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -3224,7 +3224,7 @@ static inline bool entry_mergeable(const MapEntry *curr, const MapEntry *next)
     return true;
 }
 
-static int img_map(int argc, char **argv)
+static int img_map(const char *cmdname, int argc, char **argv)
 {
     int c;
     OutputFormat output_format = OFORMAT_HUMAN;
@@ -3261,10 +3261,10 @@ static int img_map(int argc, char **argv)
         }
         switch (c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -3299,7 +3299,7 @@ static int img_map(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_exit(cmdname, "Expecting one image file name");
     }
     filename = argv[optind];
 
@@ -3373,7 +3373,7 @@ out:
 #define SNAPSHOT_APPLY  3
 #define SNAPSHOT_DELETE 4
 
-static int img_snapshot(int argc, char **argv)
+static int img_snapshot(const char *cmdname, int argc, char **argv)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
@@ -3404,10 +3404,10 @@ static int img_snapshot(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -3417,7 +3417,7 @@ static int img_snapshot(int argc, char **argv)
             break;
         case 'l':
             if (action) {
-                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
+                error_exit(cmdname, "Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_LIST;
@@ -3425,7 +3425,7 @@ static int img_snapshot(int argc, char **argv)
             break;
         case 'a':
             if (action) {
-                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
+                error_exit(cmdname, "Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_APPLY;
@@ -3433,7 +3433,7 @@ static int img_snapshot(int argc, char **argv)
             break;
         case 'c':
             if (action) {
-                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
+                error_exit(cmdname, "Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_CREATE;
@@ -3441,7 +3441,7 @@ static int img_snapshot(int argc, char **argv)
             break;
         case 'd':
             if (action) {
-                error_exit("Cannot mix '-l', '-a', '-c', '-d'");
+                error_exit(cmdname, "Cannot mix '-l', '-a', '-c', '-d'");
                 return 0;
             }
             action = SNAPSHOT_DELETE;
@@ -3463,7 +3463,7 @@ static int img_snapshot(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_exit(cmdname, "Expecting one image file name");
     }
     filename = argv[optind++];
 
@@ -3534,7 +3534,7 @@ static int img_snapshot(int argc, char **argv)
     return 0;
 }
 
-static int img_rebase(int argc, char **argv)
+static int img_rebase(const char *cmdname, int argc, char **argv)
 {
     BlockBackend *blk = NULL, *blk_old_backing = NULL, *blk_new_backing = NULL;
     uint8_t *buf_old = NULL;
@@ -3578,10 +3578,10 @@ static int img_rebase(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -3630,10 +3630,10 @@ static int img_rebase(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_exit(cmdname, "Expecting one image file name");
     }
     if (!unsafe && !out_baseimg) {
-        error_exit("Must specify backing file (-b) or use unsafe mode (-u)");
+        error_exit(cmdname, "Must specify backing file (-b) or use unsafe mode (-u)");
     }
     filename = argv[optind++];
 
@@ -4027,7 +4027,7 @@ out:
     return 0;
 }
 
-static int img_resize(int argc, char **argv)
+static int img_resize(const char *cmdname, int argc, char **argv)
 {
     Error *err = NULL;
     int c, ret, relative;
@@ -4057,7 +4057,7 @@ static int img_resize(int argc, char **argv)
     /* Remove size from argv manually so that negative numbers are not treated
      * as options by getopt. */
     if (argc < 3) {
-        error_exit("Not enough arguments");
+        error_exit(cmdname, "Not enough arguments");
         return 1;
     }
 
@@ -4081,10 +4081,10 @@ static int img_resize(int argc, char **argv)
         }
         switch(c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -4115,7 +4115,7 @@ static int img_resize(int argc, char **argv)
         }
     }
     if (optind != argc - 1) {
-        error_exit("Expecting image file name and size");
+        error_exit(cmdname, "Expecting image file name and size");
     }
     filename = argv[optind++];
 
@@ -4240,7 +4240,7 @@ static int print_amend_option_help(const char *format)
     return 0;
 }
 
-static int img_amend(int argc, char **argv)
+static int img_amend(const char *cmdname, int argc, char **argv)
 {
     Error *err = NULL;
     int c, ret = 0;
@@ -4273,10 +4273,10 @@ static int img_amend(int argc, char **argv)
 
         switch (c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -4312,7 +4312,7 @@ static int img_amend(int argc, char **argv)
     }
 
     if (!options) {
-        error_exit("Must specify options (-o)");
+        error_exit(cmdname, "Must specify options (-o)");
     }
 
     if (quiet) {
@@ -4504,7 +4504,7 @@ static void bench_cb(void *opaque, int ret)
     }
 }
 
-static int img_bench(int argc, char **argv)
+static int img_bench(const char *cmdname, int argc, char **argv)
 {
     int c, ret = 0;
     const char *fmt = NULL, *filename;
@@ -4547,10 +4547,10 @@ static int img_bench(int argc, char **argv)
 
         switch (c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -4674,7 +4674,7 @@ static int img_bench(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_exit(cmdname, "Expecting one image file name");
     }
     filename = argv[argc - 1];
 
@@ -4774,7 +4774,7 @@ typedef struct ImgBitmapAction {
     QSIMPLEQ_ENTRY(ImgBitmapAction) next;
 } ImgBitmapAction;
 
-static int img_bitmap(int argc, char **argv)
+static int img_bitmap(const char *cmdname, int argc, char **argv)
 {
     Error *err = NULL;
     int c, ret = 1;
@@ -4816,10 +4816,10 @@ static int img_bitmap(int argc, char **argv)
 
         switch (c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -5074,7 +5074,7 @@ static int img_dd_skip(const char *arg,
     return 0;
 }
 
-static int img_dd(int argc, char **argv)
+static int img_dd(const char *cmdname, int argc, char **argv)
 {
     int ret = 0;
     char *arg = NULL;
@@ -5136,10 +5136,10 @@ static int img_dd(int argc, char **argv)
             fmt = optarg;
             break;
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(cmdname, argv[optind - 1]);
             break;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
             help();
@@ -5342,7 +5342,7 @@ static void dump_json_block_measure_info(BlockMeasureInfo *info)
     g_string_free(str, true);
 }
 
-static int img_measure(int argc, char **argv)
+static int img_measure(const char *cmdname, int argc, char **argv)
 {
     static const struct option long_options[] = {
         {"help", no_argument, 0, 'h'},
@@ -5570,7 +5570,7 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QOM);
     bdrv_init();
     if (argc < 2) {
-        error_exit("Not enough arguments");
+        error_exit(NULL, "Not enough arguments");
     }
 
     qemu_add_opts(&qemu_source_opts);
@@ -5579,10 +5579,10 @@ int main(int argc, char **argv)
     while ((c = getopt_long(argc, argv, "+:hVT:", long_options, NULL)) != -1) {
         switch (c) {
         case ':':
-            missing_argument(argv[optind - 1]);
+            missing_argument(NULL, argv[optind - 1]);
             return 0;
         case '?':
-            unrecognized_option(argv[optind - 1]);
+            unrecognized_option(NULL, argv[optind - 1]);
             return 0;
         case 'h':
             help();
@@ -5615,10 +5615,10 @@ int main(int argc, char **argv)
     /* find the command */
     for (cmd = img_cmds; cmd->name != NULL; cmd++) {
         if (!strcmp(cmdname, cmd->name)) {
-            return cmd->handler(argc, argv);
+            return cmd->handler(cmdname, argc, argv);
         }
     }
 
     /* not found */
-    error_exit("Command not found: %s", cmdname);
+    error_exit(NULL, "Command not found: %s", cmdname);
 }
-- 
2.39.2

[PATCH 2/8] qemu-img: refresh options/--help for "create" subcommand

[Michael Tokarev]

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.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index a634747701..b94622fadb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -126,6 +126,16 @@ void unrecognized_option(const char *cmdname, const char *option)
     error_exit(cmdname, "unrecognized option '%s'", option);
 }
 
+static G_NORETURN
+void cmd_help(const char *cmdname, const char *syntax,
+              const char *description, const char *arguments)
+{
+    printf("qemu-img %s %s\n%s.\n"
+           " -h|--help - print this help and exit\n%s",
+           cmdname, syntax, description, arguments);
+    exit(EXIT_SUCCESS);
+}
+
 /* Please keep in synch with docs/tools/qemu-img.rst */
 static G_NORETURN
 void help(void)
@@ -524,7 +534,13 @@ static int img_create(const char *cmdname, 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 +556,22 @@ static int img_create(const char *cmdname, int argc, char **argv)
             unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
-            help();
+            cmd_help(cmdname, "[OPTIONS] FILE [SIZE[bkKMGTPE]]",
+                     "Creates and initializes a new image",
+" -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_IMG - stack new image on top of BACKING_IMG\n"
+"  (for formats which support stacking)\n"
+" -F|--backing-format BACKING_FMT - specify format of BACKING_IMG\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"
+" FILE - image file to create. FILE will be overriden if exists\n"
+" SIZE - image size with optional suffix: 'b' (byte, default),\n"
+"  'k' or 'K' (kilobyte, *1024), '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");
             break;
         case 'F':
             base_fmt = optarg;
-- 
2.39.2

[PATCH 3/8] qemu-img: factor out parse_output_format() and use it in the code

[Michael Tokarev]

Use common code and simplify error message

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 63 ++++++++++++++++--------------------------------------
 1 file changed, 18 insertions(+), 45 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b94622fadb..f851b290cc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -136,6 +136,17 @@ void cmd_help(const char *cmdname, const char *syntax,
     exit(EXIT_SUCCESS);
 }
 
+static OutputFormat parse_output_format(const char *cmdname, const char *arg)
+{
+    if (!strcmp(arg, "json")) {
+        return OFORMAT_JSON;
+    } else if (!strcmp(arg, "human")) {
+        return OFORMAT_HUMAN;
+    } else {
+        error_exit(cmdname, "--output expects 'human' or 'json' not '%s'", arg);
+    }
+}
+
 /* Please keep in synch with docs/tools/qemu-img.rst */
 static G_NORETURN
 void help(void)
@@ -751,7 +762,7 @@ static int img_check(const char *cmdname, int argc, char **argv)
 {
     int c, ret;
     OutputFormat output_format = OFORMAT_HUMAN;
-    const char *filename, *fmt, *output, *cache;
+    const char *filename, *fmt, *cache;
     BlockBackend *blk;
     BlockDriverState *bs;
     int fix = 0;
@@ -763,7 +774,6 @@ static int img_check(const char *cmdname, int argc, char **argv)
     bool force_share = false;
 
     fmt = NULL;
-    output = NULL;
     cache = BDRV_DEFAULT_CACHE;
 
     for(;;) {
@@ -809,7 +819,7 @@ static int img_check(const char *cmdname, int argc, char **argv)
             }
             break;
         case OPTION_OUTPUT:
-            output = optarg;
+            output_format = parse_output_format(cmdname, optarg);
             break;
         case 'T':
             cache = optarg;
@@ -833,15 +843,6 @@ static int img_check(const char *cmdname, int argc, char **argv)
     }
     filename = argv[optind++];
 
-    if (output && !strcmp(output, "json")) {
-        output_format = OFORMAT_JSON;
-    } else if (output && !strcmp(output, "human")) {
-        output_format = OFORMAT_HUMAN;
-    } else if (output) {
-        error_report("--output must be used with human or json as argument.");
-        return 1;
-    }
-
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -3035,13 +3036,12 @@ static int img_info(const char *cmdname, int argc, char **argv)
     int c;
     OutputFormat output_format = OFORMAT_HUMAN;
     bool chain = false;
-    const char *filename, *fmt, *output;
+    const char *filename, *fmt;
     BlockGraphInfoList *list;
     bool image_opts = false;
     bool force_share = false;
 
     fmt = NULL;
-    output = NULL;
     for(;;) {
         int option_index = 0;
         static const struct option long_options[] = {
@@ -3076,7 +3076,7 @@ static int img_info(const char *cmdname, int argc, char **argv)
             force_share = true;
             break;
         case OPTION_OUTPUT:
-            output = optarg;
+            output_format = parse_output_format(cmdname, optarg);
             break;
         case OPTION_BACKING_CHAIN:
             chain = true;
@@ -3094,15 +3094,6 @@ static int img_info(const char *cmdname, int argc, char **argv)
     }
     filename = argv[optind++];
 
-    if (output && !strcmp(output, "json")) {
-        output_format = OFORMAT_JSON;
-    } else if (output && !strcmp(output, "human")) {
-        output_format = OFORMAT_HUMAN;
-    } else if (output) {
-        error_report("--output must be used with human or json as argument.");
-        return 1;
-    }
-
     list = collect_image_info_list(image_opts, filename, fmt, chain,
                                    force_share);
     if (!list) {
@@ -3261,7 +3252,7 @@ static int img_map(const char *cmdname, int argc, char **argv)
     OutputFormat output_format = OFORMAT_HUMAN;
     BlockBackend *blk;
     BlockDriverState *bs;
-    const char *filename, *fmt, *output;
+    const char *filename, *fmt;
     int64_t length;
     MapEntry curr = { .length = 0 }, next;
     int ret = 0;
@@ -3271,7 +3262,6 @@ static int img_map(const char *cmdname, int argc, char **argv)
     int64_t max_length = -1;
 
     fmt = NULL;
-    output = NULL;
     for (;;) {
         int option_index = 0;
         static const struct option long_options[] = {
@@ -3307,7 +3297,7 @@ static int img_map(const char *cmdname, int argc, char **argv)
             force_share = true;
             break;
         case OPTION_OUTPUT:
-            output = optarg;
+            output_format = parse_output_format(cmdname, optarg);
             break;
         case 's':
             start_offset = cvtnum("start offset", optarg);
@@ -3334,15 +3324,6 @@ static int img_map(const char *cmdname, int argc, char **argv)
     }
     filename = argv[optind];
 
-    if (output && !strcmp(output, "json")) {
-        output_format = OFORMAT_JSON;
-    } else if (output && !strcmp(output, "human")) {
-        output_format = OFORMAT_HUMAN;
-    } else if (output) {
-        error_report("--output must be used with human or json as argument.");
-        return 1;
-    }
-
     blk = img_open(image_opts, filename, fmt, 0, false, false, force_share);
     if (!blk) {
         return 1;
@@ -5445,15 +5426,7 @@ static int img_measure(const char *cmdname, int argc, char **argv)
             image_opts = true;
             break;
         case OPTION_OUTPUT:
-            if (!strcmp(optarg, "json")) {
-                output_format = OFORMAT_JSON;
-            } else if (!strcmp(optarg, "human")) {
-                output_format = OFORMAT_HUMAN;
-            } else {
-                error_report("--output must be used with human or json "
-                             "as argument.");
-                goto out;
-            }
+            output_format = parse_output_format(cmdname, optarg);
             break;
         case OPTION_SIZE:
         {
-- 
2.39.2

[PATCH 4/8] qemu-img: refresh options/--help for "check" command

[Michael Tokarev]

Add missing long options and --help output.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index f851b290cc..e7d5b1b24b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -780,7 +780,9 @@ static int img_check(const char *cmdname, 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},
@@ -801,7 +803,20 @@ static int img_check(const char *cmdname, int argc, char **argv)
             unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
-            help();
+            cmd_help(cmdname, "[OPTIONS] FILENAME",
+                     "Checks image file validity",
+" -q|--quiet - quiet operations\n"
+" -f|--format FMT - specifies format of the image explicitly\n"
+" --image-opts - indicates that FILENAME is a complete image specification\n"
+"  instead of a file name (incompatible with --format)\n"
+" -T|--cache CACHE_MODE - cache mode when opening image (" BDRV_DEFAULT_CACHE ")\n"
+" -U|--force-share - open image in shared mode for concurrent access\n"
+" --output human|json - output format\n"
+" -r|--repair leaks|all - repair particular aspect of the image\n"
+"  (image will be open in read-write mode, incompatible with --force-share)\n"
+" --object OBJDEF - QEMU user-creatable object (eg encryption key)\n"
+" FILENAME - the image file (or image specification) to operate on\n"
+);
             break;
         case 'f':
             fmt = optarg;
-- 
2.39.2

[PATCH 5/8] qemu-img: simplify --repair error message

[Michael Tokarev]

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e7d5b1b24b..31c1891b43 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -829,8 +829,8 @@ static int img_check(const char *cmdname, int argc, char **argv)
             } else if (!strcmp(optarg, "all")) {
                 fix = BDRV_FIX_LEAKS | BDRV_FIX_ERRORS;
             } else {
-                error_exit(cmdname, "Unknown option value for -r "
-                           "(expecting 'leaks' or 'all'): %s", optarg);
+                error_exit(cmdname,
+                           "--repair expects 'leaks' or 'all' not '%s'", optarg);
             }
             break;
         case OPTION_OUTPUT:
-- 
2.39.2

[PATCH 6/8] qemu-img: refresh options/--help for "commit" command

[Michael Tokarev]

Add missing long options and --help output.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 31c1891b43..7ff63aeea2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1015,8 +1015,15 @@ static int img_commit(const char *cmdname, 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'},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, ":f:ht:b:dpqr:",
@@ -1032,7 +1039,20 @@ static int img_commit(const char *cmdname, int argc, char **argv)
             unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
-            help();
+            cmd_help(cmdname, "[OPTIONS] FILENAME",
+                     "Commit changes recorded in FILENAME to its base image",
+" -q|--quiet - quiet operations\n"
+" -f|--format FMT - specify FILENAME image format explicitly\n"
+" --image-opts - indicates that FILENAME is a complete image specification\n"
+"  instead of a file name (incompatible with --format)\n"
+" -t|--cache CACHE_MODE cache mode when opening image (" BDRV_DEFAULT_CACHE ")\n"
+" -d|--drop - skip emptying FILENAME on completion\n"
+" -b|--base BASE_IMG - image in the backing chain to which to commit\n"
+"  changes instead of the previous one (implies --drop)\n"
+" -p|--progress - show operation progress\n"
+" -r|--rate RATE - I/O rate limit\n"
+" --object OBJDEF - QEMU user-creatable object (eg encryption key)\n"
+" FILENAME - name of the image file to operate on\n");
             break;
         case 'f':
             fmt = optarg;
-- 
2.39.2

[PATCH 7/8] qemu-img: refresh options/--help for "compare" command

[Michael Tokarev]

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7ff63aeea2..ea3fe95169 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1447,9 +1447,15 @@ static int img_compare(const char *cmdname, 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},
+            {"cache", required_argument, 0, 'T'},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"format", required_argument, 0, 'f'},
+            {"format2?", required_argument, 0, 'F'},
             {"force-share", no_argument, 0, 'U'},
+            {"strict", no_argument, 0, 's'},
+            {"progress", no_argument, 0, 'p'},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, ":hf:F:T:pqsU",
@@ -1465,7 +1471,18 @@ static int img_compare(const char *cmdname, int argc, char **argv)
             unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
-            help();
+            cmd_help(cmdname, "[OPTIONS] FILENAME1 FILENAME2",
+                     "Check if two images have the same content",
+" -q|--quiet - quiet operation\n"
+" -T|--cache - CACHE_MODE cache mode when opening images (" BDRV_DEFAULT_CACHE ")\n"
+"  instead of file names (incompatible with --format)\n"
+" -f|--format FMT - specify FILENAME1 image format explicitly\n"
+" -F|--format2? FMT - specify FILENAME2 image format explicitly\n"
+" -U|--force-share - open images in shared mode for concurrent access\n"
+" -s|--strict - strict mode, also check if sizes are equal\n"
+" -p|--progress - show operation progress\n"
+" --object OBJDEF - QEMU user-creatable object (eg encryption key)\n"
+" FILENAME1, FILENAME2 - image files to compare\n");
             break;
         case 'f':
             fmt1 = optarg;
-- 
2.39.2

[PATCH 8/8] qemu-img: refresh options/--help for "convert" command

[Michael Tokarev]

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 qemu-img.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index ea3fe95169..c8fba27e35 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2351,14 +2351,28 @@ static int img_convert(const char *cmdname, 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},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"source-format", required_argument, 0, 'f'},
+            {"source-cache", required_argument, 0, 'T'},
+            {"snapshot", required_argument, 0, 'l'},
+            {"sparse-size", required_argument, 0, 'S'},
+            {"output-format", required_argument, 0, 'O'},
+            {"options", required_argument, 0, 'o'},
+            {"output-cache", required_argument, 0, 't'},
+            {"backing", required_argument, 0, 'B'},
+            {"backing-format", required_argument, 0, 'F'},
             {"force-share", no_argument, 0, 'U'},
             {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
             {"salvage", no_argument, 0, OPTION_SALVAGE},
             {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
             {"bitmaps", no_argument, 0, OPTION_BITMAPS},
             {"skip-broken-bitmaps", no_argument, 0, OPTION_SKIP_BROKEN},
+            {"rate", required_argument, 0, 'r'},
+            {"parallel", required_argument, 0, 'm'},
+            {"oob-writes", no_argument, 0, 'W'},
+            {"copy-range-offloading", no_argument, 0, 'C'},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, ":hf:O:B:CcF:o:l:S:pt:T:qnm:WUr:",
@@ -2374,7 +2388,37 @@ static int img_convert(const char *cmdname, int argc, char **argv)
             unrecognized_option(cmdname, argv[optind - 1]);
             break;
         case 'h':
-            help();
+            cmd_help(cmdname, "[OPTIONS] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME",
+                     "Convert image FILENAME (or a snapshot) to new image OUTPUT_FILENAME",
+" -q|--quiet - quiet operations\n"
+" -f|--source-format SFMT - specify FILENAME source image format explicitly\n"
+" --image-opts - indicates that FILENAME is a complete image specification\n"
+"  instead of a file name (incompatible with --source-format)\n"
+" -T|--source-cache CACHE_MODE - cache mode when opening source image (" BDRV_DEFAULT_CACHE ")\n"
+" -O|--output-format OFMT - specify OUTPUT_FILENAME image format (default is raw)\n"
+" --target-image-opts - indicates that OUTPUT_FILENAME is a complete image specification\n"
+"  instead of a file name (incompatible with --output-format)\n"
+" -o|--options OPTS - OFMT-specific options\n"
+" -c|--compress - create compressed output image (qcow and qcow2 format only)\n"
+" -t|--output-cache CACHE_MODE - cache mode when opening output image (" BDRV_DEFAULT_CACHE ")\n"
+" -B|--backing BACKING_FILENAME - create output to be a CoW on top of BACKING_FILENAME\n"
+" -F|--backing-format BFMT - specify BACKING_FILENAME image format explicitly\n"
+" --target-is-zero\n"
+" -l|--snapshot SNAPSHOT_PARAMS\n"
+" -S|--sparse-size SPARSE_SIZE\n"
+" --bitmaps - also copy any persistent bitmaps present in source\n"
+" --skip-broken-bitmaps - skip (do not error out) any broken bitmaps\n"
+" -U|--force-share - open images in shared mode for concurrent access\n"
+" -p|--progress - show operation progress\n"
+" -r|--rate RATE\n"
+" -m|--parallel NUM_COROUTINES - specify parallelism (default 8)\n"
+" -C|--copy-range-offloading - use copy_range offloading\n"
+" --salvage\n"
+" -W - enable out-of-order writes to improve performance\n"
+" -n|--no-create - omit target volume creation (eg on rbd)\n"
+" --object OBJDEF - QEMU user-creatable object (eg encryption key)\n"
+" FILENAME - source image file name\n"
+" OUTPUT_FILENAME - destination (output) image file name\n");
             break;
         case 'f':
             fmt = optarg;
-- 
2.39.2

Re: [RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working

[Daniel P. Berrangé]

On Wed, Feb 07, 2024 at 08:58:09PM +0300, Michael Tokarev wrote:

>  - create more or less consistent set of options between different
>    subcommands
>  - provide long options which can be used without figuring out which
>    -T/-t, -f|-F|-O etc to use for which of the two images given
>  - have qemu-img --help provide just a list of subcommands
>  - have qemu-img COMMAND --help to describe just this subcommand
>  - get rid of qemu-img-opts.hx, instead finish documentation in
>    qemu-img.rst based on the actual options implemented in
>    qemu-img.c.

Yes, yes, yes, yes & more yes :-)

I cry every time I have to read the qemu-img --help output,
and I'm not much of a fan of the man page either to be fair,
as I don't like the global list of options at the top which
is divorced from which commands actually use them.

These days I see many programs with subcommands switching to
a separate man page per sub-command. Still, I'm not asking
you todo that too, its just an idea for the gallery.

> I started converting subcommands one by one, providing long options
> and --help output.  And immediately faced some questions which needs
> wider discussion.
> 
>  o We have --image-opts and --target-image-opts.  Do we really need both?
>    In my view, --image-opts should be sort of global, turning *all*
>    filenames on this command line into complete image specifications,
>    there's no need to have separate image-opts and --target-image-opts.
>    Don't know what to do wrt compatibility though.  It shouldn't be made
>    this way from the beginning.  As a possible solution, introduce a new
>    option and deprecate current set.

This is basically a crutch for incomplete conversion of the block
layer APIs, which meant we had a situation where we wanted to use
image opts syntax for the source, but were unable todo so for
the target:

  commit 305b4c60f200ee8e6267ac75f3f5b5d09fda1079
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Mon May 15 17:47:11 2017 +0100

    qemu-img: introduce --target-image-opts for 'convert' command
    
    The '--image-opts' flag indicates whether the source filename
    includes options. The target filename has to remain in the
    plain filename format though, since it needs to be passed to
    bdrv_create().  When using --skip-create though, it would be
    possible to use image-opts syntax. This adds --target-image-opts
    to indicate that the target filename includes options. Currently
    this mandates use of the --skip-create flag too.

we do have internal support for creating block devices using
the full QAPI schema, via BlockdevCreateOptions.

I'm not sure if bdrv_create can be made to create using the
image-opts syntax. If it can, there is still the additional
problem that after creation it then needs to re-open the
file, and the image-opts for open is defined by BlockdevOptions
not BlockdevCreateOptions. So we would need a way to convert
from the latter to the former.



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

Re: [RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working

[Manos Pitsidianakis]

Hello Michael,

Such changes are long overdue. However given the complexity of
commands and arguments, maybe it'd be a good idea to write a code
generator for the command line interface, This way you could also
generate --help outputs, manpages, shell completions just from the
command line spec we'd use to generate the argv parsing routines.


On Wed, 7 Feb 2024 at 19:58, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> This is an incomplete first attempt only, there's a lot left to do.
>
> All the options in qemu-img is a complete mess, - no, inconsistent or
> incomplete syntax in documentation, many undocumented options, option
> names are used inconsistently and differently for different commands,
> no long options exists for many short options, --help output is a huge
> mess by its own, and the way how it all is generated is another story.
> docs/tools/qemu-img.rst with qemu-img-opts.hx is yet another.
>
> I hoped to fix just an option or two, but it ended up in a large task,
> and I need some help and discussion, hence the RFC.
>
> The idea is:
>
>  - create more or less consistent set of options between different
>    subcommands
>  - provide long options which can be used without figuring out which
>    -T/-t, -f|-F|-O etc to use for which of the two images given
>  - have qemu-img --help provide just a list of subcommands
>  - have qemu-img COMMAND --help to describe just this subcommand
>  - get rid of qemu-img-opts.hx, instead finish documentation in
>    qemu-img.rst based on the actual options implemented in
>    qemu-img.c.
>
> I started converting subcommands one by one, providing long options
> and --help output.  And immediately faced some questions which needs
> wider discussion.
>
>  o We have --image-opts and --target-image-opts.  Do we really need both?
>    In my view, --image-opts should be sort of global, turning *all*
>    filenames on this command line into complete image specifications,
>    there's no need to have separate image-opts and --target-image-opts.
>    Don't know what to do wrt compatibility though.  It shouldn't be made
>    this way from the beginning.  As a possible solution, introduce a new
>    option and deprecate current set.
>
>  o For conversion (convert, dd, etc), we've source and destination,
>    so it's easy to distinguish using long options, like --source-format
>    --target-cache etc (-t/-T -f/-F is a mess here already).  What to
>    do with compare? --format1 --format2 is ugly, maybe --a-format and
>    --b-format?  Maybe we can get off with --source (a) and --target (b)
>    instead of filename1 & filename2?
>    (--cache in this context is global for both).
>
>  o qemu-img convert.  It's the most messy one, and it is not really
>    documented (nor in qemu-img.rst , eg there's nothing in there about
>    FILENAME2, -B is difficult to understand, etc).
>    At the very least, I'd say the options should be
>     --source-format, --source-cache etc
>     --target-format, --target-options
>     --target-image-opts - this shold go IMHO
>
>  o check and commit - inconsistent cache flags?
>    In convert, cache is backwards (source/target)?
>
> At first, I tried to have more or less common option descriptions,
> using various parameters, variables or #defines, but in different
> commands the same options has slightly different wording, and in
> some option names are different, so it looks like it's best to
> have complete text in each subcommand.
>
>
> Michael Tokarev (8):
>   qemu-img: pass current cmdname into command handlers
>   qemu-img: refresh options/--help for "create" subcommand
>   qemu-img: factor out parse_output_format() and use it in the code
>    (this one has been sent in a separate patch, here it is just for completness)
>   qemu-img: refresh options/--help for "check" command
>   qemu-img: simplify --repair error message
>   qemu-img: refresh options/--help for "commit" command
>   qemu-img: refresh options/--help for "compare" command
>   qemu-img: refresh options/--help for "convert" command
>
>  qemu-img.c | 352 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 226 insertions(+), 126 deletions(-)
>
> --
> 2.39.2
>
>


-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

Re: [RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working

[Michael Tokarev]

07.02.2024 22:01, Manos Pitsidianakis:
> Hello Michael,
> 
> Such changes are long overdue. However given the complexity of
> commands and arguments, maybe it'd be a good idea to write a code
> generator for the command line interface, This way you could also
> generate --help outputs, manpages, shell completions just from the
> command line spec we'd use to generate the argv parsing routines.

I tried starting with just a set of common options in --help output,
but quickly abandoned that idea.  Even there, and I mentioned that
in the email you're replying to, we should have slightly different
text in different contexts.  So it seems like it's better to just
write it in two places.  Two *different* places, - which is the
--help output and qemu-img.rst documentation (from which the manpage
is generated).  The two places are really different, because --help
is just a very brief usage/options summary, while the doc is a much
more complete descriptive guide.

There's one more context, - the option parsing code. There are ways
to make it easier, like libpopt for example, but in my view, in an
attempt to make life easier, it makes it even more complex.  In my
taste anyway, YMMV.

In short, - while this stuff seems like a good candidate for some
automation, it might be not, and the first step IMHO is to get the
first task done, - to review and refresh all options, see what can
be done with the messy difference of the meanings for subcommands,
describe them.  Maybe after that whole thing can be automated (if
it's possible to do with this differently-named hell and with
readable output).

/mjt

Re: [RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working

[Kevin Wolf]

Am 07.02.2024 um 18:58 hat Michael Tokarev geschrieben:
> This is an incomplete first attempt only, there's a lot left to do.
> 
> All the options in qemu-img is a complete mess, - no, inconsistent or
> incomplete syntax in documentation, many undocumented options, option
> names are used inconsistently and differently for different commands,
> no long options exists for many short options, --help output is a huge
> mess by its own, and the way how it all is generated is another story.
> docs/tools/qemu-img.rst with qemu-img-opts.hx is yet another.
> 
> I hoped to fix just an option or two, but it ended up in a large task,
> and I need some help and discussion, hence the RFC.
> 
> The idea is:
> 
>  - create more or less consistent set of options between different
>    subcommands
>  - provide long options which can be used without figuring out which
>    -T/-t, -f|-F|-O etc to use for which of the two images given
>  - have qemu-img --help provide just a list of subcommands
>  - have qemu-img COMMAND --help to describe just this subcommand

The help desperately needs some cleanup like this, so thank you for
doing something about it.

>  - get rid of qemu-img-opts.hx, instead finish documentation in
>    qemu-img.rst based on the actual options implemented in
>    qemu-img.c.

You mean qemu-img-cmds.hx? The one advantage it has is that it makes it
obvious if there is a mismatch in the options we show in the help output
and in the documentation.

But I'm not overly concerned either way. I would probably have left it
alone just because leaving it is less work than changing it and the
result isn't very different.

> I started converting subcommands one by one, providing long options
> and --help output.  And immediately faced some questions which needs
> wider discussion.
> 
>  o We have --image-opts and --target-image-opts.  Do we really need both?
>    In my view, --image-opts should be sort of global, turning *all*
>    filenames on this command line into complete image specifications,
>    there's no need to have separate image-opts and --target-image-opts.
>    Don't know what to do wrt compatibility though.  It shouldn't be made
>    this way from the beginning.  As a possible solution, introduce a new
>    option and deprecate current set.

I think it's better not to touch things like this. qemu-img is much more
likely to be used directly by human users (and their small scripts) than
QEMU itself, so we need to be even more careful with deprecating things.

In fact, I'm not even sure if combining them would make it easier to
use. Often, it's only source _or_ target that have a complicated setup
that requires blockdev-type descriptions. As a human user, I prefer if I
can still just use the file name for the other image instead of getting
the full -blockdev verbosity there, too.

>  o For conversion (convert, dd, etc), we've source and destination,
>    so it's easy to distinguish using long options, like --source-format
>    --target-cache etc (-t/-T -f/-F is a mess here already).  What to
>    do with compare? --format1 --format2 is ugly, maybe --a-format and
>    --b-format?  Maybe we can get off with --source (a) and --target (b)
>    instead of filename1 & filename2?
>    (--cache in this context is global for both).

For those commands where there is a source and a target, --source-format
and --target-format have a clear advantage, they are easy to remember
and hard to confuse.

For compare, as you saw, there is no clear naming. 1/2 or a/b don't make
the command any clearer than -f/-F. So maybe just don't add long
versions there?

>  o qemu-img convert.  It's the most messy one, and it is not really
>    documented (nor in qemu-img.rst , eg there's nothing in there about
>    FILENAME2, -B is difficult to understand, etc).
>    At the very least, I'd say the options should be
>     --source-format, --source-cache etc
>     --target-format, --target-options

These seem obvious. (Actually, --target-options vs. just --options isn't
completely obvious - after all, there are fundamentally no create
options for source.)

--source-* are also a bit weird because 'qemu-img convert' takes
multiple source images and then it applies the same format to all of
them. But that's more about how it works, so not a problem with the
option _name_.

>     --target-image-opts - this shold go IMHO

This one less so. Even generally speaking, changing interfaces
incompatibly comes with a cost that is probably too big to justify it
just for interfaces that look a bit nicer, but don't provide any new
functionality.

And as I said above, I don't agree that image-opts should be global.

>  o check and commit - inconsistent cache flags?
>    In convert, cache is backwards (source/target)?

It's a bit messy because the cache mode options -T/-t work the opposite
way of -f/-F. But I think the commands are consistent with each other?

-T was added as a source cache parameter to all of the subcommands at
the same time, in commit 40055951a.

> At first, I tried to have more or less common option descriptions,
> using various parameters, variables or #defines, but in different
> commands the same options has slightly different wording, and in
> some option names are different, so it looks like it's best to
> have complete text in each subcommand.

Yeah, let's not make this more complicated than it has to be.

Kevin