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

[Markus Armbruster]

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;
>>