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