[libvirt] [PATCH v4 1/5] migration: add compress method option

[Nikolay Shirokovskiy]

On 23.03.2016 18:57, Jiri Denemark wrote:
> On Wed, Mar 23, 2016 at 13:22:45 +0300, Nikolay Shirokovskiy wrote:
>> On 18.03.2016 16:47, Jiri Denemark wrote:
>>>>  static int
>>>> +qemuMigrationSetCompression(virQEMUDriverPtr driver,
>>>> +                            virDomainObjPtr vm,
>>>> +                            qemuDomainAsyncJob job,
>>>> +                            qemuMigrationCompressionPtr compression,
>>>> +                            unsigned int flags)
>>>> +{
>>>> +    /*
>>>> +     * if compression methods are not set explicitly use flags to
>>>> +     * set default compression methods
>>>> +     */
>>>> +    if (compression == NULL || compression->methods == 0) {
>>>> +        return qemuMigrationSetOption(driver, vm,
>>>> +                                      QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
>>>> +                                      flags & VIR_MIGRATE_COMPRESSED,
>>>> +                                      job);
>>>> +    }
>>>
>>> I think it would be cleaner if qemuMigrationCompressionParseParams got
>>> the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods
>>> in case no compression parametr is used. Doing that would even fix a bug
>>> in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter
>>> is used, you still want to explicitly disable any supported compression
>>> capability. In other words, you'd still have to call
>>> qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if
>>> statement above. Changing qemuMigrationCompressionParseParams to do the
>>> right thing will avoid this code duplication.
>>>
>>
>> Hi, Jiri. Thanx for reviewing.
>>
>> I think we'd better stick with this approach.
> 
> It's quite unclear which approach you refer to by "this" :-)
> 
>> The reason is that we can get here thru old API where only flags are
>> set and compression parameter is set to NULL on the way.
> 
> Yes, and if we always do the right thing in
> qemuMigrationCompressionParseParams we don't need to think about
> backward compatibility elsewhere and complicate the code there.
> 

...

> 
>>
>>>
>>>> +    }
>>>
>>> And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and
>>> flags enabled compression.
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int
>>>> +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression,
>>>> +                                   virTypedParameterPtr *params,
>>>> +                                   int *nparams,
>>>> +                                   int *maxparams)
>>>> +{
>>>> +    if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) &&
>>>> +        virTypedParamsAddString(params, nparams, maxparams,
>>>> +                                VIR_MIGRATE_PARAM_COMPRESSION,
>>>> +                                "xbzrle") < 0)
>>>> +        return -1;
>>>> +
>>>> +    if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) &&
>>>> +        virTypedParamsAddString(params, nparams, maxparams,
>>>> +                                VIR_MIGRATE_PARAM_COMPRESSION,
>>>> +                                "multithread") < 0)
>>>> +        return -1;
>>>
>>> For backward compatibility this would just need to keep params empty if
>>> xbzrle was the only compression method specified and use only flags to
>>> express that.
>>
>> I suggest that we don't mix flags and compression parameters here and
>> in parse function too. This way we do not get backward compatibility
>> issues. If VIR_MIGRATE_COMPRESSED is specified we don't touch
>> it and do not convert to compression parameters. This way the old
>> value space is passed transparently by the old means. On dump compression.methods
>> will be 0 and we do not add params unrecognized by older libvirt.
> 
> The backward compatibility code will be limited to Parse/Dump parameters
> and the rest of the code will just see a nice struct of compression
> parameters without having to worry about how it was called. And dealing
> with it in *Dump is easy, just set don't output any parameters if only
> XBZRLE method was selected and no compression parameter is set.
> 
> Jirka
> 

Then I should never pass NULL value of compression pointer anywhere.
That is I should fix all the places where this is done in current version.
Below is the complete list of functions where NULL is passed:

qemuDomainMigratePrepare2
qemuDomainMigratePrepare3
qemuDomainMigratePerform
qemuDomainMigratePerform3

qemuMigrationPrepareTunnel
doPeer2PeerMigrate2
qemuMigrationPerformJob

I need to add code to parse/free in every of this places. This is
my concern. Except this I totally agree that trying to put
backward compatibility issues in one place like parse/dump 
functions is good. But on this way I still need to touch
a lot of places in less trivial way (in comparsion to
don't mixing flags / compression parameters).

Nikolay