Re: [PATCH 10/16] qdev: Improve netdev property override error a bit
Philippe Mathieu-Daudé writes:
> 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@.
Not sure it's worthwhile. It's just an error message, and nobody
complained so far.
>> 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
>> Signed-off-by: Markus Armbruster
>> ---
>> 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()?
Use of -global is one way to make it fail. Another is duplicated key in
-device (but not device_add). I believe no third way exists. The
function sets a specific error when it finds -global, else a vague
error.
check_prop_still_unset()?
>> + 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)) {
>>
Re: [PATCH 10/16] qdev: Improve netdev property override error a bit
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
> Signed-off-by: Markus Armbruster
> ---
> 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
[PATCH 10/16] qdev: Improve netdev property override error a bit
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'
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
Signed-off-by: Markus Armbruster
---
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,
+ 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)
+{
+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)) {
+return p;
+}
+}
+return NULL;
+}
+
int qdev_prop_check_globals(void)
{
int i, ret = 0;
--
2.26.2
