Re: [Qemu-devel] [PATCH v14 02/13] Add migration capabilities

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> Add migration capabilities that can be queried by the management.
> The management can query the source QEMU and the destination QEMU in order to
> verify both support some migration capability (currently only XBZRLE).
> The management can enable a capability for the next migration by using
> migrate_set_parameter command.

Couple of comment nits below.  Also, if you don't mind a bit of
bike-shedding...

[roughly translated - feel free to ignore the bulk of this email; the
patch as-is works, if you don't think my alternate design makes sense to
implement]

> +++ b/hmp-commands.hx
> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration.
>  ETEXI
>  
>      {
> +        .name       = "migrate_set_parameter",
> +        .args_type  = "capability:s,state:b",
> +        .params     = "",
> +        .help       = "Enable the usage of a capability for migration",
> +        .mhandler.cmd = hmp_migrate_set_parameter,

This requires the 'state' parameter to be passed.  But wouldn't it be
easier to default things to state=true if no parameter was present,
since the most common usage of this command will be to enable, rather
than disable, a migration parameter?

> -        /* no migration has happened ever */
> +        /* no migration has happened ever show enabled capabilities */

Missing some punctuation, so this was hard to read.  Maybe:

/* no migration has ever happened; show enabled capabilities */

> +++ b/qapi-schema.json

> +##
> +# @MigrationCapabilityInfo
> +#
> +# Migration capability information
> +#
> +# @capability: capability enum
> +#
> +# Since: 1.2
> +##
> +{ 'type': 'MigrationCapabilityInfo',
> +  'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } }

Back to the bike-shedding - if you would mark this '*state':'bool', then
the enabled parameter becomes optional on input (it should still always
be present on query-migration-capabilities output, though).

> +migrate_set_parameters
> +-------
> +
> +Enable/Disable migration capabilities
> +
> +- "xbzrle": xbzrle support
> +
> +Arguments:
> +
> +Example:
> +
> +-> { "execute": "migrate_set_parameters" , "arguments":
> +     { "parameters":  { "capability": "xbzrle", "state": true } ] } }

Missing a '[' after "parameters":

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]