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

[Markus Armbruster <armbru@redhat.com>]

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
            ...
    
    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".

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.

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.

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?