[libvirt] [PATCH 10/15] qemu: capabilities: Add support for QMP schema introspection

[Eric Blake]

On 10/20/2016 10:25 AM, Peter Krempa wrote:
> Allow detecting capabilities according to the qemu QMP schema. This is
> necessary as sometimes the availability of certain options depends on
> the presence of a field in the schema.
> 
> This patch adds support for loading the QMP schema when detecting qemu
> capabilities and adds a very simple query language to allow traversing
> the schema and selecting a certain element from it.
> 
> The infrastructure in this patch allows to use a query path and set a

s/allows to use a query path and set/uses a query path to set/

> specific capability flag according to the availability of the given
> element in the schema.
> ---
> 
>  struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
> @@ -1691,6 +1693,10 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsUSBNECXHCI[] = {
>      { "p3", QEMU_CAPS_NEC_USB_XHCI_PORTS },
>  };
> 
> +static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
> +    { "bogus/path/to/satisfy/compiler", 0 },

Cute. But this is the only example of an actual query. Is it worth
adding comments, either in the commit message or here, describing the
query language?

/me reads ahead
Okay, there's more details below.

> +
> +static const char *
> +virQEMUCapsQMPSchemaObjectGetType(const char *name,
> +                                  const char *field,
> +                                  const char *namefield,

A comment might go a long way to explain this one, since 'name',
'field', and 'namefield' alone don't make it immediately apparent.

> +                                  virJSONValuePtr elem)
> +{
> +    virJSONValuePtr arr;
> +    virJSONValuePtr cur;
> +    const char *curname;
> +    const char *type;
> +    size_t i;
> +
> +    if (!(arr = virJSONValueObjectGetArray(elem, field)))
> +        return NULL;

So this part is extracting an array from elem, where the array is either
named "members" or "variants";

> +
> +    for (i = 0; i < virJSONValueArraySize(arr); i++) {
> +        if (!(cur = virJSONValueArrayGet(arr, i)) ||

then within that array, it checks each entry;

> +            !(curname = virJSONValueObjectGetString(cur, namefield)) ||

if the array was a 'members', we are looking for the "name" key of the
entry; if the array was a 'variants', we are looking for the "case" key;

> +            !(type = virJSONValueObjectGetString(cur, "type")))

and regardless of which array type it was, it should have a 'type' key;

> +            continue;
> +
> +        if (STREQ(name, curname))
> +            return type;

If we found the name we want (whether the members array with key "name",
or the variants array with key "case"), we know what type that name has,
and return it.

Okay, I think it works (but again, a comment might help).

> +    }
> +
> +    return NULL;
> +}
> +
> +
> +static virJSONValuePtr
> +virQEMUCapsQMPSchemaTraverse(const char *basename,
> +                             char **query,
> +                             virHashTablePtr schema)
> +{
> +    virJSONValuePtr base;
> +    const char *metatype;
> +
> +    do {
> +        if (!(base = virHashLookup(schema, basename)))
> +            return NULL;
> +
> +        if (!*query)
> +            return base;
> +
> +        if (!(metatype = virJSONValueObjectGetString(base, "meta-type")))
> +            return NULL;

A missing meta-type would be a qemu bug.

> +
> +        /* flatten arrays by default */
> +        if (STREQ(metatype, "array")) {
> +            if (!(basename = virJSONValueObjectGetString(base, "element-type")))
> +                return NULL;
> +
> +            continue;
> +        } else if (STREQ(metatype, "object")) {
> +            if (**query == '+')
> +                basename = virQEMUCapsQMPSchemaObjectGetType(*query + 1,
> +                                                             "variants",
> +                                                             "case", base);
> +            else
> +                basename = virQEMUCapsQMPSchemaObjectGetType(*query,
> +                                                             "members",
> +                                                             "name",
> +                                                             base);

I'm a bit worried here.  Qemu promises that a name should not disappear
from a QMP command, but warns that between releases, locating that name
may migrate from the 'names' array (always present) to the 'variants'
array (present only under some circumstances), based on what else was
added to the command in the meantime.  I guess what that means is that
when qemu re-does a type layout, it may mean that we need two separate
query paths (one for the old layout, one for the new) to properly detect
the existence of that capability under all versions of qemu.  I don't
think it is a show-stopper, but is something to be aware of.

> +
> +            if (!basename)
> +                return NULL;
> +        } else if (STREQ(metatype, "command") ||
> +                   STREQ(metatype, "event")) {
> +            if (!(basename = virJSONValueObjectGetString(base, *query)))
> +                return NULL;
> +        } else {
> +            /* alternates, basic types and enums can't be entered */
> +            return NULL;
> +        }
> +
> +        query++;
> +    } while (*query);
> +
> +    return base;
> +}
> +
> +
> +/**
> + * virQEMUCapsQMPSchemaGetByPath:
> + * @query: string specifying the required data type (see below)
> + * @schema: hash table containing the schema data
> + * @entry: filled with the located schema object requested by @query
> + *
> + * Retrieves the requested schema entry specified by @query to @entry. The
> + * @query parameter has the following syntax which is very closely tied to the
> + * qemu schema syntax entries separated by slashes with a few special characters:
> + *
> + * "command_or_event/attribute/subattribute/+variant_discriminator/subattribute"
> + *
> + * command_or_event: name of the event or attribute to introspect
> + * attribute: selects whether arguments or return type should be introspected
> + *            ("arg-type" or "ret-type" for commands, "arg-type" for events)
> + * subattribute: specifies member name of object types
> + * +variant_discriminator: In case of unionized objects allows to select a

'allows to' is not idiomatic English.  How about:

In the case of unionized objects, select a

> + *                         specific case to introspect.
> + *
> + * Array types are automatically flattened to the singular type. Alternate
> + * types are currently not supported.
> + *
> + * The above types can be chained arbitrarily using slashes to construct any
> + * path into the schema tree.
> + *
> + * Returns 0 on success (including if the requested schema was not found) and
> + * fills @entry appropriately. On failure returns -1 and sets an appropriate
> + * error message.
> + */
> +static int
> +virQEMUCapsQMPSchemaGetByPath(const char *query,
> +                              virHashTablePtr schema,
> +                              virJSONValuePtr *entry)
> +{

I guess the next few patches will see it in action.

Overall looks like it is heading in a useful direction, but I think I'd
better see a v2 (and review the rest of the series) before giving ack.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161020/d4ce071f/attachment-0001.sig>