| Message ID | 20200605145625.2920920-11-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Crazy shit around -global (pardon my french) | expand |
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; >
Philippe Mathieu-Daudé <philmd@redhat.com> 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 <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()? 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)) { >> 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 > ---------------- [...] In case of if/else, while/for, align the secondary lines just after the opening parenthesis of the first. > For example: > > .. code-block:: c > > if (a == 1 && > b == 2) { > > while (a == 1 && > b == 2) { This is about how much to indent, not where to break the line. Existing practice is inconsistent. > 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. For what it's worth, Python's PEP 8: Should a Line Break Before or After a Binary Operator? For decades the recommended style was to break after binary operators. But this can hurt readability in two ways: the operators tend to get scattered across different columns on the screen, and each operator is moved away from its operand and onto the previous line. Here, the eye has to do extra work to tell which items are added and which are subtracted: # Wrong: # operators sit far away from their operands income = (gross_wages + taxable_interest + (dividends - qualified_dividends) - ira_deduction - student_loan_interest) To solve this readability problem, mathematicians and their publishers follow the opposite convention. Donald Knuth explains the traditional rule in his Computers and Typesetting series: "Although formulas within a paragraph always break after binary operations and relations, displayed formulas always break before binary operations" [3]. Following the tradition from mathematics usually results in more readable code: # Correct: # easy to match operators with operands income = (gross_wages + taxable_interest + (dividends - qualified_dividends) - ira_deduction - student_loan_interest) In Python code, it is permissible to break before or after a binary operator, as long as the convention is consistent locally. For new code Knuth's style is suggested. https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator >> + return p; >> + } >> + } >> + return NULL; >> +} >> + >> int qdev_prop_check_globals(void) >> { >> int i, ret = 0; >>
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;
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 <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(-)