qemu: Don't parse device twice in attach/detach

Some members 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.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 601dc8b525a871b3ab72e9e35f08201e0a331698..f421b72da58efe91dd6edf2901d9e527b79817ff 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14590,3 +14590,87 @@ virDomainNetFind(virDomainDefPtr def, const char *device)
 
     return net;
 }
+
+/**
+ * virDomainDeviceDefCopy:
+ * @caps: Capabilities
+ * @def: Domain definition to which @src belongs
+ * @src: source to be copied
+ *
+ * virDomainDeviceDefCopy does a deep copy of only the parts of a
+ * DeviceDef that are valid when just the flag VIR_DOMAIN_XML_INACTIVE is
+ * set. This means that any part of the device xml that is conditionally
+ * parsed/formatted based on some other flag being set (or on the INACTIVE
+ * flag being reset) *will not* be copied to the destination. Caveat emptor.
+ *
+ * Returns a pointer to copied @src or NULL in case of error.
+ */
+virDomainDeviceDefPtr
+virDomainDeviceDefCopy(virCapsPtr caps,
+                       const virDomainDefPtr def,
+                       virDomainDeviceDefPtr src)
+{
+    virDomainDeviceDefPtr ret = NULL;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    int flags = VIR_DOMAIN_XML_INACTIVE;
+    char *xmlStr = NULL;
+    int rc = -1;
+
+    switch(src->type) {
+    case VIR_DOMAIN_DEVICE_DISK:
+        rc = virDomainDiskDefFormat(&buf, src->data.disk, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_LEASE:
+        rc = virDomainLeaseDefFormat(&buf, src->data.lease);
+        break;
+    case VIR_DOMAIN_DEVICE_FS:
+        rc = virDomainFSDefFormat(&buf, src->data.fs, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_NET:
+        rc = virDomainNetDefFormat(&buf, src->data.net, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_INPUT:
+        rc = virDomainInputDefFormat(&buf, src->data.input, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_SOUND:
+        rc = virDomainSoundDefFormat(&buf, src->data.sound, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_VIDEO:
+        rc = virDomainVideoDefFormat(&buf, src->data.video, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_HOSTDEV:
+        rc = virDomainHostdevDefFormat(&buf, src->data.hostdev, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+        rc = virDomainWatchdogDefFormat(&buf, src->data.watchdog, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_CONTROLLER:
+        rc = virDomainControllerDefFormat(&buf, src->data.controller, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_GRAPHICS:
+        rc = virDomainGraphicsDefFormat(&buf, src->data.graphics, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_HUB:
+        rc = virDomainHubDefFormat(&buf, src->data.hub, flags);
+        break;
+    case VIR_DOMAIN_DEVICE_REDIRDEV:
+        rc = virDomainRedirdevDefFormat(&buf, src->data.redirdev, flags);
+        break;
+    default:
+        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+                             _("Copying definition of '%d' type "
+                               "is not implemented yet."),
+                             src->type);
+        goto cleanup;
+    }
+
+    if (rc < 0)
+        goto cleanup;
+
+    xmlStr = virBufferContentAndReset(&buf);
+    ret = virDomainDeviceDefParse(caps, def, xmlStr, flags);
+
+cleanup:
+    VIR_FREE(xmlStr);
+    return ret;
+}

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 23c1947822fd24a4913a965c9ec7980d79b7d1bd..6a2c28e46afc948ae2013d8301b7882bc8c8f696 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1794,6 +1794,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 1439d3b5a968f260cd82105b1419e27361aecef6..ff321ab1de60b6e5d41e2cf09686d5ee6b88b7db 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -287,6 +287,7 @@ virDomainDeviceAddressIsValid;
 virDomainDeviceAddressPciMultiTypeFromString;
 virDomainDeviceAddressPciMultiTypeToString;
 virDomainDeviceAddressTypeToString;
+virDomainDeviceDefCopy;
 virDomainDeviceDefFree;
 virDomainDeviceDefParse;
 virDomainDeviceInfoIterate;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 56896c714a6ee2e75b3417e89ffdfa40085e0d1e..d2c45446c3bb873a174018b243c2f82ed737dde9 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5567,7 +5567,7 @@ 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;
     int ret = -1;
     unsigned int affect;
@@ -5614,12 +5614,23 @@ 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;
+    }
 
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
         /* Make a copy for updated domain. */
         vmdef = virDomainObjCopyPersistentDef(driver->caps, vm);
         if (!vmdef)
@@ -5645,24 +5656,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,
@@ -5699,6 +5701,8 @@ endjob:
 
 cleanup:
     virDomainDefFree(vmdef);
+    if (dev != dev_copy)
+        virDomainDeviceDefFree(dev_copy);
     virDomainDeviceDefFree(dev);
     if (vm)
         virDomainObjUnlock(vm);