Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 15.03.2021 um 16:26 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> >> Markus Armbruster writes:
> >>
> >> > Paolo Bonzini 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
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Kevin Wolf writes:
> Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
>> Markus Armbruster writes:
>>
>> > Paolo Bonzini 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.
> Any other cases?
I don't think so.
>> 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
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> Markus Armbruster writes:
>
> > Paolo Bonzini 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 limi
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Markus Armbruster writes:
> Paolo Bonzini 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?
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On Fri, Mar 12, 2021 at 09:46:54 +0100, Paolo Bonzini wrote:
> On 12/03/21 09:14, Markus Armbruster wrote:
> > Paolo Bonzini 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.
>
> Yes, it's a good reason to possibly even delay those 3 patches to 6.1.
Is there a chance we could get the json syntax for -object for now? I
think it would simplify libvirt's code a bit and sidestep the issue of
converting the already existing parameters from JSON form we have into
the commandline form.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On 12/03/21 09:14, Markus Armbruster wrote:
Paolo Bonzini 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.
Yes, it's a good reason to possibly even delay those 3 patches to 6.1.
Paolo
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Paolo Bonzini 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.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
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...
Paolo
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Paolo Bonzini writes:
> On 11/03/21 11:38, Markus Armbruster wrote:
>> Here's a differently terrible hack. We have
>> keyval_parse() visitor
>> optarg > QObject > QAPI type
>> Idea: hack the QObject. If we're working for -object, and QObject
>> maps
>> key "qom-type" to value "memory-backend-ram", get the value of
>> host-nodes, and if it's a string, parse it into a list like the opts
>> visitor does, and put that back, replacing the string value.
>> Same for other uses of Memdev and NumaNodeOptions with -object, if
>> they
>> exist.
>
> This doesn't help with backwards compatibility, since keyval loses the
> first of host-nodes=0,host-nodes=4.
You're right, I missed the fact that we rely on both hacks working
together for the full syntax.
> 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.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Kevin Wolf writes:
> Am 11.03.2021 um 12:24 hat Peter Krempa geschrieben:
>> On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote:
>> > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben:
>> > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote:
>> > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
>> > > > > On 10/03/21 15:22, Peter Krempa wrote:
>>
>> [...]
>>
>> > > -object
>> > > memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind
>> >
>> > Oh, we have ranges, too... That makes the compatibility code even
>> > nastier to write. I doubt that we can implement this in the keyval
>> > parser alone without touching the visitor. :-/
>> >
>> > > If any of the above is to be deprecated we'll need to adjust our
>> > > JSON->commandline generator accordignly.
>> > >
>> > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is
>> > > actually modular to allow plugging an array interpretor which actually
>> > > does the above conversion from a JSON array.
>> > >
>> > > So, what is the preferred syntax here? Additionally we might need a
>> > > witness property to detect when to use the new syntax if basing it on
>> > > the object-add qapification will not be enough.
>> >
>> > The list syntax supported by the keyval parser is the one you know from
>> > -blockdev: host-nodes.0=3,host-nodes.1=7, ...
>>
>> I think that should be easy enough to convert to.
>
> We could also support JSON syntax in QEMU. That would probably be even
> more convenient for libvirt?
Cleanly QAPIfied options like -blockdev do
if (optarg[0] == '{') {
parse @optarg as JSON with qobject_from_json()
convert to C type with qobject_input_visitor_new()
} else {
parse @optarg with keyval_parse()
convert to C type with qobject_input_visitor_new_keyval()
}
Options where compatibility problems defeat keyval_parse() can do
if (optarg[0] == '{') {
parse @optarg as JSON with qobject_from_json()
convert to C type with qobject_input_visitor_new()
} else {
parse the old way
convert to C type somehow
}
Precedence: -display. There, the old way is an ad hoc parser, and the
conversion to C type DisplayOptions is manual. For -object, the old way
would be QemuOpts, and the conversion would use the opts visitor.
>> Is it safe to do right away (with the old parser?). Otherwise we need
>> to agree on something which will let us detect when it's safe to
>> change.
>
> Neither keyval nor JSON syntax work with the old QemuOpts parser.
>
> What is the usual way to do this for command line options? If we don't
> have a good way there, we can always tie it to something in the QAPI
> schema. If we still get this solved in time for 6.0, we could use the
> existence of ObjectOptions. Or all else failing, we can use a feature
> flag.
You should not look for types in output of query-qmp-schema, only for
commands and events. To discourage looking for types, query-qmp-schema
masks the names of user-defined types.
A feature flag is fine as last resort. That's what they were designed
for.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On Thu, Mar 11, 2021 at 12:41:42 +0100, Kevin Wolf wrote: > Am 11.03.2021 um 12:24 hat Peter Krempa geschrieben: > > On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote: > > > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben: > > > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote: > > > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > > > > On 10/03/21 15:22, Peter Krempa wrote: > > > > [...] > > > > > > -object > > > > memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind > > > > > > Oh, we have ranges, too... That makes the compatibility code even > > > nastier to write. I doubt that we can implement this in the keyval > > > parser alone without touching the visitor. :-/ > > > > > > > If any of the above is to be deprecated we'll need to adjust our > > > > JSON->commandline generator accordignly. > > > > > > > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is > > > > actually modular to allow plugging an array interpretor which actually > > > > does the above conversion from a JSON array. > > > > > > > > So, what is the preferred syntax here? Additionally we might need a > > > > witness property to detect when to use the new syntax if basing it on > > > > the object-add qapification will not be enough. > > > > > > The list syntax supported by the keyval parser is the one you know from > > > -blockdev: host-nodes.0=3,host-nodes.1=7, ... > > > > I think that should be easy enough to convert to. > > We could also support JSON syntax in QEMU. That would probably be even > more convenient for libvirt? Definitely yes. We already do have the JSON internal representation, so outputing JSON directly just skips the commandlinificator. > > Is it safe to do right away (with the old parser?). Otherwise we need > > to agree on something which will let us detect when it's safe to > > change. > > Neither keyval nor JSON syntax work with the old QemuOpts parser. > > What is the usual way to do this for command line options? If we don't > have a good way there, we can always tie it to something in the QAPI > schema. If we still get this solved in time for 6.0, we could use the > existence of ObjectOptions. Or all else failing, we can use a feature > flag. Yup, in this release I'd use what I have for detecting qapification of -object. If we can do JSON with this, it would be ideal.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 11.03.2021 um 12:24 hat Peter Krempa geschrieben: > On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote: > > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben: > > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote: > > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > > > On 10/03/21 15:22, Peter Krempa wrote: > > [...] > > > > -object > > > memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind > > > > Oh, we have ranges, too... That makes the compatibility code even > > nastier to write. I doubt that we can implement this in the keyval > > parser alone without touching the visitor. :-/ > > > > > If any of the above is to be deprecated we'll need to adjust our > > > JSON->commandline generator accordignly. > > > > > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is > > > actually modular to allow plugging an array interpretor which actually > > > does the above conversion from a JSON array. > > > > > > So, what is the preferred syntax here? Additionally we might need a > > > witness property to detect when to use the new syntax if basing it on > > > the object-add qapification will not be enough. > > > > The list syntax supported by the keyval parser is the one you know from > > -blockdev: host-nodes.0=3,host-nodes.1=7, ... > > I think that should be easy enough to convert to. We could also support JSON syntax in QEMU. That would probably be even more convenient for libvirt? > Is it safe to do right away (with the old parser?). Otherwise we need > to agree on something which will let us detect when it's safe to > change. Neither keyval nor JSON syntax work with the old QemuOpts parser. What is the usual way to do this for command line options? If we don't have a good way there, we can always tie it to something in the QAPI schema. If we still get this solved in time for 6.0, we could use the existence of ObjectOptions. Or all else failing, we can use a feature flag. Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote: > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben: > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote: > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > > On 10/03/21 15:22, Peter Krempa wrote: [...] > > -object > > memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind > > Oh, we have ranges, too... That makes the compatibility code even > nastier to write. I doubt that we can implement this in the keyval > parser alone without touching the visitor. :-/ > > > If any of the above is to be deprecated we'll need to adjust our > > JSON->commandline generator accordignly. > > > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is > > actually modular to allow plugging an array interpretor which actually > > does the above conversion from a JSON array. > > > > So, what is the preferred syntax here? Additionally we might need a > > witness property to detect when to use the new syntax if basing it on > > the object-add qapification will not be enough. > > The list syntax supported by the keyval parser is the one you know from > -blockdev: host-nodes.0=3,host-nodes.1=7, ... I think that should be easy enough to convert to. Is it safe to do right away (with the old parser?). Otherwise we need to agree on something which will let us detect when it's safe to change.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On 11/03/21 11:38, Markus Armbruster wrote:
Here's a differently terrible hack. We have
keyval_parse() visitor
optarg > QObject > QAPI type
Idea: hack the QObject. If we're working for -object, and QObject maps
key "qom-type" to value "memory-backend-ram", get the value of
host-nodes, and if it's a string, parse it into a list like the opts
visitor does, and put that back, replacing the string value.
Same for other uses of Memdev and NumaNodeOptions with -object, if they
exist.
This doesn't help with backwards compatibility, since keyval loses the
first of host-nodes=0,host-nodes=4.
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.
Paolo
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Kevin Wolf writes:
> Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
>> On 10/03/21 15:22, Peter Krempa wrote:
>> > I've stumbled upon a regression with this patchset applied:
>> >
>> > error: internal error: process exited while connecting to monitor:
>> > qemu-system-x86_64: -object
>> > memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind:
>> > Invalid parameter type for 'host-nodes', expected: array
>>
>> This is the magic conversion of "string containing a list of integers"
>> to "list of integers".
>
> Ah, crap. This one wouldn't have been a problem when converting only
> object-add, and I trusted your audit that user creatable objects don't
> depend on any QemuOpts magic. I should have noticed it, too, of course,
> but during the convertion I didn't have QemuOpts in mind, only QOM and
> QAPI.
>
> I checked all object types, and it seems this is the only one that is
> affected. We have a second list in AuthZListProperties, but it contains
> structs, so it was never supported on the command line anyway.
>
>> The relevant code is in qapi/string-input-visitor.c, but I'm not sure where
>> to stick it in the keyval-based parsing flow (i.e.
>> qobject_input_visitor_new_keyval). Markus, any ideas?
>
> The best I can think of at the moment would be adding a flag to the
> keyval parser that would enable the feature only for -object (and only
> in the system emulator, because memory-backend-ram doesn't exist in the
> tools):
>
> The keyval parser would create a list if multiple values are given for
> the same key. Some care needs to be taken to avoid mixing the magic
> list feature with the normal indexed list options.
You're cursing^Wtalking about the wrong list hack, I'm afraid.
QemuOpts can indeed be used in a way so that key=val1,key=val2,... get
collected into a list val1, val2, ... For an example, see how
qemu_semihosting_config_opts() processes the option argument of
-semihosting: repeated arg=... get collected in array
semihosting.argv[].
QOM property "host-nodes" employs a different hack. The QAPI schema
defines it as
{ 'struct': 'Memdev',
'data': {
...
'host-nodes': ['uint16'],
... }}
The QObject input visitor treats it like any other list. To get node 0
and 4, you say
"host-nodes": [0,4]
with its JSON flavor, and
host-nodes.0=0,host-nodes.1=4
with its dotted keys flavor.
The string input visitor and the opts visitor only support *scalar*
values, *except* they both have a hack to support lists of small
integers.
With the opts visitor, saying
host-nodes=0,host-nodes=4
gets you node 0 and 4, and both
host-nodes=0,host-nodes=1
host-nodes=0-1
gets you nodes 0 and 1. This is what parses -object.
Setting NumaNode member @cpus via -numa cpus=... is another user of this
hack.
With the string input visitor, repeating the key doesn't work (there is
no syntax for keys, in fact), but comma does. This is what parses
-global and HMP qom-set.
The problem Peter reported is that switching -object from QemuOpts +
opts visitor to keyval_parse() + QObject input visitor loses the opts
visitor's list-of-integers hack.
The obvious solution is to transplant the hack to the QObject input
visitor. "Ich kann nicht soviel fressen wie ich kotzen möchte" (okay, I
better don't translate this; all you need to know is that I find the
idea utterly disgusting).
There is the more general problem of human-friendly syntax support.
QAPI/QMP eschew encoding complex data in strings. They want you to use
complex data types.
Fine for QMP, machines are generally better off with formatting /
parsing verbose JSON than with formatting / parsing lots of ad hoc
little languages.
Not always fine for humans. Case in point:
host-nodes.0=0,host-nodes.1=4 is decidedly inferior to something like
host-nodes=0+4[*].
Perhaps we need to provide means to define ad hoc little languages for
human use. Specify the parsing function to use for human input in the
QAPI schema.
Doesn't really help us now, because we can't pull it off in time for the
soft freeze.
Here's a differently terrible hack. We have
keyval_parse() visitor
optarg > QObject > QAPI type
Idea: hack the QObject. If we're working for -object, and QObject maps
key "qom-type" to value "memory-backend-ram", get the value of
host-nodes, and if it's a string, parse it into a list like the opts
visitor does, and put that back, replacing the string value.
Same for other uses of Memdev and NumaNodeOptions with -object, if they
exist.
I told you it's terrible :)
[...]
[*] 0+4 and not 0,4 because ',' would have to be doubled in dotted key
syntax.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On 11/03/21 09:45, Kevin Wolf wrote: I think it's only patch 29 and 30 that we would have to drop, actually. Unfortunately, that still removes one of the most immediately useful features, which is non-scalar properties for -object in the system emulator. But of course, a lot better than not merging it at all. Who is going to include this series in the next pull request, Markus or myself? The time is ticking for soft freeze. QOM is probably the right subsystem, so that would be you. Or I can just merge it myself as long as everyone is fine with it. Eric has some minor comments that I think could be addressed while applying the series. Or should I send a v4 for that (and for dropping patches 29 and 30)? I'd say just send a pull request. :) Paolo
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 11.03.2021 um 09:14 hat Paolo Bonzini geschrieben: > On 10/03/21 18:30, Kevin Wolf wrote: > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > On 10/03/21 15:22, Peter Krempa wrote: > > > > I've stumbled upon a regression with this patchset applied: > > > > > > > > error: internal error: process exited while connecting to monitor: > > > > qemu-system-x86_64: -object > > > > memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: > > > > Invalid parameter type for 'host-nodes', expected: array > > > > > > This is the magic conversion of "string containing a list of integers" > > > to "list of integers". > > > > Ah, crap. This one wouldn't have been a problem when converting only > > object-add, and I trusted your audit that user creatable objects don't > > depend on any QemuOpts magic. I should have noticed it, too, of course, > > but during the convertion I didn't have QemuOpts in mind, only QOM and > > QAPI. > > Yeah, let's just drop the -object conversion for now. It will just remove a > few patches. I think it's only patch 29 and 30 that we would have to drop, actually. Unfortunately, that still removes one of the most immediately useful features, which is non-scalar properties for -object in the system emulator. But of course, a lot better than not merging it at all. > Who is going to include this series in the next pull request, Markus or > myself? The time is ticking for soft freeze. QOM is probably the right subsystem, so that would be you. Or I can just merge it myself as long as everyone is fine with it. Eric has some minor comments that I think could be addressed while applying the series. Or should I send a v4 for that (and for dropping patches 29 and 30)? Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben: > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote: > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > > On 10/03/21 15:22, Peter Krempa wrote: > > [...] > > > The keyval parser would create a list if multiple values are given for > > the same key. Some care needs to be taken to avoid mixing the magic > > list feature with the normal indexed list options. > > > > The QAPI schema would then use an alternate between 'int' and ['int'], > > with the the memory-backend-ram implementation changed accordingly. > > > > We could consider immediately deprecating the syntax and printing a > > warning in the keyval parser when it automatically creates a list from > > two values for a key, so that users don't start using this syntax > > By 'creating a list from two values for a key' you mean: > > host-nodes=0,host-nodes=1 > > to be converted into [0, 1] ? > > > instead of the normal list syntax in other places. We'd probably still > > leave the implementation around for -device and other users of the same > > magic. > > There's three options actually that libvirt uses, visible in one our > test files [1] > > For a single value we format: > > -object > memory-backend-ram,id=ram-node0,size=20971520,host-nodes=3,policy=preferred > > For a contiguous list: > > -object > memory-backend-ram,id=ram-node1,size=676331520,host-nodes=0-7,policy=bind > > And for an interleaved list: > > -object > memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind Oh, we have ranges, too... That makes the compatibility code even nastier to write. I doubt that we can implement this in the keyval parser alone without touching the visitor. :-/ > If any of the above is to be deprecated we'll need to adjust our > JSON->commandline generator accordignly. > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is > actually modular to allow plugging an array interpretor which actually > does the above conversion from a JSON array. > > So, what is the preferred syntax here? Additionally we might need a > witness property to detect when to use the new syntax if basing it on > the object-add qapification will not be enough. The list syntax supported by the keyval parser is the one you know from -blockdev: host-nodes.0=3,host-nodes.1=7, ... Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On 11/03/21 08:47, Peter Krempa wrote: And for an interleaved list: -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind Uhm, I doubt this works? I would have expected "host-nodes=1-2,,5,,7" instead. Paolo
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On 10/03/21 18:30, Kevin Wolf wrote: Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: On 10/03/21 15:22, Peter Krempa wrote: I've stumbled upon a regression with this patchset applied: error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array This is the magic conversion of "string containing a list of integers" to "list of integers". Ah, crap. This one wouldn't have been a problem when converting only object-add, and I trusted your audit that user creatable objects don't depend on any QemuOpts magic. I should have noticed it, too, of course, but during the convertion I didn't have QemuOpts in mind, only QOM and QAPI. Yeah, let's just drop the -object conversion for now. It will just remove a few patches. Who is going to include this series in the next pull request, Markus or myself? The time is ticking for soft freeze. Paolo I checked all object types, and it seems this is the only one that is affected. We have a second list in AuthZListProperties, but it contains structs, so it was never supported on the command line anyway. The relevant code is in qapi/string-input-visitor.c, but I'm not sure where to stick it in the keyval-based parsing flow (i.e. qobject_input_visitor_new_keyval). Markus, any ideas? The best I can think of at the moment would be adding a flag to the keyval parser that would enable the feature only for -object (and only in the system emulator, because memory-backend-ram doesn't exist in the tools): The keyval parser would create a list if multiple values are given for the same key. Some care needs to be taken to avoid mixing the magic list feature with the normal indexed list options. The QAPI schema would then use an alternate between 'int' and ['int'], with the the memory-backend-ram implementation changed accordingly. We could consider immediately deprecating the syntax and printing a warning in the keyval parser when it automatically creates a list from two values for a key, so that users don't start using this syntax instead of the normal list syntax in other places. We'd probably still leave the implementation around for -device and other users of the same magic. Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote: > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > > On 10/03/21 15:22, Peter Krempa wrote: [...] > The keyval parser would create a list if multiple values are given for > the same key. Some care needs to be taken to avoid mixing the magic > list feature with the normal indexed list options. > > The QAPI schema would then use an alternate between 'int' and ['int'], > with the the memory-backend-ram implementation changed accordingly. > > We could consider immediately deprecating the syntax and printing a > warning in the keyval parser when it automatically creates a list from > two values for a key, so that users don't start using this syntax By 'creating a list from two values for a key' you mean: host-nodes=0,host-nodes=1 to be converted into [0, 1] ? > instead of the normal list syntax in other places. We'd probably still > leave the implementation around for -device and other users of the same > magic. There's three options actually that libvirt uses, visible in one our test files [1] For a single value we format: -object memory-backend-ram,id=ram-node0,size=20971520,host-nodes=3,policy=preferred For a contiguous list: -object memory-backend-ram,id=ram-node1,size=676331520,host-nodes=0-7,policy=bind And for an interleaved list: -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind If any of the above is to be deprecated we'll need to adjust our JSON->commandline generator accordignly. Luckily the 'host-nodes' is storeable as a bitmap and the generator is actually modular to allow plugging an array interpretor which actually does the above conversion from a JSON array. So, what is the preferred syntax here? Additionally we might need a witness property to detect when to use the new syntax if basing it on the object-add qapification will not be enough. [1] https://gitlab.com/libvirt/libvirt/-/blob/master/tests/qemuxml2argvdata/numatune-memnode.args
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben: > On 10/03/21 15:22, Peter Krempa wrote: > > I've stumbled upon a regression with this patchset applied: > > > > error: internal error: process exited while connecting to monitor: > > qemu-system-x86_64: -object > > memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: > > Invalid parameter type for 'host-nodes', expected: array > > This is the magic conversion of "string containing a list of integers" > to "list of integers". Ah, crap. This one wouldn't have been a problem when converting only object-add, and I trusted your audit that user creatable objects don't depend on any QemuOpts magic. I should have noticed it, too, of course, but during the convertion I didn't have QemuOpts in mind, only QOM and QAPI. I checked all object types, and it seems this is the only one that is affected. We have a second list in AuthZListProperties, but it contains structs, so it was never supported on the command line anyway. > The relevant code is in qapi/string-input-visitor.c, but I'm not sure where > to stick it in the keyval-based parsing flow (i.e. > qobject_input_visitor_new_keyval). Markus, any ideas? The best I can think of at the moment would be adding a flag to the keyval parser that would enable the feature only for -object (and only in the system emulator, because memory-backend-ram doesn't exist in the tools): The keyval parser would create a list if multiple values are given for the same key. Some care needs to be taken to avoid mixing the magic list feature with the normal indexed list options. The QAPI schema would then use an alternate between 'int' and ['int'], with the the memory-backend-ram implementation changed accordingly. We could consider immediately deprecating the syntax and printing a warning in the keyval parser when it automatically creates a list from two values for a key, so that users don't start using this syntax instead of the normal list syntax in other places. We'd probably still leave the implementation around for -device and other users of the same magic. Kevin
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On Wed, Mar 10, 2021 at 15:31:57 +0100, Paolo Bonzini wrote: > On 10/03/21 15:22, Peter Krempa wrote: > > I've stumbled upon a regression with this patchset applied: > > > > error: internal error: process exited while connecting to monitor: > > qemu-system-x86_64: -object > > memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: > > Invalid parameter type for 'host-nodes', expected: array > > This is the magic conversion of "string containing a list of integers" to > "list of integers". > > The relevant code is in qapi/string-input-visitor.c, but I'm not sure where > to stick it in the keyval-based parsing flow (i.e. > qobject_input_visitor_new_keyval). Markus, any ideas? I've got a slightly off-topic question/idea. Would it be reasonably easy/straightforward to run qemu's init code which parses arguments and possibly validates them but quit before actually starting to initiate resources? The use case would be to plug it (optionally?) into libvirt's qemuxml2argvtest to prevent such a thing from happening. It's not feasible to run all the configurations we have with a real VM but a partial validation would possibly make sense if it's easy enough.
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On 10/03/21 15:22, Peter Krempa wrote: I've stumbled upon a regression with this patchset applied: error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array This is the magic conversion of "string containing a list of integers" to "list of integers". The relevant code is in qapi/string-input-visitor.c, but I'm not sure where to stick it in the keyval-based parsing flow (i.e. qobject_input_visitor_new_keyval). Markus, any ideas?
Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
On Mon, Mar 08, 2021 at 17:54:10 +0100, Kevin Wolf wrote: > This series adds a QAPI type for the properties of all user creatable > QOM types and finally makes the --object command line option (in all > binaries) and the object-add monitor commands (in QMP and HMP) use the > new ObjectOptions union. > > This change improves things in more than just one way: > > 1. Documentation for QOM object types has always been lacking. Adding >the schema, we get documentation for every property. > > 2. It prevents bugs by performing parts of the input validation (e.g. >checking presence of mandatory properties) already in QAPI instead of >relying on separate manual implementations in each class. > > 3. It provides QAPI introspection for user creatable objects. > > 4. Non-scalar properties are now supported everywhere because the >command line parsers (including HMP) use the keyval parser now. > > > If you are in the CC list and didn't expect this series, it's probably > because you're the maintainer of one of the objects for which I'm adding > a QAPI schema description. Please just have a look at the specific patch > for your object and check whether the schema and its documentation make > sense to you. You can ignore all other patches. > > > In a next step after this series, we can add make use of the QAPI > structs in the implementation of the object and separate their > configuration from the runtime state. Specifically, the plan is to > add a .configure() callback to ObjectClass that allows configuring the > object in one place at creation time and keeping QOM property setters > only for properties that can actually be changed at runtime. Paolo made > an example of what the state could look like after this: > > https://wiki.qemu.org/Features/QOM-QAPI_integration > > Finally, the intention is to extend the QAPI schema to have separate > 'object' entities and generate some of the code that was written > manually in the intermediate state before. > > > This series is available as a git tag at: > > https://repo.or.cz/qemu/kevin.git qapi-object-v3 > > > v3: > - Removed now useless QAuthZListRuleListHack > - Made some more ObjectOptions branches conditional > - Improved documentation for some properties > - Fixed 'qemu-img compare' exit code for option parsing failure > > v2: > - Convert not only object-add, but all external interfaces so that the > schema will always be enforced and mismatch between implementation and > schema can't go unnoticed. > - Rebased, covering properties and object types added since v1 (yes, > things do become outdated rather quickly when you touch all user > creatable objects) > - Changed the "Since:" version number in the schema documentation to > refer to the version when the object was introduced rather than 6.0 > where the schema will (hopefully) be added > - Probably some other minor changes I've stumbled upon a regression with this patchset applied: error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array Full commandline is: LC_ALL=C \ PATH=/root/.local/bin:/root/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin \ HOME=/var/lib/libvirt/qemu/domain-15-cd \ USER=root \ LOGNAME=root \ XDG_DATA_HOME=/var/lib/libvirt/qemu/domain-15-cd/.local/share \ XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain-15-cd/.cache \ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain-15-cd/.config \ /home/pipo/git/qemu.git/build/x86_64-softmmu/qemu-system-x86_64 \ -name guest=cd,debug-threads=on \ -S \ -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-15-cd/master-key.aes \ -machine pc-i440fx-2.9,accel=kvm,usb=off,vmport=off,dump-guest-core=off,memory-backend=pc.ram \ -cpu EPYC-Rome,x2apic=on,tsc-deadline=on,hypervisor=on,tsc-adjust=on,stibp=on,arch-capabilities=on,ssbd=on,xsaves=on,cmp-legacy=on,amd-ssbd=on,virt-ssbd=on,rdctl-no=on,skip-l1dfl-vmentry=on,mds-no=on,pschange-mc-no=on \ -m 1000 \ -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind \ -overcommit mem-lock=off \ -smp 2,maxcpus=8,sockets=8,cores=1,threads=1 \ -uuid 8e70100a-64b4-4186-aff9-e055c3075cb0 \ -no-user-config \ -nodefaults \ -device sga \ -chardev socket,id=charmonitor,fd=42,server=on,wait=off \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc,driftfix=slew \ -global kvm-pit.lost_tick_policy=delay \ -no-hpet \ -no-shutdown \ -global PIIX4_PM.disable_s3=1 \ -global PIIX4_PM.disable_s4=1 \ -boot menu=on,strict=on \ -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 \ -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 \ -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 \ -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 \ -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x9 \
