| Message ID | 1502389816-29772-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <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!
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.
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
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.
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; }
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(-)