Re: [Qemu-devel] [PATCH for-1.5] qmp: add query-drive-mirror-capabilities

[Eric Blake]

On 04/24/2013 02:36 PM, Luiz Capitulino wrote:
> The drive-mirror command was extended in QEMU v1.3.0 with two new

introduced in 1.3, extended in 1.4

> optional arguments 'granularity' and 'buf-size'. However, there's
> no way for libvirt to query for the existence of the new arguments.
> 
> This commit solves this problem by adding the
> query-drive-mirror-capabilities command, which reports the
> existence of both arguments.
> 
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---

What if we instead have a generic command querying setup, instead of
introducing one query per command?  I'm typing this without looking at
your patch...

{ 'type': 'CommandCapability',
  'data': { 'command': 'str',
            'capabilities': [ 'str' ] } }
{ 'command': 'query-command-capabilities',
  'arguments': { '*command', 'string' },
  'returns': [ 'CommandCapability' ] }

with a sample usage:

-> { "execute": "query-command-capabilities" }
<- { [ { "command": "drive-mirror",
         "capabilities": [ "granularity", "buf-size" ] },
       { "command", ... }
     ] }


Or maybe play a bit with QMP unions to make things more strongly typed:

{ 'enum': 'DriveMirrorCapability',
  'data': { 'buf-size', 'granularity' } }
{ 'union': 'CommandCapability',
  'data': { 'drive-mirror': [ 'DriveMirrorCapability' ],
            ...: [ ... ] } }
{ 'command': 'query-command-capabilities',
  'arguments': { '*command', 'string' },
  'returns': [ 'CommandCapability' ] }

with a sample usage:

-> { "execute": "query-command-capabilities" }
<- { [ { "type": "drive-mirror",
         "data": [ "granularity", "buf-size" ] },
       { "type", ... }
     ] }

And whether a '*command' argument should be optional for filtered
output, vs. always unconditionally dumping all information on all
commands with capabilities, vs. mandatory (can only get capabilities for
one command at a time), all goes back to the larger question of whether
query-* commands should allow filtering.

> 
> I'm just mimicking query-migrate-capabilities. I don't know if this
> is the best way of doing this because we don't allow drive-mirror
> capabilities to be set and they're always on.
> 
> On the other hand, if we take a simpler approach like returning a
> single string of supported new arguments, we may regret it later if
> we end up having to support capabilities negotiation.
> 
> Having said that, I don't mind doing this one way or the other and
> slightly prefer the simpler approach.

...Now I'm looking at your patch.  If we don't go with a fully-generic
(or even fully-generic but strongly-typed) version, then your proposal
of a new query-drive-mirror-capabilities is probably okay.

> 
>  blockdev.c       | 21 +++++++++++++++++++++
>  qapi-schema.json | 40 ++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |  7 +++++++
>  3 files changed, 68 insertions(+)

> +++ b/qapi-schema.json
> @@ -1715,6 +1715,46 @@
>              '*speed': 'int' } }
>  
>  ##
> +# @DriveMirrorCapability
> +#
> +# Capabilities supported by the driver-mirror command
> +#
> +# @granularity: supports setting the dirty bitmap's granularity through the
> +#               'granularity' argument
> +#
> +# @buf-size: supports setting the amount of data to be sent from source
> +#            to target through the 'buf-size' argument
> +#
> +# Since: 1.5.0
> +##
> +{ 'enum': 'DriveMirrorCapability', 'data': [ 'granularity', 'buf-size' ] }

Hey, that matches what I mentioned for the strongly-typed generic command :)

> +
> +##
> +# @DriveMirrorCapabilityStatus
> +#
> +# Status of drive-mirror capabilities
> +#
> +# @capability: capability enumeration
> +#
> +# @state: True if supported False otherwise
> +#
> +# Since: 1.5.0
> +##
> +{ 'type': 'DriveMirrorCapabilityStatus',
> +  'data': { 'capability': 'DriveMirrorCapability', 'state': 'bool' } }

state will always be true unless we allow negotiation to disable the
feature; this may be a bit of overkill for now, but at least the command
is designed for extensibility.

> +
> +##
> +# @query-drive-mirror-capabilities
> +#
> +# Returns information about current drive-mirror's capabilities status
> +#
> +# Returns: @DriveMirrorCapabilityStatus list
> +#
> +# Since: 1.5.0
> +##
> +{ 'command': 'query-drive-mirror-capabilities', 'returns': [ 
> 'DriveMirrorCapabilityStatus' ] }

Seems usable, but I still wonder if my more generic approach is worth
considering.  Also, I'm not sure if we HAVE to rush this into 1.5;
libvirt doesn't (yet) expose the buf-size or granularity options on to
the end user (although I do have plans to get to that point); and Paolo
pointed out that the try-and-fail approach (omit the argument if the
user requests the default, and try the argument relying on qemu's error,
instead of probing for the capability) may be sufficient even if we
don't have this introspection.  Yes, libvirt could give better error
messages and/or be more efficient without having to do a try-and-fail
approach, but this is something where if we _don't_ add the command to
1.5, and instead focus on getting the command right for 1.6, then
downstream distros could easily backport the new command into their
build of 1.5, as a quality-of-implementation improvement.

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..5b4e559 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2715,6 +2715,13 @@ EQMP
>      },
>  
>      {
> +        .name       = "query-drive-mirror-capabilities",
> +        .args_type  = "",
> +        .mhandler.cmd_new = 
> qmp_marshal_input_query_drive_mirror_capabilities,
> +    },

No example usage?

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

signature.asc
Description: OpenPGP digital signature