[v2,11/12] qdev: Make netdev properties work as list elements

The 'name' parameter of QOM setters is primarily used to specify the name
of the currently parsed input element in the visitor interface. For
top-level qdev properties, this is always set and matches 'prop->name'.

However, for list elements it is NULL, because each element of a list
doesn't have a separate name. Passing a non-NULL value runs into
assertion failures in the visitor code.

Therefore, using 'name' in error messages is not right for property
types that are used in lists, because "(null)" isn't very helpful to
identify what QEMU is complaining about.

Change netdev properties to use 'prop->name' instead, which will contain
the name of the array property after switching array properties to lists
in the external interface.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/core/qdev-properties-system.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Kevin Wolf <kwolf@redhat.com> writes:

> The 'name' parameter of QOM setters is primarily used to specify the name
> of the currently parsed input element in the visitor interface. For
> top-level qdev properties, this is always set and matches 'prop->name'.
>
> However, for list elements it is NULL, because each element of a list
> doesn't have a separate name. Passing a non-NULL value runs into
> assertion failures in the visitor code.

Yes.  visitor.h's big comment:

 * The @name parameter of visit_type_FOO() describes the relation
 * between this QAPI value and its parent container.  When visiting
 * the root of a tree, @name is ignored; when visiting a member of an
 * object, @name is the key associated with the value; when visiting a
 * member of a list, @name is NULL; and when visiting the member of an
 * alternate, @name should equal the name used for visiting the
 * alternate.

> Therefore, using 'name' in error messages is not right for property
> types that are used in lists, because "(null)" isn't very helpful to
> identify what QEMU is complaining about.

We get "(null)" on some hosts, and SEGV on others.

Same problem in all property setters and getters (qdev and QOM), I
presume.  The @name parameter looks like a death trap.  Thoughts?

Any reproducer known before the next patch?

> Change netdev properties to use 'prop->name' instead, which will contain
> the name of the array property after switching array properties to lists
> in the external interface.

Points at the entire array property without telling the user which of
the elements is at fault.  Sure better than NULL.  I'm not asking you to
improve the error message further.  Mention the error message's lack of
precision in the commit message?

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 7c6dfab128..bf243d72d6 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -450,7 +450,7 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
>      peers_ptr->queues = queues;
>  
>  out:
> -    error_set_from_qdev_prop_error(errp, err, obj, name, str);
> +    error_set_from_qdev_prop_error(errp, err, obj, prop->name, str);
>      g_free(str);
>  }

Am 02.11.2023 um 13:55 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > The 'name' parameter of QOM setters is primarily used to specify the name
> > of the currently parsed input element in the visitor interface. For
> > top-level qdev properties, this is always set and matches 'prop->name'.
> >
> > However, for list elements it is NULL, because each element of a list
> > doesn't have a separate name. Passing a non-NULL value runs into
> > assertion failures in the visitor code.
> 
> Yes.  visitor.h's big comment:
> 
>  * The @name parameter of visit_type_FOO() describes the relation
>  * between this QAPI value and its parent container.  When visiting
>  * the root of a tree, @name is ignored; when visiting a member of an
>  * object, @name is the key associated with the value; when visiting a
>  * member of a list, @name is NULL; and when visiting the member of an
>  * alternate, @name should equal the name used for visiting the
>  * alternate.
> 
> > Therefore, using 'name' in error messages is not right for property
> > types that are used in lists, because "(null)" isn't very helpful to
> > identify what QEMU is complaining about.
> 
> We get "(null)" on some hosts, and SEGV on others.

Fair, I can add "(or even a segfault)" to the commit message.

> Same problem in all property setters and getters (qdev and QOM), I
> presume.  The @name parameter looks like a death trap.  Thoughts?

All property setters and getters that abuse @name instead of just
passing it to the visitor. I don't know how many do, but let's see:

* qdev_prop_size32 uses @name in an error message

* Array properties themselves use @name in an error message, too
  (preexisting)

* qdev-properties-system.c has more complicated properties, which have
  the same problem: drive, chardev, macaddr, netdev (before this patch),
  reserved_region, pci_devfn (one error path explicitly considers name
  == NULL, but another doesn't), uuid

* Outside of hw/core/ we may have some problematic properties, too.
  I found qdev_prop_tpm, and am now too lazy to go through all devices.
  These are probably specialised enough that they are less likely to be
  used in arrays.

We can fix these on top of this series, though we should probably first
figure out what the best way to replace it is to avoid touching
everything twice.

> Any reproducer known before the next patch?

Not to me.

> > Change netdev properties to use 'prop->name' instead, which will contain
> > the name of the array property after switching array properties to lists
> > in the external interface.
> 
> Points at the entire array property without telling the user which of
> the elements is at fault.  Sure better than NULL.  I'm not asking you to
> improve the error message further.  Mention the error message's lack of
> precision in the commit message?

Can do.

At least the input visitor has better ways internally to identify the
currently visited object with full_name(). If other visitor types can do
similar things, maybe extending the public visitor interface to access
the name would be possible?

Kevin

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.11.2023 um 13:55 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > The 'name' parameter of QOM setters is primarily used to specify the name
>> > of the currently parsed input element in the visitor interface. For
>> > top-level qdev properties, this is always set and matches 'prop->name'.
>> >
>> > However, for list elements it is NULL, because each element of a list
>> > doesn't have a separate name. Passing a non-NULL value runs into
>> > assertion failures in the visitor code.
>> 
>> Yes.  visitor.h's big comment:
>> 
>>  * The @name parameter of visit_type_FOO() describes the relation
>>  * between this QAPI value and its parent container.  When visiting
>>  * the root of a tree, @name is ignored; when visiting a member of an
>>  * object, @name is the key associated with the value; when visiting a
>>  * member of a list, @name is NULL; and when visiting the member of an
>>  * alternate, @name should equal the name used for visiting the
>>  * alternate.
>> 
>> > Therefore, using 'name' in error messages is not right for property
>> > types that are used in lists, because "(null)" isn't very helpful to
>> > identify what QEMU is complaining about.
>> 
>> We get "(null)" on some hosts, and SEGV on others.
>
> Fair, I can add "(or even a segfault)" to the commit message.
>
>> Same problem in all property setters and getters (qdev and QOM), I
>> presume.  The @name parameter looks like a death trap.  Thoughts?
>
> All property setters and getters that abuse @name instead of just
> passing it to the visitor. I don't know how many do, but let's see:
>
> * qdev_prop_size32 uses @name in an error message
>
> * Array properties themselves use @name in an error message, too
>   (preexisting)
>
> * qdev-properties-system.c has more complicated properties, which have
>   the same problem: drive, chardev, macaddr, netdev (before this patch),
>   reserved_region, pci_devfn (one error path explicitly considers name
>   == NULL, but another doesn't), uuid
>
> * Outside of hw/core/ we may have some problematic properties, too.
>   I found qdev_prop_tpm, and am now too lazy to go through all devices.
>   These are probably specialised enough that they are less likely to be
>   used in arrays.
>
> We can fix these on top of this series, though we should probably first
> figure out what the best way to replace it is to avoid touching
> everything twice.

Makes sense.

>> Any reproducer known before the next patch?
>
> Not to me.
>
>> > Change netdev properties to use 'prop->name' instead, which will contain
>> > the name of the array property after switching array properties to lists
>> > in the external interface.
>> 
>> Points at the entire array property without telling the user which of
>> the elements is at fault.  Sure better than NULL.  I'm not asking you to
>> improve the error message further.  Mention the error message's lack of
>> precision in the commit message?
>
> Can do.
>
> At least the input visitor has better ways internally to identify the
> currently visited object with full_name(). If other visitor types can do
> similar things, maybe extending the public visitor interface to access
> the name would be possible?

Yes, there's a need for better error reporting around visitors, ideally
without doing it in every visitor like the QObject input visitor does.
I've thought about it, but only briefly, and without a conclusion.

I figure your idea is to provide a visitor method to get a malloced
string describing the thing being visited.  Might work, but to know for
sure, we have to try.

This could also help with getting rid of the dangerous @name use we
discussed above.

Thanks!