Am 15.03.2021 um 16:26 hat Markus Armbruster geschrieben:
Kevin Wolf <kwolf@redhat.com> writes:
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"?
I think so.
The difference between structs and unions, and different branches inside the union isn't visible for the visitor anyway.
Yes, only the code using the visitor deals with that.
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.
Correct.
This case should probably be fixed in qapi/qapi-visit-core.c to do the check and simply return an error.
I retro-documented what I inherited: qapi-visit-core.c code expects the visitors to implement the full visitor-impl.h interface, but some visitors don't. So I documented "method must be set to visit FOOs" in visitor-impl.h, and for the visitors that don't, I documented "can't visit FOOs".
If the crashing behavior we've always had gets in the way, there are two ways to change it:
1. Complicate qapi-visit-core.c slightly to cope with incomplete visitor implementations.
2. Complete the visitor implementations: add dummy callbacks that fail.
I prefer 2., because I feel it keeps the visitor-impl.h interface simpler, and puts the extra complications where they belong.
I suggested making the callbacks optional because I expected that there might be more than one visitor that doesn't support a callback and I wouldn't like duplicating dummy callbacks in multiple places. But if it's only the OptsVisitor, then we wouldn't get any duplication either way and it becomes a matter of taste. Kevin