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":
>
