[PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up

[PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up

[Markus Armbruster]

v2:
* PATCH 1,4,5: Cover "?" in addition to "help" [Kevin]
* PATCH 4-6: Old PATCH 4 moved after old PATCH 5+6, commit message
  adjusted
* PATCH 4: Unbreak "?" [Eric]
* PATCH 8: Commit message tweaked
* PATCH 9: New

Markus Armbruster (9):
  tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()
  qemu-options: Factor out get_opt_name_value() helper
  qemu-option: Fix sloppy recognition of "id=..." after ",,"
  qemu-option: Fix has_help_option()'s sloppy parsing
  test-qemu-opts: Simplify test_has_help_option() after bug fix
  qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
  qemu-img: Factor out accumulate_options() helper
  qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite
  qemu-img: Reject broken -o ""

 include/qemu/option.h  |   1 -
 qemu-img.c             |  87 ++++++++++-------
 tests/test-qemu-opts.c |  46 ++++++++-
 util/qemu-option.c     | 210 ++++++++++++++++++++---------------------
 4 files changed, 199 insertions(+), 145 deletions(-)

-- 
2.21.1

[PATCH v2 for-5.1 1/9] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

The two turn out to be inconsistent for "a,b,,help".  Test case
marked /* BUG */.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qemu-opts.c | 44 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index ef96e84aed..88a3e7bdf4 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -728,6 +728,49 @@ static void test_opts_parse_size(void)
     qemu_opts_reset(&opts_list_02);
 }
 
+static void test_has_help_option(void)
+{
+    static const struct {
+        const char *params;
+        /* expected value of has_help_option() */
+        bool expect_has_help_option;
+        /* expected value of qemu_opt_has_help_opt() with implied=false */
+        bool expect_opt_has_help_opt;
+        /* expected value of qemu_opt_has_help_opt() with implied=true */
+        bool expect_opt_has_help_opt_implied;
+    } test[] = {
+        { "help", true, true, false },
+        { "?", true, true, false },
+        { "helpme", false, false, false },
+        { "?me", false, false, false },
+        { "a,help", true, true, true },
+        { "a,?", true, true, true },
+        { "a=0,help,b", true, true, true },
+        { "a=0,?,b", true, true, true },
+        { "help,b=1", true, true, false },
+        { "?,b=1", true, true, false },
+        { "a,b,,help", false /* BUG */, true, true },
+        { "a,b,,?", false /* BUG */, true, true },
+    };
+    int i;
+    QemuOpts *opts;
+
+    for (i = 0; i < ARRAY_SIZE(test); i++) {
+        g_assert_cmpint(has_help_option(test[i].params),
+                        ==, test[i].expect_has_help_option);
+        opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
+                               &error_abort);
+        g_assert_cmpint(qemu_opt_has_help_opt(opts),
+                        ==, test[i].expect_opt_has_help_opt);
+        qemu_opts_del(opts);
+        opts = qemu_opts_parse(&opts_list_03, test[i].params, true,
+                               &error_abort);
+        g_assert_cmpint(qemu_opt_has_help_opt(opts),
+                        ==, test[i].expect_opt_has_help_opt_implied);
+        qemu_opts_del(opts);
+    }
+}
+
 static void append_verify_list_01(QemuOptDesc *desc, bool with_overlapping)
 {
     int i = 0;
@@ -990,6 +1033,7 @@ int main(int argc, char *argv[])
     g_test_add_func("/qemu-opts/opts_parse/bool", test_opts_parse_bool);
     g_test_add_func("/qemu-opts/opts_parse/number", test_opts_parse_number);
     g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size);
+    g_test_add_func("/qemu-opts/has_help_option", test_has_help_option);
     g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null);
     g_test_add_func("/qemu-opts/append", test_opts_append);
     g_test_add_func("/qemu-opts/to_qdict/basic", test_opts_to_qdict_basic);
-- 
2.21.1

Re: [PATCH v2 for-5.1 1/9] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Eric Blake]

On 4/15/20 2:49 AM, Markus Armbruster wrote:
> The two turn out to be inconsistent for "a,b,,help".  Test case
> marked /* BUG */.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qemu-opts.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)

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


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 for-5.1 2/9] qemu-options: Factor out get_opt_name_value() helper

[Markus Armbruster]

The next commits will put it to use.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 46 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa..f08f4bc458 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -805,61 +805,71 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
     }
 }
 
+static const char *get_opt_name_value(const char *params,
+                                      const char *firstname,
+                                      char **name, char **value)
+{
+    const char *p, *pe, *pc;
+
+    pe = strchr(params, '=');
+    pc = strchr(params, ',');
+
+    if (!pe || (pc && pc < pe)) {
+        /* found "foo,more" */
+        if (firstname) {
+            /* implicitly named first option */
+            *name = g_strdup(firstname);
+            p = get_opt_value(params, value);
+        } else {
+            /* option without value, must be a flag */
+            p = get_opt_name(params, name, ',');
+            if (strncmp(*name, "no", 2) == 0) {
+                memmove(*name, *name + 2, strlen(*name + 2) + 1);
+                *value = g_strdup("off");
+            } else {
+                *value = g_strdup("on");
+            }
+        }
+    } else {
+        /* found "foo=bar,more" */
+        p = get_opt_name(params, name, '=');
+        assert(*p == '=');
+        p++;
+        p = get_opt_value(p, value);
+    }
+
+    assert(!*p || *p == ',');
+    if (*p == ',') {
+        p++;
+    }
+    return p;
+}
+
 static void opts_do_parse(QemuOpts *opts, const char *params,
                           const char *firstname, bool prepend,
                           bool *invalidp, Error **errp)
 {
-    char *option = NULL;
-    char *value = NULL;
-    const char *p,*pe,*pc;
     Error *local_err = NULL;
+    char *option, *value;
+    const char *p;
 
-    for (p = params; *p != '\0'; p++) {
-        pe = strchr(p, '=');
-        pc = strchr(p, ',');
-        if (!pe || (pc && pc < pe)) {
-            /* found "foo,more" */
-            if (p == params && firstname) {
-                /* implicitly named first option */
-                option = g_strdup(firstname);
-                p = get_opt_value(p, &value);
-            } else {
-                /* option without value, probably a flag */
-                p = get_opt_name(p, &option, ',');
-                if (strncmp(option, "no", 2) == 0) {
-                    memmove(option, option+2, strlen(option+2)+1);
-                    value = g_strdup("off");
-                } else {
-                    value = g_strdup("on");
-                }
-            }
-        } else {
-            /* found "foo=bar,more" */
-            p = get_opt_name(p, &option, '=');
-            assert(*p == '=');
-            p++;
-            p = get_opt_value(p, &value);
-        }
-        if (strcmp(option, "id") != 0) {
-            /* store and parse */
-            opt_set(opts, option, value, prepend, invalidp, &local_err);
-            value = NULL;
-            if (local_err) {
-                error_propagate(errp, local_err);
-                goto cleanup;
-            }
-        }
-        if (*p != ',') {
-            break;
+    for (p = params; *p;) {
+        p = get_opt_name_value(p, firstname, &option, &value);
+        firstname = NULL;
+
+        if (!strcmp(option, "id")) {
+            g_free(option);
+            g_free(value);
+            continue;
         }
+
+        opt_set(opts, option, value, prepend, invalidp, &local_err);
         g_free(option);
-        g_free(value);
-        option = value = NULL;
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
-
- cleanup:
-    g_free(option);
-    g_free(value);
 }
 
 /**
-- 
2.21.1

[PATCH v2 for-5.1 3/9] qemu-option: Fix sloppy recognition of "id=..." after ", , "

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/test-qemu-opts.c |  4 ++--
 util/qemu-option.c     | 27 +++++++++++++++++++--------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 88a3e7bdf4..8ff97268d8 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -500,10 +500,10 @@ static void test_opts_parse(void)
     g_assert(!opts);
     /* TODO Cover .merge_lists = true */
 
-    /* Buggy ID recognition */
+    /* Buggy ID recognition (fixed) */
     opts = qemu_opts_parse(&opts_list_03, "x=,,id=bar", false, &error_abort);
     g_assert_cmpuint(opts_count(opts), ==, 1);
-    g_assert_cmpstr(qemu_opts_id(opts), ==, "bar"); /* BUG */
+    g_assert(!qemu_opts_id(opts));
     g_assert_cmpstr(qemu_opt_get(opts, "x"), ==, ",id=bar");
 
     /* Anti-social ID */
diff --git a/util/qemu-option.c b/util/qemu-option.c
index f08f4bc458..d2956082bd 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -872,6 +872,24 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
     }
 }
 
+static char *opts_parse_id(const char *params)
+{
+    const char *p;
+    char *name, *value;
+
+    for (p = params; *p;) {
+        p = get_opt_name_value(p, NULL, &name, &value);
+        if (!strcmp(name, "id")) {
+            g_free(name);
+            return value;
+        }
+        g_free(name);
+        g_free(value);
+    }
+
+    return NULL;
+}
+
 /**
  * Store options parsed from @params into @opts.
  * If @firstname is non-null, the first key=value in @params may omit
@@ -889,20 +907,13 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
                             bool *invalidp, Error **errp)
 {
     const char *firstname;
-    char *id = NULL;
-    const char *p;
+    char *id = opts_parse_id(params);
     QemuOpts *opts;
     Error *local_err = NULL;
 
     assert(!permit_abbrev || list->implied_opt_name);
     firstname = permit_abbrev ? list->implied_opt_name : NULL;
 
-    if (strncmp(params, "id=", 3) == 0) {
-        get_opt_value(params + 3, &id);
-    } else if ((p = strstr(params, ",id=")) != NULL) {
-        get_opt_value(p + 4, &id);
-    }
-
     /*
      * This code doesn't work for defaults && !list->merge_lists: when
      * params has no id=, and list has an element with !opts->id, it
-- 
2.21.1

[PATCH v2 for-5.1 4/9] qemu-option: Fix has_help_option()'s sloppy parsing

[Markus Armbruster]

has_help_option() uses its own parser.  It's inconsistent with
qemu_opts_parse(), as demonstrated by test-qemu-opts case
/qemu-opts/has_help_option.  Fix by reusing the common parser.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qemu-opts.c |  4 ++--
 util/qemu-option.c     | 39 +++++++++++++++++++--------------------
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 8ff97268d8..77c944c4aa 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -749,8 +749,8 @@ static void test_has_help_option(void)
         { "a=0,?,b", true, true, true },
         { "help,b=1", true, true, false },
         { "?,b=1", true, true, false },
-        { "a,b,,help", false /* BUG */, true, true },
-        { "a,b,,?", false /* BUG */, true, true },
+        { "a,b,,help", true, true, true },
+        { "a,b,,?", true, true, true },
     };
     int i;
     QemuOpts *opts;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index d2956082bd..0abf26b61f 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value,
     *ret = size;
 }
 
-bool has_help_option(const char *param)
-{
-    const char *p = param;
-    bool result = false;
-
-    while (*p && !result) {
-        char *value;
-
-        p = get_opt_value(p, &value);
-        if (*p) {
-            p++;
-        }
-
-        result = is_help_option(value);
-        g_free(value);
-    }
-
-    return result;
-}
-
 bool is_valid_option_list(const char *p)
 {
     char *value = NULL;
@@ -890,6 +870,25 @@ static char *opts_parse_id(const char *params)
     return NULL;
 }
 
+bool has_help_option(const char *params)
+{
+    const char *p;
+    char *name, *value;
+    bool ret;
+
+    for (p = params; *p;) {
+        p = get_opt_name_value(p, NULL, &name, &value);
+        ret = is_help_option(name);
+        g_free(name);
+        g_free(value);
+        if (ret) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 /**
  * Store options parsed from @params into @opts.
  * If @firstname is non-null, the first key=value in @params may omit
-- 
2.21.1

Re: [PATCH v2 for-5.1 4/9] qemu-option: Fix has_help_option()'s sloppy parsing

[Eric Blake]

On 4/15/20 2:49 AM, Markus Armbruster wrote:
> has_help_option() uses its own parser.  It's inconsistent with
> qemu_opts_parse(), as demonstrated by test-qemu-opts case
> /qemu-opts/has_help_option.  Fix by reusing the common parser.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qemu-opts.c |  4 ++--
>   util/qemu-option.c     | 39 +++++++++++++++++++--------------------
>   2 files changed, 21 insertions(+), 22 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 for-5.1 5/9] test-qemu-opts: Simplify test_has_help_option() after bug fix

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/test-qemu-opts.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 77c944c4aa..2a0f42a09b 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -732,41 +732,39 @@ static void test_has_help_option(void)
 {
     static const struct {
         const char *params;
-        /* expected value of has_help_option() */
-        bool expect_has_help_option;
         /* expected value of qemu_opt_has_help_opt() with implied=false */
-        bool expect_opt_has_help_opt;
+        bool expect;
         /* expected value of qemu_opt_has_help_opt() with implied=true */
-        bool expect_opt_has_help_opt_implied;
+        bool expect_implied;
     } test[] = {
-        { "help", true, true, false },
-        { "?", true, true, false },
-        { "helpme", false, false, false },
-        { "?me", false, false, false },
-        { "a,help", true, true, true },
-        { "a,?", true, true, true },
-        { "a=0,help,b", true, true, true },
-        { "a=0,?,b", true, true, true },
-        { "help,b=1", true, true, false },
-        { "?,b=1", true, true, false },
-        { "a,b,,help", true, true, true },
-        { "a,b,,?", true, true, true },
+        { "help", true, false },
+        { "?", true, false },
+        { "helpme", false, false },
+        { "?me", false, false },
+        { "a,help", true, true },
+        { "a,?", true, true },
+        { "a=0,help,b", true, true },
+        { "a=0,?,b", true, true },
+        { "help,b=1", true, false },
+        { "?,b=1", true, false },
+        { "a,b,,help", true, true },
+        { "a,b,,?", true, true },
     };
     int i;
     QemuOpts *opts;
 
     for (i = 0; i < ARRAY_SIZE(test); i++) {
         g_assert_cmpint(has_help_option(test[i].params),
-                        ==, test[i].expect_has_help_option);
+                        ==, test[i].expect);
         opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
                                &error_abort);
         g_assert_cmpint(qemu_opt_has_help_opt(opts),
-                        ==, test[i].expect_opt_has_help_opt);
+                        ==, test[i].expect);
         qemu_opts_del(opts);
         opts = qemu_opts_parse(&opts_list_03, test[i].params, true,
                                &error_abort);
         g_assert_cmpint(qemu_opt_has_help_opt(opts),
-                        ==, test[i].expect_opt_has_help_opt_implied);
+                        ==, test[i].expect_implied);
         qemu_opts_del(opts);
     }
 }
-- 
2.21.1

[PATCH v2 for-5.1 6/9] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()

[Markus Armbruster]

When opts_parse() sets @invalidp to true, qemu_opts_parse_noisily()
uses has_help_option() to decide whether to print help.  This parses
the input string a second time.

Easy to avoid: replace @invalidp by @help_wanted.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 util/qemu-option.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 0abf26b61f..2d0d24ee27 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -519,7 +519,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
 }
 
 static void opt_set(QemuOpts *opts, const char *name, char *value,
-                    bool prepend, bool *invalidp, Error **errp)
+                    bool prepend, bool *help_wanted, Error **errp)
 {
     QemuOpt *opt;
     const QemuOptDesc *desc;
@@ -529,8 +529,8 @@ static void opt_set(QemuOpts *opts, const char *name, char *value,
     if (!desc && !opts_accepts_any(opts)) {
         g_free(value);
         error_setg(errp, QERR_INVALID_PARAMETER, name);
-        if (invalidp) {
-            *invalidp = true;
+        if (help_wanted && is_help_option(name)) {
+            *help_wanted = true;
         }
         return;
     }
@@ -827,7 +827,7 @@ static const char *get_opt_name_value(const char *params,
 
 static void opts_do_parse(QemuOpts *opts, const char *params,
                           const char *firstname, bool prepend,
-                          bool *invalidp, Error **errp)
+                          bool *help_wanted, Error **errp)
 {
     Error *local_err = NULL;
     char *option, *value;
@@ -843,7 +843,7 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             continue;
         }
 
-        opt_set(opts, option, value, prepend, invalidp, &local_err);
+        opt_set(opts, option, value, prepend, help_wanted, &local_err);
         g_free(option);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -903,7 +903,7 @@ void qemu_opts_do_parse(QemuOpts *opts, const char *params,
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
                             bool permit_abbrev, bool defaults,
-                            bool *invalidp, Error **errp)
+                            bool *help_wanted, Error **errp)
 {
     const char *firstname;
     char *id = opts_parse_id(params);
@@ -928,7 +928,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         return NULL;
     }
 
-    opts_do_parse(opts, params, firstname, defaults, invalidp, &local_err);
+    opts_do_parse(opts, params, firstname, defaults, help_wanted, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         qemu_opts_del(opts);
@@ -964,11 +964,11 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
 {
     Error *err = NULL;
     QemuOpts *opts;
-    bool invalidp = false;
+    bool help_wanted = false;
 
-    opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
+    opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
     if (err) {
-        if (invalidp && has_help_option(params)) {
+        if (help_wanted) {
             qemu_opts_print_help(list, true);
             error_free(err);
         } else {
-- 
2.21.1

[PATCH v2 for-5.1 7/9] qemu-img: Factor out accumulate_options() helper

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 59 +++++++++++++++++++++---------------------------------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 821cbf610e..d36b21b758 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -223,6 +223,25 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
     return true;
 }
 
+static int accumulate_options(char **options, char *optarg)
+{
+    char *new_options;
+
+    if (!is_valid_option_list(optarg)) {
+        error_report("Invalid option list: %s", optarg);
+        return -1;
+    }
+
+    if (!*options) {
+        *options = g_strdup(optarg);
+    } else {
+        new_options = g_strdup_printf("%s,%s", *options, optarg);
+        g_free(*options);
+        *options = new_options;
+    }
+    return 0;
+}
+
 static QemuOptsList qemu_source_opts = {
     .name = "source",
     .implied_opt_name = "file",
@@ -482,17 +501,9 @@ static int img_create(int argc, char **argv)
             fmt = optarg;
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 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;
@@ -2127,17 +2138,9 @@ static int img_convert(int argc, char **argv)
             s.compressed = true;
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 goto fail_getopt;
             }
-            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 'l':
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
@@ -3953,18 +3956,10 @@ static int img_amend(int argc, char **argv)
             help();
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 ret = -1;
                 goto out_no_progress;
             }
-            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;
@@ -4855,17 +4850,9 @@ static int img_measure(int argc, char **argv)
             out_fmt = optarg;
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 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 'l':
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
-- 
2.21.1

[PATCH v2 for-5.1 8/9] qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite

[Markus Armbruster]

is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
join multiple parameter strings separated by ',' like this:

        g_strdup_printf("%s,%s", params1, params2);

How it does that is anything but obvious.  A close reading of the code
reveals that it fails exactly when its argument starts with ',' or
ends with an odd number of ','.  Makes sense, actually, because when
the argument starts with ',', a separating ',' preceding it would get
escaped, and when it ends with an odd number of ',', a separating ','
following it would get escaped.

Move it to qemu-img.c and rewrite it the obvious way.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qemu/option.h |  1 -
 qemu-img.c            | 26 ++++++++++++++++++++++++++
 util/qemu-option.c    | 22 ----------------------
 3 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 844587cab3..eb4097889d 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -33,7 +33,6 @@ const char *get_opt_value(const char *p, char **value);
 void parse_option_size(const char *name, const char *value,
                        uint64_t *ret, Error **errp);
 bool has_help_option(const char *param);
-bool is_valid_option_list(const char *param);
 
 enum QemuOptType {
     QEMU_OPT_STRING = 0,  /* no parsing (use string as-is)                        */
diff --git a/qemu-img.c b/qemu-img.c
index d36b21b758..cc51db7ed4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
     return true;
 }
 
+/*
+ * Is @optarg safe for accumulate_options()?
+ * It is when multiple of them can be joined together separated by ','.
+ * To make that work, @optarg must not start with ',' (or else a
+ * separating ',' preceding it gets escaped), and it must not end with
+ * an odd number of ',' (or else a separating ',' following it gets
+ * escaped).
+ */
+static bool is_valid_option_list(const char *optarg)
+{
+    size_t len = strlen(optarg);
+    size_t i;
+
+    if (optarg[0] == ',') {
+        return false;
+    }
+
+    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
+    }
+    if ((len - i) % 2) {
+        return false;
+    }
+
+    return true;
+}
+
 static int accumulate_options(char **options, char *optarg)
 {
     char *new_options;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 2d0d24ee27..9542988183 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -165,28 +165,6 @@ void parse_option_size(const char *name, const char *value,
     *ret = size;
 }
 
-bool is_valid_option_list(const char *p)
-{
-    char *value = NULL;
-    bool result = false;
-
-    while (*p) {
-        p = get_opt_value(p, &value);
-        if ((*p && !*++p) ||
-            (!*value || *value == ',')) {
-            goto out;
-        }
-
-        g_free(value);
-        value = NULL;
-    }
-
-    result = true;
-out:
-    g_free(value);
-    return result;
-}
-
 static const char *opt_type_to_string(enum QemuOptType type)
 {
     switch (type) {
-- 
2.21.1

[PATCH v2 for-5.1 9/9] qemu-img: Reject broken -o ""

[Markus Armbruster]

qemu-img create, convert, amend, and measure use accumulate_options()
to merge multiple -o options.  This is broken for -o "":

    $ qemu-img create -f qcow2 -o backing_file=a -o "" -o backing_fmt=raw,size=1M new.qcow2
    qemu-img: warning: Could not verify backing image. This may become an error in future versions.
    Could not open 'a,backing_fmt=raw': No such file or directory
    Formatting 'new.qcow2', fmt=qcow2 size=1048576 backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
    $ qemu-img info new.qcow2
    image: new.qcow2
    file format: qcow2
    virtual size: 1 MiB (1048576 bytes)
    disk size: 196 KiB
    cluster_size: 65536
--> backing file: a,backing_fmt=raw
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false

Merging these three -o the obvious way is wrong, because it results in
an unwanted ',' escape:

    backing_file=a,,backing_fmt=raw,size=1M
                  ~~

We could silently drop -o "", but Kevin asked me to reject it instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-img.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index cc51db7ed4..a2369766f0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -229,14 +229,16 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
  * To make that work, @optarg must not start with ',' (or else a
  * separating ',' preceding it gets escaped), and it must not end with
  * an odd number of ',' (or else a separating ',' following it gets
- * escaped).
+ * escaped), or be empty (or else a separating ',' preceding it can
+ * escape a separating ',' following it).
+ * 
  */
 static bool is_valid_option_list(const char *optarg)
 {
     size_t len = strlen(optarg);
     size_t i;
 
-    if (optarg[0] == ',') {
+    if (!optarg[0] || optarg[0] == ',') {
         return false;
     }
 
-- 
2.21.1

Re: [PATCH v2 for-5.1 9/9] qemu-img: Reject broken -o ""

[Eric Blake]

On 4/15/20 2:49 AM, Markus Armbruster wrote:
> qemu-img create, convert, amend, and measure use accumulate_options()
> to merge multiple -o options.  This is broken for -o "":
> 
>      $ qemu-img create -f qcow2 -o backing_file=a -o "" -o backing_fmt=raw,size=1M new.qcow2
>      qemu-img: warning: Could not verify backing image. This may become an error in future versions.
>      Could not open 'a,backing_fmt=raw': No such file or directory
>      Formatting 'new.qcow2', fmt=qcow2 size=1048576 backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
>      $ qemu-img info new.qcow2
>      image: new.qcow2
>      file format: qcow2
>      virtual size: 1 MiB (1048576 bytes)
>      disk size: 196 KiB
>      cluster_size: 65536
> --> backing file: a,backing_fmt=raw
>      Format specific information:
>          compat: 1.1
>          lazy refcounts: false
>          refcount bits: 16
>          corrupt: false
> 
> Merging these three -o the obvious way is wrong, because it results in
> an unwanted ',' escape:
> 
>      backing_file=a,,backing_fmt=raw,size=1M
>                    ~~
> 
> We could silently drop -o "", but Kevin asked me to reject it instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qemu-img.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

This results in the error message:
qemu-img: Invalid option list:

with a trailing space and no indication that it was an empty string we 
were trying to warn about.  But that's tolerable.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200415074927.19897-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up
Message-id: 20200415074927.19897-1-armbru@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200415083048.14339-1-armbru@redhat.com -> patchew/20200415083048.14339-1-armbru@redhat.com
Switched to a new branch 'test'
3b33be5 qemu-img: Reject broken -o ""
7b5dadd qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite
30bbe34 qemu-img: Factor out accumulate_options() helper
c2990e9 qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
2f6d0e4 test-qemu-opts: Simplify test_has_help_option() after bug fix
6268d34 qemu-option: Fix has_help_option()'s sloppy parsing
9698bcc qemu-option: Fix sloppy recognition of "id=..." after ", , "
9537d23 qemu-options: Factor out get_opt_name_value() helper
0f6839c tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

=== OUTPUT BEGIN ===
1/9 Checking commit 0f6839cdf754 (tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt())
WARNING: Block comments use a leading /* on a separate line
#42: FILE: tests/test-qemu-opts.c:752:
+        { "a,b,,help", false /* BUG */, true, true },

WARNING: Block comments use a leading /* on a separate line
#43: FILE: tests/test-qemu-opts.c:753:
+        { "a,b,,?", false /* BUG */, true, true },

total: 0 errors, 2 warnings, 56 lines checked

Patch 1/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/9 Checking commit 9537d23096a7 (qemu-options: Factor out get_opt_name_value() helper)
3/9 Checking commit 9698bcc1c1fe (qemu-option: Fix sloppy recognition of "id=..." after ", , ")
4/9 Checking commit 6268d3455410 (qemu-option: Fix has_help_option()'s sloppy parsing)
5/9 Checking commit 2f6d0e4867b6 (test-qemu-opts: Simplify test_has_help_option() after bug fix)
6/9 Checking commit c2990e93eb56 (qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily())
7/9 Checking commit 30bbe34c8b6f (qemu-img: Factor out accumulate_options() helper)
8/9 Checking commit 7b5daddaec17 (qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite)
ERROR: suspect code indent for conditional statements (4, 4)
#62: FILE: qemu-img.c:243:
+    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
+    }

total: 1 errors, 0 warnings, 67 lines checked

Patch 8/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

9/9 Checking commit 3b33be56023f (qemu-img: Reject broken -o "")
ERROR: trailing whitespace
#49: FILE: qemu-img.c:234:
+ * $

total: 1 errors, 0 warnings, 18 lines checked

Patch 9/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200415074927.19897-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up

[Markus Armbruster]

Queued.