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

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/19/24 13:03, Kevin Wolf wrote:
> 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.

Indeed, set_pci_devfn() predates alternates.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> 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;
>       }
> -    *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,