[PATCH v2 03/11] qemu-option: Remove short form options support

[Fabiano Rosas <farosas@suse.de>]

Option parameters without a value (i.e. key vs. key=val) are
deprecated. The missing value is currently implied to be either "on"
or "off" depending on whether the parameter name starts with "no".

Remove the implied behavior and start rejecting option parameters
without values by emitting the error message:

 "Parameter 'foo' without a value."

Two special cases remain:

1) The 'help' parameter is special and still supported. Keep a
   positive value attached to it ("on").

   Note that currently there is no validation for the value (if any)
   attached to the help parameter. help, help=on, help=off, help=foo
   all result in the help text being emitted. This is not changed by
   this patch.

2) The empty value is allowed if the key is also empty. This can be
   achieved in the command line by adding commas.
   E.g.: share=on,, is parsed as:
     key:"share" value:"on" and
     key:"" value:"on"

One effect of this change that might not be obvious is that parameter
combinations of the form:

 id=mem0,share,help

no longer produce the help output. The lack of value for the 'share'
parameter is reported with precedence. Users will need to either use a
valid value or omit the parameter entirely:

 id=mem0,share=on,help
 id=mem0,help

About testing:

A few strings from the test_has_help_option test were removed because
they now trigger the missing value error during parsing and therefore
cannot be used for testing the qemu_opt_has_help_opt() routine which
needs the parsed QemuOpts.

The two "Implied value" tests were replaced with 8 new tests.

The "Implied key" test has had the extra parameters with implied
values removed, otherwise the implied key cannot be tested due to the
rejected missing values.

The test_inet_parse_all_implicit_bool_good was removed entirely
because it only tested the use case that is being removed.

A few block tests were updated to use the key=value form.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 docs/about/deprecated.rst      | 13 -------
 tests/qemu-iotests/084         |  2 +-
 tests/qemu-iotests/146         |  2 +-
 tests/qemu-iotests/197         |  2 +-
 tests/qemu-iotests/215         |  2 +-
 tests/unit/test-qemu-opts.c    | 67 +++++++++++++++++++++-------------
 tests/unit/test-util-sockets.c | 33 -----------------
 util/qemu-option.c             | 57 ++++++++++++++---------------
 8 files changed, 73 insertions(+), 105 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index ccbc421949..f54905eba9 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -30,19 +30,6 @@ deprecated.
 System emulator command line arguments
 --------------------------------------
 
-Short-form boolean options (since 6.0)
-''''''''''''''''''''''''''''''''''''''
-
-Boolean options such as ``share=on``/``share=off`` could be written
-in short form as ``share`` and ``noshare``.  This is now deprecated
-and will cause a warning.
-
-``delay`` option for socket character devices (since 6.0)
-'''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on``
-rather than ``delay=off``.
-
 Plugin argument passing through ``arg=<string>`` (since 6.1)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
diff --git a/tests/qemu-iotests/084 b/tests/qemu-iotests/084
index 1181cb7cd0..f905396e72 100755
--- a/tests/qemu-iotests/084
+++ b/tests/qemu-iotests/084
@@ -51,7 +51,7 @@ bii_offset=384 # block in image field offset
 echo
 echo "=== Statically allocated image creation ==="
 echo
-_make_test_img $size -o static
+_make_test_img $size -o static=on
 _img_info
 stat -c"disk image file size in bytes: %s" "${TEST_IMG}"
 _cleanup_test_img
diff --git a/tests/qemu-iotests/146 b/tests/qemu-iotests/146
index f63291d0ff..a7853e03c9 100755
--- a/tests/qemu-iotests/146
+++ b/tests/qemu-iotests/146
@@ -165,7 +165,7 @@ echo
 echo === Testing Image create, force_size ===
 echo
 
-_make_test_img -o force_size 4G
+_make_test_img -o force_size=on 4G
 
 echo
 echo === Read created image, default opts ====
diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
index 69849c800e..271d1283c5 100755
--- a/tests/qemu-iotests/197
+++ b/tests/qemu-iotests/197
@@ -63,7 +63,7 @@ echo
 # Prep the images
 # VPC rounds image sizes to a specific geometry, force a specific size.
 if [ "$IMGFMT" = "vpc" ]; then
-    IMGOPTS=$(_optstr_add "$IMGOPTS" "force_size")
+    IMGOPTS=$(_optstr_add "$IMGOPTS" "force_size=on")
 fi
 _make_test_img 4G
 $QEMU_IO -c "write -P 55 3G 1k" "$TEST_IMG" | _filter_qemu_io
diff --git a/tests/qemu-iotests/215 b/tests/qemu-iotests/215
index 6babbcdc1f..6a518a9f57 100755
--- a/tests/qemu-iotests/215
+++ b/tests/qemu-iotests/215
@@ -60,7 +60,7 @@ echo
 # Prep the images
 # VPC rounds image sizes to a specific geometry, force a specific size.
 if [ "$IMGFMT" = "vpc" ]; then
-    IMGOPTS=$(_optstr_add "$IMGOPTS" "force_size")
+    IMGOPTS=$(_optstr_add "$IMGOPTS" "force_size=on")
 fi
 _make_test_img 4G
 $QEMU_IO -c "write -P 55 3G 1k" "$TEST_IMG" | _filter_qemu_io
diff --git a/tests/unit/test-qemu-opts.c b/tests/unit/test-qemu-opts.c
index 4ccb79d406..07ede50920 100644
--- a/tests/unit/test-qemu-opts.c
+++ b/tests/unit/test-qemu-opts.c
@@ -481,26 +481,10 @@ static void test_opts_parse(void)
     error_free_or_abort(&err);
     g_assert(!opts);
 
-    /* Implied value (qemu_opts_parse warns but accepts it) */
-    opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=",
-                           false, &error_abort);
-    g_assert_cmpuint(opts_count(opts), ==, 3);
-    g_assert_cmpstr(qemu_opt_get(opts, "an"), ==, "on");
-    g_assert_cmpstr(qemu_opt_get(opts, "aus"), ==, "off");
-    g_assert_cmpstr(qemu_opt_get(opts, "noaus"), ==, "");
-
-    /* Implied value, negated empty key */
-    opts = qemu_opts_parse(&opts_list_03, "no", false, &error_abort);
-    g_assert_cmpuint(opts_count(opts), ==, 1);
-    g_assert_cmpstr(qemu_opt_get(opts, ""), ==, "off");
-
     /* Implied key */
-    opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=", true,
-                           &error_abort);
-    g_assert_cmpuint(opts_count(opts), ==, 3);
+    opts = qemu_opts_parse(&opts_list_03, "an", true, &error_abort);
+    g_assert_cmpuint(opts_count(opts), ==, 1);
     g_assert_cmpstr(qemu_opt_get(opts, "implied"), ==, "an");
-    g_assert_cmpstr(qemu_opt_get(opts, "aus"), ==, "off");
-    g_assert_cmpstr(qemu_opt_get(opts, "noaus"), ==, "");
 
     /* Implied key with empty value */
     opts = qemu_opts_parse(&opts_list_03, ",", true, &error_abort);
@@ -523,6 +507,45 @@ static void test_opts_parse(void)
     error_free_or_abort(&err);
     g_assert(!opts);
 
+    /* Implied value */
+    opts = qemu_opts_parse(&opts_list_03, "an", false, &err);
+    error_free_or_abort(&err);
+    g_assert(!opts);
+
+    /* Implied value, unknown key */
+    opts = qemu_opts_parse(&opts_list_01, "nonexistent", false, &err);
+    error_free_or_abort(&err);
+    g_assert(!opts);
+
+    /* Implied value, negated key */
+    opts = qemu_opts_parse(&opts_list_03, "noaus", false, &err);
+    error_free_or_abort(&err);
+    g_assert(!opts);
+
+    /* Implied value, negated empty key */
+    opts = qemu_opts_parse(&opts_list_03, "no", false, &err);
+    error_free_or_abort(&err);
+    g_assert(!opts);
+
+    /* Empty value */
+    opts = qemu_opts_parse(&opts_list_03, "aus=", false, &error_abort);
+    g_assert_cmpuint(opts_count(opts), ==, 1);
+
+    /* Implied key */
+    opts = qemu_opts_parse(&opts_list_03, "an", true, &error_abort);
+    g_assert_cmpuint(opts_count(opts), ==, 1);
+
+    /* Implied key, implied value */
+    opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=", true, &err);
+    error_free_or_abort(&err);
+    g_assert(!opts);
+
+    /* Implied key, empty value */
+    opts = qemu_opts_parse(&opts_list_03, "an,noaus=", true, &error_abort);
+    g_assert_cmpuint(opts_count(opts), ==, 2);
+    g_assert_cmpstr(qemu_opt_get(opts, "implied"), ==, "an");
+    g_assert_cmpstr(qemu_opt_get(opts, "noaus"), ==, "");
+
     qemu_opts_reset(&opts_list_01);
     qemu_opts_reset(&opts_list_03);
 }
@@ -720,16 +743,8 @@ static void test_has_help_option(void)
     } test[] = {
         { "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;
diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
index 006f5e579c..2875c547a0 100644
--- a/tests/unit/test-util-sockets.c
+++ b/tests/unit/test-util-sockets.c
@@ -516,36 +516,6 @@ static void test_inet_parse_all_options_good(void)
         , &exp_addr, true);
 }
 
-static void test_inet_parse_all_implicit_bool_good(void)
-{
-    char host[] = "::1";
-    char port[] = "5000";
-    InetSocketAddress exp_addr = {
-        .host = host,
-        .port = port,
-        .has_numeric = true,
-        .numeric =  true,
-        .has_to = true,
-        .to = 5006,
-        .has_ipv4 = true,
-        .ipv4 = true,
-        .has_ipv6 = true,
-        .ipv6 = true,
-        .has_keep_alive = true,
-        .keep_alive = true,
-#ifdef HAVE_IPPROTO_MPTCP
-        .has_mptcp = true,
-        .mptcp = true,
-#endif
-    };
-    inet_parse_test_helper(
-        "[::1]:5000,numeric,to=5006,ipv4,ipv6,keep-alive"
-#ifdef HAVE_IPPROTO_MPTCP
-        ",mptcp"
-#endif
-        , &exp_addr, true);
-}
-
 int main(int argc, char **argv)
 {
     bool has_ipv4, has_ipv6;
@@ -613,9 +583,6 @@ int main(int argc, char **argv)
                     test_inet_parse_hostname_good);
     g_test_add_func("/util/socket/inet-parse/all-options-good",
                     test_inet_parse_all_options_good);
-    g_test_add_func("/util/socket/inet-parse/all-bare-bool-good",
-                    test_inet_parse_all_implicit_bool_good);
-
 end:
     return g_test_run();
 }
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 557d5d929c..8ac848bbc0 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -509,6 +509,11 @@ static bool opt_validate(QemuOpt *opt, Error **errp)
     const QemuOptDesc *desc;
     const QemuOptsList *list = opt->opts->list;
 
+    if (!opt->str) {
+        error_setg(errp, "Parameter '%s' without a value", opt->name);
+        return false;
+    }
+
     desc = find_desc_by_name(list->desc, opt->name);
     if (!desc && !opts_accepts_any(list)) {
         error_setg(errp, "Invalid parameter '%s'", opt->name);
@@ -755,12 +760,10 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
 
 static const char *get_opt_name_value(const char *params,
                                       const char *firstname,
-                                      bool warn_on_flag,
                                       bool *help_wanted,
                                       char **name, char **value)
 {
     const char *p;
-    const char *prefix = "";
     size_t len;
     bool is_help = false;
 
@@ -772,23 +775,22 @@ static const char *get_opt_name_value(const char *params,
             *name = g_strdup(firstname);
             p = get_opt_value(params, value);
         } else {
-            /* option without value, must be a flag */
             p = get_opt_name(params, name, len);
-            if (strncmp(*name, "no", 2) == 0) {
-                memmove(*name, *name + 2, strlen(*name + 2) + 1);
-                *value = g_strdup("off");
-                prefix = "no";
-            } else {
+
+            /*
+             * 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");
-                is_help = is_help_option(*name);
             }
-            if (!is_help && warn_on_flag) {
-                warn_report("short-form boolean option '%s%s' deprecated", prefix, *name);
-                if (g_str_equal(*name, "delay")) {
-                    error_printf("Please use nodelay=%s instead\n", prefix[0] ? "on" : "off");
-                } else {
-                    error_printf("Please use %s=%s instead\n", *name, *value);
-                }
+
+            /* a missing, non-implicit key, i.e. a single comma ',' */
+            if (g_str_equal(*name, "") && *p == ',') {
+                *value = g_strdup("on");
             }
         }
     } else {
@@ -811,7 +813,7 @@ static const char *get_opt_name_value(const char *params,
 
 static bool opts_do_parse(QemuOpts *opts, const char *params,
                           const char *firstname,
-                          bool warn_on_flag, bool *help_wanted, Error **errp)
+                          bool *help_wanted, Error **errp)
 {
     const char *p;
     QemuOpt *opt;
@@ -820,7 +822,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, warn_on_flag, help_wanted, &option, &value);
+        p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
         if (help_wanted && *help_wanted) {
             return false;
         }
@@ -848,7 +850,7 @@ 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, false, NULL, &name, &value);
+        p = get_opt_name_value(p, NULL, NULL, &name, &value);
         if (!strcmp(name, "id")) {
             return g_steal_pointer(&value);
         }
@@ -866,7 +868,7 @@ bool has_help_option(const char *params)
         g_autofree char *name = NULL;
         g_autofree char *value = NULL;
 
-        p = get_opt_name_value(p, NULL, false, &ret, &name, &value);
+        p = get_opt_name_value(p, NULL, &ret, &name, &value);
         if (ret) {
             return true;
         }
@@ -884,12 +886,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, false, NULL, errp);
+    return opts_do_parse(opts, params, firstname, NULL, errp);
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
-                            bool permit_abbrev,
-                            bool warn_on_flag, bool *help_wanted, Error **errp)
+                            bool permit_abbrev, bool *help_wanted, Error **errp)
 {
     const char *firstname;
     char *id = opts_parse_id(params);
@@ -904,8 +905,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         return NULL;
     }
 
-    if (!opts_do_parse(opts, params, firstname,
-                       warn_on_flag, help_wanted, errp)) {
+    if (!opts_do_parse(opts, params, firstname, help_wanted, errp)) {
         qemu_opts_del(opts);
         return NULL;
     }
@@ -923,7 +923,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, false, NULL, errp);
+    return opts_parse(list, params, permit_abbrev, NULL, errp);
 }
 
 /**
@@ -941,9 +941,8 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
     QemuOpts *opts;
     bool help_wanted = false;
 
-    opts = opts_parse(list, params, permit_abbrev, true,
-                      opts_accepts_any(list) ? NULL : &help_wanted,
-                      &err);
+    opts = opts_parse(list, params, permit_abbrev,
+                      opts_accepts_any(list) ? NULL : &help_wanted, &err);
     if (!opts) {
         assert(!!err + !!help_wanted == 1);
         if (help_wanted) {
-- 
2.53.0