Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf <kwolf@redhat.com>]

Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >
> >> On 11/03/21 15:08, Markus Armbruster wrote:
> >>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
> >>>> syntax that you have in qobject_input_visitor_new_str, and whenever
> >>>> you need to walk all -object arguments, use something like this:
> >>>>
> >>>>      typedef struct ObjectArgument {
> >>>>          const char *id;
> >>>>          QDict *json;    /* or NULL for QemuOpts */
> >>>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
> >>>>      }
> >>>>
> >>>> I already had patches in my queue to store -object in a GSList of
> >>>> dictionaries, changing it to use the above is easy enough.
> >>> 
> >>> I think I'd prefer following -display's precedence.  See my reply to
> >>> Kevin for details.
> >>
> >> Yeah, I got independently to the same conclusion and posted patches
> >> for that.  I was scared that visit_type_ObjectOptions was too much for 
> >> OptsVisitor but it seems to work...
> >
> > We have reason to be scared.  I'll try to cover this in my review.
> 
> The opts visitor has serious limitations.  From its header:
> 
>  * The Opts input visitor does not implement support for visiting QAPI
>  * alternates, numbers (other than integers), null, or arbitrary
>  * QTypes.  It also requires a non-null list argument to
>  * visit_start_list().
> 
> This is retro-documentation for hairy code.  I don't trust it.  Commit
> eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
> restrictions:
> 
>     The type tree in the schema, corresponding to an option with a
>     discriminator, must have the following structure:
>     
>       struct
>         scalar member for non-discriminated optarg 1 [*]
>         list for repeating non-discriminated optarg 2 [*]
>           wrapper struct
>             single scalar member
>         union
>           struct for discriminator case 1
>             scalar member for optarg 3 [*]
>             list for repeating optarg 4 [*]
>               wrapper struct
>                 single scalar member
>             scalar member for optarg 5 [*]
>           struct for discriminator case 2
>             ...

Is this a long-winded way of saying that it has to be flat, except that
it allows lists, i.e. there must be no nested objects on the "wire"?

The difference between structs and unions, and different branches inside
the union isn't visible for the visitor anyway.

>     The "type" optarg name is fixed for the discriminator role. Its schema
>     representation is "union of structures", and each discriminator value must
>     correspond to a member name in the union.
>     
>     If the option takes no "type" descriminator, then the type subtree rooted
>     at the union must be absent from the schema (including the union itself).
>     
>     Optarg values can be of scalar types str / bool / integers / size.
> 
> Unsupported visits are treated as programming error.  Which is a nice
> way to say "they crash".

The OptsVisitor never seems to crash explicitly by calling something
like abort().

It may crash because of missing callbacks that are called without a NULL
check, like v->type_null. This case should probably be fixed in
qapi/qapi-visit-core.c to do the check and simply return an error.

Any other cases?

> Before this series, we use it for -object as follows.
> 
> user_creatable_add_opts() massages the QemuOpts into a QDict containing
> just the properties, then calls user_creatable_add_type() with the opts
> visitor wrapped around the QemuOpts, and the QDict.
> 
> user_creatable_add_type() performs a virtual visit.  The outermost
> object it visits itself.  Then it visits members one by one by calling
> object_property_set().  It uses the QDict as a list of members to visit.
> 
> As long as the object_property_set() only visit scalars other than
> floating-point numbers, we safely stay with the opts visitors'
> limitations.

Minor addition: This visits inside object_property_set() are
non-virtual, of course.

> After this series, we use the opts visitor to convert the option
> argument to a ObjectOption.  This is a non-virtual visit.  We then
> convert the ObjectOption to a QDict, and call user_creatable_add_type()
> with the QObject input visitor wrapped around the QDict, and the QDict.
> 
> Here's the difference in opts visitor use: before the patch, we visit
> exactly the members in the optarg that actually name QOM properties (for
> the ones that don't, object_property_set() fails without visiting
> anything).  Afterwards, we visit the members of ObjectOption, i.e.
> all QOM properties, by construction of ObjectOption.
> 
> As long as ObjectOption's construction is correct, the series does not
> add new visits, i.e. we're no worse off than before.
> 
> However, there is now a new way to mess things up: you can change (a
> branch of union) ObjectOption in a way that pushes it beyond the opts
> visitors limitations.  QMP and tools --object will continue to work, but
> qemu-system-FOO -object will crash.

I don't think this is very concerning because the primary way to test
changes to objects is probably -object in the system emulator. So I
think we're lucky enough to have the problem in the most obvious place.

> As is, HMP object_add doesn't crash, because it doesn't use the opts
> visitor anymore, which breaks backward compatibility.  If we rever to
> the opts visitor there, it'll crash as well.
> 
> New ways to mess things up are always kind of unwelcome.  This one
> doesn't sound *too* dangerous; we "only" have to ensure -object is
> tested thoroughly.  Still, comments next to the QAPI definitions that
> must not be messed up would be nice.
> 
> Paolo, Kevin, any comments?

We probably agree that using QemuOpts and the OptsVisitor is only a
stopgap solution for 6.0 anyway. Instead of investing a lot of thought
into how we can make this maintainable for the long term (which isn't
something we want to do anyway), let's put that work into making the
keyval visitor work for the system emulator.

Kevin