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

[Markus Armbruster]

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!