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

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

[Markus Armbruster]

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             | 74 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 75 insertions(+), 13 deletions(-)

-- 
2.7.5

[Qemu-devel] [PATCH 1/2] vl: Factor object_create() out of main()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/vl.c b/vl.c
index 8e247cc..fd98ed1 100644
--- a/vl.c
+++ b/vl.c
@@ -2845,7 +2845,6 @@ static bool object_create_initial(const char *type)
     return true;
 }
 
-
 /*
  * The remainder of object creation happens after the
  * creation of chardev, fsdev, net clients and device data types.
@@ -2855,6 +2854,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 +4398,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 +4523,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 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             | 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);
+    }
+
 #ifdef CONFIG_TPM
     if (tpm_init() < 0) {
         exit(1);
-- 
2.7.5

Re: [Qemu-devel] [PATCH 1/2] vl: Factor object_create() out of main()

[Eric Blake]

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

On 08/10/2017 07:25 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  vl.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 

> +++ b/vl.c
> @@ -2845,7 +2845,6 @@ static bool object_create_initial(const char *type)
>      return true;
>  }
>  
> -
>  /*

Spurious whitespace change? I can live with it because it adds
consistency, but it's not on a function directly touched by this patch.

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

[Eric Blake]

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

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

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

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/2] vl: Factor object_create() out of main()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 08/10/2017 07:25 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  vl.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>> 
>
>> +++ b/vl.c
>> @@ -2845,7 +2845,6 @@ static bool object_create_initial(const char *type)
>>      return true;
>>  }
>>  
>> -
>>  /*
>
> Spurious whitespace change? I can live with it because it adds
> consistency, but it's not on a function directly touched by this patch.

Not intentional (alternatively: I've since forgotten).  I'll drop it.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Markus Armbruster]

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

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

[Markus Armbruster]

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

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

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/2] vl: Factor object_create() out of main()

[Philippe Mathieu-Daudé]

On 08/10/2017 01:28 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 08/10/2017 07:25 AM, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

for the refactor:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>> ---
>>>   vl.c | 21 ++++++++++-----------
>>>   1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>
>>> +++ b/vl.c
>>> @@ -2845,7 +2845,6 @@ static bool object_create_initial(const char *type)
>>>       return true;
>>>   }
>>>   
>>> -
>>>   /*
>>
>> Spurious whitespace change? I can live with it because it adds
>> consistency, but it's not on a function directly touched by this patch.
> 
> Not intentional (alternatively: I've since forgotten).  I'll drop it.

If you ever send the whitespace change as another patch with at least 
some funny message:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Thanks!
> 

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

[Markus Armbruster]

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.