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);
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list