All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options
@ 2014-02-20 14:57 Kevin Wolf
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option() Kevin Wolf
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: Kevin Wolf @ 2014-02-20 14:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, armbru, mreitz, stefanha

Kevin Wolf (6):
  qemu-option: Introduce has_help_option()
  qemu-img create: Support multiple -o options
  qemu-img convert: Support multiple -o options
  qemu-img amend: Support multiple -o options
  qemu-img: Allow -o help with incomplete argument list
  qemu-iotests: Check qemu-img command line parsing

 include/qemu/option.h      |   1 +
 qemu-img.c                 | 119 +++++++++----
 tests/qemu-iotests/082     | 187 +++++++++++++++++++
 tests/qemu-iotests/082.out | 436 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 util/qemu-option.c         |  24 +++
 6 files changed, 730 insertions(+), 38 deletions(-)
 create mode 100755 tests/qemu-iotests/082
 create mode 100644 tests/qemu-iotests/082.out

-- 
1.8.1.4

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option()
  2014-02-20 14:57 [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options Kevin Wolf
@ 2014-02-20 14:57 ` Kevin Wolf
  2014-02-20 15:28   ` Eric Blake
  2014-02-20 20:21   ` Jeff Cody
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options Kevin Wolf
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: Kevin Wolf @ 2014-02-20 14:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, armbru, mreitz, stefanha

This new function checks if any help option ('help' or '?') occurs
anywhere in an option string, so that things like 'cluster_size=4k,help'
are recognised.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/option.h |  1 +
 util/qemu-option.c    | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 3ea871a..8d44167 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -79,6 +79,7 @@ void parse_option_size(const char *name, const char *value,
 void free_option_parameters(QEMUOptionParameter *list);
 void print_option_parameters(QEMUOptionParameter *list);
 void print_option_help(QEMUOptionParameter *list);
+bool has_help_option(const char *param);
 
 /* ------------------------------------------------------------------ */
 
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 668e5d9..ce1eba8 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -450,6 +450,30 @@ fail:
     return NULL;
 }
 
+bool has_help_option(const char *param)
+{
+    size_t buflen = strlen(param) + 1;
+    char *buf = g_malloc0(buflen);
+    const char *p = param;
+    bool result = false;
+
+    while (*p) {
+        p = get_opt_value(buf, buflen, p);
+        if (*p) {
+            p++;
+        }
+
+        if (is_help_option(buf)) {
+            result = true;
+            goto out;
+        }
+    }
+
+out:
+    free(buf);
+    return result;
+}
+
 /*
  * Prints all options of a list that have a value to stdout
  */
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options
  2014-02-20 14:57 [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options Kevin Wolf
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option() Kevin Wolf
@ 2014-02-20 14:57 ` Kevin Wolf
  2014-02-20 15:52   ` Eric Blake
  2014-02-20 19:14   ` Jeff Cody
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 3/6] qemu-img convert: " Kevin Wolf
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: Kevin Wolf @ 2014-02-20 14:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, armbru, mreitz, stefanha

If you specified multiple -o options for qemu-img create, it would
silently ignore all but the last one. This patch fixes the problem.

Now 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 | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b834d52..770e4e6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -369,13 +369,19 @@ 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':
-            options = optarg;
+            if (!options) {
+                options = g_strdup(optarg);
+            } else {
+                char *old_options = options;
+                options = g_strdup_printf("%s,%s", options, optarg);
+                g_free(old_options);
+            }
             break;
         case 'q':
             quiet = true;
@@ -403,7 +409,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;
     }
@@ -411,7 +417,8 @@ static int img_create(int argc, char **argv)
         help();
     }
 
-    if (options && is_help_option(options)) {
+    if (options && has_help_option(options)) {
+        g_free(options);
         return print_block_option_help(filename, fmt);
     }
 
@@ -420,10 +427,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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options
  2014-02-20 14:57 [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options Kevin Wolf
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option() Kevin Wolf
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options Kevin Wolf
@ 2014-02-20 14:57 ` Kevin Wolf
  2014-02-20 16:01   ` Eric Blake
  2014-02-20 19:33   ` Jeff Cody
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 4/6] qemu-img amend: " Kevin Wolf
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: Kevin Wolf @ 2014-02-20 14:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, armbru, mreitz, stefanha

Instead of ignoring all option values but the last one, multiple -o
options now have 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 | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 770e4e6..ba6e82d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1160,6 +1160,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 = "raw";
     cache = "unsafe";
@@ -1191,13 +1194,21 @@ 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':
-            options = optarg;
+            if (!options) {
+                options = g_strdup(optarg);
+            } else {
+                char *old_options = options;
+                options = g_strdup_printf("%s,%s", options, optarg);
+                g_free(old_options);
+            }
             break;
         case 's':
             snapshot_name = optarg;
@@ -1208,7 +1219,8 @@ static int img_convert(int argc, char **argv)
                 if (!sn_opts) {
                     error_report("Failed in parsing snapshot param '%s'",
                                  optarg);
-                    return 1;
+                    ret = -1;
+                    goto out;
                 }
             } else {
                 snapshot_name = optarg;
@@ -1221,7 +1233,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;
@@ -1253,10 +1266,7 @@ 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 && is_help_option(options)) {
+    if (options && has_help_option(options)) {
         ret = print_block_option_help(out_filename, out_fmt);
         goto out;
     }
@@ -1649,6 +1659,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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v2 4/6] qemu-img amend: Support multiple -o options
  2014-02-20 14:57 [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options Kevin Wolf
                   ` (2 preceding siblings ...)
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 3/6] qemu-img convert: " Kevin Wolf
@ 2014-02-20 14:57 ` Kevin Wolf
  2014-02-20 16:18   ` Eric Blake
  2014-02-20 19:39   ` Jeff Cody
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list Kevin Wolf
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing Kevin Wolf
  5 siblings, 2 replies; 21+ messages in thread
From: Kevin Wolf @ 2014-02-20 14:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, armbru, mreitz, stefanha

Instead of ignoring all option values but the last one, multiple -o
options now have 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, 10 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ba6e82d..d6dc7ec 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2658,7 +2658,13 @@ static int img_amend(int argc, char **argv)
                 help();
                 break;
             case 'o':
-                options = optarg;
+                if (!options) {
+                    options = g_strdup(optarg);
+                } else {
+                    char *old_options = options;
+                    options = g_strdup_printf("%s,%s", options, optarg);
+                    g_free(old_options);
+                }
                 break;
             case 'f':
                 fmt = optarg;
@@ -2688,7 +2694,7 @@ static int img_amend(int argc, char **argv)
 
     fmt = bs->drv->format_name;
 
-    if (is_help_option(options)) {
+    if (has_help_option(options)) {
         ret = print_block_option_help(filename, fmt);
         goto out;
     }
@@ -2715,6 +2721,8 @@ out:
     }
     free_option_parameters(create_options);
     free_option_parameters(options_param);
+    g_free(options);
+
     if (ret) {
         return 1;
     }
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list
  2014-02-20 14:57 [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options Kevin Wolf
                   ` (3 preceding siblings ...)
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 4/6] qemu-img amend: " Kevin Wolf
@ 2014-02-20 14:57 ` Kevin Wolf
  2014-02-20 16:58   ` Eric Blake
  2014-02-20 20:05   ` Jeff Cody
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing Kevin Wolf
  5 siblings, 2 replies; 21+ messages in thread
From: Kevin Wolf @ 2014-02-20 14:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, armbru, mreitz, stefanha

This patch allows using 'qemu-img $subcmd -o help' for the create,
convert and amend subcommands, without specifying the previously
required filename arguments.

Note that it's still allowed and meaningful to specify a filename: An
invocation like 'qemu-img create -o help sheepdog:foo' will also display
options that are provided by the Sheepdog driver.

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

diff --git a/qemu-img.c b/qemu-img.c
index d6dc7ec..98ddbe7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -250,16 +250,19 @@ static int print_block_option_help(const char *filename, const char *fmt)
         return 1;
     }
 
-    proto_drv = bdrv_find_protocol(filename, true);
-    if (!proto_drv) {
-        error_report("Unknown protocol '%s'", filename);
-        return 1;
-    }
-
     create_options = append_option_parameters(create_options,
                                               drv->create_options);
-    create_options = append_option_parameters(create_options,
-                                              proto_drv->create_options);
+
+    if (filename) {
+        proto_drv = bdrv_find_protocol(filename, true);
+        if (!proto_drv) {
+            error_report("Unknown protocol '%s'", filename);
+            return 1;
+        }
+        create_options = append_option_parameters(create_options,
+                                                  proto_drv->create_options);
+    }
+
     print_option_help(create_options);
     free_option_parameters(create_options);
     return 0;
@@ -390,10 +393,16 @@ static int img_create(int argc, char **argv)
     }
 
     /* Get the filename */
+    filename = (optind < argc) ? argv[optind] : NULL;
+    if (options && has_help_option(options)) {
+        g_free(options);
+        return print_block_option_help(filename, fmt);
+    }
+
     if (optind >= argc) {
         help();
     }
-    filename = argv[optind++];
+    optind++;
 
     /* Get image size, if specified */
     if (optind < argc) {
@@ -417,11 +426,6 @@ static int img_create(int argc, char **argv)
         help();
     }
 
-    if (options && has_help_option(options)) {
-        g_free(options);
-        return print_block_option_help(filename, fmt);
-    }
-
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
                     options, img_size, BDRV_O_FLAGS, &local_err, quiet);
     if (error_is_set(&local_err)) {
@@ -1260,17 +1264,18 @@ static int img_convert(int argc, char **argv)
     }
 
     bs_n = argc - optind - 1;
-    if (bs_n < 1) {
-        help();
-    }
-
-    out_filename = argv[argc - 1];
+    out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
 
     if (options && has_help_option(options)) {
         ret = print_block_option_help(out_filename, out_fmt);
         goto out;
     }
 
+    if (bs_n < 1) {
+        help();
+    }
+
+
     if (bs_n > 1 && out_baseimg) {
         error_report("-B makes no sense when concatenating multiple input "
                      "images");
@@ -2675,15 +2680,21 @@ static int img_amend(int argc, char **argv)
         }
     }
 
-    if (optind != argc - 1) {
+    if (!options) {
         help();
     }
 
-    if (!options) {
-        help();
+    filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
+    if (fmt && has_help_option(options)) {
+        /* If a format is explicitly specified (and possibly no filename is
+         * given), print option help here */
+        ret = print_block_option_help(filename, fmt);
+        goto out;
     }
 
-    filename = argv[argc - 1];
+    if (optind != argc - 1) {
+        help();
+    }
 
     bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet);
     if (!bs) {
@@ -2695,6 +2706,7 @@ static int img_amend(int argc, char **argv)
     fmt = bs->drv->format_name;
 
     if (has_help_option(options)) {
+        /* If the format was auto-detected, print option help here */
         ret = print_block_option_help(filename, fmt);
         goto out;
     }
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing
  2014-02-20 14:57 [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options Kevin Wolf
                   ` (4 preceding siblings ...)
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list Kevin Wolf
@ 2014-02-20 14:57 ` Kevin Wolf
  2014-02-20 17:51   ` Eric Blake
  2014-02-20 18:42   ` Jeff Cody
  5 siblings, 2 replies; 21+ messages in thread
From: Kevin Wolf @ 2014-02-20 14:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, famz, armbru, mreitz, stefanha

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/082     | 187 +++++++++++++++++++
 tests/qemu-iotests/082.out | 436 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 624 insertions(+)
 create mode 100755 tests/qemu-iotests/082
 create mode 100644 tests/qemu-iotests/082.out

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
new file mode 100755
index 0000000..10a4b1e
--- /dev/null
+++ b/tests/qemu-iotests/082
@@ -0,0 +1,187 @@
+#!/bin/bash
+#
+# Test qemu-img command line parsing
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function run_qemu_img()
+{
+    echo
+    echo Testing: "$@" | _filter_testdir
+    $QEMU_IMG "$@" 2>&1 | _filter_testdir
+}
+
+size=128M
+
+echo
+echo === create: Options specified more than once ===
+
+# Last -f should win
+run_qemu_img create -f foo -f $IMGFMT $TEST_IMG $size
+run_qemu_img info $TEST_IMG
+
+# Multiple -o should be merged
+run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on $TEST_IMG $size
+run_qemu_img info $TEST_IMG
+
+# If the same -o key is specified more than once, the last one wins
+run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k $TEST_IMG $size
+run_qemu_img info $TEST_IMG
+run_qemu_img create -f $IMGFMT -o cluster_size=4k,cluster_size=8k $TEST_IMG $size
+run_qemu_img info $TEST_IMG
+
+echo
+echo === create: help for -o ===
+
+# Adding the help option to a command without other -o options
+run_qemu_img create -f $IMGFMT -o help $TEST_IMG $size
+run_qemu_img create -f $IMGFMT -o \? $TEST_IMG $size
+
+# Adding the help option to the same -o option
+run_qemu_img create -f $IMGFMT -o cluster_size=4k,help $TEST_IMG $size
+run_qemu_img create -f $IMGFMT -o cluster_size=4k,\? $TEST_IMG $size
+
+# Adding the help option to a separate -o option
+run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help $TEST_IMG $size
+run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? $TEST_IMG $size
+
+# Looks like a help option, but is part of the backing file name
+run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG $size
+run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG $size
+
+# Leave out everything that isn't needed
+run_qemu_img create -f $IMGFMT -o help
+run_qemu_img create -o help
+
+echo
+echo === convert: Options specified more than once ===
+
+# We need a valid source image
+run_qemu_img create -f $IMGFMT $TEST_IMG $size
+
+# Last -f should win
+run_qemu_img convert -f foo -f $IMGFMT $TEST_IMG $TEST_IMG.base
+run_qemu_img info $TEST_IMG.base
+
+# Last -O should win
+run_qemu_img convert -O foo -O $IMGFMT $TEST_IMG $TEST_IMG.base
+run_qemu_img info $TEST_IMG.base
+
+# Multiple -o should be merged
+run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on $TEST_IMG $TEST_IMG.base
+run_qemu_img info $TEST_IMG.base
+
+# If the same -o key is specified more than once, the last one wins
+run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k $TEST_IMG $TEST_IMG.base
+run_qemu_img info $TEST_IMG.base
+run_qemu_img convert -O $IMGFMT -o cluster_size=4k,cluster_size=8k $TEST_IMG $TEST_IMG.base
+run_qemu_img info $TEST_IMG.base
+
+echo
+echo === convert: help for -o ===
+
+# Adding the help option to a command without other -o options
+run_qemu_img convert -O $IMGFMT -o help $TEST_IMG $TEST_IMG.base
+run_qemu_img convert -O $IMGFMT -o \? $TEST_IMG $TEST_IMG.base
+
+# Adding the help option to the same -o option
+run_qemu_img convert -O $IMGFMT -o cluster_size=4k,help $TEST_IMG $TEST_IMG.base
+run_qemu_img convert -O $IMGFMT -o cluster_size=4k,\? $TEST_IMG $TEST_IMG.base
+
+# Adding the help option to a separate -o option
+run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o help $TEST_IMG $TEST_IMG.base
+run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o \? $TEST_IMG $TEST_IMG.base
+
+# Looks like a help option, but is part of the backing file name
+run_qemu_img convert -O $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG $TEST_IMG.base
+run_qemu_img convert -O $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG $TEST_IMG.base
+
+# Leave out everything that isn't needed
+run_qemu_img convert -O $IMGFMT -o help
+run_qemu_img convert -o help
+
+echo
+echo === amend: Options specified more than once ===
+
+# Last -f should win
+run_qemu_img amend -f foo -f $IMGFMT -o lazy_refcounts=on $TEST_IMG
+run_qemu_img info $TEST_IMG
+
+# Multiple -o should be merged
+run_qemu_img amend -f $IMGFMT -o size=130M -o lazy_refcounts=off $TEST_IMG
+run_qemu_img info $TEST_IMG
+
+# If the same -o key is specified more than once, the last one wins
+run_qemu_img amend -f $IMGFMT -o size=8M -o lazy_refcounts=on -o size=132M $TEST_IMG
+run_qemu_img info $TEST_IMG
+run_qemu_img amend -f $IMGFMT -o size=4M,size=148M $TEST_IMG
+run_qemu_img info $TEST_IMG
+
+echo
+echo === amend: help for -o ===
+
+# Adding the help option to a command without other -o options
+run_qemu_img amend -f $IMGFMT -o help $TEST_IMG
+run_qemu_img amend -f $IMGFMT -o \? $TEST_IMG
+
+# Adding the help option to the same -o option
+run_qemu_img amend -f $IMGFMT -o cluster_size=4k,help $TEST_IMG
+run_qemu_img amend -f $IMGFMT -o cluster_size=4k,\? $TEST_IMG
+
+# Adding the help option to a separate -o option
+run_qemu_img amend -f $IMGFMT -o cluster_size=4k -o help $TEST_IMG
+run_qemu_img amend -f $IMGFMT -o cluster_size=4k -o \? $TEST_IMG
+
+# Looks like a help option, but is part of the backing file name
+run_qemu_img amend -f $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG
+run_qemu_img rebase -u -b "" -f $IMGFMT $TEST_IMG
+
+run_qemu_img amend -f $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG
+run_qemu_img rebase -u -b "" -f $IMGFMT $TEST_IMG
+
+# Leave out everything that isn't needed
+run_qemu_img amend -f $IMGFMT -o help
+run_qemu_img convert -o help
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
new file mode 100644
index 0000000..b775d00
--- /dev/null
+++ b/tests/qemu-iotests/082.out
@@ -0,0 +1,436 @@
+QA output created by 082
+
+=== create: Options specified more than once ===
+
+Testing: create -f foo -f qcow2 TEST_DIR/t.qcow2 128M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=65536 lazy_refcounts=off 
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+
+Testing: create -f qcow2 -o cluster_size=4k -o lazy_refcounts=on TEST_DIR/t.qcow2 128M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=4096 lazy_refcounts=on 
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 16K
+cluster_size: 4096
+Format specific information:
+    compat: 1.1
+    lazy refcounts: true
+
+Testing: create -f qcow2 -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k TEST_DIR/t.qcow2 128M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=8192 lazy_refcounts=on 
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 28K
+cluster_size: 8192
+Format specific information:
+    compat: 1.1
+    lazy refcounts: true
+
+Testing: create -f qcow2 -o cluster_size=4k,cluster_size=8k TEST_DIR/t.qcow2 128M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=8192 lazy_refcounts=off 
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 28K
+cluster_size: 8192
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+
+=== create: help for -o ===
+
+Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2,help' encryption=off cluster_size=65536 lazy_refcounts=off 
+
+Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 128M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2,?' encryption=off cluster_size=65536 lazy_refcounts=off 
+
+Testing: create -f qcow2 -o help
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: create -o help
+Supported options:
+size             Virtual disk size
+
+=== convert: Options specified more than once ===
+
+Testing: create -f qcow2 TEST_DIR/t.qcow2 128M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=65536 lazy_refcounts=off 
+
+Testing: convert -f foo -f qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+
+Testing: info TEST_DIR/t.qcow2.base
+image: TEST_DIR/t.qcow2.base
+file format: raw
+virtual size: 128M (134217728 bytes)
+disk size: 0
+
+Testing: convert -O foo -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+
+Testing: info TEST_DIR/t.qcow2.base
+image: TEST_DIR/t.qcow2.base
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+
+Testing: convert -O qcow2 -o cluster_size=4k -o lazy_refcounts=on TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+
+Testing: info TEST_DIR/t.qcow2.base
+image: TEST_DIR/t.qcow2.base
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 16K
+cluster_size: 4096
+Format specific information:
+    compat: 1.1
+    lazy refcounts: true
+
+Testing: convert -O qcow2 -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+
+Testing: info TEST_DIR/t.qcow2.base
+image: TEST_DIR/t.qcow2.base
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 28K
+cluster_size: 8192
+Format specific information:
+    compat: 1.1
+    lazy refcounts: true
+
+Testing: convert -O qcow2 -o cluster_size=4k,cluster_size=8k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+
+Testing: info TEST_DIR/t.qcow2.base
+image: TEST_DIR/t.qcow2.base
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 28K
+cluster_size: 8192
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+
+=== convert: help for -o ===
+
+Testing: convert -O qcow2 -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,help': No such file or directory
+
+Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,?': No such file or directory
+
+Testing: convert -O qcow2 -o help
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -o help
+Supported options:
+size             Virtual disk size
+
+=== amend: Options specified more than once ===
+
+Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 128M (134217728 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: true
+
+Testing: amend -f qcow2 -o size=130M -o lazy_refcounts=off TEST_DIR/t.qcow2
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 130M (136314880 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+
+Testing: amend -f qcow2 -o size=8M -o lazy_refcounts=on -o size=132M TEST_DIR/t.qcow2
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 132M (138412032 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: true
+
+Testing: amend -f qcow2 -o size=4M,size=148M TEST_DIR/t.qcow2
+
+Testing: info TEST_DIR/t.qcow2
+image: TEST_DIR/t.qcow2
+file format: qcow2
+virtual size: 148M (155189248 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: true
+
+=== amend: help for -o ===
+
+Testing: amend -f qcow2 -o help TEST_DIR/t.qcow2
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
+
+Testing: rebase -u -b  -f qcow2 TEST_DIR/t.qcow2
+
+Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2
+
+Testing: rebase -u -b  -f qcow2 TEST_DIR/t.qcow2
+
+Testing: amend -f qcow2 -o help
+Supported options:
+size             Virtual disk size
+compat           Compatibility level (0.10 or 1.1)
+backing_file     File name of a base image
+backing_fmt      Image format of the base image
+encryption       Encrypt the image
+cluster_size     qcow2 cluster size
+preallocation    Preallocation mode (allowed values: off, metadata)
+lazy_refcounts   Postpone refcount updates
+
+Testing: convert -o help
+Supported options:
+size             Virtual disk size
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d8be74a..80b181f 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -83,3 +83,4 @@
 074 rw auto
 077 rw auto
 079 rw auto
+082 rw auto quick
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option()
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option() Kevin Wolf
@ 2014-02-20 15:28   ` Eric Blake
  2014-02-20 20:21   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Eric Blake @ 2014-02-20 15:28 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: famz, armbru, stefanha, mreitz

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

On 02/20/2014 07:57 AM, Kevin Wolf wrote:
> This new function checks if any help option ('help' or '?') occurs
> anywhere in an option string, so that things like 'cluster_size=4k,help'
> are recognised.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c    | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options Kevin Wolf
@ 2014-02-20 15:52   ` Eric Blake
  2014-02-20 16:24     ` Eric Blake
  2014-02-20 19:14   ` Jeff Cody
  1 sibling, 1 reply; 21+ messages in thread
From: Eric Blake @ 2014-02-20 15:52 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: famz, armbru, stefanha, mreitz

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

On 02/20/2014 07:57 AM, Kevin Wolf wrote:
> If you specified multiple -o options for qemu-img create, it would
> silently ignore all but the last one. This patch fixes the problem.
> 
> Now 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 | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

Much nicer than v1!

Reviewed-by: Eric Blake <eblake@redhat.com>

>          case 'o':
> -            options = optarg;
> +            if (!options) {
> +                options = g_strdup(optarg);
> +            } else {
> +                char *old_options = options;
> +                options = g_strdup_printf("%s,%s", options, optarg);
> +                g_free(old_options);
> +            }

Corner case:

Previously:

qemu-img create -f qcow2 -o backing_file=/dev/null, xyz 1M

creates xyz with a backing file of /dev/null and unspecified backing
format (and ignores the empty trailing option).

Now:

qemu-img create -f qcow2 -o backing_file=/dev/null, \
  -o backing_fmt=qcow2 xyz 1M

creates xyz with a backing file of '/dev/null,backing_fmt=qcow2' and
with an unspecified backing format (and NOT with a backing format of
qcow2).  Yikes!  What we thought was an empty trailing option instead
got turned into a literal comma in the backing_file option.

Maybe you should add a followup patch that errors out on trailing
commas, so that we don't end up creating ',,' in the concatenated
version.  And maybe patch get_opt_value() to detect and reject empty
trailing options as well.

But as this patch is already a strict improvement, it does not have to
be done in this commit.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 3/6] qemu-img convert: " Kevin Wolf
@ 2014-02-20 16:01   ` Eric Blake
  2014-02-20 19:33   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Eric Blake @ 2014-02-20 16:01 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: famz, armbru, stefanha, mreitz

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

On 02/20/2014 07:57 AM, Kevin Wolf wrote:
> Instead of ignoring all option values but the last one, multiple -o
> options now have 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 | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)

Same comment as in 2/6 about the surprise of a trailing comma empty
option turning into a literal comma.

> @@ -1191,13 +1194,21 @@ 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;

I had to look, but the out: label does indeed turn a ret of -1 into a
'return 1' at the end of the day.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/6] qemu-img amend: Support multiple -o options
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 4/6] qemu-img amend: " Kevin Wolf
@ 2014-02-20 16:18   ` Eric Blake
  2014-02-20 19:39   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Eric Blake @ 2014-02-20 16:18 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: famz, armbru, stefanha, mreitz

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

On 02/20/2014 07:57 AM, Kevin Wolf wrote:
> Instead of ignoring all option values but the last one, multiple -o
> options now have 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, 10 insertions(+), 2 deletions(-)

Same comment as in 2/6 about the surprise of a trailing comma empty
option turning into a literal comma.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index ba6e82d..d6dc7ec 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2658,7 +2658,13 @@ static int img_amend(int argc, char **argv)
>                  help();
>                  break;

Noticed this in context - this break is dead code, since help() never
returns.  (And remind me again WHY help has to return with non-zero
status, even when explicitly requested?)

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options
  2014-02-20 15:52   ` Eric Blake
@ 2014-02-20 16:24     ` Eric Blake
  0 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2014-02-20 16:24 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: famz, armbru, stefanha, mreitz

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

On 02/20/2014 08:52 AM, Eric Blake wrote:

> Previously:
> 
> qemu-img create -f qcow2 -o backing_file=/dev/null, xyz 1M
> 
> creates xyz with a backing file of /dev/null and unspecified backing
> format (and ignores the empty trailing option).

>   And maybe patch get_opt_value() to detect and reject empty
> trailing options as well.

Further evidence for my desire to improve get_opt_value() for
consistency: we already reject empty leading options...

$ qemu-img create -f qcow2 -o ,backing_file=/dev/null xyz 1MUnknown
option ''
qemu-img: xyz: Invalid options for file format 'qcow2'.

...although we happily ignore an empty option:

$ qemu-img create -f qcow2 -o '' xyz 1M
Formatting 'xyz', fmt=qcow2 size=1048576 encryption=off
cluster_size=65536 lazy_refcounts=off

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list Kevin Wolf
@ 2014-02-20 16:58   ` Eric Blake
  2014-02-20 20:05   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Eric Blake @ 2014-02-20 16:58 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: famz, armbru, stefanha, mreitz

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

On 02/20/2014 07:57 AM, Kevin Wolf wrote:
> This patch allows using 'qemu-img $subcmd -o help' for the create,
> convert and amend subcommands, without specifying the previously
> required filename arguments.
> 

Oh cool!  My complaint worked!

> Note that it's still allowed and meaningful to specify a filename: An
> invocation like 'qemu-img create -o help sheepdog:foo' will also display
> options that are provided by the Sheepdog driver.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-img.c | 58 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 23 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing Kevin Wolf
@ 2014-02-20 17:51   ` Eric Blake
  2014-02-20 18:42   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Eric Blake @ 2014-02-20 17:51 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: famz, armbru, stefanha, mreitz

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

On 02/20/2014 07:57 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/082     | 187 +++++++++++++++++++
>  tests/qemu-iotests/082.out | 436 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 624 insertions(+)
>  create mode 100755 tests/qemu-iotests/082
>  create mode 100644 tests/qemu-iotests/082.out
> 

> +
> +seq=`basename $0`

[rambling side note - I ought to submit a patch that updates all this
copy-and-pasted boilerplate to use $() instead of ``]


> +echo === create: Options specified more than once ===
> +
> +# Last -f should win
> +run_qemu_img create -f foo -f $IMGFMT $TEST_IMG $size
> +run_qemu_img info $TEST_IMG
> +
> +# Multiple -o should be merged
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on $TEST_IMG $size
> +run_qemu_img info $TEST_IMG

If we later fix my corner case of trailing ',', don't forget to also add
a test for the new behavior :)

> +
> +# Adding the help option to the same -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,\? $TEST_IMG $size

You should probably also test -o help,cluster_size=4k.

> +
> +# Adding the help option to a separate -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? $TEST_IMG $size
> +
> +# Looks like a help option, but is part of the backing file name
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG $size

Or, per my corner case:

-o backing_file=$TESET_IMG, -o help

is currently perversely part of the file name.

Even with my suggestions for further tests and improvements, this is
strictly better than what we previously had, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing Kevin Wolf
  2014-02-20 17:51   ` Eric Blake
@ 2014-02-20 18:42   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Jeff Cody @ 2014-02-20 18:42 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: mreitz, famz, qemu-devel, stefanha, armbru

On Thu, Feb 20, 2014 at 03:57:23PM +0100, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/082     | 187 +++++++++++++++++++
>  tests/qemu-iotests/082.out | 436 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 624 insertions(+)
>  create mode 100755 tests/qemu-iotests/082
>  create mode 100644 tests/qemu-iotests/082.out
> 
> diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
> new file mode 100755
> index 0000000..10a4b1e
> --- /dev/null
> +++ b/tests/qemu-iotests/082
> @@ -0,0 +1,187 @@
> +#!/bin/bash
> +#
> +# Test qemu-img command line parsing
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=kwolf@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +	_cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +function run_qemu_img()
> +{
> +    echo
> +    echo Testing: "$@" | _filter_testdir
> +    $QEMU_IMG "$@" 2>&1 | _filter_testdir

$QEMU_IMG needs quoted: "$QEMU_IMG"

> +}
> +
> +size=128M
> +
> +echo
> +echo === create: Options specified more than once ===
> +
> +# Last -f should win
> +run_qemu_img create -f foo -f $IMGFMT $TEST_IMG $size
> +run_qemu_img info $TEST_IMG
> +
> +# Multiple -o should be merged
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on $TEST_IMG $size

s/\$TEST_IMG/\$\"TEST_IMG\"/g

After those two changes, ./check -qcow2 082 runs fine in my spaced
directory test.


> +run_qemu_img info $TEST_IMG
> +
> +# If the same -o key is specified more than once, the last one wins
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k $TEST_IMG $size
> +run_qemu_img info $TEST_IMG
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,cluster_size=8k $TEST_IMG $size
> +run_qemu_img info $TEST_IMG
> +
> +echo
> +echo === create: help for -o ===
> +
> +# Adding the help option to a command without other -o options
> +run_qemu_img create -f $IMGFMT -o help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o \? $TEST_IMG $size
> +
> +# Adding the help option to the same -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,\? $TEST_IMG $size
> +
> +# Adding the help option to a separate -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? $TEST_IMG $size
> +
> +# Looks like a help option, but is part of the backing file name
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG $size
> +
> +# Leave out everything that isn't needed
> +run_qemu_img create -f $IMGFMT -o help
> +run_qemu_img create -o help
> +
> +echo
> +echo === convert: Options specified more than once ===
> +
> +# We need a valid source image
> +run_qemu_img create -f $IMGFMT $TEST_IMG $size
> +
> +# Last -f should win
> +run_qemu_img convert -f foo -f $IMGFMT $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +# Last -O should win
> +run_qemu_img convert -O foo -O $IMGFMT $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +# Multiple -o should be merged
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +# If the same -o key is specified more than once, the last one wins
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k,cluster_size=8k $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +echo
> +echo === convert: help for -o ===
> +
> +# Adding the help option to a command without other -o options
> +run_qemu_img convert -O $IMGFMT -o help $TEST_IMG $TEST_IMG.base
> +run_qemu_img convert -O $IMGFMT -o \? $TEST_IMG $TEST_IMG.base
> +
> +# Adding the help option to the same -o option
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k,help $TEST_IMG $TEST_IMG.base
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k,\? $TEST_IMG $TEST_IMG.base
> +
> +# Adding the help option to a separate -o option
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o help $TEST_IMG $TEST_IMG.base
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o \? $TEST_IMG $TEST_IMG.base
> +
> +# Looks like a help option, but is part of the backing file name
> +run_qemu_img convert -O $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG $TEST_IMG.base
> +run_qemu_img convert -O $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG $TEST_IMG.base
> +
> +# Leave out everything that isn't needed
> +run_qemu_img convert -O $IMGFMT -o help
> +run_qemu_img convert -o help
> +
> +echo
> +echo === amend: Options specified more than once ===
> +
> +# Last -f should win
> +run_qemu_img amend -f foo -f $IMGFMT -o lazy_refcounts=on $TEST_IMG
> +run_qemu_img info $TEST_IMG
> +
> +# Multiple -o should be merged
> +run_qemu_img amend -f $IMGFMT -o size=130M -o lazy_refcounts=off $TEST_IMG
> +run_qemu_img info $TEST_IMG
> +
> +# If the same -o key is specified more than once, the last one wins
> +run_qemu_img amend -f $IMGFMT -o size=8M -o lazy_refcounts=on -o size=132M $TEST_IMG
> +run_qemu_img info $TEST_IMG
> +run_qemu_img amend -f $IMGFMT -o size=4M,size=148M $TEST_IMG
> +run_qemu_img info $TEST_IMG
> +
> +echo
> +echo === amend: help for -o ===
> +
> +# Adding the help option to a command without other -o options
> +run_qemu_img amend -f $IMGFMT -o help $TEST_IMG
> +run_qemu_img amend -f $IMGFMT -o \? $TEST_IMG
> +
> +# Adding the help option to the same -o option
> +run_qemu_img amend -f $IMGFMT -o cluster_size=4k,help $TEST_IMG
> +run_qemu_img amend -f $IMGFMT -o cluster_size=4k,\? $TEST_IMG
> +
> +# Adding the help option to a separate -o option
> +run_qemu_img amend -f $IMGFMT -o cluster_size=4k -o help $TEST_IMG
> +run_qemu_img amend -f $IMGFMT -o cluster_size=4k -o \? $TEST_IMG
> +
> +# Looks like a help option, but is part of the backing file name
> +run_qemu_img amend -f $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG
> +run_qemu_img rebase -u -b "" -f $IMGFMT $TEST_IMG
> +
> +run_qemu_img amend -f $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG
> +run_qemu_img rebase -u -b "" -f $IMGFMT $TEST_IMG
> +
> +# Leave out everything that isn't needed
> +run_qemu_img amend -f $IMGFMT -o help
> +run_qemu_img convert -o help
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
> new file mode 100644
> index 0000000..b775d00
> --- /dev/null
> +++ b/tests/qemu-iotests/082.out
> @@ -0,0 +1,436 @@
> +QA output created by 082
> +
> +=== create: Options specified more than once ===
> +
> +Testing: create -f foo -f qcow2 TEST_DIR/t.qcow2 128M
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=65536 lazy_refcounts=off 
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 196K
> +cluster_size: 65536
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +
> +Testing: create -f qcow2 -o cluster_size=4k -o lazy_refcounts=on TEST_DIR/t.qcow2 128M
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=4096 lazy_refcounts=on 
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 16K
> +cluster_size: 4096
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: true
> +
> +Testing: create -f qcow2 -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k TEST_DIR/t.qcow2 128M
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=8192 lazy_refcounts=on 
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 28K
> +cluster_size: 8192
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: true
> +
> +Testing: create -f qcow2 -o cluster_size=4k,cluster_size=8k TEST_DIR/t.qcow2 128M
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=8192 lazy_refcounts=off 
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 28K
> +cluster_size: 8192
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +
> +=== create: help for -o ===
> +
> +Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2,help' encryption=off cluster_size=65536 lazy_refcounts=off 
> +
> +Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 128M
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2,?' encryption=off cluster_size=65536 lazy_refcounts=off 
> +
> +Testing: create -f qcow2 -o help
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: create -o help
> +Supported options:
> +size             Virtual disk size
> +
> +=== convert: Options specified more than once ===
> +
> +Testing: create -f qcow2 TEST_DIR/t.qcow2 128M
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=65536 lazy_refcounts=off 
> +
> +Testing: convert -f foo -f qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +
> +Testing: info TEST_DIR/t.qcow2.base
> +image: TEST_DIR/t.qcow2.base
> +file format: raw
> +virtual size: 128M (134217728 bytes)
> +disk size: 0
> +
> +Testing: convert -O foo -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +
> +Testing: info TEST_DIR/t.qcow2.base
> +image: TEST_DIR/t.qcow2.base
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 196K
> +cluster_size: 65536
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +
> +Testing: convert -O qcow2 -o cluster_size=4k -o lazy_refcounts=on TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +
> +Testing: info TEST_DIR/t.qcow2.base
> +image: TEST_DIR/t.qcow2.base
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 16K
> +cluster_size: 4096
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: true
> +
> +Testing: convert -O qcow2 -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +
> +Testing: info TEST_DIR/t.qcow2.base
> +image: TEST_DIR/t.qcow2.base
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 28K
> +cluster_size: 8192
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: true
> +
> +Testing: convert -O qcow2 -o cluster_size=4k,cluster_size=8k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +
> +Testing: info TEST_DIR/t.qcow2.base
> +image: TEST_DIR/t.qcow2.base
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 28K
> +cluster_size: 8192
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +
> +=== convert: help for -o ===
> +
> +Testing: convert -O qcow2 -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,help': No such file or directory
> +
> +Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> +qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,?': No such file or directory
> +
> +Testing: convert -O qcow2 -o help
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -o help
> +Supported options:
> +size             Virtual disk size
> +
> +=== amend: Options specified more than once ===
> +
> +Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 128M (134217728 bytes)
> +disk size: 196K
> +cluster_size: 65536
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: true
> +
> +Testing: amend -f qcow2 -o size=130M -o lazy_refcounts=off TEST_DIR/t.qcow2
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 130M (136314880 bytes)
> +disk size: 196K
> +cluster_size: 65536
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +
> +Testing: amend -f qcow2 -o size=8M -o lazy_refcounts=on -o size=132M TEST_DIR/t.qcow2
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 132M (138412032 bytes)
> +disk size: 196K
> +cluster_size: 65536
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: true
> +
> +Testing: amend -f qcow2 -o size=4M,size=148M TEST_DIR/t.qcow2
> +
> +Testing: info TEST_DIR/t.qcow2
> +image: TEST_DIR/t.qcow2
> +file format: qcow2
> +virtual size: 148M (155189248 bytes)
> +disk size: 196K
> +cluster_size: 65536
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: true
> +
> +=== amend: help for -o ===
> +
> +Testing: amend -f qcow2 -o help TEST_DIR/t.qcow2
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
> +
> +Testing: rebase -u -b  -f qcow2 TEST_DIR/t.qcow2
> +
> +Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2
> +
> +Testing: rebase -u -b  -f qcow2 TEST_DIR/t.qcow2
> +
> +Testing: amend -f qcow2 -o help
> +Supported options:
> +size             Virtual disk size
> +compat           Compatibility level (0.10 or 1.1)
> +backing_file     File name of a base image
> +backing_fmt      Image format of the base image
> +encryption       Encrypt the image
> +cluster_size     qcow2 cluster size
> +preallocation    Preallocation mode (allowed values: off, metadata)
> +lazy_refcounts   Postpone refcount updates
> +
> +Testing: convert -o help
> +Supported options:
> +size             Virtual disk size
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index d8be74a..80b181f 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -83,3 +83,4 @@
>  074 rw auto
>  077 rw auto
>  079 rw auto
> +082 rw auto quick
> -- 
> 1.8.1.4
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options Kevin Wolf
  2014-02-20 15:52   ` Eric Blake
@ 2014-02-20 19:14   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Jeff Cody @ 2014-02-20 19:14 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: mreitz, famz, qemu-devel, stefanha, armbru

On Thu, Feb 20, 2014 at 03:57:19PM +0100, Kevin Wolf wrote:
> If you specified multiple -o options for qemu-img create, it would
> silently ignore all but the last one. This patch fixes the problem.
> 
> Now 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 | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b834d52..770e4e6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -369,13 +369,19 @@ 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':
> -            options = optarg;
> +            if (!options) {
> +                options = g_strdup(optarg);
> +            } else {
> +                char *old_options = options;
> +                options = g_strdup_printf("%s,%s", options, optarg);
> +                g_free(old_options);
> +            }
>              break;
>          case 'q':
>              quiet = true;
> @@ -403,7 +409,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;
>      }
> @@ -411,7 +417,8 @@ static int img_create(int argc, char **argv)
>          help();
>      }
>  
> -    if (options && is_help_option(options)) {
> +    if (options && has_help_option(options)) {
> +        g_free(options);
>          return print_block_option_help(filename, fmt);
>      }
>  
> @@ -420,10 +427,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;

One minor preference: there are 3 separate cleanup spots to free
'options'.  If a ret variable was used, the label 'fail:' could become
'out:', and those 3 cleanups could be consolidated to a single
g_free(options).  That would help any future committer not forget a
cleanup spot.

But, as it is correct as-is:

Reviewed-by: Jeff Cody <jcody@redhat.com>

>  }
>  
>  static void dump_json_image_check(ImageCheck *check, bool quiet)
> -- 
> 1.8.1.4
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 3/6] qemu-img convert: " Kevin Wolf
  2014-02-20 16:01   ` Eric Blake
@ 2014-02-20 19:33   ` Jeff Cody
  2014-02-20 19:38     ` Eric Blake
  1 sibling, 1 reply; 21+ messages in thread
From: Jeff Cody @ 2014-02-20 19:33 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: mreitz, famz, qemu-devel, stefanha, armbru

On Thu, Feb 20, 2014 at 03:57:20PM +0100, Kevin Wolf wrote:
> Instead of ignoring all option values but the last one, multiple -o
> options now have 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 | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 770e4e6..ba6e82d 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1160,6 +1160,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 = "raw";
>      cache = "unsafe";
> @@ -1191,13 +1194,21 @@ 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':
> -            options = optarg;
> +            if (!options) {
> +                options = g_strdup(optarg);
> +            } else {
> +                char *old_options = options;
> +                options = g_strdup_printf("%s,%s", options, optarg);
> +                g_free(old_options);
> +            }
>              break;
>          case 's':
>              snapshot_name = optarg;
> @@ -1208,7 +1219,8 @@ static int img_convert(int argc, char **argv)
>                  if (!sn_opts) {
>                      error_report("Failed in parsing snapshot param '%s'",
>                                   optarg);
> -                    return 1;
> +                    ret = -1;
> +                    goto out;
>                  }
>              } else {
>                  snapshot_name = optarg;
> @@ -1221,7 +1233,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;
>              }

There is a 'return -1' that is missed, that I think needs to be 'goto
out;' for cleanup.  It is right after the call to
bdrv_parse_cache_flags() in the img_convert() function:

    ret = bdrv_parse_cache_flags(cache, &flags);
    if (ret < 0) {
        error_report("Invalid cache option: %s", cache);
        return -1;
    }

Looks like this has been this way for a while, though.  And this is an
instance (the only instance) where img_convert returns -1 instead of 0
or 1.  I'm not sure the implications if that changed, and became '1',
so we'd probably want to preserve the return value.

Since this doesn't have anything to do with this patch (although
leaking 'options' now, in addition to the other stuff leaked), I'll go
ahead and give my reviewed-by, and we can fix this in another
follow-up patch:

Reviewed-by: Jeff Cody <jcody@redhat.com>

>  
>              min_sparse = sval / BDRV_SECTOR_SIZE;
> @@ -1253,10 +1266,7 @@ 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 && is_help_option(options)) {
> +    if (options && has_help_option(options)) {
>          ret = print_block_option_help(out_filename, out_fmt);
>          goto out;
>      }
> @@ -1649,6 +1659,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
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options
  2014-02-20 19:33   ` Jeff Cody
@ 2014-02-20 19:38     ` Eric Blake
  0 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2014-02-20 19:38 UTC (permalink / raw)
  To: Jeff Cody, Kevin Wolf; +Cc: armbru, famz, qemu-devel, stefanha, mreitz

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

On 02/20/2014 12:33 PM, Jeff Cody wrote:
> On Thu, Feb 20, 2014 at 03:57:20PM +0100, Kevin Wolf wrote:
>> Instead of ignoring all option values but the last one, multiple -o
>> options now have the same meaning as having a single option with all
>> settings in the order of their respective -o options.
>>

> There is a 'return -1' that is missed, that I think needs to be 'goto
> out;' for cleanup.  It is right after the call to
> bdrv_parse_cache_flags() in the img_convert() function:
> 
>     ret = bdrv_parse_cache_flags(cache, &flags);
>     if (ret < 0) {
>         error_report("Invalid cache option: %s", cache);
>         return -1;
>     }
> 
> Looks like this has been this way for a while, though.  And this is an
> instance (the only instance) where img_convert returns -1 instead of 0
> or 1.  I'm not sure the implications if that changed, and became '1',
> so we'd probably want to preserve the return value.

No, exit status of 255 on an invalid cache option is a bug; fixing it to
return EXIT_FAILURE (1) is indeed correct.

[Hmm, this shows that I only reviewed the changed sections, rather than
also looking for all other 'return' statements - thanks Jeff for the
more thorough review]

> 
> Since this doesn't have anything to do with this patch (although
> leaking 'options' now, in addition to the other stuff leaked), I'll go
> ahead and give my reviewed-by, and we can fix this in another
> follow-up patch:

Agreed to that plan of attack (we've now racked up several good followups).

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/6] qemu-img amend: Support multiple -o options
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 4/6] qemu-img amend: " Kevin Wolf
  2014-02-20 16:18   ` Eric Blake
@ 2014-02-20 19:39   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Jeff Cody @ 2014-02-20 19:39 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: mreitz, famz, qemu-devel, stefanha, armbru

On Thu, Feb 20, 2014 at 03:57:21PM +0100, Kevin Wolf wrote:
> Instead of ignoring all option values but the last one, multiple -o
> options now have 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>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  qemu-img.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index ba6e82d..d6dc7ec 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2658,7 +2658,13 @@ static int img_amend(int argc, char **argv)
>                  help();
>                  break;
>              case 'o':
> -                options = optarg;
> +                if (!options) {
> +                    options = g_strdup(optarg);
> +                } else {
> +                    char *old_options = options;
> +                    options = g_strdup_printf("%s,%s", options, optarg);
> +                    g_free(old_options);
> +                }
>                  break;
>              case 'f':
>                  fmt = optarg;
> @@ -2688,7 +2694,7 @@ static int img_amend(int argc, char **argv)
>  
>      fmt = bs->drv->format_name;
>  
> -    if (is_help_option(options)) {
> +    if (has_help_option(options)) {
>          ret = print_block_option_help(filename, fmt);
>          goto out;
>      }
> @@ -2715,6 +2721,8 @@ out:
>      }
>      free_option_parameters(create_options);
>      free_option_parameters(options_param);
> +    g_free(options);
> +
>      if (ret) {
>          return 1;
>      }
> -- 
> 1.8.1.4
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list Kevin Wolf
  2014-02-20 16:58   ` Eric Blake
@ 2014-02-20 20:05   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Jeff Cody @ 2014-02-20 20:05 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: mreitz, famz, qemu-devel, stefanha, armbru

On Thu, Feb 20, 2014 at 03:57:22PM +0100, Kevin Wolf wrote:
> This patch allows using 'qemu-img $subcmd -o help' for the create,
> convert and amend subcommands, without specifying the previously
> required filename arguments.
> 
> Note that it's still allowed and meaningful to specify a filename: An
> invocation like 'qemu-img create -o help sheepdog:foo' will also display
> options that are provided by the Sheepdog driver.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-img.c | 58 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d6dc7ec..98ddbe7 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -250,16 +250,19 @@ static int print_block_option_help(const char *filename, const char *fmt)
>          return 1;
>      }
>  
> -    proto_drv = bdrv_find_protocol(filename, true);
> -    if (!proto_drv) {
> -        error_report("Unknown protocol '%s'", filename);
> -        return 1;
> -    }
> -
>      create_options = append_option_parameters(create_options,
>                                                drv->create_options);
> -    create_options = append_option_parameters(create_options,
> -                                              proto_drv->create_options);
> +
> +    if (filename) {
> +        proto_drv = bdrv_find_protocol(filename, true);
> +        if (!proto_drv) {
> +            error_report("Unknown protocol '%s'", filename);
> +            return 1;
> +        }
> +        create_options = append_option_parameters(create_options,
> +                                                  proto_drv->create_options);
> +    }
> +
>      print_option_help(create_options);
>      free_option_parameters(create_options);
>      return 0;
> @@ -390,10 +393,16 @@ static int img_create(int argc, char **argv)
>      }
>  
>      /* Get the filename */
> +    filename = (optind < argc) ? argv[optind] : NULL;
> +    if (options && has_help_option(options)) {
> +        g_free(options);
> +        return print_block_option_help(filename, fmt);
> +    }
> +

Same comment as patch #2 with regards to common cleanup code in
img_create().

Reviewed-by: Jeff Cody <jcody@redhat.com>

>      if (optind >= argc) {
>          help();
>      }
> -    filename = argv[optind++];
> +    optind++;
>  
>      /* Get image size, if specified */
>      if (optind < argc) {
> @@ -417,11 +426,6 @@ static int img_create(int argc, char **argv)
>          help();
>      }
>  
> -    if (options && has_help_option(options)) {
> -        g_free(options);
> -        return print_block_option_help(filename, fmt);
> -    }
> -
>      bdrv_img_create(filename, fmt, base_filename, base_fmt,
>                      options, img_size, BDRV_O_FLAGS, &local_err, quiet);
>      if (error_is_set(&local_err)) {
> @@ -1260,17 +1264,18 @@ static int img_convert(int argc, char **argv)
>      }
>  
>      bs_n = argc - optind - 1;
> -    if (bs_n < 1) {
> -        help();
> -    }
> -
> -    out_filename = argv[argc - 1];
> +    out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
>  
>      if (options && has_help_option(options)) {
>          ret = print_block_option_help(out_filename, out_fmt);
>          goto out;
>      }
>  
> +    if (bs_n < 1) {
> +        help();
> +    }
> +
> +
>      if (bs_n > 1 && out_baseimg) {
>          error_report("-B makes no sense when concatenating multiple input "
>                       "images");
> @@ -2675,15 +2680,21 @@ static int img_amend(int argc, char **argv)
>          }
>      }
>  
> -    if (optind != argc - 1) {
> +    if (!options) {
>          help();
>      }
>  
> -    if (!options) {
> -        help();
> +    filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
> +    if (fmt && has_help_option(options)) {
> +        /* If a format is explicitly specified (and possibly no filename is
> +         * given), print option help here */
> +        ret = print_block_option_help(filename, fmt);
> +        goto out;
>      }
>  
> -    filename = argv[argc - 1];
> +    if (optind != argc - 1) {
> +        help();
> +    }
>  
>      bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet);
>      if (!bs) {
> @@ -2695,6 +2706,7 @@ static int img_amend(int argc, char **argv)
>      fmt = bs->drv->format_name;
>  
>      if (has_help_option(options)) {
> +        /* If the format was auto-detected, print option help here */
>          ret = print_block_option_help(filename, fmt);
>          goto out;
>      }
> -- 
> 1.8.1.4
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option()
  2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option() Kevin Wolf
  2014-02-20 15:28   ` Eric Blake
@ 2014-02-20 20:21   ` Jeff Cody
  1 sibling, 0 replies; 21+ messages in thread
From: Jeff Cody @ 2014-02-20 20:21 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: mreitz, famz, qemu-devel, stefanha, armbru

On Thu, Feb 20, 2014 at 03:57:18PM +0100, Kevin Wolf wrote:
> This new function checks if any help option ('help' or '?') occurs
> anywhere in an option string, so that things like 'cluster_size=4k,help'
> are recognised.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c    | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 3ea871a..8d44167 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -79,6 +79,7 @@ void parse_option_size(const char *name, const char *value,
>  void free_option_parameters(QEMUOptionParameter *list);
>  void print_option_parameters(QEMUOptionParameter *list);
>  void print_option_help(QEMUOptionParameter *list);
> +bool has_help_option(const char *param);
>  
>  /* ------------------------------------------------------------------ */
>  
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 668e5d9..ce1eba8 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -450,6 +450,30 @@ fail:
>      return NULL;
>  }
>  
> +bool has_help_option(const char *param)
> +{
> +    size_t buflen = strlen(param) + 1;
> +    char *buf = g_malloc0(buflen);
> +    const char *p = param;
> +    bool result = false;
> +
> +    while (*p) {
> +        p = get_opt_value(buf, buflen, p);
> +        if (*p) {
> +            p++;
> +        }
> +
> +        if (is_help_option(buf)) {
> +            result = true;
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    free(buf);
> +    return result;
> +}
> +
>  /*
>   * Prints all options of a list that have a value to stdout
>   */
> -- 
> 1.8.1.4
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2014-02-20 20:21 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-20 14:57 [Qemu-devel] [PATCH v2 0/6] qemu-img: Support multiple -o options Kevin Wolf
2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option() Kevin Wolf
2014-02-20 15:28   ` Eric Blake
2014-02-20 20:21   ` Jeff Cody
2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options Kevin Wolf
2014-02-20 15:52   ` Eric Blake
2014-02-20 16:24     ` Eric Blake
2014-02-20 19:14   ` Jeff Cody
2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 3/6] qemu-img convert: " Kevin Wolf
2014-02-20 16:01   ` Eric Blake
2014-02-20 19:33   ` Jeff Cody
2014-02-20 19:38     ` Eric Blake
2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 4/6] qemu-img amend: " Kevin Wolf
2014-02-20 16:18   ` Eric Blake
2014-02-20 19:39   ` Jeff Cody
2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list Kevin Wolf
2014-02-20 16:58   ` Eric Blake
2014-02-20 20:05   ` Jeff Cody
2014-02-20 14:57 ` [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing Kevin Wolf
2014-02-20 17:51   ` Eric Blake
2014-02-20 18:42   ` Jeff Cody

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.