[Qemu-devel] [PATCH v3 0/6] qemu-img: Support multiple -o options

[Qemu-devel] [PATCH v3 0/6] qemu-img: Support multiple -o options

[Kevin Wolf]

Kevin Wolf (6):
  qemu-option: has_help_option() and is_valid_option_list()
  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      |   2 +
 qemu-img.c                 | 133 ++++++++----
 tests/qemu-iotests/082     | 208 ++++++++++++++++++
 tests/qemu-iotests/082.out | 529 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 util/qemu-option.c         |  49 +++++
 6 files changed, 884 insertions(+), 38 deletions(-)
 create mode 100755 tests/qemu-iotests/082
 create mode 100644 tests/qemu-iotests/082.out

-- 
1.8.1.4

[Qemu-devel] [PATCH v3 1/6] qemu-option: has_help_option() and is_valid_option_list()

[Kevin Wolf]

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

is_valid_option_list() ensures that the option list doesn't have options
with leading commas or trailing unescaped commas.

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

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 3ea871a..8c0ac34 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -79,6 +79,8 @@ 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);
+bool is_valid_option_list(const char *param);
 
 /* ------------------------------------------------------------------ */
 
diff --git a/util/qemu-option.c b/util/qemu-option.c
index fd76cd2..9d898af 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -450,6 +450,55 @@ 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;
+}
+
+bool is_valid_option_list(const char *param)
+{
+    size_t buflen = strlen(param) + 1;
+    char *buf = g_malloc0(buflen);
+    const char *p = param;
+    bool result = true;
+
+    while (*p) {
+        p = get_opt_value(buf, buflen, p);
+        if (*p && !*++p) {
+            result = false;
+            goto out;
+        }
+
+        if (!*buf || *buf == ',') {
+            result = false;
+            goto out;
+        }
+    }
+
+out:
+    free(buf);
+    return result;
+}
+
 /*
  * Prints all options of a list that have a value to stdout
  */
-- 
1.8.1.4

[Qemu-devel] [PATCH v3 2/6] qemu-img create: Support multiple -o options

[Kevin Wolf]

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 | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 79ab3e8..9c1643d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -369,13 +369,23 @@ 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 (!is_valid_option_list(optarg)) {
+                error_report("Invalid option list: %s", optarg);
+                goto fail;
+            }
+            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 +413,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 +421,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 +431,15 @@ static int img_create(int argc, char **argv)
     if (local_err) {
         error_report("%s: %s", filename, error_get_pretty(local_err));
         error_free(local_err);
-        return 1;
+        goto fail;
     }
 
+    g_free(options);
     return 0;
+
+fail:
+    g_free(options);
+    return 1;
 }
 
 static void dump_json_image_check(ImageCheck *check, bool quiet)
-- 
1.8.1.4

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

[Kevin Wolf]

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 | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 9c1643d..3fd2168 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1164,6 +1164,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";
@@ -1195,13 +1198,26 @@ 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 (!is_valid_option_list(optarg)) {
+                error_report("Invalid option list: %s", optarg);
+                ret = -1;
+                goto out;
+            }
+            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;
@@ -1212,7 +1228,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;
@@ -1225,7 +1242,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;
@@ -1257,10 +1275,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;
     }
@@ -1653,6 +1668,7 @@ out:
     free_option_parameters(create_options);
     free_option_parameters(param);
     qemu_vfree(buf);
+    g_free(options);
     if (sn_opts) {
         qemu_opts_del(sn_opts);
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH v3 4/6] qemu-img amend: Support multiple -o options

[Kevin Wolf]

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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 3fd2168..6ceaeb2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2667,7 +2667,18 @@ static int img_amend(int argc, char **argv)
                 help();
                 break;
             case 'o':
-                options = optarg;
+                if (!is_valid_option_list(optarg)) {
+                    error_report("Invalid option list: %s", optarg);
+                    ret = -1;
+                    goto out;
+                }
+                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;
@@ -2697,7 +2708,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;
     }
@@ -2724,6 +2735,8 @@ out:
     }
     free_option_parameters(create_options);
     free_option_parameters(options_param);
+    g_free(options);
+
     if (ret) {
         return 1;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH v3 5/6] qemu-img: Allow -o help with incomplete argument list

[Kevin Wolf]

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 6ceaeb2..5512f5b 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;
@@ -394,10 +397,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) {
@@ -421,11 +430,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 (local_err) {
@@ -1269,17 +1273,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");
@@ -2689,15 +2694,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) {
@@ -2709,6 +2720,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

[Qemu-devel] [PATCH v3 6/6] qemu-iotests: Check qemu-img command line parsing

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/082     | 208 ++++++++++++++++++
 tests/qemu-iotests/082.out | 529 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 738 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..f6eb75f
--- /dev/null
+++ b/tests/qemu-iotests/082
@@ -0,0 +1,208 @@
+#!/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
+run_qemu_img create -f $IMGFMT -o help,cluster_size=4k "$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
+
+# Try to trick qemu-img into creating escaped commas
+run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG" $size
+run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG" -o ,help "$TEST_IMG" $size
+run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$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
+run_qemu_img convert -O $IMGFMT -o help,cluster_size=4k "$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
+
+# Try to trick qemu-img into creating escaped commas
+run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG" "$TEST_IMG".base
+run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG" -o ,help "$TEST_IMG" "$TEST_IMG".base
+run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$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"
+run_qemu_img amend -f $IMGFMT -o help,cluster_size=4k "$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"
+
+# Try to trick qemu-img into creating escaped commas
+run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG"
+run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" -o ,help "$TEST_IMG"
+run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$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..28309a0
--- /dev/null
+++ b/tests/qemu-iotests/082.out
@@ -0,0 +1,529 @@
+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 help,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 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 backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2 128M
+qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2,
+
+Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,help TEST_DIR/t.qcow2 128M
+qemu-img: Invalid option list: ,help
+
+Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help TEST_DIR/t.qcow2 128M
+qemu-img: Invalid option list: ,,
+
+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 help,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 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 backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2,
+
+Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+qemu-img: Invalid option list: ,help
+
+Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
+qemu-img: Invalid option list: ,,
+
+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 help,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 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 backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2
+qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2,
+
+Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,help TEST_DIR/t.qcow2
+qemu-img: Invalid option list: ,help
+
+Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help TEST_DIR/t.qcow2
+qemu-img: Invalid option list: ,,
+
+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

Re: [Qemu-devel] [PATCH v3 0/6] qemu-img: Support multiple -o options

[Jeff Cody]

On Fri, Feb 21, 2014 at 04:24:02PM +0100, Kevin Wolf wrote:
> Kevin Wolf (6):
>   qemu-option: has_help_option() and is_valid_option_list()
>   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      |   2 +
>  qemu-img.c                 | 133 ++++++++----
>  tests/qemu-iotests/082     | 208 ++++++++++++++++++
>  tests/qemu-iotests/082.out | 529 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  util/qemu-option.c         |  49 +++++
>  6 files changed, 884 insertions(+), 38 deletions(-)
>  create mode 100755 tests/qemu-iotests/082
>  create mode 100644 tests/qemu-iotests/082.out
> 
> -- 
> 1.8.1.4
> 
> 

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

Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: has_help_option() and is_valid_option_list()

[Eric Blake]

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

On 02/21/2014 08:24 AM, Kevin Wolf wrote:
> has_help_option() checks if any help option ('help' or '?') occurs
> anywhere in an option string, so that things like 'cluster_size=4k,help'
> are recognised.
> 
> is_valid_option_list() ensures that the option list doesn't have options
> with leading commas or trailing unescaped commas.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +
> +    while (*p) {
> +        p = get_opt_value(buf, buflen, p);
> +        if (*p) {
> +            p++;
> +        }
> +
> +        if (is_help_option(buf)) {
> +            result = true;
> +            goto out;

If this were 'break;',

> +        }
> +    }
> +
> +out:

then you wouldn't need this label.  But that's cosmetic.

> +    free(buf);
> +    return result;
> +}
> +
> +bool is_valid_option_list(const char *param)
> +{
> +    size_t buflen = strlen(param) + 1;
> +    char *buf = g_malloc0(buflen);
> +    const char *p = param;
> +    bool result = true;
> +
> +    while (*p) {
> +        p = get_opt_value(buf, buflen, p);
> +        if (*p && !*++p) {
> +            result = false;
> +            goto out;
> +        }

Rejects trailing commas.

> +
> +        if (!*buf || *buf == ',') {

Rejects empty options, but also rejects values beginning with a comma.
But we have legacy users that accept implicitly named first options (see
opts_do_parse()).  For example, this is a valid command line (albeit one
that prints a list of valid machines):

qemu-kvm -machine ,,blah

as shorthand for

qemu-kvm -machine type=,,blah

and where *buf would indeed be validly ','.

> +            result = false;
> +            goto out;
> +        }
> +    }
> +
> +out:

Same observation about the possibility of using break to avoid goto.

> +    free(buf);
> +    return result;
> +}

That said, none of our users of -o options have an implicitly-named
first option, and therefore THIS patch is safe, even if we have a
ticking time-bomb if any other option with implicit first name starts
using this function.  And break vs. goto is cosmetic.  So I'm okay
taking this as-is.

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

Re: [Qemu-devel] [PATCH v3 6/6] qemu-iotests: Check qemu-img command line parsing

[Eric Blake]

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

On 02/21/2014 08:24 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +
> +# Try to trick qemu-img into creating escaped commas
> +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG"
> +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" -o ,help "$TEST_IMG"
> +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST_IMG"

Interesting.  The first two are definitely invalid, but the third is a
case where if -o had an implicit first key name, then ',' would be the
value of that key for the second -o.

Maybe the trick in your patch 1 is to add a bool parameter to
is_valid_option_list() that states whether the option accepts an
implicit first key name: no key name starts with a comma, and leading
comma is valid only if you can have an implicit key name.  In this case,
-o does not have an implicit first key name, so your test proves that
you were right to reject the comma for our current usage.

But again, until we actually have a client of is_valid_option_list that
also cares about implicit first key name, this patch is fine as-is.

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

Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: has_help_option() and is_valid_option_list()

[Kevin Wolf]

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

Am 21.02.2014 um 21:22 hat Eric Blake geschrieben:
> On 02/21/2014 08:24 AM, Kevin Wolf wrote:
> > has_help_option() checks if any help option ('help' or '?') occurs
> > anywhere in an option string, so that things like 'cluster_size=4k,help'
> > are recognised.
> > 
> > is_valid_option_list() ensures that the option list doesn't have options
> > with leading commas or trailing unescaped commas.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> > +
> > +    while (*p) {
> > +        p = get_opt_value(buf, buflen, p);
> > +        if (*p) {
> > +            p++;
> > +        }
> > +
> > +        if (is_help_option(buf)) {
> > +            result = true;
> > +            goto out;
> 
> If this were 'break;',
> 
> > +        }
> > +    }
> > +
> > +out:
> 
> then you wouldn't need this label.  But that's cosmetic.
> 
> > +    free(buf);
> > +    return result;
> > +}
> > +
> > +bool is_valid_option_list(const char *param)
> > +{
> > +    size_t buflen = strlen(param) + 1;
> > +    char *buf = g_malloc0(buflen);
> > +    const char *p = param;
> > +    bool result = true;
> > +
> > +    while (*p) {
> > +        p = get_opt_value(buf, buflen, p);
> > +        if (*p && !*++p) {
> > +            result = false;
> > +            goto out;
> > +        }
> 
> Rejects trailing commas.
> 
> > +
> > +        if (!*buf || *buf == ',') {
> 
> Rejects empty options, but also rejects values beginning with a comma.
> But we have legacy users that accept implicitly named first options (see
> opts_do_parse()).  For example, this is a valid command line (albeit one
> that prints a list of valid machines):
> 
> qemu-kvm -machine ,,blah
> 
> as shorthand for
> 
> qemu-kvm -machine type=,,blah
> 
> and where *buf would indeed be validly ','.

Right, but I can't allow this without allowing '-o ,,' which breaks the
real use case. So the lesson is that you can concatenate option strings
and use implicit option names at the same time.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: has_help_option() and is_valid_option_list()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 21.02.2014 um 21:22 hat Eric Blake geschrieben:
>> On 02/21/2014 08:24 AM, Kevin Wolf wrote:
>> > has_help_option() checks if any help option ('help' or '?') occurs
>> > anywhere in an option string, so that things like 'cluster_size=4k,help'
>> > are recognised.
>> > 
>> > is_valid_option_list() ensures that the option list doesn't have options
>> > with leading commas or trailing unescaped commas.

Care to explain why these are bad?  I dimly remember Eric and you
discussing it last week, but I sure won't next time I look at this
code...

>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> 
>> > +
>> > +    while (*p) {
>> > +        p = get_opt_value(buf, buflen, p);
>> > +        if (*p) {
>> > +            p++;
>> > +        }
>> > +
>> > +        if (is_help_option(buf)) {
>> > +            result = true;
>> > +            goto out;
>> 
>> If this were 'break;',
>> 
>> > +        }
>> > +    }
>> > +
>> > +out:
>> 
>> then you wouldn't need this label.  But that's cosmetic.
>> 
>> > +    free(buf);
>> > +    return result;
>> > +}
>> > +
>> > +bool is_valid_option_list(const char *param)
>> > +{
>> > +    size_t buflen = strlen(param) + 1;
>> > +    char *buf = g_malloc0(buflen);
>> > +    const char *p = param;
>> > +    bool result = true;
>> > +
>> > +    while (*p) {
>> > +        p = get_opt_value(buf, buflen, p);
>> > +        if (*p && !*++p) {
>> > +            result = false;
>> > +            goto out;
>> > +        }
>> 
>> Rejects trailing commas.
>> 
>> > +
>> > +        if (!*buf || *buf == ',') {
>> 
>> Rejects empty options, but also rejects values beginning with a comma.
>> But we have legacy users that accept implicitly named first options (see
>> opts_do_parse()).  For example, this is a valid command line (albeit one
>> that prints a list of valid machines):
>> 
>> qemu-kvm -machine ,,blah
>> 
>> as shorthand for
>> 
>> qemu-kvm -machine type=,,blah
>> 
>> and where *buf would indeed be validly ','.
>
> Right, but I can't allow this without allowing '-o ,,' which breaks the
> real use case. So the lesson is that you can concatenate option strings
> and use implicit option names at the same time.

Another wart on the freakishly complex QemuOpts.

Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: has_help_option() and is_valid_option_list()

[Kevin Wolf]

Am 24.02.2014 um 09:42 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 21.02.2014 um 21:22 hat Eric Blake geschrieben:
> >> On 02/21/2014 08:24 AM, Kevin Wolf wrote:
> >> > has_help_option() checks if any help option ('help' or '?') occurs
> >> > anywhere in an option string, so that things like 'cluster_size=4k,help'
> >> > are recognised.
> >> > 
> >> > is_valid_option_list() ensures that the option list doesn't have options
> >> > with leading commas or trailing unescaped commas.
> 
> Care to explain why these are bad?  I dimly remember Eric and you
> discussing it last week, but I sure won't next time I look at this
> code...

Because with them you can trick qemu-img into producing escaped commas
when it tries to concatenate option strings ('-o backing_file=foo, -o
help' results in '-o backing_file=foo,,help').

> >> Rejects empty options, but also rejects values beginning with a comma.
> >> But we have legacy users that accept implicitly named first options (see
> >> opts_do_parse()).  For example, this is a valid command line (albeit one
> >> that prints a list of valid machines):
> >> 
> >> qemu-kvm -machine ,,blah
> >> 
> >> as shorthand for
> >> 
> >> qemu-kvm -machine type=,,blah
> >> 
> >> and where *buf would indeed be validly ','.
> >
> > Right, but I can't allow this without allowing '-o ,,' which breaks the
> > real use case. So the lesson is that you can concatenate option strings
> > and use implicit option names at the same time.
> 
> Another wart on the freakishly complex QemuOpts.

Rather what you get for operating on strings instead of directly going
to QemuOpts. We're parsing the option strings in the wrong place and
qemu-img shouldn't be concatenating option strings. But it takes a
larger cleanup until we can have that and I didn't want to block this
improvement on infrastructure work.

Kevin

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

[Peter Lieven]

On 21.02.2014 16:24, 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 | 34 +++++++++++++++++++++++++---------
>   1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 9c1643d..3fd2168 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1164,6 +1164,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);
> +
moving this up here breaks progrss output because progress is always false
at this point.

Peter

>       fmt = NULL;
>       out_fmt = "raw";
>       cache = "unsafe";
> @@ -1195,13 +1198,26 @@ 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 (!is_valid_option_list(optarg)) {
> +                error_report("Invalid option list: %s", optarg);
> +                ret = -1;
> +                goto out;
> +            }
> +            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;
> @@ -1212,7 +1228,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;
> @@ -1225,7 +1242,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;
> @@ -1257,10 +1275,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;
>       }
> @@ -1653,6 +1668,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);
>       }

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

[Kevin Wolf]

Am 03.03.2014 um 12:43 hat Peter Lieven geschrieben:
> On 21.02.2014 16:24, 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 | 34 +++++++++++++++++++++++++---------
> >  1 file changed, 25 insertions(+), 9 deletions(-)
> >
> >diff --git a/qemu-img.c b/qemu-img.c
> >index 9c1643d..3fd2168 100644
> >--- a/qemu-img.c
> >+++ b/qemu-img.c
> >@@ -1164,6 +1164,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);
> >+
> moving this up here breaks progrss output because progress is always false
> at this point.

Meh. You're right, of course, this was a stupid one.

I wonder if -p output is consistent enough that we can make a
qemu-iotests case for it. At the moment I can't see a reason why it
wouldn't be consistent.

Kevin