[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Thu, Mar 24, 2016 at 11:08:31 +0300, Nikolay Shirokovskiy wrote:
> > 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.
> 
> 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

None of these accepts any compression parameters (except for the flag
itself) so you could use a simple struct with only xbzrle set (or not
depending on the flag) and place it on the stack, but the code would
look cleaner if you just called the parse/free function too even though
they wouldn't do much without any typed parameters passed in.

> 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).

Right, but you touch various entry points and the internal code that
actually does something interesting doesn't need to care about the
origin of compression parameters. That said, I didn't really try
changing the code to see how it looks like but having a single structure
describing compression methods and parameters no matter how they were
specified by a user seems cleaner to me.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]