On 22.09.2016 15:39, Jiri Denemark wrote:
On Tue, Sep 20, 2016 at 15:54:54 +0200, Michal Privoznik wrote:
Looks like nobody tried migrations lately.
This statement sounds a bit too strong for such a narrow use case.
I just did and found interesting bug. On the destination qemu binary was at different path. So I've dumped domain XML I was trying ti migrate and just changed the emulator path. All of a sudden I observed plenty of errors. Problem is that whilst parsing the user provided migration XML (and doing other operations on it), because of parse callbacks we need qemuCaps for the binary. However, we just take the def->emulator and expect it to exist. Well, it doesn't.
I think you're just the first one who tried migrating to a host where QEMU is installed in a different path. Or at least the first one who didn't just make a symlink to make QEMU binary reachable with the same path :-)
Michal Privoznik (9): conf: Introduce virDomainDefPostParseOpaque conf: Introduce virDomainDefParseStringOpaque qemuDomainDefPostParse: Fetch qemuCaps from domain object conf: Extend virDomainDeviceDefPostParse for parseOpaque qemuDomainDeviceDefPostParse: Fetch caps from domain object conf: Extend virDomainDefAssignAddressesCallback for parseOpaque qemuDomainDefAssignAddresses: Fetch caps from domain object domain_conf: Introduce VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE conf: Skip post parse callbacks when creating copy
Overall, the series looks OK, except for the new *Opaque functions. They are all internal APIs and I think just adding an extra parameter to the existing functions is much better than introducing new ones with even longer and uglier names. And git grep doesn't even show a lot of callers so the change to the existing prototypes won't be too invasive.
Ah, okay, I can stick with that approach then. I thought that this is going to be more acceptable to the upstream. Don't really know why now.
And do you plan to actually use vm pointers anywhere in the post-parse callbacks in the future? If it's not the case, I think passing priv->qemuCaps directly would be a bit better.
Well, okay. I think domain objects are more versatile, but if somebody needs them, we can always change that. So let me respin version 2. Michal