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 <jasow...@redhat.com> > Signed-off-by: Markus Armbruster <arm...@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; >