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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 1/2] vl: Factor object_create() out of main()

Markus Armbruster
Signed-off-by: Markus Armbruster <[hidden email]>
Reviewed-by: Eric Blake <[hidden email]>
Reviewed-by: Philippe Mathieu-Daudé <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Markus Armbruster
In reply to this post by 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 <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eric Blake
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 <[hidden email]>
> ---
>  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 <[hidden email]>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


signature.asc (632 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Markus Armbruster
Eric Blake <[hidden email]> 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 <[hidden email]>
>> ---
>>  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 <[hidden email]>

Thanks!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel P. Berrange-2
In reply to this post by Eric Blake
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 <[hidden email]>
> > ---
> >  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 :|

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Markus Armbruster
"Daniel P. Berrange" <[hidden email]> 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 <[hidden email]>
>> > ---
>> >  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?

Loading...