Re: [PATCH 10/16] qdev: Improve netdev property override error a bit

[Philippe Mathieu-Daudé]

On 6/5/20 4:56 PM, Markus Armbruster wrote:
> qdev_prop_set_netdev() fails when the property already has a non-null
> value.  Seems to go back to commit 30c367ed44
> "qdev-properties-system.c: Allow vlan or netdev for -device, not
> both", v1.7.0.  Board code doesn't expect failure, and crashes:
> 
>     $ qemu-system-x86_64 --nodefaults -nic user -netdev user,id=nic0 -global 
> e1000.netdev=nic0
>     Unexpected error in error_set_from_qdev_prop_error() at 
> /work/armbru/qemu/hw/core/qdev-properties.c:1101:
>     qemu-system-x86_64: Property 'e1000.netdev' doesn't take value 
> '__org.qemu.nic0
>     '
>     Aborted (core dumped)
> 
> -device and device_add handle the failure:
> 
>     $ qemu-system-x86_64 -nodefaults -netdev user,id=net0 -netdev 
> user,id=net1 -device e1000,netdev=net0,netdev=net1
>     qemu-system-x86_64: -device e1000,netdev=net0,netdev=net1: Property 
> 'e1000.netdev' doesn't take value 'net1'
>     $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -netdev 
> user,id=net0 -netdev user,id=net1 -global e1000.netdev=net0
>     QEMU 5.0.50 monitor - type 'help' for more information
>     (qemu) qemu-system-x86_64: warning: netdev net0 has no peer
>     qemu-system-x86_64: warning: netdev net1 has no peer
>     device_add e1000,netdev=net1
>     Error: Property 'e1000.netdev' doesn't take value 'net1'

If patch accepted as it, might be worth Cc'ing qemu-stable@.

> 
> Perhaps netdev property override could be made to work.  Perhaps it
> should.  I'm not the right guy to figure this out.  What I can do is
> improve the error message a bit:
> 
>     (qemu) device_add e1000,netdev=net1
>     Error: -global e1000.netdev=... conflicts with netdev=net1
> 
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/hw/qdev-properties.h     |  2 ++
>  hw/core/qdev-properties-system.c | 30 +++++++++++++++++++++++++++---
>  hw/core/qdev-properties.c        | 17 +++++++++++++++++
>  3 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index f161604fb6..566419f379 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -248,6 +248,8 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char 
> *name,
>  void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
>  
>  void qdev_prop_register_global(GlobalProperty *prop);
> +const GlobalProperty *qdev_find_global_prop(DeviceState *dev,
> +                                            const char *name);
>  int qdev_prop_check_globals(void);
>  void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index 9aa80495ee..20fd58e8f9 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -25,6 +25,28 @@
>  #include "sysemu/iothread.h"
>  #include "sysemu/tpm_backend.h"
>  
> +static bool check_non_null(DeviceState *dev, const char *name,

I see this is a static qdev function, but can we have a name that
better match the content? Maybe qdev_global_prop_exists()?

> +                           const void *old_val, const char *new_val,
> +                           Error **errp)
> +{
> +    const GlobalProperty *prop = qdev_find_global_prop(dev, name);
> +
> +    if (!old_val) {
> +        return true;
> +    }
> +
> +    if (prop) {
> +        error_setg(errp, "-global %s.%s=... conflicts with %s=%s",
> +                   prop->driver, prop->property, name, new_val);
> +    } else {
> +        /* Error message is vague, but a better one would be hard */
> +        error_setg(errp, "%s=%s conflicts, and override is not implemented",
> +                   name, new_val);
> +    }
> +    return false;
> +}
> +
> +
>  /* --- drive --- */
>  
>  static void get_drive(Object *obj, Visitor *v, const char *name, void 
> *opaque,
> @@ -299,14 +321,16 @@ static void set_netdev(Object *obj, Visitor *v, const 
> char *name,
>      }
>  
>      for (i = 0; i < queues; i++) {
> -
>          if (peers[i]->peer) {
>              err = -EEXIST;
>              goto out;
>          }
>  
> -        if (ncs[i]) {
> -            err = -EINVAL;
> +        /*
> +         * TODO Should this really be an error?  If no, the old value
> +         * needs to be released before we store the new one.
> +         */
> +        if (!check_non_null(dev, name, ncs[i], str, errp)) {
>              goto out;
>          }
>  
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index cc924815da..8c8beb56b2 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1181,6 +1181,23 @@ void qdev_prop_register_global(GlobalProperty *prop)
>      g_ptr_array_add(global_props(), prop);
>  }
>  
> +const GlobalProperty *qdev_find_global_prop(DeviceState *dev,
> +                                            const char *name)

Do you mind splitting this patch in 2? Adding qdev_find_global_prop
first, then using it to avoid the crash.

> +{
> +    GPtrArray *props = global_props();
> +    const GlobalProperty *p;
> +    int i;
> +
> +    for (i = 0; i < props->len; i++) {
> +        p = g_ptr_array_index(props, i);
> +        if (object_dynamic_cast(OBJECT(dev), p->driver)
> +            && !strcmp(p->property, name)) {

Laszlo pointed out elsewhere this doesn't match our CODING_STYLE.rst:

Multiline Indent
----------------

For example:

.. code-block:: c

    if (a == 1 &&
        b == 2) {

    while (a == 1 &&
           b == 2) {

I prefer the style you used, it looks safer. Eric once explained why
it is safer but I can't find his rationals anymore. I'll keep
searching to suggest a CODING_STYLE update.

> +            return p;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  int qdev_prop_check_globals(void)
>  {
>      int i, ret = 0;
>