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

[Orit Wasserman]

On 07/03/2012 09:36 PM, Eric Blake wrote:
> 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?
After thinking about it , I prefer to leave to command as it is.
For me it feels strange that the enable will be different from the disable.

Orit
> 
>> -        /* 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":
>