On 03/01/2012 01:56 PM, Michal Privoznik wrote:

Some nits are generated during XML parse (e.g. MAC address of an interface); However, with current implementation, if we are plugging a device both to persistent and live config, we parse given XML twice: first time for live, second for config. This is wrong then as the second time we are not guaranteed to generate same values as we did for the first time. To prevent that we need to create a copy of DeviceDefPtr; This is done through format/parse process instead of writing functions for deep copy as it is easier to maintain: adding new field to any virDomain*DefPtr doesn't require change of copying function. --- src/conf/domain_conf.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 40 +++++++++++-------- 4 files changed, 121 insertions(+), 17 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f9654f1..f001319 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14118,3 +14118,97 @@ virDomainNetFind(virDomainDefPtr def, const char *device)

return net; } + +#define VIR_DOMAIN_DEVICE_FORMAT(func) \ + if (func < 0) \ + goto cleanup; \ + xmlStr = virBufferContentAndReset(&buf); \ + ret = virDomainDeviceDefParse(caps, def, xmlStr, flags) + +virDomainDeviceDefPtr +virDomainDeviceDefCopy(virCapsPtr caps, + const virDomainDefPtr def, + virDomainDeviceDefPtr src) +{ + virDomainDeviceDefPtr ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + int flags = VIR_DOMAIN_XML_INACTIVE;

I'm concerned that this function has the appearance of being general purpose, but actually isn't. There are many bits of data in virDomainNetDef that won't be copied, because they're only parsed/formatted if extra flags are given in the calls to Parse and Format (VIR_DOMAIN_XML_INTERNAL_STATUS, VIR_DOMAIN_XML_INTERNAL_STATUS_ACTUAL_NET, etc). For the current use case, it should work adequately, since we know that we just created the object by parsing with a limited flag set, but I can imagine the day 6 months from now when somebody who doesn't know the history/limitations of ths new function finds it and mistakenly believes it's the answer to their current (probably very different) problem (and then commits code with a bug that doesn't show up until it's in the field) It's probably too much trouble in the short term to try and make this function completely general purpose, but I think at least the function name should reflect the limited nature and/or both the declaration and definition of the function should be accompanied by a comment noting the limitations. Beyond that, this looks like the right way to fix the problem. Note that libxl_driver.c:libxmlDomainModifyDeviceFlags() has exactly the same problem, so maybe its fix can be included in this patch.

+ char *xmlStr = NULL; + + switch(src->type) { + case VIR_DOMAIN_DEVICE_DISK: + VIR_DOMAIN_DEVICE_FORMAT(virDomainDiskDefFormat(&buf, + src->data.disk, + flags)); + break; + case VIR_DOMAIN_DEVICE_LEASE: + VIR_DOMAIN_DEVICE_FORMAT(virDomainLeaseDefFormat(&buf, + src->data.lease)); + break; + case VIR_DOMAIN_DEVICE_FS: + VIR_DOMAIN_DEVICE_FORMAT(virDomainFSDefFormat(&buf, + src->data.fs, + flags)); + break; + case VIR_DOMAIN_DEVICE_NET: + VIR_DOMAIN_DEVICE_FORMAT(virDomainNetDefFormat(&buf, + src->data.net, + flags)); + break; + case VIR_DOMAIN_DEVICE_INPUT: + VIR_DOMAIN_DEVICE_FORMAT(virDomainInputDefFormat(&buf, + src->data.input, + flags)); + break; + case VIR_DOMAIN_DEVICE_SOUND: + VIR_DOMAIN_DEVICE_FORMAT(virDomainSoundDefFormat(&buf, + src->data.sound, + flags)); + break; + case VIR_DOMAIN_DEVICE_VIDEO: + VIR_DOMAIN_DEVICE_FORMAT(virDomainVideoDefFormat(&buf, + src->data.video, + flags)); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + VIR_DOMAIN_DEVICE_FORMAT(virDomainHostdevDefFormat(&buf, + src->data.hostdev, + flags)); + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + VIR_DOMAIN_DEVICE_FORMAT(virDomainWatchdogDefFormat(&buf, + src->data.watchdog, + flags)); + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + VIR_DOMAIN_DEVICE_FORMAT(virDomainControllerDefFormat(&buf, + src->data.controller, + flags)); + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: + VIR_DOMAIN_DEVICE_FORMAT(virDomainGraphicsDefFormat(&buf, + src->data.graphics, + flags)); + break; + case VIR_DOMAIN_DEVICE_HUB: + VIR_DOMAIN_DEVICE_FORMAT(virDomainHubDefFormat(&buf, + src->data.hub, + flags)); + break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + VIR_DOMAIN_DEVICE_FORMAT(virDomainRedirdevDefFormat(&buf, + src->data.redirdev, + flags)); + break; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Copying definition of '%d' type " + "is not implemented yet."), + src->type); + break; + } + +cleanup: + VIR_FREE(xmlStr); + return ret; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 596be4d..7e4a464 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1750,6 +1750,9 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); +virDomainDeviceDefPtr virDomainDeviceDefCopy(virCapsPtr caps, + const virDomainDefPtr def, + virDomainDeviceDefPtr src); int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a104e70..53f27e7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -286,6 +286,7 @@ virDomainDeviceAddressIsValid; virDomainDeviceAddressPciMultiTypeFromString; virDomainDeviceAddressPciMultiTypeToString; virDomainDeviceAddressTypeToString; +virDomainDeviceDefCopy; virDomainDeviceDefFree; virDomainDeviceDefParse; virDomainDeviceInfoIterate; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c6bdd29..3a52ded 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5561,8 +5561,9 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; - virDomainDeviceDefPtr dev = NULL; + virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; + bool free_dev_copy = false; int ret = -1; unsigned int affect;

@@ -5608,12 +5609,24 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, goto endjob; }

- if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - dev = virDomainDeviceDefParse(driver->caps, vm->def, xml, - VIR_DOMAIN_XML_INACTIVE); - if (dev == NULL) + dev = dev_copy = virDomainDeviceDefParse(driver->caps, vm->def, xml, + VIR_DOMAIN_XML_INACTIVE); + if (dev == NULL) + 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(driver->caps, vm->def, dev); + if (!dev_copy) goto endjob; + free_dev_copy = true; + }

+ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(driver->caps, vm); if (!vmdef) @@ -5639,24 +5652,15 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, }

if (flags & VIR_DOMAIN_AFFECT_LIVE) { - /* If dev exists it was created to modify the domain config. Free it. */ - virDomainDeviceDefFree(dev); - dev = virDomainDeviceDefParse(driver->caps, vm->def, xml, - VIR_DOMAIN_XML_INACTIVE); - if (dev == NULL) { - ret = -1; - goto endjob; - } - switch (action) { case QEMU_DEVICE_ATTACH: - ret = qemuDomainAttachDeviceLive(vm, dev, dom); + ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom); break; case QEMU_DEVICE_DETACH: - ret = qemuDomainDetachDeviceLive(vm, dev, dom); + ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom); break; case QEMU_DEVICE_UPDATE: - ret = qemuDomainUpdateDeviceLive(vm, dev, dom, force); + ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force); break; default: qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -5694,6 +5698,8 @@ endjob: cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); + if (free_dev_copy) + virDomainDeviceDefFree(dev_copy); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver);