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

[Kevin Wolf <kwolf@redhat.com>]

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