[Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options

[Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options

[Kevin Wolf]

If you specify the same option more than once (e.g. -o cluster_size=4k
-o lazy_refcounts=on), qemu-img silently ignores all but the last one. This
series fixes it to either consider all options or to give an error message.

Boolean option can still be given more than once as they aren't problematic
in this respect (ten times -q is still quiet).

Kevin Wolf (7):
  qemu-img create: Detect options specified more than once
  qemu-img convert: Detect options specified more than once
  qemu-img amend: Detect options specified more than once
  qemu-img: Detect options specified more than once
  qemu-img create: Support multiple -o options
  qemu-img convert: Support multiple -o options
  qemu-img amend: Support multiple -o options

 qemu-img.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 177 insertions(+), 21 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once

[Kevin Wolf]

If you specified multiple -o options for qemu-img create, it would
silently ignore all but the last one. Similarly, for other options the
last occurence wins (which is at least a bit less surprising). Error out
instead.

The only exception is a -o help option, which may be added to any valid
qemu-img create command and ignores all other options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c989850..6a64fe1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -341,13 +341,14 @@ static int img_create(int argc, char **argv)
 {
     int c;
     uint64_t img_size = -1;
-    const char *fmt = "raw";
+    const char *fmt = NULL;
     const char *base_fmt = NULL;
     const char *filename;
     const char *base_filename = NULL;
     char *options = NULL;
     Error *local_err = NULL;
     bool quiet = false;
+    bool options_help = false;
 
     for(;;) {
         c = getopt(argc, argv, "F:b:f:he6o:q");
@@ -360,12 +361,24 @@ static int img_create(int argc, char **argv)
             help();
             break;
         case 'F':
+            if (base_fmt) {
+                error_report("-F may only be specified once");
+                return 1;
+            }
             base_fmt = optarg;
             break;
         case 'b':
+            if (base_filename) {
+                error_report("-b may only be specified once");
+                return 1;
+            }
             base_filename = optarg;
             break;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case 'e':
@@ -377,7 +390,16 @@ static int img_create(int argc, char **argv)
                   "compat6\' instead!");
             return 1;
         case 'o':
-            options = optarg;
+            if (is_help_option(optarg)) {
+                options_help = true;
+            } else if (!options) {
+                options = optarg;
+            } else {
+                error_report("-o cannot be used multiple times. Please use a "
+                             "single -o option with comma-separated settings "
+                             "instead.");
+                return 1;
+            }
             break;
         case 'q':
             quiet = true;
@@ -385,6 +407,10 @@ static int img_create(int argc, char **argv)
         }
     }
 
+    if (!fmt) {
+        fmt = "raw";
+    }
+
     /* Get the filename */
     if (optind >= argc) {
         help();
@@ -413,7 +439,7 @@ static int img_create(int argc, char **argv)
         help();
     }
 
-    if (options && is_help_option(options)) {
+    if (options_help) {
         return print_block_option_help(filename, fmt);
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/7] qemu-img convert: Detect options specified more than once

[Kevin Wolf]

Instead of ignoring all option values but the last one, error out if an
option is set multiple times.

Again, the only exception is a -o help option, which may be added to any
valid qemu-img convert command and ignores all other options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6a64fe1..55c2c75 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1170,6 +1170,7 @@ static int img_convert(int argc, char **argv)
     QEMUOptionParameter *param = NULL, *create_options = NULL;
     QEMUOptionParameter *out_baseimg_param;
     char *options = NULL;
+    bool options_help = false;
     const char *snapshot_name = NULL;
     int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
     bool quiet = false;
@@ -1177,8 +1178,8 @@ static int img_convert(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
 
     fmt = NULL;
-    out_fmt = "raw";
-    cache = "unsafe";
+    out_fmt = NULL;
+    cache = NULL;
     out_baseimg = NULL;
     compress = 0;
     skip_create = 0;
@@ -1193,12 +1194,24 @@ static int img_convert(int argc, char **argv)
             help();
             break;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case 'O':
+            if (out_fmt) {
+                error_report("-O may only be specified once");
+                return 1;
+            }
             out_fmt = optarg;
             break;
         case 'B':
+            if (out_baseimg) {
+                error_report("-B may only be specified once");
+                return 1;
+            }
             out_baseimg = optarg;
             break;
         case 'c':
@@ -1213,12 +1226,29 @@ static int img_convert(int argc, char **argv)
                   "compat6\' instead!");
             return 1;
         case 'o':
-            options = optarg;
+            if (is_help_option(optarg)) {
+                options_help = true;
+            } else if (!options) {
+                options = optarg;
+            } else {
+                error_report("-o cannot be used multiple times. Please use a "
+                             "single -o option with comma-separated settings "
+                             "instead.");
+                return 1;
+            }
             break;
         case 's':
+            if (sn_opts || snapshot_name) {
+                error_report("-l/-s may only be specified once");
+                return 1;
+            }
             snapshot_name = optarg;
             break;
         case 'l':
+            if (sn_opts || snapshot_name) {
+                error_report("-l/-s may only be specified once");
+                return 1;
+            }
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
                 sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
                 if (!sn_opts) {
@@ -1247,6 +1277,10 @@ static int img_convert(int argc, char **argv)
             progress = 1;
             break;
         case 't':
+            if (cache) {
+                error_report("-t may only be specified once");
+                return 1;
+            }
             cache = optarg;
             break;
         case 'q':
@@ -1258,6 +1292,13 @@ static int img_convert(int argc, char **argv)
         }
     }
 
+    if (!out_fmt) {
+        out_fmt = "raw";
+    }
+    if (!cache) {
+        cache = "unsafe";
+    }
+
     if (quiet) {
         progress = 0;
     }
@@ -1272,7 +1313,7 @@ static int img_convert(int argc, char **argv)
     /* Initialize before goto out */
     qemu_progress_init(progress, 1.0);
 
-    if (options && is_help_option(options)) {
+    if (options_help) {
         ret = print_block_option_help(out_filename, out_fmt);
         goto out;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/7] qemu-img amend: Detect options specified more than once

[Kevin Wolf]

Instead of ignoring all option values but the last one, error out if an
option is set multiple times.

Again, the only exception is a -o help option, which may be added to any
valid qemu-img amend command and ignores all other options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 55c2c75..247987d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2687,6 +2687,7 @@ static int img_amend(int argc, char **argv)
 {
     int c, ret = 0;
     char *options = NULL;
+    bool options_help = false;
     QEMUOptionParameter *create_options = NULL, *options_param = NULL;
     const char *fmt = NULL, *filename;
     bool quiet = false;
@@ -2704,9 +2705,22 @@ static int img_amend(int argc, char **argv)
                 help();
                 break;
             case 'o':
-                options = optarg;
+                if (is_help_option(optarg)) {
+                    options_help = true;
+                } else if (!options) {
+                    options = optarg;
+                } else {
+                    error_report("-o cannot be used multiple times. Please use a "
+                                 "single -o option with comma-separated settings "
+                                 "instead.");
+                    return 1;
+                }
                 break;
             case 'f':
+                if (fmt) {
+                    error_report("-f may only be specified once");
+                    return 1;
+                }
                 fmt = optarg;
                 break;
             case 'q':
@@ -2734,7 +2748,7 @@ static int img_amend(int argc, char **argv)
 
     fmt = bs->drv->format_name;
 
-    if (is_help_option(options)) {
+    if (options_help) {
         ret = print_block_option_help(filename, fmt);
         goto out;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/7] qemu-img: Detect options specified more than once

[Kevin Wolf]

Instead of ignoring all option values but the last one, error out if an
option is set multiple times. These are the simpler subcommands without
-o, so the patch becomes a bit easier and we can fix the remaining
subcommands in a single patch.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 247987d..a889c84 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -594,6 +594,10 @@ static int img_check(int argc, char **argv)
             help();
             break;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case 'r':
@@ -608,6 +612,10 @@ static int img_check(int argc, char **argv)
             }
             break;
         case OPTION_OUTPUT:
+            if (output) {
+                error_report("--output may only be specified once");
+                return 1;
+            }
             output = optarg;
             break;
         case 'q':
@@ -716,9 +724,17 @@ static int img_commit(int argc, char **argv)
             help();
             break;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case 't':
+            if (cache) {
+                error_report("-t may only be specified once");
+                return 1;
+            }
             cache = optarg;
             break;
         case 'q':
@@ -949,9 +965,17 @@ static int img_compare(int argc, char **argv)
             help();
             break;
         case 'f':
+            if (fmt1) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt1 = optarg;
             break;
         case 'F':
+            if (fmt2) {
+                error_report("-F may only be specified once");
+                return 1;
+            }
             fmt2 = optarg;
             break;
         case 'p':
@@ -1906,9 +1930,17 @@ static int img_info(int argc, char **argv)
             help();
             break;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case OPTION_OUTPUT:
+            if (output) {
+                error_report("--output may only be specified once");
+                return 1;
+            }
             output = optarg;
             break;
         case OPTION_BACKING_CHAIN:
@@ -2074,9 +2106,17 @@ static int img_map(int argc, char **argv)
             help();
             break;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case OPTION_OUTPUT:
+            if (output) {
+                error_report("--output may only be specified once");
+                return 1;
+            }
             output = optarg;
             break;
         }
@@ -2282,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
 
     /* Parse commandline parameters */
     fmt = NULL;
-    cache = BDRV_DEFAULT_CACHE;
+    cache = NULL;
     out_baseimg = NULL;
     out_basefmt = NULL;
     for(;;) {
@@ -2296,12 +2336,24 @@ static int img_rebase(int argc, char **argv)
             help();
             return 0;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case 'F':
+            if (out_basefmt) {
+                error_report("-F may only be specified once");
+                return 1;
+            }
             out_basefmt = optarg;
             break;
         case 'b':
+            if (out_baseimg) {
+                error_report("-b may only be specified once");
+                return 1;
+            }
             out_baseimg = optarg;
             break;
         case 'u':
@@ -2311,6 +2363,10 @@ static int img_rebase(int argc, char **argv)
             progress = 1;
             break;
         case 't':
+            if (cache) {
+                error_report("-t may only be specified once");
+                return 1;
+            }
             cache = optarg;
             break;
         case 'q':
@@ -2319,6 +2375,10 @@ static int img_rebase(int argc, char **argv)
         }
     }
 
+    if (!cache) {
+        cache = BDRV_DEFAULT_CACHE;
+    }
+
     if (quiet) {
         progress = 0;
     }
@@ -2603,6 +2663,10 @@ static int img_resize(int argc, char **argv)
             help();
             break;
         case 'f':
+            if (fmt) {
+                error_report("-f may only be specified once");
+                return 1;
+            }
             fmt = optarg;
             break;
         case 'q':
-- 
1.8.1.4

[Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options

[Kevin Wolf]

Multiple -o options has the same meaning as having a single option with
all settings in the order of their respective -o options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index a889c84..30273ad 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -384,21 +384,18 @@ static int img_create(int argc, char **argv)
         case 'e':
             error_report("option -e is deprecated, please use \'-o "
                   "encryption\' instead!");
-            return 1;
+            goto fail;
         case '6':
             error_report("option -6 is deprecated, please use \'-o "
                   "compat6\' instead!");
-            return 1;
+            goto fail;
         case 'o':
             if (is_help_option(optarg)) {
                 options_help = true;
             } else if (!options) {
-                options = optarg;
+                options = g_strdup(optarg);
             } else {
-                error_report("-o cannot be used multiple times. Please use a "
-                             "single -o option with comma-separated settings "
-                             "instead.");
-                return 1;
+                options = g_strdup_printf("%s,%s", options, optarg);
             }
             break;
         case 'q':
@@ -431,7 +428,7 @@ static int img_create(int argc, char **argv)
                 error_report("kilobytes, megabytes, gigabytes, terabytes, "
                              "petabytes and exabytes.");
             }
-            return 1;
+            goto fail;
         }
         img_size = (uint64_t)sval;
     }
@@ -440,6 +437,7 @@ static int img_create(int argc, char **argv)
     }
 
     if (options_help) {
+        g_free(options);
         return print_block_option_help(filename, fmt);
     }
 
@@ -448,10 +446,15 @@ static int img_create(int argc, char **argv)
     if (error_is_set(&local_err)) {
         error_report("%s: %s", filename, error_get_pretty(local_err));
         error_free(local_err);
-        return 1;
+        goto fail;
     }
 
+    g_free(options);
     return 0;
+
+fail:
+    g_free(options);
+    return 1;
 }
 
 static void dump_json_image_check(ImageCheck *check, bool quiet)
-- 
1.8.1.4

[Qemu-devel] [PATCH 6/7] qemu-img convert: Support multiple -o options

[Kevin Wolf]

Multiple -o options has the same meaning as having a single option with
all settings in the order of their respective -o options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 30273ad..e0d19f8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1204,6 +1204,9 @@ static int img_convert(int argc, char **argv)
     Error *local_err = NULL;
     QemuOpts *sn_opts = NULL;
 
+    /* Initialize before goto out */
+    qemu_progress_init(progress, 1.0);
+
     fmt = NULL;
     out_fmt = NULL;
     cache = NULL;
@@ -1223,21 +1226,24 @@ static int img_convert(int argc, char **argv)
         case 'f':
             if (fmt) {
                 error_report("-f may only be specified once");
-                return 1;
+                ret = -1;
+                goto out;
             }
             fmt = optarg;
             break;
         case 'O':
             if (out_fmt) {
                 error_report("-O may only be specified once");
-                return 1;
+                ret = -1;
+                goto out;
             }
             out_fmt = optarg;
             break;
         case 'B':
             if (out_baseimg) {
                 error_report("-B may only be specified once");
-                return 1;
+                ret = -1;
+                goto out;
             }
             out_baseimg = optarg;
             break;
@@ -1247,41 +1253,43 @@ static int img_convert(int argc, char **argv)
         case 'e':
             error_report("option -e is deprecated, please use \'-o "
                   "encryption\' instead!");
-            return 1;
+            ret = -1;
+            goto out;
         case '6':
             error_report("option -6 is deprecated, please use \'-o "
                   "compat6\' instead!");
-            return 1;
+            ret = -1;
+            goto out;
         case 'o':
             if (is_help_option(optarg)) {
                 options_help = true;
             } else if (!options) {
-                options = optarg;
+                options = g_strdup(optarg);
             } else {
-                error_report("-o cannot be used multiple times. Please use a "
-                             "single -o option with comma-separated settings "
-                             "instead.");
-                return 1;
+                options = g_strdup_printf("%s,%s", options, optarg);
             }
             break;
         case 's':
             if (sn_opts || snapshot_name) {
                 error_report("-l/-s may only be specified once");
-                return 1;
+                ret = -1;
+                goto out;
             }
             snapshot_name = optarg;
             break;
         case 'l':
             if (sn_opts || snapshot_name) {
                 error_report("-l/-s may only be specified once");
-                return 1;
+                ret = -1;
+                goto out;
             }
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
                 sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
                 if (!sn_opts) {
                     error_report("Failed in parsing snapshot param '%s'",
                                  optarg);
-                    return 1;
+                    ret = -1;
+                    goto out;
                 }
             } else {
                 snapshot_name = optarg;
@@ -1294,7 +1302,8 @@ static int img_convert(int argc, char **argv)
             sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);
             if (sval < 0 || *end) {
                 error_report("Invalid minimum zero buffer size for sparse output specified");
-                return 1;
+                ret = -1;
+                goto out;
             }
 
             min_sparse = sval / BDRV_SECTOR_SIZE;
@@ -1306,7 +1315,8 @@ static int img_convert(int argc, char **argv)
         case 't':
             if (cache) {
                 error_report("-t may only be specified once");
-                return 1;
+                ret = -1;
+                goto out;
             }
             cache = optarg;
             break;
@@ -1337,9 +1347,6 @@ static int img_convert(int argc, char **argv)
 
     out_filename = argv[argc - 1];
 
-    /* Initialize before goto out */
-    qemu_progress_init(progress, 1.0);
-
     if (options_help) {
         ret = print_block_option_help(out_filename, out_fmt);
         goto out;
@@ -1733,6 +1740,7 @@ out:
     free_option_parameters(create_options);
     free_option_parameters(param);
     qemu_vfree(buf);
+    g_free(options);
     if (sn_opts) {
         qemu_opts_del(sn_opts);
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 7/7] qemu-img amend: Support multiple -o options

[Kevin Wolf]

Multiple -o options has the same meaning as having a single option with
all settings in the order of their respective -o options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e0d19f8..1a98926 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2783,18 +2783,16 @@ static int img_amend(int argc, char **argv)
                 if (is_help_option(optarg)) {
                     options_help = true;
                 } else if (!options) {
-                    options = optarg;
+                    options = g_strdup(optarg);
                 } else {
-                    error_report("-o cannot be used multiple times. Please use a "
-                                 "single -o option with comma-separated settings "
-                                 "instead.");
-                    return 1;
+                    options = g_strdup_printf("%s,%s", options, optarg);
                 }
                 break;
             case 'f':
                 if (fmt) {
                     error_report("-f may only be specified once");
-                    return 1;
+                    ret = -1;
+                    goto out;
                 }
                 fmt = optarg;
                 break;
@@ -2850,6 +2848,8 @@ out:
     }
     free_option_parameters(create_options);
     free_option_parameters(options_param);
+    g_free(options);
+
     if (ret) {
         return 1;
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once

[Fam Zheng]

On Wed, 02/19 16:12, Kevin Wolf wrote:
> If you specified multiple -o options for qemu-img create, it would
> silently ignore all but the last one. Similarly, for other options the
> last occurence wins (which is at least a bit less surprising). Error out
> instead.
> 
> The only exception is a -o help option, which may be added to any valid
> qemu-img create command and ignores all other options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-img.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index c989850..6a64fe1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -341,13 +341,14 @@ static int img_create(int argc, char **argv)
>  {
>      int c;
>      uint64_t img_size = -1;
> -    const char *fmt = "raw";
> +    const char *fmt = NULL;
>      const char *base_fmt = NULL;
>      const char *filename;
>      const char *base_filename = NULL;
>      char *options = NULL;
>      Error *local_err = NULL;
>      bool quiet = false;
> +    bool options_help = false;
>  
>      for(;;) {
>          c = getopt(argc, argv, "F:b:f:he6o:q");
> @@ -360,12 +361,24 @@ static int img_create(int argc, char **argv)
>              help();
>              break;
>          case 'F':
> +            if (base_fmt) {
> +                error_report("-F may only be specified once");
> +                return 1;
> +            }
>              base_fmt = optarg;
>              break;
>          case 'b':
> +            if (base_filename) {
> +                error_report("-b may only be specified once");
> +                return 1;
> +            }
>              base_filename = optarg;
>              break;
>          case 'f':
> +            if (fmt) {
> +                error_report("-f may only be specified once");
> +                return 1;
> +            }
>              fmt = optarg;
>              break;
>          case 'e':
> @@ -377,7 +390,16 @@ static int img_create(int argc, char **argv)
>                    "compat6\' instead!");
>              return 1;
>          case 'o':
> -            options = optarg;
> +            if (is_help_option(optarg)) {
> +                options_help = true;
> +            } else if (!options) {
> +                options = optarg;
> +            } else {
> +                error_report("-o cannot be used multiple times. Please use a "
> +                             "single -o option with comma-separated settings "
> +                             "instead.");

Not consistent with ending "." but not a big problem.

Reviewed-by: Fam Zheng <famz@redhat.com>

> +                return 1;
> +            }
>              break;
>          case 'q':
>              quiet = true;
> @@ -385,6 +407,10 @@ static int img_create(int argc, char **argv)
>          }
>      }
>  
> +    if (!fmt) {
> +        fmt = "raw";
> +    }
> +
>      /* Get the filename */
>      if (optind >= argc) {
>          help();
> @@ -413,7 +439,7 @@ static int img_create(int argc, char **argv)
>          help();
>      }
>  
> -    if (options && is_help_option(options)) {
> +    if (options_help) {
>          return print_block_option_help(filename, fmt);
>      }
>  
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options

[Fam Zheng]

On Wed, 02/19 16:12, Kevin Wolf wrote:
> Multiple -o options has the same meaning as having a single option with
> all settings in the order of their respective -o options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-img.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index a889c84..30273ad 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -384,21 +384,18 @@ static int img_create(int argc, char **argv)
>          case 'e':
>              error_report("option -e is deprecated, please use \'-o "
>                    "encryption\' instead!");
> -            return 1;
> +            goto fail;
>          case '6':
>              error_report("option -6 is deprecated, please use \'-o "
>                    "compat6\' instead!");
> -            return 1;
> +            goto fail;
>          case 'o':
>              if (is_help_option(optarg)) {
>                  options_help = true;
>              } else if (!options) {
> -                options = optarg;
> +                options = g_strdup(optarg);
>              } else {
> -                error_report("-o cannot be used multiple times. Please use a "
> -                             "single -o option with comma-separated settings "
> -                             "instead.");
> -                return 1;
> +                options = g_strdup_printf("%s,%s", options, optarg);

OK. So what's done in patch 1 is masked here. Why not squash them and avoid the
intermediate state?  And, isn't original options leaked here?

Fam

>              }
>              break;
>          case 'q':
> @@ -431,7 +428,7 @@ static int img_create(int argc, char **argv)
>                  error_report("kilobytes, megabytes, gigabytes, terabytes, "
>                               "petabytes and exabytes.");
>              }
> -            return 1;
> +            goto fail;
>          }
>          img_size = (uint64_t)sval;
>      }
> @@ -440,6 +437,7 @@ static int img_create(int argc, char **argv)
>      }
>  
>      if (options_help) {
> +        g_free(options);
>          return print_block_option_help(filename, fmt);
>      }
>  
> @@ -448,10 +446,15 @@ static int img_create(int argc, char **argv)
>      if (error_is_set(&local_err)) {
>          error_report("%s: %s", filename, error_get_pretty(local_err));
>          error_free(local_err);
> -        return 1;
> +        goto fail;
>      }
>  
> +    g_free(options);
>      return 0;
> +
> +fail:
> +    g_free(options);
> +    return 1;
>  }
>  
>  static void dump_json_image_check(ImageCheck *check, bool quiet)
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On 02/19/2014 08:12 AM, Kevin Wolf wrote:
> If you specified multiple -o options for qemu-img create, it would
> silently ignore all but the last one. Similarly, for other options the
> last occurence wins (which is at least a bit less surprising). Error out

s/occurence/occurrence/

> instead.

For other options, erroring out is okay.  But for -o, I would prefer if
we could concatenate multiple -o as if they had been passed in one
larger -o, rather than erroring out.  That is, I'd rather treat:

-o backing_file=/path/to/foo -o backing_fmt=qcow2

as a synonym of

-o backing_file=/path/to/foo,backing_fmt=qcow2

> 
> The only exception is a -o help option, which may be added to any valid
> qemu-img create command and ignores all other options.

If you consider my above request for concatenating multiple -o options,
rather than rejecting duplicates, then that means:

-o backing_file=/path/to/foo,help

would have to do the right thing about displaying help.

I also have the complaint that:

qemu-img create -o help -f qcow2

does not tell me the qcow2 specific options; I have to do something like:

qemu-img create -o help -f qcow2 /dev/null

It would be nice if we could make -o help smarter so that it no longer
requires the presence of a file name (probably a separate patch).

In other words, I like that your series is trying to improve things, but
I don't think it is making the right improvement.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On 02/19/2014 08:12 AM, Kevin Wolf wrote:
> Multiple -o options has the same meaning as having a single option with
> all settings in the order of their respective -o options.

Ah, this addresses (part of) the problem I raised about patch 1.  But it
feels a bit awkward to have the intermediate state.

>          case 'o':
>              if (is_help_option(optarg)) {
>                  options_help = true;
>              } else if (!options) {
> -                options = optarg;
> +                options = g_strdup(optarg);
>              } else {
> -                error_report("-o cannot be used multiple times. Please use a "
> -                             "single -o option with comma-separated settings "
> -                             "instead.");
> -                return 1;
> +                options = g_strdup_printf("%s,%s", options, optarg);

In addition to the memleak that Fam pointed out, I still think you have
the problem that you aren't detecting:

-o backing_file=/path/to/foo,help

as specifying options_help.  I think you instead need to collect ALL -o
options without special-casing options_help, then at the end, call
contains_help_option which looks for "help" or "?" among all the
comma-separated options.


>  
> +    g_free(options);
>      return 0;
> +
> +fail:
> +    g_free(options);
> +    return 1;
>  }

I'd also like if we started using EXIT_FAILURE instead of the magic
number 1 (but that's probably a separate cleanup).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> If you specify the same option more than once (e.g. -o cluster_size=4k
> -o lazy_refcounts=on), qemu-img silently ignores all but the last one. This

Sounds like perfectly common behavior to me.

> series fixes it to either consider all options or to give an error message.

Doesn't this break usage like "Compiled-in default is no good for me,
create wrapper script setting my defaults followed by "$@", then
override them on command line"?

> Boolean option can still be given more than once as they aren't problematic
> in this respect (ten times -q is still quiet).

Unless it's a toggle, but I sure hope we don't have those.

Re: [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options

[Kevin Wolf]

Am 20.02.2014 um 08:18 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > If you specify the same option more than once (e.g. -o cluster_size=4k
> > -o lazy_refcounts=on), qemu-img silently ignores all but the last one. This
> 
> Sounds like perfectly common behavior to me.

I guess it depends on the kind of option. For -o it's no less surprising
than 'gcc -Wall -Werror' giving you only -Werror without -Wall.

> > series fixes it to either consider all options or to give an error message.
> 
> Doesn't this break usage like "Compiled-in default is no good for me,
> create wrapper script setting my defaults followed by "$@", then
> override them on command line"?

It does. Would you be happy with a series that accumulates -o options
(like this one does) and leaves the other options alone?

Kevin

Re: [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 20.02.2014 um 08:18 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > If you specify the same option more than once (e.g. -o cluster_size=4k
>> > -o lazy_refcounts=on), qemu-img silently ignores all but the last one. This
>> 
>> Sounds like perfectly common behavior to me.
>
> I guess it depends on the kind of option. For -o it's no less surprising
> than 'gcc -Wall -Werror' giving you only -Werror without -Wall.

I agree that multiple -OPT KEY=VALUE,... accumulating is nicer than the
last one wiping out its predecessors.

>> > series fixes it to either consider all options or to give an error message.
>> 
>> Doesn't this break usage like "Compiled-in default is no good for me,
>> create wrapper script setting my defaults followed by "$@", then
>> override them on command line"?
>
> It does. Would you be happy with a series that accumulates -o options
> (like this one does) and leaves the other options alone?

No objections.