Re: [PATCH] qdev: Fix set_pci_devfn() to visit option only once

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> pci_devfn properties accept either a string or an integer as input. To
> implement this, set_pci_devfn() first tries to visit the option as a
> string, and if that fails, it visits it as an integer instead. While the
> QemuOpts visitor happens to accept this, it is invalid according to the
> visitor interface. QObject input visitors run into an assertion failure
> when this is done.
>
> QObject input visitors are used with the JSON syntax version of -device
> on the command line:
>
> $ ./qemu-system-x86_64 -enable-kvm -M q35 -device pcie-pci-bridge,id=pci.1,bus=pcie.0 -blockdev null-co,node-name=disk -device '{ "driver": "virtio-blk-pci", "drive": "disk", "id": "virtio-disk0", "bus": "pci.1", "addr": 1 }'
> qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143: QObject *qobject_input_try_get_object(QObjectInputVisitor *, const char *, _Bool): Assertion `removed' failed.
>
> The proper way to accept both strings and integers is using the
> alternate mechanism, which tells us the type of the input before it's
> visited. With this information, we can directly visit it as the right
> type.
>
> This fixes set_pci_devfn() by using the alternate mechanism.
>
> Cc: qemu-stable@nongnu.org
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 54 +++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 35deef05f3..91d3ff4719 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -790,39 +790,57 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name,
>                            void *opaque, Error **errp)
>  {
>      Property *prop = opaque;
> +    GenericAlternate *alt;
>      int32_t value, *ptr = object_field_prop_ptr(obj, prop);
>      unsigned int slot, fn, n;
> -    char *str;
> +    g_autofree char *str = NULL;
> +
> +    if (!visit_start_alternate(v, name, &alt, sizeof(*alt), errp)) {
> +        return;
> +    }
> +
> +    switch (alt->type) {
> +    case QTYPE_QSTRING:
> +        if (!visit_type_str(v, name, &str, errp)) {
> +            goto out;
> +        }
>  
> -    if (!visit_type_str(v, name, &str, NULL)) {
> +        if (sscanf(str, "%x.%x%n", &slot, &fn, &n) != 2) {
> +            fn = 0;
> +            if (sscanf(str, "%x%n", &slot, &n) != 1) {
> +                goto invalid;
> +            }
> +        }
> +        if (str[n] != '\0' || fn > 7 || slot > 31) {
> +            goto invalid;
> +        }
> +        *ptr = slot << 3 | fn;
> +        break;
> +
> +    case QTYPE_QNUM:
>          if (!visit_type_int32(v, name, &value, errp)) {
> -            return;
> +            goto out;
>          }
>          if (value < -1 || value > 255) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                         name ? name : "null", "a value between -1 and 255");
> -            return;
> +            goto out;
>          }
>          *ptr = value;
> -        return;
> -    }
> +        break;
>  
> -    if (sscanf(str, "%x.%x%n", &slot, &fn, &n) != 2) {
> -        fn = 0;
> -        if (sscanf(str, "%x%n", &slot, &n) != 1) {
> -            goto invalid;
> -        }
> -    }
> -    if (str[n] != '\0' || fn > 7 || slot > 31) {
> -        goto invalid;
> +    default:
> +        error_setg(errp, "Invalid parameter type for '%s', expected int or str",
> +                   name ? name : "null");
> +        goto out;

This goto is redundant.

>      }
> -    *ptr = slot << 3 | fn;
> -    g_free(str);
> -    return;
> +
> +    goto out;
>  
>  invalid:
>      error_set_from_qdev_prop_error(errp, EINVAL, obj, name, str);
> -    g_free(str);
> +out:
> +    visit_end_alternate(v, (void **) &alt);
>  }
>  
>  static int print_pci_devfn(Object *obj, Property *prop, char *dest,

Ugly control flow, but cleaning that up is not the patch's goal.
Instead, it aims to just fix the bug.  Fair.

Reviewed-by: Markus Armbruster <armbru@redhat.com>