Changeset
qapi-schema.json | 14 ++++++++---
vl.c             | 71 ++++++++++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 72 insertions(+), 13 deletions(-)
Test passed: FreeBSD

loading

Test passed: s390x

loading

Test passed: checkpatch

loading

Test passed: docker

loading

Git apply log
Switched to a new branch '1502467522-32237-1-git-send-email-armbru@redhat.com'
Applying: vl: Factor object_create() out of main()
Applying: vl: Partial support for non-scalar properties with -object
To https://github.com/patchew-project/qemu
 + a5e83effdb...526ec38023 patchew/1502467522-32237-1-git-send-email-armbru@redhat.com -> patchew/1502467522-32237-1-git-send-email-armbru@redhat.com (forced update)
[Qemu-devel] [PATCH v2 0/2] vl: Partial support for non-scalar properties with -object
Posted by Markus Armbruster, 9 weeks ago
v2:
* PATCH 1: Whitespace change dropped [Eric]
* PATCH 2: Deallocation done differently [Paolo], R-by dropped
           Commit message typo [Eric]

Markus Armbruster (2):
  vl: Factor object_create() out of main()
  vl: Partial support for non-scalar properties with -object

 qapi-schema.json | 14 ++++++++---
 vl.c             | 71 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 72 insertions(+), 13 deletions(-)

-- 
2.7.5


[Qemu-devel] [PATCH v2 1/2] vl: Factor object_create() out of main()
Posted by Markus Armbruster, 9 weeks ago
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 vl.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/vl.c b/vl.c
index 8e247cc..96c5da1 100644
--- a/vl.c
+++ b/vl.c
@@ -2855,6 +2855,14 @@ static bool object_create_delayed(const char *type)
     return !object_create_initial(type);
 }
 
+static void object_create(bool (*type_predicate)(const char *))
+{
+    if (qemu_opts_foreach(qemu_find_opts("object"),
+                          user_creatable_add_opts_foreach,
+                          type_predicate, NULL)) {
+        exit(1);
+    }
+}
 
 static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
                                MachineClass *mc)
@@ -4391,11 +4399,7 @@ int main(int argc, char **argv, char **envp)
     page_size_init();
     socket_init();
 
-    if (qemu_opts_foreach(qemu_find_opts("object"),
-                          user_creatable_add_opts_foreach,
-                          object_create_initial, NULL)) {
-        exit(1);
-    }
+    object_create(object_create_initial);
 
     if (qemu_opts_foreach(qemu_find_opts("chardev"),
                           chardev_init_func, NULL, NULL)) {
@@ -4520,11 +4524,7 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    if (qemu_opts_foreach(qemu_find_opts("object"),
-                          user_creatable_add_opts_foreach,
-                          object_create_delayed, NULL)) {
-        exit(1);
-    }
+    object_create(object_create_delayed);
 
 #ifdef CONFIG_TPM
     if (tpm_init() < 0) {
-- 
2.7.5


[Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object
Posted by Markus Armbruster, 9 weeks ago
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(-)

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) {
-- 
2.7.5


Re: [Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object
Posted by Eric Blake, 9 weeks ago
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, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object
Posted by Markus Armbruster, 9 weeks ago
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!

Re: [Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object
Posted by Daniel P. Berrange, 9 weeks ago
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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object
Posted by Markus Armbruster, 9 weeks ago
"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?