[PATCH v2 04/11] qemu-option: Fix 'help' parameter parsing

[Fabiano Rosas <farosas@suse.de>]

Reject invalid values such as help= and help=foo.

Reject valid, but not applicable values such has help=on and help=off.

To achieve the above:

Add an 'is_help' member to QemuOpt so that information can be stored
while creating the option. This allows moving the help handling out of
get_opt_name_value(), which is necessary because that function doesn't
take an Error.

At the caller, opts_do_parse(), let the QemuOpt be created with the
name and value fields as they come out of get_opt_name_value().

With a new routine, validate the new QemuOpt against the 'help'
parameter rules (to fix the bug), then let the existing opt_validate()
reject any NULL values. The two validations need to be separate
because in between there needs to be a setting of opt->str to "on"
whenever the special short-form parameters require it.

The "on" value is required because of the conversions of QemuOpts
from/to QDict since QDict cannot take a NULL value. The conversions
also cause opt_validate() to be reentrant via qemu_opt_set(), so the
initial validation of 'help' needs to be out of that function,
otherwise there's no way to distinguish an user-set help=on vs. the
help=on set for compatibility with QDict (the "in between" part from
the previous paragraph).

The passing around of help_wanted becomes redundant since opt->is_help
can be checked throughout. Remove that argument everywhere.

Stop returning false from opts_do_parse() when is_help=true. Only
return false when there's an error and let the callers decide what to
do when is_help=true. However, still break the loop when is_help=true
to keep the behavior of stopping parsing once the 'help' parameter is
seen.

At qemu_opts_parse_noisily(), due to the change of return value
mentioned above, it becomes clear when to report the error and when to
print the help text. Use qemu_opt_has_help_opt() to obtain the
is_help information from QemuOpt.

Since get_opt_name_value() can now return while value=NULL, the other
two callsites need to be updated. At opts_parse_id(), assert value !=
NULL. At has_help_option(), use the NULL value to keep the existing
behavior of rejecting anything but 'help'.

The unit tests are updated accordingly.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/qemu/option_int.h   |   1 +
 tests/unit/test-qemu-opts.c |   9 ++--
 util/qemu-option.c          | 104 ++++++++++++++++++++----------------
 3 files changed, 63 insertions(+), 51 deletions(-)

diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h
index 5dd9a5162d..c31bd6e3ab 100644
--- a/include/qemu/option_int.h
+++ b/include/qemu/option_int.h
@@ -32,6 +32,7 @@
 struct QemuOpt {
     char *name;
     char *str;
+    bool is_help;
 
     const QemuOptDesc *desc;
     union {
diff --git a/tests/unit/test-qemu-opts.c b/tests/unit/test-qemu-opts.c
index 07ede50920..7cedfba332 100644
--- a/tests/unit/test-qemu-opts.c
+++ b/tests/unit/test-qemu-opts.c
@@ -777,16 +777,15 @@ static void test_has_help_option_with_value(void)
     };
     int i;
     QemuOpts *opts;
+    Error *err = NULL;
 
     for (i = 0; i < ARRAY_SIZE(test); i++) {
         /* reject all */
         g_assert_cmpint(has_help_option(test[i].params), ==, false);
 
-        /* accept all */
-        opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
-                               &error_abort);
-        g_assert_cmpint(qemu_opt_has_help_opt(opts), ==, true);
-        qemu_opts_del(opts);
+        opts = qemu_opts_parse(&opts_list_03, test[i].params, false, &err);
+        error_free_or_abort(&err);
+        g_assert(!opts);
     }
 }
 
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 8ac848bbc0..483e6cbf70 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -331,7 +331,7 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
     QemuOpt *opt;
 
     QTAILQ_FOREACH_REVERSE(opt, &opts->head, next) {
-        if (is_help_option(opt->name)) {
+        if (opt->is_help) {
             return true;
         }
     }
@@ -499,6 +499,7 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value)
     opt->name = g_strdup(name);
     opt->str = value;
     opt->opts = opts;
+    opt->is_help = is_help_option(name);
     QTAILQ_INSERT_TAIL(&opts->head, opt, next);
 
     return opt;
@@ -760,12 +761,10 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
 
 static const char *get_opt_name_value(const char *params,
                                       const char *firstname,
-                                      bool *help_wanted,
                                       char **name, char **value)
 {
     const char *p;
     size_t len;
-    bool is_help = false;
 
     len = strcspn(params, "=,");
     if (params[len] != '=') {
@@ -776,22 +775,6 @@ static const char *get_opt_name_value(const char *params,
             p = get_opt_value(params, value);
         } else {
             p = get_opt_name(params, name, len);
-
-            /*
-             * Short-form flags (i.e. without any value) are not
-             * supported, except for two cases:
-             */
-
-            /* the 'help' or '?' flag */
-            is_help = is_help_option(*name);
-            if (is_help) {
-                *value = g_strdup("on");
-            }
-
-            /* a missing, non-implicit key, i.e. a single comma ',' */
-            if (g_str_equal(*name, "") && *p == ',') {
-                *value = g_strdup("on");
-            }
         }
     } else {
         /* found "foo=bar,more" */
@@ -802,18 +785,46 @@ static const char *get_opt_name_value(const char *params,
     }
 
     assert(!*p || *p == ',');
-    if (help_wanted && is_help) {
-        *help_wanted = true;
-    }
     if (*p == ',') {
         p++;
     }
     return p;
 }
 
+/*
+ * Short-form parameters (i.e. without any value) are not
+ * supported, except for two cases:
+ *
+ * - the 'help' or '?' flag
+ * - the empty, non-implicit key combined with an empty
+ *   non-implicit value, i.e. a single comma ','
+ *
+ * The rest of the code is not prepared to deal with empty values, set
+ * the special cases to "on".
+ */
+static bool opt_short_form_compat(QemuOpt *opt, Error **errp)
+{
+    if (opt->str) {
+        /*
+         * Validate the 'help' parameter at this point because the
+         * opt_validate() routine can be reentrant and this very check
+         * would fail once the "on" value is set below.
+         */
+        if (opt->is_help) {
+            error_setg(errp, "Parameter '%s' doesn't take any values",
+                       opt->name);
+            return false;
+        }
+    } else {
+        if (opt->is_help || g_str_equal(opt->name, "")) {
+            opt->str = g_strdup("on");
+        }
+    }
+    return true;
+}
+
 static bool opts_do_parse(QemuOpts *opts, const char *params,
-                          const char *firstname,
-                          bool *help_wanted, Error **errp)
+                          const char *firstname, Error **errp)
 {
     const char *p;
     QemuOpt *opt;
@@ -822,10 +833,7 @@ static bool opts_do_parse(QemuOpts *opts, const char *params,
         g_autofree char *option = NULL;
         g_autofree char *value = NULL;
 
-        p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
-        if (help_wanted && *help_wanted) {
-            return false;
-        }
+        p = get_opt_name_value(p, firstname, &option, &value);
         firstname = NULL;
 
         if (!strcmp(option, "id")) {
@@ -833,10 +841,15 @@ static bool opts_do_parse(QemuOpts *opts, const char *params,
         }
 
         opt = opt_create(opts, option, g_steal_pointer(&value));
-        if (!opt_validate(opt, errp)) {
+        if (!opt_short_form_compat(opt, errp) ||
+            !opt_validate(opt, errp)) {
             qemu_opt_del(opt);
             return false;
         }
+
+        if (opt->is_help) {
+            return true;
+        }
     }
 
     return true;
@@ -850,8 +863,9 @@ static char *opts_parse_id(const char *params)
         g_autofree char *name = NULL;
         g_autofree char *value = NULL;
 
-        p = get_opt_name_value(p, NULL, NULL, &name, &value);
+        p = get_opt_name_value(p, NULL, &name, &value);
         if (!strcmp(name, "id")) {
+            assert(value);
             return g_steal_pointer(&value);
         }
     }
@@ -862,14 +876,13 @@ static char *opts_parse_id(const char *params)
 bool has_help_option(const char *params)
 {
     const char *p;
-    bool ret = false;
 
     for (p = params; *p;) {
         g_autofree char *name = NULL;
         g_autofree char *value = NULL;
 
-        p = get_opt_name_value(p, NULL, &ret, &name, &value);
-        if (ret) {
+        p = get_opt_name_value(p, NULL, &name, &value);
+        if (is_help_option(name) && !value) {
             return true;
         }
     }
@@ -886,11 +899,11 @@ bool has_help_option(const char *params)
 bool qemu_opts_do_parse(QemuOpts *opts, const char *params,
                        const char *firstname, Error **errp)
 {
-    return opts_do_parse(opts, params, firstname, NULL, errp);
+    return opts_do_parse(opts, params, firstname, errp);
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
-                            bool permit_abbrev, bool *help_wanted, Error **errp)
+                            bool permit_abbrev, Error **errp)
 {
     const char *firstname;
     char *id = opts_parse_id(params);
@@ -905,7 +918,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         return NULL;
     }
 
-    if (!opts_do_parse(opts, params, firstname, help_wanted, errp)) {
+    if (!opts_do_parse(opts, params, firstname, errp)) {
         qemu_opts_del(opts);
         return NULL;
     }
@@ -923,7 +936,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
                           bool permit_abbrev, Error **errp)
 {
-    return opts_parse(list, params, permit_abbrev, NULL, errp);
+    return opts_parse(list, params, permit_abbrev, errp);
 }
 
 /**
@@ -939,17 +952,16 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
 {
     Error *err = NULL;
     QemuOpts *opts;
-    bool help_wanted = false;
 
-    opts = opts_parse(list, params, permit_abbrev,
-                      opts_accepts_any(list) ? NULL : &help_wanted, &err);
+    opts = opts_parse(list, params, permit_abbrev, &err);
     if (!opts) {
-        assert(!!err + !!help_wanted == 1);
-        if (help_wanted) {
-            qemu_opts_print_help(list, true);
-        } else {
-            error_report_err(err);
-        }
+        error_report_err(err);
+        return NULL;
+    }
+
+    if (qemu_opt_has_help_opt(opts) && !opts_accepts_any(list)) {
+        qemu_opts_print_help(list, true);
+        return NULL;
     }
     return opts;
 }
-- 
2.53.0