[Qemu-devel] [PATCH v2 0/3] qmp: Return extra information on qom-list-types

[Qemu-devel] [PATCH v2 0/3] qmp: Return extra information on qom-list-types

[Eduardo Habkost]

This series adds 'abstract' and 'parent' fields to the output of
qom-list-types.

For reference, below are the sizes of the output of
"qom-list-types abstract=true" on qemu-system-x86_64, before and
after applying the patches:

* before: 11724 bytes
* with 'abstract' field: 13146 bytes
* with 'abstract' and 'parent': 24046 bytes

Changes v1 (RFC) -> v2:
* Make 'parent' field optional, to make output shorter
* Return only immediate parent type on 'parent' field, instead of
  all parent-types on a 'parent-types' field

Eduardo Habkost (3):
  tests: Simplify abstract-interfaces check with a helper
  qmp: Include 'abstract' field on 'qom-list-types' output
  qmp: Include parent type on 'qom-list-types' output

 qapi-schema.json               |   7 +-
 qmp.c                          |   6 ++
 tests/device-introspect-test.c | 156 +++++++++++++++++++++++++++++++++--------
 3 files changed, 139 insertions(+), 30 deletions(-)

-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH v2 1/3] tests: Simplify abstract-interfaces check with a helper

[Eduardo Habkost]

Add a new type_list_find() helper to device-introspect-test.c, to
simplify the code at test_abstract_interfaces().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/device-introspect-test.c | 43 ++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index b1abb2ad89..f1e6dddfa3 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -45,6 +45,22 @@ static QList *qom_list_types(const char *implements, bool abstract)
     return ret;
 }
 
+/* Find an entry on a list returned by qom-list-types */
+static QDict *type_list_find(QList *types, const char *name)
+{
+    QListEntry *e;
+
+    QLIST_FOREACH_ENTRY(types, e) {
+        QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+        const char *ename = qdict_get_str(d, "name");
+        if (!strcmp(ename, name)) {
+            return d;
+        }
+    }
+
+    return NULL;
+}
+
 static QList *device_type_list(bool abstract)
 {
     return qom_list_types("device", abstract);
@@ -125,7 +141,7 @@ static void test_abstract_interfaces(void)
 {
     QList *all_types;
     QList *obj_types;
-    QListEntry *ae;
+    QListEntry *e;
 
     qtest_start(common_args);
     /* qom-list-types implements=interface would return any type
@@ -138,24 +154,15 @@ static void test_abstract_interfaces(void)
     all_types = qom_list_types(NULL, false);
     obj_types = qom_list_types("object", false);
 
-    QLIST_FOREACH_ENTRY(all_types, ae) {
-        QDict *at = qobject_to_qdict(qlist_entry_obj(ae));
-        const char *aname = qdict_get_str(at, "name");
-        QListEntry *oe;
-        const char *found = NULL;
-
-        QLIST_FOREACH_ENTRY(obj_types, oe) {
-            QDict *ot = qobject_to_qdict(qlist_entry_obj(oe));
-            const char *oname = qdict_get_str(ot, "name");
-            if (!strcmp(aname, oname)) {
-                found = oname;
-                break;
-            }
-        }
+    QLIST_FOREACH_ENTRY(all_types, e) {
+        QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+
+        /*
+         * An interface (incorrectly) marked as non-abstract would
+         * appear on all_types, but not on obj_types:
+         */
+        g_assert(type_list_find(obj_types, qdict_get_str(d, "name")));
 
-        /* Using g_assert_cmpstr() will give more useful failure
-         * messages than g_assert(found) */
-        g_assert_cmpstr(aname, ==, found);
     }
 
     QDECREF(all_types);
-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH v2 2/3] qmp: Include 'abstract' field on 'qom-list-types' output

[Eduardo Habkost]

A client may be interested in getting the list of both abstract and
non-abstract types.  Instead of requiring them to make multiple queries
with different filter arguments, just return an 'abstract' field in
'qom-list-types'.

In addition to the new test code for validating this field, update the
abstract-interfaces test case to query for all 'interface' subtypes
(including abstract ones), and to look at the 'abstract' field directly.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Omit field if false, to keep command output smaller.
---
 qapi-schema.json               |  5 +++-
 qmp.c                          |  1 +
 tests/device-introspect-test.c | 53 +++++++++++++++++++++++++++++++-----------
 3 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index e38c5f0423..6af4b38851 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3056,12 +3056,15 @@
 #
 # @name: the type name found in the search
 #
+# @abstract: the type is abstract and can't be directly instantiated.
+#            Omitted if false. (since 2.10)
+#
 # Since: 1.1
 #
 # Notes: This command is experimental and may change syntax in future releases.
 ##
 { 'struct': 'ObjectTypeInfo',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str', '*abstract': 'bool' } }
 
 ##
 # @qom-list-types:
diff --git a/qmp.c b/qmp.c
index f656940769..413a917802 100644
--- a/qmp.c
+++ b/qmp.c
@@ -443,6 +443,7 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
 
     info = g_malloc0(sizeof(*info));
     info->name = g_strdup(object_class_get_name(klass));
+    info->has_abstract = info->abstract = object_class_is_abstract(klass);
 
     e = g_malloc0(sizeof(*e));
     e->value = info;
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index f1e6dddfa3..78d4811ba3 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -103,6 +103,33 @@ static void test_device_intro_list(void)
     qtest_end();
 }
 
+static void test_qom_list_fields(void)
+{
+    QList *all_types;
+    QList *non_abstract;
+    QListEntry *e;
+
+    qtest_start(common_args);
+
+    all_types = qom_list_types(NULL, true);
+    non_abstract = qom_list_types(NULL, false);
+
+    QLIST_FOREACH_ENTRY(all_types, e) {
+        QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+        const char *name = qdict_get_str(d, "name");
+        bool abstract = qdict_haskey(d, "abstract") ?
+                        qdict_get_bool(d, "abstract") :
+                        false;
+        bool expected_abstract = !type_list_find(non_abstract, name);
+
+        g_assert(abstract == expected_abstract);
+    }
+
+    QDECREF(all_types);
+    QDECREF(non_abstract);
+    qtest_end();
+}
+
 static void test_device_intro_none(void)
 {
     qtest_start(common_args);
@@ -144,25 +171,24 @@ static void test_abstract_interfaces(void)
     QListEntry *e;
 
     qtest_start(common_args);
-    /* qom-list-types implements=interface would return any type
-     * that implements _any_ interface (not just interface types),
-     * so use a trick to find the interface type names:
-     * - list all object types
-     * - list all types, and look for items that are not
-     *   on the first list
+    /*
+     * qom-list-types implements=interface returns all types that
+     * implement _any_ interface (not just interface types), but
+     * we can filter them out because interfaces don't implement
+     * the "object" type.
      */
-    all_types = qom_list_types(NULL, false);
-    obj_types = qom_list_types("object", false);
+    all_types = qom_list_types("interface", true);
+    obj_types = qom_list_types("object", true);
 
     QLIST_FOREACH_ENTRY(all_types, e) {
         QDict *d = qobject_to_qdict(qlist_entry_obj(e));
 
-        /*
-         * An interface (incorrectly) marked as non-abstract would
-         * appear on all_types, but not on obj_types:
-         */
-        g_assert(type_list_find(obj_types, qdict_get_str(d, "name")));
+        if (type_list_find(obj_types, qdict_get_str(d, "name"))) {
+            /* Not an interface type */
+            continue;
+        }
 
+        g_assert(qdict_haskey(d, "abstract") && qdict_get_bool(d, "abstract"));
     }
 
     QDECREF(all_types);
@@ -175,6 +201,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("device/introspect/list", test_device_intro_list);
+    qtest_add_func("device/introspect/list-fields", test_qom_list_fields);
     qtest_add_func("device/introspect/none", test_device_intro_none);
     qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
     qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH v2 3/3] qmp: Include parent type on 'qom-list-types' output

[Eduardo Habkost]

Include name of parent type of each type on 'qom-list-types' output.

Without this, there's no way to figure out the parents of a given type
without making additional 'qom-list-types' queries.

In addition to the test case for the new feature, update the
abstract-interface test case to use the new field and avoid the
"qom-list-types implements=object" trick.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Include only immediate parent type, rename new field to 'parent'
---
 qapi-schema.json               |  4 +-
 qmp.c                          |  5 +++
 tests/device-introspect-test.c | 84 +++++++++++++++++++++++++++++++++++++-----
 3 files changed, 82 insertions(+), 11 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 6af4b38851..14b09fd069 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3059,12 +3059,14 @@
 # @abstract: the type is abstract and can't be directly instantiated.
 #            Omitted if false. (since 2.10)
 #
+# @parent: Name of parent type, if any (since 2.10)
+#
 # Since: 1.1
 #
 # Notes: This command is experimental and may change syntax in future releases.
 ##
 { 'struct': 'ObjectTypeInfo',
-  'data': { 'name': 'str', '*abstract': 'bool' } }
+  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str' } }
 
 ##
 # @qom-list-types:
diff --git a/qmp.c b/qmp.c
index 413a917802..db87a74315 100644
--- a/qmp.c
+++ b/qmp.c
@@ -440,10 +440,15 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
 {
     ObjectTypeInfoList *e, **pret = data;
     ObjectTypeInfo *info;
+    ObjectClass *parent = object_class_get_parent(klass);
 
     info = g_malloc0(sizeof(*info));
     info->name = g_strdup(object_class_get_name(klass));
     info->has_abstract = info->abstract = object_class_is_abstract(klass);
+    if (parent) {
+        info->has_parent = true;
+        info->parent = g_strdup(object_class_get_name(parent));
+    }
 
     e = g_malloc0(sizeof(*e));
     e->value = info;
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index 78d4811ba3..f7162c023f 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -45,6 +45,40 @@ static QList *qom_list_types(const char *implements, bool abstract)
     return ret;
 }
 
+/* Build a name -> ObjectTypeInfo index from a ObjectTypeInfo list */
+static QDict *qom_type_index(QList *types)
+{
+    QDict *index = qdict_new();
+    QListEntry *e;
+
+    QLIST_FOREACH_ENTRY(types, e) {
+        QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+        const char *name = qdict_get_str(d, "name");
+        QINCREF(d);
+        qdict_put(index, name, d);
+    }
+    return index;
+}
+
+/* Check if @parent is present in the parent chain of @type */
+static bool qom_has_parent(QDict *index, const char *type, const char *parent)
+{
+    while (type) {
+        QDict *d = qdict_get_qdict(index, type);
+        const char *p = d && qdict_haskey(d, "parent") ?
+                        qdict_get_str(d, "parent") :
+                        NULL;
+
+        if (!strcmp(type, parent)) {
+            return true;
+        }
+
+        type = p;
+    }
+
+    return false;
+}
+
 /* Find an entry on a list returned by qom-list-types */
 static QDict *type_list_find(QList *types, const char *name)
 {
@@ -103,6 +137,30 @@ static void test_device_intro_list(void)
     qtest_end();
 }
 
+/*
+ * Ensure all entries returned by qom-list-types implements=<parent>
+ * have <parent> as a parent.
+ */
+static void test_qom_list_parents(const char *parent)
+{
+    QList *types;
+    QListEntry *e;
+    QDict *index;
+
+    types = qom_list_types(parent, true);
+    index = qom_type_index(types);
+
+    QLIST_FOREACH_ENTRY(types, e) {
+        QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+        const char *name = qdict_get_str(d, "name");
+
+        g_assert(qom_has_parent(index, name, parent));
+    }
+
+    QDECREF(types);
+    QDECREF(index);
+}
+
 static void test_qom_list_fields(void)
 {
     QList *all_types;
@@ -125,6 +183,10 @@ static void test_qom_list_fields(void)
         g_assert(abstract == expected_abstract);
     }
 
+    test_qom_list_parents("object");
+    test_qom_list_parents("device");
+    test_qom_list_parents("sys-bus-device");
+
     QDECREF(all_types);
     QDECREF(non_abstract);
     qtest_end();
@@ -167,23 +229,25 @@ static void test_device_intro_concrete(void)
 static void test_abstract_interfaces(void)
 {
     QList *all_types;
-    QList *obj_types;
     QListEntry *e;
+    QDict *index;
 
     qtest_start(common_args);
-    /*
-     * qom-list-types implements=interface returns all types that
-     * implement _any_ interface (not just interface types), but
-     * we can filter them out because interfaces don't implement
-     * the "object" type.
-     */
+
     all_types = qom_list_types("interface", true);
-    obj_types = qom_list_types("object", true);
+    index = qom_type_index(all_types);
 
     QLIST_FOREACH_ENTRY(all_types, e) {
         QDict *d = qobject_to_qdict(qlist_entry_obj(e));
+        const char *name = qdict_get_str(d, "name");
 
-        if (type_list_find(obj_types, qdict_get_str(d, "name"))) {
+        /*
+         * qom-list-types implements=interface returns all types
+         * that implement _any_ interface (not just interface
+         * types), so skip the ones that don't have "interface"
+         * on the parent type chain.
+         */
+        if (!qom_has_parent(index, name, "interface")) {
             /* Not an interface type */
             continue;
         }
@@ -192,7 +256,7 @@ static void test_abstract_interfaces(void)
     }
 
     QDECREF(all_types);
-    QDECREF(obj_types);
+    QDECREF(index);
     qtest_end();
 }
 
-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [PATCH v2 1/3] tests: Simplify abstract-interfaces check with a helper

[Eric Blake]

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

On 05/26/2017 03:23 PM, Eduardo Habkost wrote:
> Add a new type_list_find() helper to device-introspect-test.c, to
> simplify the code at test_abstract_interfaces().
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  tests/device-introspect-test.c | 43 ++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 

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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/3] qmp: Include 'abstract' field on 'qom-list-types' output

[Eric Blake]

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

On 05/26/2017 03:23 PM, Eduardo Habkost wrote:
> A client may be interested in getting the list of both abstract and
> non-abstract types.  Instead of requiring them to make multiple queries
> with different filter arguments, just return an 'abstract' field in
> 'qom-list-types'.
> 
> In addition to the new test code for validating this field, update the
> abstract-interfaces test case to query for all 'interface' subtypes
> (including abstract ones), and to look at the 'abstract' field directly.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -3056,12 +3056,15 @@
>  #
>  # @name: the type name found in the search
>  #
> +# @abstract: the type is abstract and can't be directly instantiated.
> +#            Omitted if false. (since 2.10)
> +#

A little bit awkward in that libvirt will have to see if ANY entry in
the large returned result has "abstract":true before it can assume
"abstract":false on the remaining entries (that is, older qemu don't
output abstract ever, but still mentions abstract types, so blindly
writing code to assume absence of "abstract" means false breaks on those
older versions).  Or a query of introspection to see if the field name
is even supported would do.  Not insurmountable, though.

>  # Since: 1.1
>  #
>  # Notes: This command is experimental and may change syntax in future releases.

Is this note still helpful, or should we delete it now?

>  ##
>  { 'struct': 'ObjectTypeInfo',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str', '*abstract': 'bool' } }

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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] qmp: Include parent type on 'qom-list-types' output

[Eric Blake]

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

On 05/26/2017 03:23 PM, Eduardo Habkost wrote:
> Include name of parent type of each type on 'qom-list-types' output.
> 
> Without this, there's no way to figure out the parents of a given type
> without making additional 'qom-list-types' queries.
> 
> In addition to the test case for the new feature, update the
> abstract-interface test case to use the new field and avoid the
> "qom-list-types implements=object" trick.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/3] qmp: Include 'abstract' field on 'qom-list-types' output

[Eduardo Habkost]

On Fri, May 26, 2017 at 03:51:53PM -0500, Eric Blake wrote:
> On 05/26/2017 03:23 PM, Eduardo Habkost wrote:
> > A client may be interested in getting the list of both abstract and
> > non-abstract types.  Instead of requiring them to make multiple queries
> > with different filter arguments, just return an 'abstract' field in
> > 'qom-list-types'.
> > 
> > In addition to the new test code for validating this field, update the
> > abstract-interfaces test case to query for all 'interface' subtypes
> > (including abstract ones), and to look at the 'abstract' field directly.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -3056,12 +3056,15 @@
> >  #
> >  # @name: the type name found in the search
> >  #
> > +# @abstract: the type is abstract and can't be directly instantiated.
> > +#            Omitted if false. (since 2.10)
> > +#
> 
> A little bit awkward in that libvirt will have to see if ANY entry in
> the large returned result has "abstract":true before it can assume
> "abstract":false on the remaining entries (that is, older qemu don't
> output abstract ever, but still mentions abstract types, so blindly
> writing code to assume absence of "abstract" means false breaks on those
> older versions).  Or a query of introspection to see if the field name
> is even supported would do.  Not insurmountable, though.

True.

I have the impression that management is not likely to look at
this field in the near future.  Probably the new field will be
more useful for automated test code like
device-instrospect-test.c and the device-crash-test script I
submitted recently.

> 
> >  # Since: 1.1
> >  #
> >  # Notes: This command is experimental and may change syntax in future releases.
> 
> Is this note still helpful, or should we delete it now?

The patch removing this line is already in a pending pull request
submitted by Markus last Tuesday.

> 
> >  ##
> >  { 'struct': 'ObjectTypeInfo',
> > -  'data': { 'name': 'str' } }
> > +  'data': { 'name': 'str', '*abstract': 'bool' } }
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

-- 
Eduardo