Message ID | 1502367921-17730-3-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
On 08/10/2017 07:25 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 > Trailing dot? > 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 | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+), 3 deletions(-) > 2.11 material. > @@ -4078,6 +4102,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] == '{') { So we DON'T allow " {...}", even though that is valid JSON. I'm okay with stating that { is magic only as the first byte. Reviewed-by: Eric Blake <eblake@redhat.com> Do we need any documentation additions (whether in --help output, or in the man page, or ?)
On 10/08/2017 14:25, 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 | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+), 3 deletions(-) > > 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 fd98ed1..db4680b 100644 > --- a/vl.c > +++ b/vl.c > @@ -2854,8 +2854,32 @@ 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; > + > + QSIMPLEQ_FOREACH(e, &oo_queue, entry) { > + 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); > + } > + > if (qemu_opts_foreach(qemu_find_opts("object"), > user_creatable_add_opts_foreach, > type_predicate, NULL)) { > @@ -4078,6 +4102,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) { > @@ -4525,6 +4572,14 @@ int main(int argc, char **argv, char **envp) > > object_create(object_create_delayed); > > + while (!QSIMPLEQ_EMPTY(&oo_queue)) { > + ObjectOptionsQueueEntry *e = QSIMPLEQ_FIRST(&oo_queue); > + > + QSIMPLEQ_REMOVE_HEAD(&oo_queue, entry); > + qapi_free_ObjectOptions(e->oo); > + g_free(e); > + } Why not free the queue entry in object_create, and assert here that it's empty? Paolo > + > #ifdef CONFIG_TPM > if (tpm_init() < 0) { > exit(1); >
Eric Blake <eblake@redhat.com> writes: > On 08/10/2017 07:25 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 >> > > Trailing dot? Will fix. >> 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 | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 66 insertions(+), 3 deletions(-) >> > > 2.11 material. Yes. >> @@ -4078,6 +4102,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] == '{') { > > So we DON'T allow " {...}", even though that is valid JSON. I'm okay > with stating that { is magic only as the first byte. Consistent with -blockdev. Can improve later, if we want to. > Reviewed-by: Eric Blake <eblake@redhat.com> > > Do we need any documentation additions (whether in --help output, or in > the man page, or ?) Let's not advertise this just yet. It's mostly so that Manos can make progress while we work on the full solution (he wants to use -object for throttle filters, and needs non-scalar properties).
Paolo Bonzini <pbonzini@redhat.com> writes: > On 10/08/2017 14:25, 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 | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 66 insertions(+), 3 deletions(-) >> >> 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 fd98ed1..db4680b 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2854,8 +2854,32 @@ 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; >> + >> + QSIMPLEQ_FOREACH(e, &oo_queue, entry) { >> + 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); >> + } >> + >> if (qemu_opts_foreach(qemu_find_opts("object"), >> user_creatable_add_opts_foreach, >> type_predicate, NULL)) { >> @@ -4078,6 +4102,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) { >> @@ -4525,6 +4572,14 @@ int main(int argc, char **argv, char **envp) >> >> object_create(object_create_delayed); >> >> + while (!QSIMPLEQ_EMPTY(&oo_queue)) { >> + ObjectOptionsQueueEntry *e = QSIMPLEQ_FIRST(&oo_queue); >> + >> + QSIMPLEQ_REMOVE_HEAD(&oo_queue, entry); >> + qapi_free_ObjectOptions(e->oo); >> + g_free(e); >> + } > > Why not free the queue entry in object_create, and assert here that it's > empty? Assumes object_create_delayed(TYPE) == !object_create_initial(TYPE), which is the case. Fewer assumptions is good. Less code is also good. Pick your goodness, please :) > > Paolo > >> + >> #ifdef CONFIG_TPM >> if (tpm_init() < 0) { >> exit(1); >>
On 10/08/2017 18:36, Markus Armbruster wrote: >>> + while (!QSIMPLEQ_EMPTY(&oo_queue)) { >>> + ObjectOptionsQueueEntry *e = QSIMPLEQ_FIRST(&oo_queue); >>> + >>> + QSIMPLEQ_REMOVE_HEAD(&oo_queue, entry); >>> + qapi_free_ObjectOptions(e->oo); >>> + g_free(e); >>> + } >> Why not free the queue entry in object_create, and assert here that it's >> empty? > > Assumes object_create_delayed(TYPE) == !object_create_initial(TYPE), > which is the case. Fewer assumptions is good. Less code is also good. > Pick your goodness, please :) I think the assumption is not that object_create_delayed(TYPE) == !object_create_initial(TYPE), but rather that all -object options are dealt with (and they shouldn't be dealt more than once). It's a reasonable assumption, methinks. :) Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 10/08/2017 18:36, Markus Armbruster wrote: >>>> + while (!QSIMPLEQ_EMPTY(&oo_queue)) { >>>> + ObjectOptionsQueueEntry *e = QSIMPLEQ_FIRST(&oo_queue); >>>> + >>>> + QSIMPLEQ_REMOVE_HEAD(&oo_queue, entry); >>>> + qapi_free_ObjectOptions(e->oo); >>>> + g_free(e); >>>> + } >>> Why not free the queue entry in object_create, and assert here that it's >>> empty? >> >> Assumes object_create_delayed(TYPE) == !object_create_initial(TYPE), >> which is the case. Fewer assumptions is good. Less code is also good. >> Pick your goodness, please :) > > I think the assumption is not that object_create_delayed(TYPE) == > !object_create_initial(TYPE), but rather that all -object options are > dealt with (and they shouldn't be dealt more than once). It's a > reasonable assumption, methinks. :) Okay.
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 fd98ed1..db4680b 100644 --- a/vl.c +++ b/vl.c @@ -2854,8 +2854,32 @@ 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; + + QSIMPLEQ_FOREACH(e, &oo_queue, entry) { + 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); + } + if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_add_opts_foreach, type_predicate, NULL)) { @@ -4078,6 +4102,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) { @@ -4525,6 +4572,14 @@ int main(int argc, char **argv, char **envp) object_create(object_create_delayed); + while (!QSIMPLEQ_EMPTY(&oo_queue)) { + ObjectOptionsQueueEntry *e = QSIMPLEQ_FIRST(&oo_queue); + + QSIMPLEQ_REMOVE_HEAD(&oo_queue, entry); + qapi_free_ObjectOptions(e->oo); + g_free(e); + } + #ifdef CONFIG_TPM if (tpm_init() < 0) { exit(1);
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 | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-)