[Qemu-devel] [PATCH 0/3] fix query-memdev not repporting IDs of memory backends

[Qemu-devel] [PATCH 0/3] fix query-memdev not repporting IDs of memory backends

[Igor Mammedov]

Series is a couple of preparratory cleanups which simplify fix
and a fix itself.

Before fix HMP 'info memdevs' for CLI:
qemu-system-x86_64 -object memory-backend-ram,id=mem0,size=1G
outputs:

  memory backend: 0
    size:  1073741824
    merge: true
    dump: true
    prealloc: false
    policy: default
    host nodes: 128

after fix:

  memory backend: mem0
    size:  1073741824
    merge: true
    dump: true
    prealloc: false
    policy: default
    host nodes: 128

it should help to avoid remembering hotplugged IDs as they
could be queried back via HMP/QMP interface.

CC: ehabkost@redhat.com
CC: armbru@redhat.com
CC: dgilbert@redhat.com
CC: eblake@redhat.com
CC: afaerber@suse.de

Igor Mammedov (3):
  cleanup: remove not used header
  reuse user_creatable_add_opts() instead of user_creatable_add() in
    monitor
  fix qmp/hmp query-memdev not repporting IDs of memory backends

 include/qom/object_interfaces.h | 19 +---------
 include/sysemu/hostmem.h        |  1 +
 backends/hostmem.c              | 26 ++++++++++++++
 docs/qmp-commands.txt           |  1 +
 hmp.c                           | 10 ++----
 numa.c                          |  3 ++
 qapi-schema.json                |  3 ++
 qom/object_interfaces.c         | 78 ++++++++++++-----------------------------
 8 files changed, 59 insertions(+), 82 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] cleanup: remove not used header

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qom/object_interfaces.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ded4d84..4b880d0 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -3,7 +3,6 @@
 #include "qom/object_interfaces.h"
 #include "qemu/module.h"
 #include "qapi-visit.h"
-#include "qapi/qobject-output-visitor.h"
 #include "qapi/opts-visitor.h"
 
 void user_creatable_complete(Object *obj, Error **errp)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] cleanup: remove not used header

[Eric Blake]

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

On 01/02/2017 09:44 AM, Igor Mammedov wrote:

perhaps s/not used/unused/ in the subject

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

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  qom/object_interfaces.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index ded4d84..4b880d0 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -3,7 +3,6 @@
>  #include "qom/object_interfaces.h"
>  #include "qemu/module.h"
>  #include "qapi-visit.h"
> -#include "qapi/qobject-output-visitor.h"
>  #include "qapi/opts-visitor.h"
>  
>  void user_creatable_complete(Object *obj, Error **errp)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 1/3] cleanup: remove not used header

[Andreas Färber]

Am 03.01.2017 um 16:25 schrieb Eric Blake:
> On 01/02/2017 09:44 AM, Igor Mammedov wrote:
> 
> perhaps s/not used/unused/ in the subject

... and qom: instead of cleanup: if this gets re-spun.

Andreas

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>  qom/object_interfaces.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index ded4d84..4b880d0 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -3,7 +3,6 @@
>>  #include "qom/object_interfaces.h"
>>  #include "qemu/module.h"
>>  #include "qapi-visit.h"
>> -#include "qapi/qobject-output-visitor.h"
>>  #include "qapi/opts-visitor.h"
>>  
>>  void user_creatable_complete(Object *obj, Error **errp)
>>
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/3] cleanup: remove not used header

[Igor Mammedov]

On Tue, 3 Jan 2017 16:26:16 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 03.01.2017 um 16:25 schrieb Eric Blake:
> > On 01/02/2017 09:44 AM, Igor Mammedov wrote:
> > 
> > perhaps s/not used/unused/ in the subject
> 
> ... and qom: instead of cleanup: if this gets re-spun.
sure

> 
> Andreas
> 
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> ---
> >>  qom/object_interfaces.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> >> index ded4d84..4b880d0 100644
> >> --- a/qom/object_interfaces.c
> >> +++ b/qom/object_interfaces.c
> >> @@ -3,7 +3,6 @@
> >>  #include "qom/object_interfaces.h"
> >>  #include "qemu/module.h"
> >>  #include "qapi-visit.h"
> >> -#include "qapi/qobject-output-visitor.h"
> >>  #include "qapi/opts-visitor.h"
> >>  
> >>  void user_creatable_complete(Object *obj, Error **errp)
> >>
> > 
> 
> 

[Qemu-devel] [PATCH 2/3] reuse user_creatable_add_opts() instead of user_creatable_add() in monitor

[Igor Mammedov]

Simplify code by dropping ~57LOC by merging user_creatable_add()
into user_creatable_add_opts() and using the later from monutor.
Along with it allocate opts_visitor_new() once in user_creatable_add_opts().

As result we have one less API func and a more readable/simple
user_creatable_add_opts() vs user_creatable_add().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/qom/object_interfaces.h | 17 ----------
 hmp.c                           |  5 +--
 qom/object_interfaces.c         | 71 ++++++++++-------------------------------
 3 files changed, 18 insertions(+), 75 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 8b17f4d..fdd7603 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -76,23 +76,6 @@ void user_creatable_complete(Object *obj, Error **errp);
 bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp);
 
 /**
- * user_creatable_add:
- * @qdict: the object definition
- * @v: the visitor
- * @errp: if an error occurs, a pointer to an area to store the error
- *
- * Create an instance of the user creatable object whose type
- * is defined in @qdict by the 'qom-type' field, placing it
- * in the object composition tree with name provided by the
- * 'id' field. The remaining fields in @qdict are used to
- * initialize the object properties.
- *
- * Returns: the newly created object or NULL on error
- */
-Object *user_creatable_add(const QDict *qdict,
-                           Visitor *v, Error **errp);
-
-/**
  * user_creatable_add_type:
  * @type: the object type name
  * @id: the unique ID for the object
diff --git a/hmp.c b/hmp.c
index b869617..e7bead5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1808,7 +1808,6 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
     QemuOpts *opts;
-    Visitor *v;
     Object *obj = NULL;
 
     opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
@@ -1817,9 +1816,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    v = opts_visitor_new(opts);
-    obj = user_creatable_add(qdict, v, &err);
-    visit_free(v);
+    obj = user_creatable_add_opts(opts, &err);
     qemu_opts_del(opts);
 
     if (err) {
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 4b880d0..9b4155a 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -34,57 +34,6 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp)
     }
 }
 
-
-Object *user_creatable_add(const QDict *qdict,
-                           Visitor *v, Error **errp)
-{
-    char *type = NULL;
-    char *id = NULL;
-    Object *obj = NULL;
-    Error *local_err = NULL;
-    QDict *pdict;
-
-    pdict = qdict_clone_shallow(qdict);
-
-    visit_start_struct(v, NULL, NULL, 0, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    qdict_del(pdict, "qom-type");
-    visit_type_str(v, "qom-type", &type, &local_err);
-    if (local_err) {
-        goto out_visit;
-    }
-
-    qdict_del(pdict, "id");
-    visit_type_str(v, "id", &id, &local_err);
-    if (local_err) {
-        goto out_visit;
-    }
-    visit_check_struct(v, &local_err);
-    if (local_err) {
-        goto out_visit;
-    }
-
-    obj = user_creatable_add_type(type, id, pdict, v, &local_err);
-
-out_visit:
-    visit_end_struct(v, NULL);
-
-out:
-    QDECREF(pdict);
-    g_free(id);
-    g_free(type);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        object_unref(obj);
-        return NULL;
-    }
-    return obj;
-}
-
-
 Object *user_creatable_add_type(const char *type, const char *id,
                                 const QDict *qdict,
                                 Visitor *v, Error **errp)
@@ -157,13 +106,27 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
 {
     Visitor *v;
     QDict *pdict;
-    Object *obj = NULL;
+    Object *obj;
+    const char *id = qemu_opts_id(opts);
+    const char *type = qemu_opt_get(opts, "qom-type");
+
+    if (!type) {
+        error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
+        return NULL;
+    }
+    if (!id) {
+        error_setg(errp, QERR_MISSING_PARAMETER, "id");
+        return NULL;
+    }
 
-    v = opts_visitor_new(opts);
     pdict = qemu_opts_to_qdict(opts, NULL);
+    qdict_del(pdict, "qom-type");
+    qdict_del(pdict, "id");
 
-    obj = user_creatable_add(pdict, v, errp);
+    v = opts_visitor_new(opts);
+    obj = user_creatable_add_type(type, id, pdict, v, errp);
     visit_free(v);
+
     QDECREF(pdict);
     return obj;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] reuse user_creatable_add_opts() instead of user_creatable_add() in monitor

[Eric Blake]

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

On 01/02/2017 09:44 AM, Igor Mammedov wrote:
> Simplify code by dropping ~57LOC by merging user_creatable_add()
> into user_creatable_add_opts() and using the later from monutor.
> Along with it allocate opts_visitor_new() once in user_creatable_add_opts().
> 
> As result we have one less API func and a more readable/simple
> user_creatable_add_opts() vs user_creatable_add().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] reuse user_creatable_add_opts() instead of user_creatable_add() in monitor

[Eric Blake]

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

On 01/02/2017 09:44 AM, Igor Mammedov wrote:
> Simplify code by dropping ~57LOC by merging user_creatable_add()
> into user_creatable_add_opts() and using the later from monutor.

s/monutor/monitor/

> Along with it allocate opts_visitor_new() once in user_creatable_add_opts().

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH 3/3] fix qmp/hmp query-memdev not repporting IDs of memory backends

[Igor Mammedov]

Do it by adding 'id' property to hostmem backends and fetch it
in query-memdev from object directly.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/qom/object_interfaces.h |  2 +-
 include/sysemu/hostmem.h        |  1 +
 backends/hostmem.c              | 26 ++++++++++++++++++++++++++
 docs/qmp-commands.txt           |  1 +
 hmp.c                           |  5 +----
 numa.c                          |  3 +++
 qapi-schema.json                |  3 +++
 qom/object_interfaces.c         |  6 +++++-
 8 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index fdd7603..06cccd9 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -90,7 +90,7 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp);
  * Returns: the newly created object or NULL on error
  */
 Object *user_creatable_add_type(const char *type, const char *id,
-                                const QDict *qdict,
+                                QDict *qdict,
                                 Visitor *v, Error **errp);
 
 /**
diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 678232a..ecae0cf 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -52,6 +52,7 @@ struct HostMemoryBackend {
     Object parent;
 
     /* protected */
+    char *id;
     uint64_t size;
     bool merge, dump;
     bool prealloc, force_prealloc, is_mapped;
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4256d24..7f5de70 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -348,6 +348,24 @@ host_memory_backend_can_be_deleted(UserCreatable *uc, Error **errp)
     }
 }
 
+static char *get_id(Object *o, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+
+    return g_strdup(backend->id);
+}
+
+static void set_id(Object *o, const char *str, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+
+    if (backend->id) {
+        error_setg(errp, "cannot change property value");
+        return;
+    }
+    backend->id = g_strdup(str);
+}
+
 static void
 host_memory_backend_class_init(ObjectClass *oc, void *data)
 {
@@ -377,6 +395,13 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
         HostMemPolicy_lookup,
         host_memory_backend_get_policy,
         host_memory_backend_set_policy, &error_abort);
+    object_class_property_add_str(oc, "id", get_id, set_id, &error_abort);
+}
+
+static void host_memory_backend_finalize(Object *o)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+    g_free(backend->id);
 }
 
 static const TypeInfo host_memory_backend_info = {
@@ -387,6 +412,7 @@ static const TypeInfo host_memory_backend_info = {
     .class_init = host_memory_backend_class_init,
     .instance_size = sizeof(HostMemoryBackend),
     .instance_init = host_memory_backend_init,
+    .instance_finalize = host_memory_backend_finalize,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_USER_CREATABLE },
         { }
diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index abf210a..18db4cd 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -3529,6 +3529,7 @@ Example (1):
          "policy": "bind"
        },
        {
+         "id": "mem1",
          "size": 536870912,
          "merge": false,
          "dump": true,
diff --git a/hmp.c b/hmp.c
index e7bead5..8522efe 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2078,13 +2078,11 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
     MemdevList *m = memdev_list;
     Visitor *v;
     char *str;
-    int i = 0;
-
 
     while (m) {
         v = string_output_visitor_new(false, &str);
         visit_type_uint16List(v, NULL, &m->value->host_nodes, NULL);
-        monitor_printf(mon, "memory backend: %d\n", i);
+        monitor_printf(mon, "memory backend: %s\n", m->value->id);
         monitor_printf(mon, "  size:  %" PRId64 "\n", m->value->size);
         monitor_printf(mon, "  merge: %s\n",
                        m->value->merge ? "true" : "false");
@@ -2100,7 +2098,6 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
         g_free(str);
         visit_free(v);
         m = m->next;
-        i++;
     }
 
     monitor_printf(mon, "\n");
diff --git a/numa.c b/numa.c
index 9c09e45..f5fc7da 100644
--- a/numa.c
+++ b/numa.c
@@ -518,6 +518,9 @@ static int query_memdev(Object *obj, void *opaque)
 
         m->value = g_malloc0(sizeof(*m->value));
 
+        m->value->id = object_property_get_str(obj, "id", NULL);
+        m->value->has_id = !!m->value->id;
+
         m->value->size = object_property_get_int(obj, "size",
                                                  &error_abort);
         m->value->merge = object_property_get_bool(obj, "merge",
diff --git a/qapi-schema.json b/qapi-schema.json
index a0d3b5d..88eb2b9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4453,6 +4453,8 @@
 #
 # Information about memory backend
 #
+# @id: #optional backend's ID if backend has 'id' property (since 2.9)
+#
 # @size: memory backend size
 #
 # @merge: enables or disables memory merge support
@@ -4469,6 +4471,7 @@
 ##
 { 'struct': 'Memdev',
   'data': {
+    '*id':        'str',
     'size':       'size',
     'merge':      'bool',
     'dump':       'bool',
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 9b4155a..7a8f520 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -4,6 +4,7 @@
 #include "qemu/module.h"
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
+#include "qapi/qmp/qstring.h"
 
 void user_creatable_complete(Object *obj, Error **errp)
 {
@@ -35,7 +36,7 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp)
 }
 
 Object *user_creatable_add_type(const char *type, const char *id,
-                                const QDict *qdict,
+                                QDict *qdict,
                                 Visitor *v, Error **errp)
 {
     Object *obj;
@@ -62,6 +63,9 @@ Object *user_creatable_add_type(const char *type, const char *id,
 
     assert(qdict);
     obj = object_new(type);
+    if (object_property_find(obj, "id", NULL)) {
+        qdict_put(qdict, "id", qstring_from_str(id));
+    }
     visit_start_struct(v, NULL, NULL, 0, &local_err);
     if (local_err) {
         goto out;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] fix qmp/hmp query-memdev not repporting IDs of memory backends

[Eric Blake]

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

On 01/02/2017 09:44 AM, Igor Mammedov wrote:

s/repporting/reporting/ in the subject

> Do it by adding 'id' property to hostmem backends and fetch it
> in query-memdev from object directly.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

> +++ b/qom/object_interfaces.c
> @@ -4,6 +4,7 @@
>  #include "qemu/module.h"
>  #include "qapi-visit.h"
>  #include "qapi/opts-visitor.h"
> +#include "qapi/qmp/qstring.h"
>  
>  void user_creatable_complete(Object *obj, Error **errp)
>  {
> @@ -35,7 +36,7 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp)
>  }
>  
>  Object *user_creatable_add_type(const char *type, const char *id,
> -                                const QDict *qdict,
> +                                QDict *qdict,
>                                  Visitor *v, Error **errp)
>  {
>      Object *obj;
> @@ -62,6 +63,9 @@ Object *user_creatable_add_type(const char *type, const char *id,
>  
>      assert(qdict);
>      obj = object_new(type);
> +    if (object_property_find(obj, "id", NULL)) {
> +        qdict_put(qdict, "id", qstring_from_str(id));
> +    }

Wait. Isn't this going to inject an 'id' dict member to every use of
user_creatable_add_type()?  But not all QAPI structs contain an id
member.  Which means that you are now explicitly relying on the visitor
to silently ignore garbage in the dictionary, rather than our desired
goal of only validating if the dictionary exactly matches what the QAPI
says it will match.

I'm not sure if I like this hack, or if there is a better way to do
things when using a strict (rather than relaxed) input visitor.

>      visit_start_struct(v, NULL, NULL, 0, &local_err);
>      if (local_err) {
>          goto out;
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 3/3] fix qmp/hmp query-memdev not repporting IDs of memory backends

[Igor Mammedov]

On Tue, 3 Jan 2017 09:34:40 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 01/02/2017 09:44 AM, Igor Mammedov wrote:
> 
> s/repporting/reporting/ in the subject
> 
> > Do it by adding 'id' property to hostmem backends and fetch it
> > in query-memdev from object directly.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> 
> > +++ b/qom/object_interfaces.c
> > @@ -4,6 +4,7 @@
> >  #include "qemu/module.h"
> >  #include "qapi-visit.h"
> >  #include "qapi/opts-visitor.h"
> > +#include "qapi/qmp/qstring.h"
> >  
> >  void user_creatable_complete(Object *obj, Error **errp)
> >  {
> > @@ -35,7 +36,7 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp)
> >  }
> >  
> >  Object *user_creatable_add_type(const char *type, const char *id,
> > -                                const QDict *qdict,
> > +                                QDict *qdict,
> >                                  Visitor *v, Error **errp)
> >  {
> >      Object *obj;
> > @@ -62,6 +63,9 @@ Object *user_creatable_add_type(const char *type, const char *id,
> >  
> >      assert(qdict);
> >      obj = object_new(type);
> > +    if (object_property_find(obj, "id", NULL)) {
> > +        qdict_put(qdict, "id", qstring_from_str(id));
> > +    }
> 
> Wait. Isn't this going to inject an 'id' dict member to every use of
> user_creatable_add_type()?  But not all QAPI structs contain an id
> member.
it would 'inject' 'id' only for objects that have 'id' property.
or more exactly it would restore 'id' that is deleted earlier from
the same qdict. Look for qdict_del() usage in this file.
This way it'd be able to process both kinds of objects that implement
'id' property and ones that don't.
 
> Which means that you are now explicitly relying on the visitor
> to silently ignore garbage in the dictionary, rather than our desired
> goal of only validating if the dictionary exactly matches what the QAPI
> says it will match.
currently 'id' is mandatory, but not enforced by QAPI structs,
for all objects created with -object/object_add as it's used for
naming object in qom tree:
      object_property_add_child(object_get_objects_root(), id, obj, &local_err); 

> 
> I'm not sure if I like this hack, or if there is a better way to do
> things when using a strict (rather than relaxed) input visitor.
I'm not sure about a better way to do this.
Currently we use 2 visitors: opts (for cli/hmp) and qobject_input for qmp path,
which accept anything as target object could take different options.

The only things we currently enforce in code is presence of implicit 'qom-type' property
and 'id' property. The rest of properties are validated by object's property setters.

> 
> >      visit_start_struct(v, NULL, NULL, 0, &local_err);
> >      if (local_err) {
> >          goto out;
> > 
> 

Re: [Qemu-devel] [PATCH 3/3] fix qmp/hmp query-memdev not repporting IDs of memory backends

[Igor Mammedov]

On Tue, 3 Jan 2017 09:34:40 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 01/02/2017 09:44 AM, Igor Mammedov wrote:
> 
> s/repporting/reporting/ in the subject
> 
> > Do it by adding 'id' property to hostmem backends and fetch it
> > in query-memdev from object directly.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---  
> 
> > +++ b/qom/object_interfaces.c
> > @@ -4,6 +4,7 @@
> >  #include "qemu/module.h"
> >  #include "qapi-visit.h"
> >  #include "qapi/opts-visitor.h"
> > +#include "qapi/qmp/qstring.h"
> >  
> >  void user_creatable_complete(Object *obj, Error **errp)
> >  {
> > @@ -35,7 +36,7 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp)
> >  }
> >  
> >  Object *user_creatable_add_type(const char *type, const char *id,
> > -                                const QDict *qdict,
> > +                                QDict *qdict,
> >                                  Visitor *v, Error **errp)
> >  {
> >      Object *obj;
> > @@ -62,6 +63,9 @@ Object *user_creatable_add_type(const char *type, const char *id,
> >  
> >      assert(qdict);
> >      obj = object_new(type);
> > +    if (object_property_find(obj, "id", NULL)) {
> > +        qdict_put(qdict, "id", qstring_from_str(id));
> > +    }  
> 
> Wait. Isn't this going to inject an 'id' dict member to every use of
> user_creatable_add_type()?  But not all QAPI structs contain an id
> member.  Which means that you are now explicitly relying on the visitor
> to silently ignore garbage in the dictionary, rather than our desired
> goal of only validating if the dictionary exactly matches what the QAPI
> says it will match.
> 
> I'm not sure if I like this hack, or if there is a better way to do
> things when using a strict (rather than relaxed) input visitor.
a bit less ugly variant but with the same basic idea would look like:
--------------
Subject: [PATCH] fix qmp/hmp query-memdev not reporting IDs of memory backends   
                                                                                 
Considering 'id' is mandatory for user_creatable objects/backends                
and user_creatable_add_type() always has it as an argument                       
regardless of where from it is called CLI/monitor or QMP,                        
Fix issue by adding 'id' property to hostmem backends and                        
set it in user_creatable_add_type() for every object that                        
implements 'id' property. Then later at query-memdev time                        
get 'id' from object directly.                                                   
                                                                                 
Signed-off-by: Igor Mammedov <imammedo@redhat.com> 

[...]

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 9b4155a..03a95c3 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -62,6 +62,12 @@ Object *user_creatable_add_type(const char *type, const char *id,
 
     assert(qdict);
     obj = object_new(type);
+    if (object_property_find(obj, "id", NULL)) {
+        object_property_set_str(obj, id, "id", &local_err);
+        if (local_err) {
+            goto out;
+        }
+    }
     visit_start_struct(v, NULL, NULL, 0, &local_err);
     if (local_err) {
         goto out;

Re: [Qemu-devel] [PATCH 3/3] fix qmp/hmp query-memdev not repporting IDs of memory backends

[Eric Blake]

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

On 01/09/2017 08:17 AM, Igor Mammedov wrote:

>> Wait. Isn't this going to inject an 'id' dict member to every use of
>> user_creatable_add_type()?  But not all QAPI structs contain an id
>> member.  Which means that you are now explicitly relying on the visitor
>> to silently ignore garbage in the dictionary, rather than our desired
>> goal of only validating if the dictionary exactly matches what the QAPI
>> says it will match.
>>
>> I'm not sure if I like this hack, or if there is a better way to do
>> things when using a strict (rather than relaxed) input visitor.
> a bit less ugly variant but with the same basic idea would look like:
> --------------
> Subject: [PATCH] fix qmp/hmp query-memdev not reporting IDs of memory backends   
>                                                                                  
> Considering 'id' is mandatory for user_creatable objects/backends                
> and user_creatable_add_type() always has it as an argument                       
> regardless of where from it is called CLI/monitor or QMP,                        
> Fix issue by adding 'id' property to hostmem backends and                        
> set it in user_creatable_add_type() for every object that                        
> implements 'id' property. Then later at query-memdev time                        
> get 'id' from object directly.                     

Yes, that seems like an improved message.

>                                                                                  
> Signed-off-by: Igor Mammedov <imammedo@redhat.com> 
> 
> [...]
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 9b4155a..03a95c3 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -62,6 +62,12 @@ Object *user_creatable_add_type(const char *type, const char *id,
>  
>      assert(qdict);
>      obj = object_new(type);
> +    if (object_property_find(obj, "id", NULL)) {
> +        object_property_set_str(obj, id, "id", &local_err);
> +        if (local_err) {
> +            goto out;
> +        }
> +    }

Works for me.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 3/3] fix qmp/hmp query-memdev not repporting IDs of memory backends

[Igor Mammedov]

On Mon, 9 Jan 2017 14:26:28 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 01/09/2017 08:17 AM, Igor Mammedov wrote:
> 
> >> Wait. Isn't this going to inject an 'id' dict member to every use of
> >> user_creatable_add_type()?  But not all QAPI structs contain an id
> >> member.  Which means that you are now explicitly relying on the visitor
> >> to silently ignore garbage in the dictionary, rather than our desired
> >> goal of only validating if the dictionary exactly matches what the QAPI
> >> says it will match.
> >>
> >> I'm not sure if I like this hack, or if there is a better way to do
> >> things when using a strict (rather than relaxed) input visitor.  
> > a bit less ugly variant but with the same basic idea would look like:
> > --------------
> > Subject: [PATCH] fix qmp/hmp query-memdev not reporting IDs of memory backends   
> >                                                                                  
> > Considering 'id' is mandatory for user_creatable objects/backends                
> > and user_creatable_add_type() always has it as an argument                       
> > regardless of where from it is called CLI/monitor or QMP,                        
> > Fix issue by adding 'id' property to hostmem backends and                        
> > set it in user_creatable_add_type() for every object that                        
> > implements 'id' property. Then later at query-memdev time                        
> > get 'id' from object directly.                       
> 
> Yes, that seems like an improved message.
> 
> >                                                                                  
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> 
> > 
> > [...]
> > 
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index 9b4155a..03a95c3 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -62,6 +62,12 @@ Object *user_creatable_add_type(const char *type, const char *id,
> >  
> >      assert(qdict);
> >      obj = object_new(type);
> > +    if (object_property_find(obj, "id", NULL)) {
> > +        object_property_set_str(obj, id, "id", &local_err);
> > +        if (local_err) {
> > +            goto out;
> > +        }
> > +    }  
> 
> Works for me.
> 

Thanks,
Then, I'll respin v2 with noted fixups in commit messages
this amended patch.