Re: [libvirt] [PATCH] qemu_migration: Check ABI stability against MIGRATABLE xml
On 09.10.2013 05:09, Eric Blake wrote:
On 10/08/2013 03:43 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=994364
>
> If user provides domain XML in migration we check if it doesn't break
> ABI via virDomainDefCheckABIStability(). However, if the provided XML
> was generated via virDomainGetXMLDesc(..., VIR_DOMAIN_XML_MIGRATABLE)
> it is missing some devices, e.g. 'pci-root' controller. Hence, the ABI
> stability check fails even though it is stable.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_migration.c | 41 +++++++++++++++++++++++++++++++++--------
> 1 file changed, 33 insertions(+), 8 deletions(-)
Oh my. How many different ways is this going to bite us? Is it worth
fixing virDomainDefCheckABIStability to do the sanitization itself,
instead of making every single caller across multiple patches repeat the
work?
What you have seems to work, but I'm really starting to wonder if it is
the best patch. Weak ACK.
Unfortunately, we can't do that simply. virDomainDefCheckABIStability()
is defined in domain_conf.c and thus meant to be driver-unaware. I mean,
in this patch I'm correctly using qemuDomainDefFormatLive() which on
MIGRATABLE flag enabled removes the default usb controller. However,
this constraint applies just for the qemu driver. So even if I'd call
virDomainDefCopy() within virDomainDefCheckABIStability() to get the
MIGRATABLE xml, it is not sufficient for qemu driver.
But what about introducing qemuDomainDefCheckABIStability:
(writing from the top of my head, doesn't have to necessary compile)
qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
virCapsPtr caps,
virDomainDefPtr src,
virDomainDefPtr dst)
{
migratableDefStr = qemuDomainDefFormatLive(driver, src, true, false);
migratableDef = virDomainDefParseString(migratableDefstr, caps, ...)
return virDomainDefCheckABIStability(migratableDef, dst);
}
And replacing (nearly?) all virDomainDefCheckABIStability() calls under
src/qemu/* with the new wrapper?
Michal