[PATCH] vl: fix machine option containing underscores

[PATCH] vl: fix machine option containing underscores

[Jean-Philippe Brucker]

Since commit d8fb7d0969d5 ("vl: switch -M parsing to keyval"),
keyval_dashify() replaces all underscores with dashes in machine
options. As a result the machine option "default_bus_bypass_iommu",
which was introduced in the same release (c9e96b04fc19 6d7a85483a06), is
not recognized:

$ qemu-system-aarch64 -M virt,default_bus_bypass_iommu=on
qemu-system-aarch64: Property 'virt-6.1-machine.default-bus-bypass-iommu' not found

Before that commit, dashification was only applied temporarily within
machine_set_property() to check the legacy options. Restore that
behavior by explicitly checking for aliases of these options instead of
transforming all machine options.

Fixes: d8fb7d0969d5 ("vl: switch -M parsing to keyval")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
My first take was renaming default_bus_bypass_iommu, since it's the only
machine option with underscores, but then we'd want to rename
bypass_iommu as well for consistency and update all the docs. I prefer
this but don't mind either way.
---
 softmmu/vl.c | 56 +++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 36 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5ca11e7469..3d3aa35279 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1660,41 +1660,25 @@ static int object_parse_property_opt(Object *obj,
     return 0;
 }
 
-/* *Non*recursively replace underscores with dashes in QDict keys.  */
-static void keyval_dashify(QDict *qdict, Error **errp)
+static const char *find_option_alias(QDict *qdict, const char *key,
+                                     const char *alias, const char **value)
 {
-    const QDictEntry *ent, *next;
-    char *p;
-
-    for (ent = qdict_first(qdict); ent; ent = next) {
-        g_autofree char *new_key = NULL;
-
-        next = qdict_next(qdict, ent);
-        if (!strchr(ent->key, '_')) {
-            continue;
-        }
-        new_key = g_strdup(ent->key);
-        for (p = new_key; *p; p++) {
-            if (*p == '_') {
-                *p = '-';
-            }
-        }
-        if (qdict_haskey(qdict, new_key)) {
-            error_setg(errp, "Conflict between '%s' and '%s'", ent->key, new_key);
-            return;
-        }
-        qobject_ref(ent->value);
-        qdict_put_obj(qdict, new_key, ent->value);
-        qdict_del(qdict, ent->key);
+    *value = qdict_get_try_str(qdict, key);
+    if (*value) {
+        return key;
+    }
+    *value = qdict_get_try_str(qdict, alias);
+    if (*value) {
+        return alias;
     }
+    return NULL;
 }
 
 static void qemu_apply_legacy_machine_options(QDict *qdict)
 {
+    const char *key;
     const char *value;
 
-    keyval_dashify(qdict, &error_fatal);
-
     /* Legacy options do not correspond to MachineState properties.  */
     value = qdict_get_try_str(qdict, "accel");
     if (value) {
@@ -1702,27 +1686,27 @@ static void qemu_apply_legacy_machine_options(QDict *qdict)
         qdict_del(qdict, "accel");
     }
 
-    value = qdict_get_try_str(qdict, "igd-passthru");
-    if (value) {
+    key = find_option_alias(qdict, "igd-passthru", "igd_passthru", &value);
+    if (key) {
         object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), "igd-passthru", value,
                                    false);
-        qdict_del(qdict, "igd-passthru");
+        qdict_del(qdict, key);
     }
 
-    value = qdict_get_try_str(qdict, "kvm-shadow-mem");
-    if (value) {
+    key = find_option_alias(qdict, "kvm-shadow-mem", "kvm_shadow_mem", &value);
+    if (key) {
         object_register_sugar_prop(ACCEL_CLASS_NAME("kvm"), "kvm-shadow-mem", value,
                                    false);
-        qdict_del(qdict, "kvm-shadow-mem");
+        qdict_del(qdict, key);
     }
 
-    value = qdict_get_try_str(qdict, "kernel-irqchip");
-    if (value) {
+    key = find_option_alias(qdict, "kernel-irqchip", "kernel_irqchip", &value);
+    if (key) {
         object_register_sugar_prop(ACCEL_CLASS_NAME("kvm"), "kernel-irqchip", value,
                                    false);
         object_register_sugar_prop(ACCEL_CLASS_NAME("whpx"), "kernel-irqchip", value,
                                    false);
-        qdict_del(qdict, "kernel-irqchip");
+        qdict_del(qdict, key);
     }
 }
 
-- 
2.32.0

Re: [PATCH] vl: fix machine option containing underscores

[Philippe Mathieu-Daudé]

+Paolo/Markus

On 8/10/21 3:12 PM, Jean-Philippe Brucker wrote:
> Since commit d8fb7d0969d5 ("vl: switch -M parsing to keyval"),
> keyval_dashify() replaces all underscores with dashes in machine
> options. As a result the machine option "default_bus_bypass_iommu",
> which was introduced in the same release (c9e96b04fc19 6d7a85483a06), is
> not recognized:
> 
> $ qemu-system-aarch64 -M virt,default_bus_bypass_iommu=on
> qemu-system-aarch64: Property 'virt-6.1-machine.default-bus-bypass-iommu' not found
> 
> Before that commit, dashification was only applied temporarily within
> machine_set_property() to check the legacy options. Restore that
> behavior by explicitly checking for aliases of these options instead of
> transforming all machine options.
> 
> Fixes: d8fb7d0969d5 ("vl: switch -M parsing to keyval")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> My first take was renaming default_bus_bypass_iommu, since it's the only
> machine option with underscores, but then we'd want to rename
> bypass_iommu as well for consistency and update all the docs. I prefer
> this but don't mind either way.
> ---
>  softmmu/vl.c | 56 +++++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 36 deletions(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 5ca11e7469..3d3aa35279 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1660,41 +1660,25 @@ static int object_parse_property_opt(Object *obj,
>      return 0;
>  }
>  
> -/* *Non*recursively replace underscores with dashes in QDict keys.  */
> -static void keyval_dashify(QDict *qdict, Error **errp)
> +static const char *find_option_alias(QDict *qdict, const char *key,
> +                                     const char *alias, const char **value)
>  {
> -    const QDictEntry *ent, *next;
> -    char *p;
> -
> -    for (ent = qdict_first(qdict); ent; ent = next) {
> -        g_autofree char *new_key = NULL;
> -
> -        next = qdict_next(qdict, ent);
> -        if (!strchr(ent->key, '_')) {
> -            continue;
> -        }
> -        new_key = g_strdup(ent->key);
> -        for (p = new_key; *p; p++) {
> -            if (*p == '_') {
> -                *p = '-';
> -            }
> -        }
> -        if (qdict_haskey(qdict, new_key)) {
> -            error_setg(errp, "Conflict between '%s' and '%s'", ent->key, new_key);
> -            return;
> -        }
> -        qobject_ref(ent->value);
> -        qdict_put_obj(qdict, new_key, ent->value);
> -        qdict_del(qdict, ent->key);
> +    *value = qdict_get_try_str(qdict, key);
> +    if (*value) {
> +        return key;
> +    }
> +    *value = qdict_get_try_str(qdict, alias);
> +    if (*value) {
> +        return alias;
>      }
> +    return NULL;
>  }
>  
>  static void qemu_apply_legacy_machine_options(QDict *qdict)
>  {
> +    const char *key;
>      const char *value;
>  
> -    keyval_dashify(qdict, &error_fatal);
> -
>      /* Legacy options do not correspond to MachineState properties.  */
>      value = qdict_get_try_str(qdict, "accel");
>      if (value) {
> @@ -1702,27 +1686,27 @@ static void qemu_apply_legacy_machine_options(QDict *qdict)
>          qdict_del(qdict, "accel");
>      }
>  
> -    value = qdict_get_try_str(qdict, "igd-passthru");
> -    if (value) {
> +    key = find_option_alias(qdict, "igd-passthru", "igd_passthru", &value);
> +    if (key) {
>          object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), "igd-passthru", value,
>                                     false);
> -        qdict_del(qdict, "igd-passthru");
> +        qdict_del(qdict, key);
>      }
>  
> -    value = qdict_get_try_str(qdict, "kvm-shadow-mem");
> -    if (value) {
> +    key = find_option_alias(qdict, "kvm-shadow-mem", "kvm_shadow_mem", &value);
> +    if (key) {
>          object_register_sugar_prop(ACCEL_CLASS_NAME("kvm"), "kvm-shadow-mem", value,
>                                     false);
> -        qdict_del(qdict, "kvm-shadow-mem");
> +        qdict_del(qdict, key);
>      }
>  
> -    value = qdict_get_try_str(qdict, "kernel-irqchip");
> -    if (value) {
> +    key = find_option_alias(qdict, "kernel-irqchip", "kernel_irqchip", &value);
> +    if (key) {
>          object_register_sugar_prop(ACCEL_CLASS_NAME("kvm"), "kernel-irqchip", value,
>                                     false);
>          object_register_sugar_prop(ACCEL_CLASS_NAME("whpx"), "kernel-irqchip", value,
>                                     false);
> -        qdict_del(qdict, "kernel-irqchip");
> +        qdict_del(qdict, key);
>      }
>  }
>  
> 

Re: [PATCH] vl: fix machine option containing underscores

[Paolo Bonzini]

On 10/08/21 15:12, Jean-Philippe Brucker wrote:
> My first take was renaming default_bus_bypass_iommu, since it's the only
> machine option with underscores, 

We should do that, since the underscore variant still works and the 
result is a simple one-line patch.

Paolo

> but then we'd want to rename
> bypass_iommu as well for consistency and update all the docs. I prefer
> this but don't mind either way.