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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list