Changeset
tests/qmp-test.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 172 insertions(+), 1 deletion(-)
Git apply log
Switched to a new branch '1502389816-29772-1-git-send-email-armbru@redhat.com'
Applying: tests/qmp-test: Add generic, basic test of query commands
To https://github.com/patchew-project/qemu
 + f69325624a...fc931afef3 patchew/1502389816-29772-1-git-send-email-armbru@redhat.com -> patchew/1502389816-29772-1-git-send-email-armbru@redhat.com (forced update)
Test passed: FreeBSD

loading

Test passed: s390x

loading

Test passed: checkpatch

loading

Test passed: docker

loading

[Qemu-devel] [PATCH] tests/qmp-test: Add generic, basic test of query commands
Posted by Markus Armbruster, 17 weeks ago
A command is a query if it has no side effect and yields a result.
Such commands are typically named query-FOO, but there are exceptions.

The basic idea is to find candidates with query-qmp-schema, filter out
the ones that aren't queries with an explicit blacklist, and test the
remaining ones against a QEMU with no special arguments.

The current blacklist is just add-fd.

query-qmp-schema reports a few commands that aren't actually
available.  See qmp_unregister_commands_hack() for details.  Work
around this flaw by accepting CommandNotFound errors, but add a TODO
to drop this when the flaw is fixed.

The test can't do queries with arguments, because it knows nothing
about the arguments.  No coverage for query-cpu-model-baseline,
query-cpu-model-comparison and query-cpu-model-expansion, because
query-rocker, query-rocker-ports, query-rocker-of-dpa-flows and
query-rocker-of-dpa-groups.

We get basic test coverage for the following commands:

    qom-list-types
    query-acpi-ospm-status
    query-balloon               (expected to fail)
    query-block
    query-block-jobs
    query-blockstats
    query-chardev
    query-chardev-backends
    query-command-line-options
    query-commands
    query-cpu-definitions
    query-cpus
    query-dump
    query-dump-guest-memory-capability
    query-events
    query-fdsets
    query-gic-capabilities
    query-hotpluggable-cpus
    query-iothreads
    query-kvm
    query-machines
    query-memdev
    query-memory-devices
    query-mice
    query-migrate
    query-migrate-cache-size
    query-migrate-capabilities
    query-migrate-parameters
    query-name
    query-named-block-nodes
    query-pci
    query-qmp-schema
    query-rx-filter
    query-spice
    query-status
    query-target
    query-tpm
    query-tpm-models
    query-tpm-types
    query-uuid
    query-version
    query-vm-generation-id      (expected to fail)
    query-vnc
    query-vnc-servers
    query-xen-replication-status

Most tested commands are expected to succeed.  The test does not check
the return value then.  A few commands are expected to fail because
they need special arguments to succeed, and this test is too dumb to
supply them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qmp-test.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 1 deletion(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 5d0260b..f62d34c 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -15,6 +15,7 @@
 #include "qapi-visit.h"
 #include "qapi/error.h"
 #include "qapi/qobject-input-visitor.h"
+#include "qapi/util.h"
 #include "qapi/visitor.h"
 
 const char common_args[] = "-nodefaults -machine none";
@@ -129,11 +130,181 @@ static void test_qmp_protocol(void)
     qtest_end();
 }
 
+static int query_error_class(const char *cmd)
+{
+    static struct {
+        const char *cmd;
+        int err_class;
+    } fails[] = {
+        { "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE },
+        { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
+        { NULL, -1 }
+    };
+    int i;
+
+    for (i = 0; fails[i].cmd; i++) {
+        if (!strcmp(cmd, fails[i].cmd)) {
+            return fails[i].err_class;
+        }
+    }
+    return -1;
+}
+
+static void test_query(const void *data)
+{
+    const char *cmd = data;
+    int expected_error_class = query_error_class(cmd);
+    QDict *resp, *error;
+    const char *error_class;
+
+    qtest_start("-nodefaults");
+
+    resp = qmp("{ 'execute': %s }", cmd);
+    error = qdict_get_qdict(resp, "error");
+    error_class = error ? qdict_get_str(error, "class") : NULL;
+
+    if (expected_error_class < 0) {
+        /*
+         * query-qmp-schema reports a few commands that aren't
+         * actually available.  See qmp_unregister_commands_hack() for
+         * details.  Work around this flaw:
+         * TODO drop when the flaw is fixed
+         */
+        if (error) {
+            g_assert_cmpstr(error_class, ==,
+                    QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND]);
+        } else {
+            g_assert(qdict_haskey(resp, "return"));
+        }
+    } else {
+        g_assert(error);
+        g_assert_cmpint(qapi_enum_parse(QapiErrorClass_lookup, error_class,
+                                        QAPI_ERROR_CLASS__MAX, -1,
+                                        &error_abort),
+                        ==, expected_error_class);
+    }
+    QDECREF(resp);
+
+    qtest_end();
+}
+
+static bool query_is_blacklisted(const char *cmd)
+{
+    const char *blacklist[] = {
+        "add-fd",
+        NULL
+    };
+    int i;
+
+    for (i = 0; blacklist[i]; i++) {
+        if (!strcmp(cmd, blacklist[i])) {
+            return true;
+        }
+    }
+    return false;
+}
+
+typedef struct {
+    SchemaInfoList *list;
+    GHashTable *hash;
+} QmpSchema;
+
+static void qmp_schema_init(QmpSchema *schema)
+{
+    QDict *resp;
+    Visitor *qiv;
+    SchemaInfoList *tail;
+
+    qtest_start("-nodefaults");
+    resp = qmp("{ 'execute': 'query-qmp-schema' }");
+
+    qiv = qobject_input_visitor_new(qdict_get(resp, "return"));
+    visit_type_SchemaInfoList(qiv, NULL, &schema->list, &error_abort);
+    visit_free(qiv);
+
+    QDECREF(resp);
+    qtest_end();
+
+    schema->hash = g_hash_table_new(g_str_hash, g_str_equal);
+
+    /* Build @schema: hash table mapping entity name to SchemaInfo */
+    for (tail = schema->list; tail; tail = tail->next) {
+        g_hash_table_insert(schema->hash, tail->value->name, tail->value);
+    }
+}
+
+static SchemaInfo *qmp_schema_lookup(QmpSchema *schema, const char *name)
+{
+    return g_hash_table_lookup(schema->hash, name);
+}
+
+static void qmp_schema_cleanup(QmpSchema *schema)
+{
+    qapi_free_SchemaInfoList(schema->list);
+    g_hash_table_destroy(schema->hash);
+}
+
+static bool object_type_has_mandatory_members(SchemaInfo *type)
+{
+    SchemaInfoObjectMemberList *tail;
+
+    g_assert(type->meta_type == SCHEMA_META_TYPE_OBJECT);
+
+    for (tail = type->u.object.members; tail; tail = tail->next) {
+        if (!tail->value->has_q_default) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static void add_query_tests(QmpSchema *schema)
+{
+    SchemaInfoList *tail;
+    SchemaInfo *si, *arg_type, *ret_type;
+    const char *test_name;
+
+    /* Test the query-like commands */
+    for (tail = schema->list; tail; tail = tail->next) {
+        si = tail->value;
+        if (si->meta_type != SCHEMA_META_TYPE_COMMAND) {
+            continue;
+        }
+
+        if (query_is_blacklisted(si->name)) {
+            continue;
+        }
+
+        arg_type = qmp_schema_lookup(schema, si->u.command.arg_type);
+        if (object_type_has_mandatory_members(arg_type)) {
+            continue;
+        }
+
+        ret_type = qmp_schema_lookup(schema, si->u.command.ret_type);
+        if (ret_type->meta_type == SCHEMA_META_TYPE_OBJECT
+            && !ret_type->u.object.members) {
+            continue;
+        }
+
+        test_name = g_strdup_printf("qmp/%s", si->name);
+        qtest_add_data_func(test_name, si->name, test_query);
+    }
+}
+
 int main(int argc, char *argv[])
 {
+    QmpSchema schema;
+    int ret;
+
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("qmp/protocol", test_qmp_protocol);
+    qmp_schema_init(&schema);
+    add_query_tests(&schema);
 
-    return g_test_run();
+    ret = g_test_run();
+
+    qmp_schema_cleanup(&schema);
+    return ret;
 }
-- 
2.7.5


Re: [Qemu-devel] [PATCH] tests/qmp-test: Add generic, basic test of query commands
Posted by Eric Blake, 17 weeks ago
On 08/10/2017 01:30 PM, Markus Armbruster wrote:
> A command is a query if it has no side effect and yields a result.
> Such commands are typically named query-FOO, but there are exceptions.
> 
> The basic idea is to find candidates with query-qmp-schema, filter out
> the ones that aren't queries with an explicit blacklist, and test the
> remaining ones against a QEMU with no special arguments.
> 
> The current blacklist is just add-fd.

I guess this is because it has no mandatory parameters.  Hmm - I wonder
if introspection should flag WHICH commands require an fd over SCM
rights (I guess just add-fd) - as that is a USEFUL piece of information
to know (I can't call command XYZ unlss I also pass an fd) - and then
you could use that real bit of the introspection rather than your
blacklist as the mechanism for filtering this command (since anything
that requires an fd is obviously not a query).

> 
> query-qmp-schema reports a few commands that aren't actually
> available.  See qmp_unregister_commands_hack() for details.  Work
> around this flaw by accepting CommandNotFound errors, but add a TODO
> to drop this when the flaw is fixed.
> 
> The test can't do queries with arguments, because it knows nothing
> about the arguments.  No coverage for query-cpu-model-baseline,
> query-cpu-model-comparison and query-cpu-model-expansion, because

s/because//

> query-rocker, query-rocker-ports, query-rocker-of-dpa-flows and
> query-rocker-of-dpa-groups.
> 
> We get basic test coverage for the following commands:

Cool!

> 
>     qom-list-types
>     query-acpi-ospm-status
>     query-balloon               (expected to fail)

>     query-vm-generation-id      (expected to fail)

> Most tested commands are expected to succeed.  The test does not check
> the return value then.  A few commands are expected to fail because
> they need special arguments to succeed, and this test is too dumb to
> supply them.

Sounds like it would just be a matter of adding additional command line
parameters to the qemu being invoked for testing those commands?

>  
> +static int query_error_class(const char *cmd)
> +{
> +    static struct {
> +        const char *cmd;
> +        int err_class;
> +    } fails[] = {
> +        { "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE },
> +        { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },

But even THIS level of testing of those commands is pretty good!


> +static void test_query(const void *data)
> +{
> +    const char *cmd = data;
> +    int expected_error_class = query_error_class(cmd);
> +    QDict *resp, *error;
> +    const char *error_class;
> +
> +    qtest_start("-nodefaults");
> +
> +    resp = qmp("{ 'execute': %s }", cmd);

Oh fun - your patch and my libqtest cleanup series will collide :)

> +static void qmp_schema_init(QmpSchema *schema)
> +{
> +    QDict *resp;
> +    Visitor *qiv;
> +    SchemaInfoList *tail;
> +
> +    qtest_start("-nodefaults");
> +    resp = qmp("{ 'execute': 'query-qmp-schema' }");
> +
> +    qiv = qobject_input_visitor_new(qdict_get(resp, "return"));
> +    visit_type_SchemaInfoList(qiv, NULL, &schema->list, &error_abort);

It's always fun to see this in action!  Our efforts to add automated
introspection code generation have been WELL worth the cost in time.

Overall, I like the 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

Re: [Qemu-devel] [PATCH] tests/qmp-test: Add generic, basic test of query commands
Posted by Markus Armbruster, 17 weeks ago
Eric Blake <eblake@redhat.com> writes:

> On 08/10/2017 01:30 PM, Markus Armbruster wrote:
>> A command is a query if it has no side effect and yields a result.
>> Such commands are typically named query-FOO, but there are exceptions.
>> 
>> The basic idea is to find candidates with query-qmp-schema, filter out
>> the ones that aren't queries with an explicit blacklist, and test the
>> remaining ones against a QEMU with no special arguments.
>> 
>> The current blacklist is just add-fd.
>
> I guess this is because it has no mandatory parameters.  Hmm - I wonder
> if introspection should flag WHICH commands require an fd over SCM
> rights (I guess just add-fd)

Actually, add-fd and getfd.

Their documentation neglects to explain how the two fit together.

Apropos documentation, here's a semi-random quote:

    # @fd: file descriptor of an already opened tap
    #
    # @fds: multiple file descriptors of already opened multiqueue capable
    # tap

This is close to useless unless you already know how everything works.

>                              - as that is a USEFUL piece of information
> to know (I can't call command XYZ unlss I also pass an fd) - and then
> you could use that real bit of the introspection rather than your
> blacklist as the mechanism for filtering this command (since anything
> that requires an fd is obviously not a query).

I doubt it's worth the trouble for just two commands.  But let's explore
how it could be done.

Naive attempt: the file descriptor is an implicit parameter, implicit
parameters don't show up in introspection, explicit ones do, so make it
one, say with a new built-in type.

The qemu_chr_fe_get_msgfd(&mon->chr) call moves from qmp_add_fd() to QMP
core.  Introspection shows the file descriptor parameter like any other
parameter.

Flaw: this kind of parameter must be passed differently than all the
others, namely with SCM_RIGHTS, not in JSON text.  Could this confuse
existing users of introspection?  Even if not: is it a good idea?

An obvious alternative is of course adding another optional member to
the command object, say a flag "takes file descriptors via SCM_RIGHTS".
Do we need to express the number of file descriptors it takes?  The
underlying infrastructure supports several (TCP_MAX_FDS in
char-socket.c), but the existing commands take just one.

>> query-qmp-schema reports a few commands that aren't actually
>> available.  See qmp_unregister_commands_hack() for details.  Work
>> around this flaw by accepting CommandNotFound errors, but add a TODO
>> to drop this when the flaw is fixed.
>> 
>> The test can't do queries with arguments, because it knows nothing
>> about the arguments.  No coverage for query-cpu-model-baseline,
>> query-cpu-model-comparison and query-cpu-model-expansion, because
>
> s/because//

Will fix.

>> query-rocker, query-rocker-ports, query-rocker-of-dpa-flows and
>> query-rocker-of-dpa-groups.
>> 
>> We get basic test coverage for the following commands:
>
> Cool!
>
>> 
>>     qom-list-types
>>     query-acpi-ospm-status
>>     query-balloon               (expected to fail)
>
>>     query-vm-generation-id      (expected to fail)
>
>> Most tested commands are expected to succeed.  The test does not check
>> the return value then.  A few commands are expected to fail because
>> they need special arguments to succeed, and this test is too dumb to
>> supply them.
>
> Sounds like it would just be a matter of adding additional command line
> parameters to the qemu being invoked for testing those commands?

In theory, setting up the state required for a query to succeed could be
too complex for this stupid test.

In practice, query-balloon should need just "-device virtio-balloon",
and query-vm-generation-id should need just "-device vmgenid" and ACPI
(I think).  Not all machines can do these devices.  Let's what I can do
in v2.

>> +static int query_error_class(const char *cmd)
>> +{
>> +    static struct {
>> +        const char *cmd;
>> +        int err_class;
>> +    } fails[] = {
>> +        { "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE },
>> +        { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
>
> But even THIS level of testing of those commands is pretty good!
>
>
>> +static void test_query(const void *data)
>> +{
>> +    const char *cmd = data;
>> +    int expected_error_class = query_error_class(cmd);
>> +    QDict *resp, *error;
>> +    const char *error_class;
>> +
>> +    qtest_start("-nodefaults");
>> +
>> +    resp = qmp("{ 'execute': %s }", cmd);
>
> Oh fun - your patch and my libqtest cleanup series will collide :)

Mine should be trivial to rebase.

>> +static void qmp_schema_init(QmpSchema *schema)
>> +{
>> +    QDict *resp;
>> +    Visitor *qiv;
>> +    SchemaInfoList *tail;
>> +
>> +    qtest_start("-nodefaults");
>> +    resp = qmp("{ 'execute': 'query-qmp-schema' }");
>> +
>> +    qiv = qobject_input_visitor_new(qdict_get(resp, "return"));
>> +    visit_type_SchemaInfoList(qiv, NULL, &schema->list, &error_abort);
>
> It's always fun to see this in action!  Our efforts to add automated
> introspection code generation have been WELL worth the cost in time.

It took a while to put it to productive use.  It's good to be there now.

> Overall, I like the patch.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] marking SCM_RIGHTS qapi commands [was: [PATCH] tests/qmp-test: Add generic, basic test of query commands]
Posted by Eric Blake, 17 weeks ago
On 08/11/2017 04:08 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 08/10/2017 01:30 PM, Markus Armbruster wrote:
>>> A command is a query if it has no side effect and yields a result.
>>> Such commands are typically named query-FOO, but there are exceptions.
>>>
>>> The basic idea is to find candidates with query-qmp-schema, filter out
>>> the ones that aren't queries with an explicit blacklist, and test the
>>> remaining ones against a QEMU with no special arguments.
>>>
>>> The current blacklist is just add-fd.
>>
>> I guess this is because it has no mandatory parameters.  Hmm - I wonder
>> if introspection should flag WHICH commands require an fd over SCM
>> rights (I guess just add-fd)
> 
> Actually, add-fd and getfd.

> 
> An obvious alternative is of course adding another optional member to
> the command object, say a flag "takes file descriptors via SCM_RIGHTS".
> Do we need to express the number of file descriptors it takes?  The
> underlying infrastructure supports several (TCP_MAX_FDS in
> char-socket.c), but the existing commands take just one.

Yeah, I was definitely leaning towards an additional annotation - maybe
where the .json file has:

{ 'command': 'add-fd', 'data': {'*fdset-id': 'int', '*opaque': 'str'},
  'fds': 1, 'returns': 'AddfdInfo' }

where the new 'fds' is what designates that the command expects to
consume 1 fd by SCM_RIGHTS (defaults to 0 when not present, and could be
larger than one if a command is ever designed to take multiple fds in
one go - although I find SCM_RIGHTS tricky enough with 1 fd that passing
multiple is probably not necessary, so maybe false/true is better than
int).  The introspection output would similarly expose the same optional
member for each command object, repeating what was present in the .json
file.

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

Re: [Qemu-devel] [PATCH] tests/qmp-test: Add generic, basic test of query commands
Posted by Thomas Huth, 14 weeks ago
On 11.08.2017 11:08, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 08/10/2017 01:30 PM, Markus Armbruster wrote:
[...]
>>> We get basic test coverage for the following commands:
>>
>> Cool!
>>
>>>
>>>     qom-list-types
>>>     query-acpi-ospm-status
>>>     query-balloon               (expected to fail)
>>
>>>     query-vm-generation-id      (expected to fail)
>>
>>> Most tested commands are expected to succeed.  The test does not check
>>> the return value then.  A few commands are expected to fail because
>>> they need special arguments to succeed, and this test is too dumb to
>>> supply them.
>>
>> Sounds like it would just be a matter of adding additional command line
>> parameters to the qemu being invoked for testing those commands?
> 
> In theory, setting up the state required for a query to succeed could be
> too complex for this stupid test.
> 
> In practice, query-balloon should need just "-device virtio-balloon",
> and query-vm-generation-id should need just "-device vmgenid" and ACPI
> (I think).  Not all machines can do these devices.  Let's what I can do
> in v2.

FWIW: I think the balloon testing could also go into
tests/virtio-balloon-test.c instead ... that test is only run on
architectures where the device should be available.

 Thomas

Re: [Qemu-devel] [PATCH] tests/qmp-test: Add generic, basic test of query commands
Posted by Markus Armbruster, 14 weeks ago
Thomas Huth <thuth@redhat.com> writes:

> On 11.08.2017 11:08, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 08/10/2017 01:30 PM, Markus Armbruster wrote:
> [...]
>>>> We get basic test coverage for the following commands:
>>>
>>> Cool!
>>>
>>>>
>>>>     qom-list-types
>>>>     query-acpi-ospm-status
>>>>     query-balloon               (expected to fail)
>>>
>>>>     query-vm-generation-id      (expected to fail)
>>>
>>>> Most tested commands are expected to succeed.  The test does not check
>>>> the return value then.  A few commands are expected to fail because
>>>> they need special arguments to succeed, and this test is too dumb to
>>>> supply them.
>>>
>>> Sounds like it would just be a matter of adding additional command line
>>> parameters to the qemu being invoked for testing those commands?
>> 
>> In theory, setting up the state required for a query to succeed could be
>> too complex for this stupid test.
>> 
>> In practice, query-balloon should need just "-device virtio-balloon",
>> and query-vm-generation-id should need just "-device vmgenid" and ACPI
>> (I think).  Not all machines can do these devices.  Let's what I can do
>> in v2.
>
> FWIW: I think the balloon testing could also go into
> tests/virtio-balloon-test.c instead ... that test is only run on
> architectures where the device should be available.

A generic test like this one can only do basic smoke testing.  More than
that would be useful for many commands, including query-balloon.  It
doesn't have to be in this file, though.  virtio-balloon-test.c looks
like a fine choice for query-balloon testing.