| Message ID | 1502467522-32237-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/11/2017 11:05 AM, Markus Armbruster wrote: > We've wanted -object to support non-scalar properties for a while. > Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based > authorization API". Review led to the conclusion that we need to > replace rather than add to QemuOpts. Initial work towards that goal > has been merged to provide -blockdev (commit 8746709), but there's > substantial work left, mostly due to an bewildering array of > compatibility problems. > > Even if a full solution is still out of reach, we can have a partial > solution now: accept -object argument in JSON syntax. This should > unblock development work that needs non-scalar properties with > -object. > > The implementation is similar to -blockdev, except we use the new > infrastructure only for the new JSON case, and stick to QemuOpts for > the existing KEY=VALUE,... case, to sidestep compatibility problems. > > If we did this for more options, we'd have to factor out common code. > But for one option, this will do. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > qapi-schema.json | 14 +++++++++++--- > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 62 insertions(+), 3 deletions(-) > > static void object_create(bool (*type_predicate)(const char *)) > { > + ObjectOptionsQueueEntry *e, *next; > + > + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { > + if (!type_predicate(e->oo->qom_type)) { > + continue; > + } > + > + loc_push_restore(&e->loc); > + qmp_object_add(e->oo->qom_type, e->oo->id, > + e->oo->has_props, e->oo->props, &error_fatal); > + loc_pop(&e->loc); > + > + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); > + qapi_free_ObjectOptions(e->oo); > + } > + > if (qemu_opts_foreach(qemu_find_opts("object"), This handles all JSON forms prior to any QemuOpt forms (within the two priority levels), such that a command line using: -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' processes the arguments in a different order than -object type,id=1,oldstyle... -object type,id=2,oldstyle But I don't see that as too bad (ideally, someone using the {} JSON style will use it consistently). Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 08/11/2017 11:05 AM, Markus Armbruster wrote: >> We've wanted -object to support non-scalar properties for a while. >> Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based >> authorization API". Review led to the conclusion that we need to >> replace rather than add to QemuOpts. Initial work towards that goal >> has been merged to provide -blockdev (commit 8746709), but there's >> substantial work left, mostly due to an bewildering array of >> compatibility problems. >> >> Even if a full solution is still out of reach, we can have a partial >> solution now: accept -object argument in JSON syntax. This should >> unblock development work that needs non-scalar properties with >> -object. >> >> The implementation is similar to -blockdev, except we use the new >> infrastructure only for the new JSON case, and stick to QemuOpts for >> the existing KEY=VALUE,... case, to sidestep compatibility problems. >> >> If we did this for more options, we'd have to factor out common code. >> But for one option, this will do. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> qapi-schema.json | 14 +++++++++++--- >> vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 62 insertions(+), 3 deletions(-) >> >> static void object_create(bool (*type_predicate)(const char *)) >> { >> + ObjectOptionsQueueEntry *e, *next; >> + >> + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { >> + if (!type_predicate(e->oo->qom_type)) { >> + continue; >> + } >> + >> + loc_push_restore(&e->loc); >> + qmp_object_add(e->oo->qom_type, e->oo->id, >> + e->oo->has_props, e->oo->props, &error_fatal); >> + loc_pop(&e->loc); >> + >> + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); >> + qapi_free_ObjectOptions(e->oo); >> + } >> + >> if (qemu_opts_foreach(qemu_find_opts("object"), > > This handles all JSON forms prior to any QemuOpt forms (within the two > priority levels), such that a command line using: > > -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' > > processes the arguments in a different order than > > -object type,id=1,oldstyle... -object type,id=2,oldstyle > > But I don't see that as too bad (ideally, someone using the {} JSON > style will use it consistently). Yes, that's another restriction: if you need your -object evaluated in a certain order, you may have to stick to one of the two syntax variants. Aside: there are two sane evaluation orders: (1) strictly left to right, and (2) order doesn't matter. QEMU of course does (3) unpredicable for humans without referring back to the source code. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
On Fri, Aug 11, 2017 at 12:47:44PM -0500, Eric Blake wrote: > On 08/11/2017 11:05 AM, Markus Armbruster wrote: > > We've wanted -object to support non-scalar properties for a while. > > Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based > > authorization API". Review led to the conclusion that we need to > > replace rather than add to QemuOpts. Initial work towards that goal > > has been merged to provide -blockdev (commit 8746709), but there's > > substantial work left, mostly due to an bewildering array of > > compatibility problems. > > > > Even if a full solution is still out of reach, we can have a partial > > solution now: accept -object argument in JSON syntax. This should > > unblock development work that needs non-scalar properties with > > -object. > > > > The implementation is similar to -blockdev, except we use the new > > infrastructure only for the new JSON case, and stick to QemuOpts for > > the existing KEY=VALUE,... case, to sidestep compatibility problems. > > > > If we did this for more options, we'd have to factor out common code. > > But for one option, this will do. > > > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > --- > > qapi-schema.json | 14 +++++++++++--- > > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 62 insertions(+), 3 deletions(-) > > > > static void object_create(bool (*type_predicate)(const char *)) > > { > > + ObjectOptionsQueueEntry *e, *next; > > + > > + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { > > + if (!type_predicate(e->oo->qom_type)) { > > + continue; > > + } > > + > > + loc_push_restore(&e->loc); > > + qmp_object_add(e->oo->qom_type, e->oo->id, > > + e->oo->has_props, e->oo->props, &error_fatal); > > + loc_pop(&e->loc); > > + > > + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); > > + qapi_free_ObjectOptions(e->oo); > > + } > > + > > if (qemu_opts_foreach(qemu_find_opts("object"), > > This handles all JSON forms prior to any QemuOpt forms (within the two > priority levels), such that a command line using: > > -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' > > processes the arguments in a different order than > > -object type,id=1,oldstyle... -object type,id=2,oldstyle > > But I don't see that as too bad (ideally, someone using the {} JSON > style will use it consistently). I don't really like such a constraint - the ordering of object creation is already complex with some objets created at a different point in startup to other objects. Adding yet another constraint feels like it is painting ourselves into a corner wrt future changes. In particular I think it is quite possible to use the dotted form primarily, and only use JSON for the immediate scenario where non-JSON form won't work - I expect that's how we would use it in libvirt - I don't see libvirt changing 100% to JSON based objects Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Fri, Aug 11, 2017 at 12:47:44PM -0500, Eric Blake wrote: >> On 08/11/2017 11:05 AM, Markus Armbruster wrote: >> > We've wanted -object to support non-scalar properties for a while. >> > Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based >> > authorization API". Review led to the conclusion that we need to >> > replace rather than add to QemuOpts. Initial work towards that goal >> > has been merged to provide -blockdev (commit 8746709), but there's >> > substantial work left, mostly due to an bewildering array of >> > compatibility problems. >> > >> > Even if a full solution is still out of reach, we can have a partial >> > solution now: accept -object argument in JSON syntax. This should >> > unblock development work that needs non-scalar properties with >> > -object. >> > >> > The implementation is similar to -blockdev, except we use the new >> > infrastructure only for the new JSON case, and stick to QemuOpts for >> > the existing KEY=VALUE,... case, to sidestep compatibility problems. >> > >> > If we did this for more options, we'd have to factor out common code. >> > But for one option, this will do. >> > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > --- >> > qapi-schema.json | 14 +++++++++++--- >> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 62 insertions(+), 3 deletions(-) >> > >> > static void object_create(bool (*type_predicate)(const char *)) >> > { >> > + ObjectOptionsQueueEntry *e, *next; >> > + >> > + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { >> > + if (!type_predicate(e->oo->qom_type)) { >> > + continue; >> > + } >> > + >> > + loc_push_restore(&e->loc); >> > + qmp_object_add(e->oo->qom_type, e->oo->id, >> > + e->oo->has_props, e->oo->props, &error_fatal); >> > + loc_pop(&e->loc); >> > + >> > + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); >> > + qapi_free_ObjectOptions(e->oo); >> > + } >> > + >> > if (qemu_opts_foreach(qemu_find_opts("object"), >> >> This handles all JSON forms prior to any QemuOpt forms (within the two >> priority levels), such that a command line using: >> >> -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' >> >> processes the arguments in a different order than >> >> -object type,id=1,oldstyle... -object type,id=2,oldstyle >> >> But I don't see that as too bad (ideally, someone using the {} JSON >> style will use it consistently). > > I don't really like such a constraint - the ordering of object > creation is already complex with some objets created at a different > point in startup to other objects. Adding yet another constraint > feels like it is painting ourselves into a corner wrt future changes. The full solution will evaluate -object left to right. This partial solution doesn't, but it's not meant for use in anger, just for unblocking development work. Can add scary warnings to deter premature use. > In particular I think it is quite possible to use the dotted > form primarily, and only use JSON for the immediate scenario > where non-JSON form won't work - I expect that's how we would > use it in libvirt - I don't see libvirt changing 100% to JSON > based objects You need the JSON form anyway for QMP, and for the cases where dotted keys break down. Doing both just for the command line requires code to do dotted keys (which may already exist), and code to decide whether it can be used (which probably doesn't exist, yet). Wouldn't this add complexity? For what benefit exactly?
On Mon, Aug 14, 2017 at 07:49:54AM +0200, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 08/11/2017 11:05 AM, Markus Armbruster wrote: > >> We've wanted -object to support non-scalar properties for a while. > >> Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based > >> authorization API". Review led to the conclusion that we need to > >> replace rather than add to QemuOpts. Initial work towards that goal > >> has been merged to provide -blockdev (commit 8746709), but there's > >> substantial work left, mostly due to an bewildering array of > >> compatibility problems. > >> > >> Even if a full solution is still out of reach, we can have a partial > >> solution now: accept -object argument in JSON syntax. This should > >> unblock development work that needs non-scalar properties with > >> -object. > >> > >> The implementation is similar to -blockdev, except we use the new > >> infrastructure only for the new JSON case, and stick to QemuOpts for > >> the existing KEY=VALUE,... case, to sidestep compatibility problems. > >> > >> If we did this for more options, we'd have to factor out common code. > >> But for one option, this will do. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- > >> qapi-schema.json | 14 +++++++++++--- > >> vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 62 insertions(+), 3 deletions(-) > >> > >> static void object_create(bool (*type_predicate)(const char *)) > >> { > >> + ObjectOptionsQueueEntry *e, *next; > >> + > >> + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { > >> + if (!type_predicate(e->oo->qom_type)) { > >> + continue; > >> + } > >> + > >> + loc_push_restore(&e->loc); > >> + qmp_object_add(e->oo->qom_type, e->oo->id, > >> + e->oo->has_props, e->oo->props, &error_fatal); > >> + loc_pop(&e->loc); > >> + > >> + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); > >> + qapi_free_ObjectOptions(e->oo); > >> + } > >> + > >> if (qemu_opts_foreach(qemu_find_opts("object"), > > > > This handles all JSON forms prior to any QemuOpt forms (within the two > > priority levels), such that a command line using: > > > > -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' > > > > processes the arguments in a different order than > > > > -object type,id=1,oldstyle... -object type,id=2,oldstyle > > > > But I don't see that as too bad (ideally, someone using the {} JSON > > style will use it consistently). > > Yes, that's another restriction: if you need your -object evaluated in a > certain order, you may have to stick to one of the two syntax variants. > > Aside: there are two sane evaluation orders: (1) strictly left to right, > and (2) order doesn't matter. QEMU of course does (3) unpredicable for > humans without referring back to the source code. IIUC, to "fix" the ordering problem we need to be able to consider the ordering of all QEMU args, not just -object. The horrible hack with the two stage setup of -object in vl.c is driven by the fact that some objects are referenced by -device/-chardev args, while objects are referencing -device/-chardev args etc. This is the big problem to untangle, and understandable you don't want to tackle that for this patch. Until we can figure out how to address the big problem, it would be nice not to introduce yet another ordering though driven off usage of json vs non-json syntax. Regards, Daniel
On Mon, Aug 14, 2017 at 01:24:17PM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > On Fri, Aug 11, 2017 at 12:47:44PM -0500, Eric Blake wrote: > >> On 08/11/2017 11:05 AM, Markus Armbruster wrote: > >> > We've wanted -object to support non-scalar properties for a while. > >> > Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based > >> > authorization API". Review led to the conclusion that we need to > >> > replace rather than add to QemuOpts. Initial work towards that goal > >> > has been merged to provide -blockdev (commit 8746709), but there's > >> > substantial work left, mostly due to an bewildering array of > >> > compatibility problems. > >> > > >> > Even if a full solution is still out of reach, we can have a partial > >> > solution now: accept -object argument in JSON syntax. This should > >> > unblock development work that needs non-scalar properties with > >> > -object. > >> > > >> > The implementation is similar to -blockdev, except we use the new > >> > infrastructure only for the new JSON case, and stick to QemuOpts for > >> > the existing KEY=VALUE,... case, to sidestep compatibility problems. > >> > > >> > If we did this for more options, we'd have to factor out common code. > >> > But for one option, this will do. > >> > > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> > --- > >> > qapi-schema.json | 14 +++++++++++--- > >> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > >> > 2 files changed, 62 insertions(+), 3 deletions(-) > >> > > >> > static void object_create(bool (*type_predicate)(const char *)) > >> > { > >> > + ObjectOptionsQueueEntry *e, *next; > >> > + > >> > + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { > >> > + if (!type_predicate(e->oo->qom_type)) { > >> > + continue; > >> > + } > >> > + > >> > + loc_push_restore(&e->loc); > >> > + qmp_object_add(e->oo->qom_type, e->oo->id, > >> > + e->oo->has_props, e->oo->props, &error_fatal); > >> > + loc_pop(&e->loc); > >> > + > >> > + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); > >> > + qapi_free_ObjectOptions(e->oo); > >> > + } > >> > + > >> > if (qemu_opts_foreach(qemu_find_opts("object"), > >> > >> This handles all JSON forms prior to any QemuOpt forms (within the two > >> priority levels), such that a command line using: > >> > >> -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' > >> > >> processes the arguments in a different order than > >> > >> -object type,id=1,oldstyle... -object type,id=2,oldstyle > >> > >> But I don't see that as too bad (ideally, someone using the {} JSON > >> style will use it consistently). > > > > I don't really like such a constraint - the ordering of object > > creation is already complex with some objets created at a different > > point in startup to other objects. Adding yet another constraint > > feels like it is painting ourselves into a corner wrt future changes. > > The full solution will evaluate -object left to right. > > This partial solution doesn't, but it's not meant for use in anger, just > for unblocking development work. Can add scary warnings to deter > premature use. > > > In particular I think it is quite possible to use the dotted > > form primarily, and only use JSON for the immediate scenario > > where non-JSON form won't work - I expect that's how we would > > use it in libvirt - I don't see libvirt changing 100% to JSON > > based objects > > You need the JSON form anyway for QMP, and for the cases where dotted > keys break down. Doing both just for the command line requires code to > do dotted keys (which may already exist), and code to decide whether it > can be used (which probably doesn't exist, yet). > > Wouldn't this add complexity? For what benefit exactly? Surprisingly, it appears we do actually have code that generates the JSON syntax for (probably) all uses of -object today. In fact we are actually generating JSON and then converting it to dotted syntax in most cases, which I didn't realize when writing the above. We'll have to keep support for dotted syntax around a while for old QEMU versions, but it looks like we could reasonably easily switch to JSON syntax for all -object usage at the same time. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Aug 14, 2017 at 01:24:17PM +0200, Markus Armbruster wrote: >> "Daniel P. Berrange" <berrange@redhat.com> writes: >> >> > On Fri, Aug 11, 2017 at 12:47:44PM -0500, Eric Blake wrote: >> >> On 08/11/2017 11:05 AM, Markus Armbruster wrote: >> >> > We've wanted -object to support non-scalar properties for a while. >> >> > Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based >> >> > authorization API". Review led to the conclusion that we need to >> >> > replace rather than add to QemuOpts. Initial work towards that goal >> >> > has been merged to provide -blockdev (commit 8746709), but there's >> >> > substantial work left, mostly due to an bewildering array of >> >> > compatibility problems. >> >> > >> >> > Even if a full solution is still out of reach, we can have a partial >> >> > solution now: accept -object argument in JSON syntax. This should >> >> > unblock development work that needs non-scalar properties with >> >> > -object. >> >> > >> >> > The implementation is similar to -blockdev, except we use the new >> >> > infrastructure only for the new JSON case, and stick to QemuOpts for >> >> > the existing KEY=VALUE,... case, to sidestep compatibility problems. >> >> > >> >> > If we did this for more options, we'd have to factor out common code. >> >> > But for one option, this will do. >> >> > >> >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> > --- >> >> > qapi-schema.json | 14 +++++++++++--- >> >> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> > 2 files changed, 62 insertions(+), 3 deletions(-) >> >> > >> >> > static void object_create(bool (*type_predicate)(const char *)) >> >> > { >> >> > + ObjectOptionsQueueEntry *e, *next; >> >> > + >> >> > + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { >> >> > + if (!type_predicate(e->oo->qom_type)) { >> >> > + continue; >> >> > + } >> >> > + >> >> > + loc_push_restore(&e->loc); >> >> > + qmp_object_add(e->oo->qom_type, e->oo->id, >> >> > + e->oo->has_props, e->oo->props, &error_fatal); >> >> > + loc_pop(&e->loc); >> >> > + >> >> > + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); >> >> > + qapi_free_ObjectOptions(e->oo); >> >> > + } >> >> > + >> >> > if (qemu_opts_foreach(qemu_find_opts("object"), >> >> >> >> This handles all JSON forms prior to any QemuOpt forms (within the two >> >> priority levels), such that a command line using: >> >> >> >> -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' >> >> >> >> processes the arguments in a different order than >> >> >> >> -object type,id=1,oldstyle... -object type,id=2,oldstyle >> >> >> >> But I don't see that as too bad (ideally, someone using the {} JSON >> >> style will use it consistently). >> > >> > I don't really like such a constraint - the ordering of object >> > creation is already complex with some objets created at a different >> > point in startup to other objects. Adding yet another constraint >> > feels like it is painting ourselves into a corner wrt future changes. >> >> The full solution will evaluate -object left to right. >> >> This partial solution doesn't, but it's not meant for use in anger, just >> for unblocking development work. Can add scary warnings to deter >> premature use. >> >> > In particular I think it is quite possible to use the dotted >> > form primarily, and only use JSON for the immediate scenario >> > where non-JSON form won't work - I expect that's how we would >> > use it in libvirt - I don't see libvirt changing 100% to JSON >> > based objects >> >> You need the JSON form anyway for QMP, and for the cases where dotted >> keys break down. Doing both just for the command line requires code to >> do dotted keys (which may already exist), and code to decide whether it >> can be used (which probably doesn't exist, yet). >> >> Wouldn't this add complexity? For what benefit exactly? > > Surprisingly, it appears we do actually have code that generates the > JSON syntax for (probably) all uses of -object today. In fact we are > actually generating JSON and then converting it to dotted syntax in > most cases, which I didn't realize when writing the above. > > We'll have to keep support for dotted syntax around a while for old > QEMU versions, but it looks like we could reasonably easily switch > to JSON syntax for all -object usage at the same time. Excellent.
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Aug 14, 2017 at 07:49:54AM +0200, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >> > On 08/11/2017 11:05 AM, Markus Armbruster wrote: >> >> We've wanted -object to support non-scalar properties for a while. >> >> Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based >> >> authorization API". Review led to the conclusion that we need to >> >> replace rather than add to QemuOpts. Initial work towards that goal >> >> has been merged to provide -blockdev (commit 8746709), but there's >> >> substantial work left, mostly due to an bewildering array of >> >> compatibility problems. >> >> >> >> Even if a full solution is still out of reach, we can have a partial >> >> solution now: accept -object argument in JSON syntax. This should >> >> unblock development work that needs non-scalar properties with >> >> -object. >> >> >> >> The implementation is similar to -blockdev, except we use the new >> >> infrastructure only for the new JSON case, and stick to QemuOpts for >> >> the existing KEY=VALUE,... case, to sidestep compatibility problems. >> >> >> >> If we did this for more options, we'd have to factor out common code. >> >> But for one option, this will do. >> >> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> --- >> >> qapi-schema.json | 14 +++++++++++--- >> >> vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 2 files changed, 62 insertions(+), 3 deletions(-) >> >> >> >> static void object_create(bool (*type_predicate)(const char *)) >> >> { >> >> + ObjectOptionsQueueEntry *e, *next; >> >> + >> >> + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { >> >> + if (!type_predicate(e->oo->qom_type)) { >> >> + continue; >> >> + } >> >> + >> >> + loc_push_restore(&e->loc); >> >> + qmp_object_add(e->oo->qom_type, e->oo->id, >> >> + e->oo->has_props, e->oo->props, &error_fatal); >> >> + loc_pop(&e->loc); >> >> + >> >> + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); >> >> + qapi_free_ObjectOptions(e->oo); >> >> + } >> >> + >> >> if (qemu_opts_foreach(qemu_find_opts("object"), >> > >> > This handles all JSON forms prior to any QemuOpt forms (within the two >> > priority levels), such that a command line using: >> > >> > -object type,id=1,oldstyle... -object '{'id':2, 'type':..., newstyle...}' >> > >> > processes the arguments in a different order than >> > >> > -object type,id=1,oldstyle... -object type,id=2,oldstyle >> > >> > But I don't see that as too bad (ideally, someone using the {} JSON >> > style will use it consistently). >> >> Yes, that's another restriction: if you need your -object evaluated in a >> certain order, you may have to stick to one of the two syntax variants. >> >> Aside: there are two sane evaluation orders: (1) strictly left to right, >> and (2) order doesn't matter. QEMU of course does (3) unpredicable for >> humans without referring back to the source code. > > IIUC, to "fix" the ordering problem we need to be able to consider the > ordering of all QEMU args, not just -object. Yes. > The horrible hack with the two stage setup of -object in vl.c is driven by > the fact that some objects are referenced by -device/-chardev args, while > objects are referencing -device/-chardev args etc. This is the big problem > to untangle, and understandable you don't want to tackle that for this > patch. Until we can figure out how to address the big problem, it would be > nice not to introduce yet another ordering though driven off usage of > json vs non-json syntax. I proposed the patch as a quick hack to unblock development. We then decided we had nothing worthwhile to unblock at the time. You now want it as a stop gap so certain features don't have to wait until I crack the more general command line expressiveness problem. I agree we should reconsider what warts and restrictions are acceptable for this different, more serious purpose. I doubt the reordering wart and its accompanying "don't mix dotted keys with JSON if you need evaluation in order" restriction is inacceptable. However, avoiding the reordering wart might be easier than debating it, so let me look into that first.
diff --git a/qapi-schema.json b/qapi-schema.json index 802ea53..7ed1db1 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3618,15 +3618,23 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} } ## -# @object-add: +# @ObjectOptions: # -# Create a QOM object. +# Options for creating an object. # # @qom-type: the class name for the object to be created # # @id: the name of the new object # # @props: a dictionary of properties to be passed to the backend +## +{ 'struct': 'ObjectOptions', + 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} } + +## +# @object-add: +# +# Create a QOM object. # # Returns: Nothing on success # Error if @qom-type is not a valid class name @@ -3642,7 +3650,7 @@ # ## { 'command': 'object-add', - 'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} } + 'data': 'ObjectOptions' } ## # @object-del: diff --git a/vl.c b/vl.c index 96c5da1..cd9ab74 100644 --- a/vl.c +++ b/vl.c @@ -2855,8 +2855,36 @@ static bool object_create_delayed(const char *type) return !object_create_initial(type); } +typedef struct ObjectOptionsQueueEntry { + ObjectOptions *oo; + Location loc; + QSIMPLEQ_ENTRY(ObjectOptionsQueueEntry) entry; +} ObjectOptionsQueueEntry; + +typedef QSIMPLEQ_HEAD(ObjectOptionsQueue, ObjectOptionsQueueEntry) + ObjectOptionsQueue; + +ObjectOptionsQueue oo_queue = QSIMPLEQ_HEAD_INITIALIZER(oo_queue); + + static void object_create(bool (*type_predicate)(const char *)) { + ObjectOptionsQueueEntry *e, *next; + + QSIMPLEQ_FOREACH_SAFE(e, &oo_queue, entry, next) { + if (!type_predicate(e->oo->qom_type)) { + continue; + } + + loc_push_restore(&e->loc); + qmp_object_add(e->oo->qom_type, e->oo->id, + e->oo->has_props, e->oo->props, &error_fatal); + loc_pop(&e->loc); + + QSIMPLEQ_REMOVE(&oo_queue, e, ObjectOptionsQueueEntry, entry); + qapi_free_ObjectOptions(e->oo); + } + if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_add_opts_foreach, type_predicate, NULL)) { @@ -4079,6 +4107,29 @@ int main(int argc, char **argv, char **envp) #endif break; case QEMU_OPTION_object: + /* + * TODO Use qobject_input_visitor_new_str() instead of + * QemuOpts, not in addition to. Not done now because + * keyval_parse() isn't wart-compatible with QemuOpts. + */ + if (optarg[0] == '{') { + Visitor *v; + ObjectOptionsQueueEntry *e; + + v = qobject_input_visitor_new_str(optarg, "qom-type", + &err); + if (!v) { + error_report_err(err); + exit(1); + } + + e = g_new(ObjectOptionsQueueEntry, 1); + visit_type_ObjectOptions(v, NULL, &e->oo, &error_fatal); + visit_free(v); + loc_save(&e->loc); + QSIMPLEQ_INSERT_TAIL(&oo_queue, e, entry); + break; + } opts = qemu_opts_parse_noisily(qemu_find_opts("object"), optarg, true); if (!opts) {
We've wanted -object to support non-scalar properties for a while. Dan Berrange tried in "[PATCH v4 00/10]Provide a QOM-based authorization API". Review led to the conclusion that we need to replace rather than add to QemuOpts. Initial work towards that goal has been merged to provide -blockdev (commit 8746709), but there's substantial work left, mostly due to an bewildering array of compatibility problems. Even if a full solution is still out of reach, we can have a partial solution now: accept -object argument in JSON syntax. This should unblock development work that needs non-scalar properties with -object. The implementation is similar to -blockdev, except we use the new infrastructure only for the new JSON case, and stick to QemuOpts for the existing KEY=VALUE,... case, to sidestep compatibility problems. If we did this for more options, we'd have to factor out common code. But for one option, this will do. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi-schema.json | 14 +++++++++++--- vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-)