Eric Blake <ebl...@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 <ebl...@redhat.com>
Thanks!
