[libvirt] [PATCH] qemu_hotplug: correctly parse device XML provided by user

Commit 1ccc7fbf fixed port checking for live device update, which revealed another bug. If we updating live domain we need to parse the device also as live. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1336134 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 47 +++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37d970e..9602f4d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8325,7 +8325,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + virDomainDeviceDefPtr devConfig = NULL; + virDomainDeviceDefPtr devLive = NULL; bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; int ret = -1; virQEMUCapsPtr qemuCaps = NULL; @@ -8355,26 +8356,24 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); - if (dev == NULL) - goto endjob; - - if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) - goto endjob; + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + devConfig = virDomainDeviceDefParse(xml, vm->def, + caps, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE); + if (!devConfig) + goto endjob; + } - if (flags & VIR_DOMAIN_AFFECT_CONFIG && - flags & VIR_DOMAIN_AFFECT_LIVE) { - /* If we are affecting both CONFIG and LIVE - * create a deep copy of device as adding - * to CONFIG takes one instance. - */ - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); - if (!dev_copy) + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + devLive = virDomainDeviceDefParse(xml, vm->def, + caps, driver->xmlopt, 0); + if (!devLive) goto endjob; } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + if (priv->qemuCaps) qemuCaps = virObjectRef(priv->qemuCaps); else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator))) @@ -8386,20 +8385,21 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev, + if (virDomainDefCompatibleDevice(vmdef, devConfig, VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) goto endjob; - if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev)) < 0) + if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, devConfig)) < 0) goto endjob; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, + if (virDomainDefCompatibleDevice(vm->def, devLive, VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) goto endjob; - if ((ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force)) < 0) + if ((ret = qemuDomainUpdateDeviceLive(dom->conn, vm, devLive, + dom, force)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be @@ -8427,9 +8427,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, cleanup: virObjectUnref(qemuCaps); virDomainDefFree(vmdef); - if (dev != dev_copy) - virDomainDeviceDefFree(dev_copy); - virDomainDeviceDefFree(dev); + virDomainDeviceDefFree(devConfig); + virDomainDeviceDefFree(devLive); virDomainObjEndAPI(&vm); virObjectUnref(caps); virObjectUnref(cfg); -- 2.8.2

On Sat, May 14, 2016 at 07:27:36PM +0200, Pavel Hrdina wrote:
Commit 1ccc7fbf fixed port checking for live device update, which revealed another bug. If we updating live domain we need to parse the device also as live.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1336134
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
NACK, this isn't correct fix. The issue here is that we drop ports if autoport is set and flag VIR_DOMAIN_DEF_PARSE_INACTIVE is set. To fix this issue we have to revert 1ccc7fbf where I didn't realize this fact.
participants (1)
-
Pavel Hrdina