Re: [Qemu-devel] [PATCH] qmp: add query-qemu-capabilities

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> QMP clients can usually detect the presence of features via schema
> introspection.  There are rare features that do not involve schema
> changes and are therefore impossible to detect with schema
> introspection.
>
> This patch adds the query-qemu-capabilities command.  It returns a list
> of capabilities that this QEMU supports.

The name "capabilities" could be confusing, because we already have QMP
capabilities, complete with command qmp_capabilities.  Would "features"
work?

> The decision to make this a command rather than something statically
> defined in the schema is intentional.  It allows QEMU to decide which
> capabilities are available at runtime, if necessary.
>
> This new interface is necessary so that QMP clients can discover that
> migrating disk image files is safe with cache.direct=off on Linux.
> There is no other way to detect whether or not QEMU supports this.

I think what's unsaid here is that we don't want to make a completely
arbitrary schema change just to carry this bit of information.  We
could, but we don't want to.  Correct?

>
> Cc: Markus Armbruster <address@hidden>
> Cc: Peter Krempa <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  qapi/misc.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>  qmp.c          | 18 ++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 8b3ca4fdd3..9cf24919a3 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -3051,3 +3051,45 @@
>    'data': 'NumaOptions',
>    'allow-preconfig': true
>  }
> +
> +##
> +# @QemuCapability:
> +#
> +# Capabilities that are not otherwise introspectable.
> +#
> +# @migrate-with-cache-direct-off-on-linux:
> +#  It is safe to migrate disk image files with cache.direct=off on Linux.
> +#  Previously only cache.direct=on was supported for live migration.

Somewhat unusual formatting due to long name.  I double-checked the doc
comment gets processed correctly.  Good.

> +#
> +# Since: 4.0
> +##
> +{ 'enum': 'QemuCapability',
> +  'data': [
> +    'migrate-with-cache-direct-off-on-linux'
> +  ]
> +}
> +
> +##
> +# @QemuCapabilities:
> +#
> +# Capability information.
> +#
> +# @capabilities: supported capabilities
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'QemuCapabilities',
> +  'data': { 'capabilities': ['QemuCapability'] } }
> +
> +##
> +# @query-qemu-capabilities:
> +#
> +# Return the capabilities supported by this QEMU.  Most features can be
> +# detected via schema introspection but some are not observable from the
> +# schema.  This command offers a way to check for the presence of such
> +# features.
> +#
> +# Since: 4.0
> +##
> +{ 'command': 'query-qemu-capabilities',
> +  'returns': 'QemuCapabilities' }

Capabilities are flags: either present or absent, thus effectively
boolean.  I don't have a specific reason to doubt limiting capabilities
to boolean.  However, doing QemuCapabilities like

   { 'struct': 'QemuCapabilities',
     'data': { 'migrate-with-cache-direct-off-on-linux': 'bool' } }

could grow to support other values more easily.  What do you think?

> diff --git a/qmp.c b/qmp.c
> index b92d62cd5f..91a1228be7 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -717,3 +717,21 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
>  
>      return mem_info;
>  }
> +
> +QemuCapabilities *qmp_query_qemu_capabilities(Error **errp)
> +{
> +    QemuCapabilities *caps = g_new0(QemuCapabilities, 1);
> +    QemuCapabilityList **prev = &caps->capabilities;
> +    QemuCapability cap_val;
> +
> +    /* Add all QemuCapability enum values defined in the schema */
> +    for (cap_val = 0; cap_val < QEMU_CAPABILITY__MAX; cap_val++) {
> +        QemuCapabilityList *cap = g_new0(QemuCapabilityList, 1);
> +
> +        cap->value = cap_val;
> +        *prev = cap;
> +        prev = &cap->next;
> +    }
> +
> +    return caps;
> +}

All capabilities known to this build of QEMU are always present.  Okay;
no need to complicate things until we need to.  I just didn't expect it
to be this simple after the commit message's "It allows QEMU to decide
which capabilities are available at runtime, if necessary."