Re: [libvirt] [PATCH 0/9] Couple of migration fixes

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.

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.