3:08 a.m.
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