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

[Markus Armbruster <armbru@redhat.com>]

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