[Qemu-devel] [PATCH v2 0/2] vl: Partial support for non-scalar properties with -object

[Qemu-devel] [PATCH v2 0/2] vl: Partial support for non-scalar properties with -object

[Markus Armbruster]

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()

[Markus Armbruster]

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

[Markus Armbruster]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2555 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object

[Markus Armbruster]

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

[Daniel P. Berrangé]

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
-- 
|: 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

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object

[Daniel P. Berrange]

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

[Markus Armbruster]

"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?

Re: [Qemu-devel] [PATCH v2 2/2] vl: Partial support for non-scalar properties with -object

[Daniel P. Berrangé]

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
-- 
|: 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

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH v2 0/2] vl: Partial support for non-scalar properties with -object

[Daniel P. Berrangé]

Just a ping to say I'd like us to restart work this patch and try to get
it mergable for the 2.13 cycle, so I can rely it on for the ACL support
I've had out of tree since 2.6 :-)

On Fri, Aug 11, 2017 at 06:05:20PM +0200, Markus Armbruster wrote:
> 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
> 

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 0/2] vl: Partial support for non-scalar properties with -object

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Just a ping to say I'd like us to restart work this patch and try to get
> it mergable for the 2.13 cycle, so I can rely it on for the ACL support
> I've had out of tree since 2.6 :-)

Fair enough.  I'll see what I can do.

Re: [Qemu-devel] [PATCH v2 0/2] vl: Partial support for non-scalar properties with -object

[Daniel P. Berrangé]

On Mon, May 28, 2018 at 11:31:03AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Just a ping to say I'd like us to restart work this patch and try to get
> > it mergable for the 2.13 cycle, so I can rely it on for the ACL support
> > I've had out of tree since 2.6 :-)
> 
> Fair enough.  I'll see what I can do.

Heads up that in retrospect I don't think this is a blocker for my
ACL work anymore. I decided it was more useful to store the access
control list in external files, so we can auto-reload them on the
fly without involving the monitor.

Still desirable long term, but no need to rush.

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 :|