Re: [Qemu-devel] [PATCH v11 2/9] Add migration capabilites

[Orit Wasserman <owasserm@redhat.com>]

On 06/01/2012 01:57 PM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> wrote:
>> Add migration capabiltes that can be queried by the management.
>> The managment can query the source QEMU and the destination QEMU in order to
>> verify both support some  migration capability (currently only XBZRLE).
>> The managment can enable a capabilty for the next migration by using
>> migrate_set_parameter command.
>>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> +void qmp_migrate_set_parameter(const char *parameter, Error **errp)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    int i;
>> +
>> +    if (s->state == MIG_STATE_ACTIVE) {
>> +        error_set(errp, QERR_MIGRATION_ACTIVE);
>> +        return;
>> +    }
>> +
>> +    for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
>> +        if (strcmp(parameter, MigrationCapability_lookup[i]) == 0) {
>> +            s->enabled_capabilities[i] = true;
>> +            return;
>> +        }
>> +    }
>> +
>> +    error_set(errp, QERR_INVALID_PARAMETER, parameter);
>> +}
> 
> Two things here:
> - Is there a way to disable capabilities?  it seems no.

In this implementation we can't disable a capability , do you see a need to add it ?

> - Would we want in the future capabilities that are not "bool"?  Just
>   asking loud, I haven't thought a lot about this.  Fixing it as a
>   paramenter, it would make trivial to fix previous comment: cap:true vs
>   cap:false, or whatever syntax we want.
That is a good idea I will change it in next patch set.

Orit
> 
>>      memset(s, 0, sizeof(*s));
>>      s->bandwidth_limit = bandwidth_limit;
>>      s->params = *params;
>> +    memcpy(s->enabled_capabilities, enabled_capabilities,
>> +           sizeof(enabled_capabilities));
>>  
>> -    s->bandwidth_limit = bandwidth_limit;
>>      s->state = MIG_STATE_SETUP;
> 
> Nice catch/cleanup.
> 
> 
>> diff --git a/savevm.c b/savevm.c
>> index dd66f2c..42937a0 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -1711,7 +1711,7 @@ static int qemu_savevm_state(QEMUFile *f)
>>      int ret;
>>      MigrationParams params = {
>>          .blk = 0,
>> -        .shared = 0
>> +        .shared = 0,
>>      };
>>  
>>      if (qemu_savevm_state_blocked(NULL)) {
> 
> This belongs to previous patch?
> 
> Later, Juan.