Unmerge attach/update/modify device APIs in drivers

The LXC, QEMU, and LibXL drivers have all merged their handling of
the attach/update/modify device APIs into one large

  'xxxxDomainModifyDeviceFlags'

which then does a 'switch()' based on the actual API being invoked.
While this saves some lines of code, it is not really all that
significant in the context of the driver API impls as a whole.

This merger of the handling of different APIs creates pain when
wanting to automated analysis of the code and do things which
are specific to individual APIs. The slight duplication of code
from unmerged the API impls, is preferrable to allow for easier
automated analysis.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

src/libxl/libxl_driver.c

@@ -3539,16 +3539,10 @@ cleanup:
     return ret;
 }
 
-/* Actions for libxlDomainModifyDeviceFlags */
-enum {
-    LIBXL_DEVICE_ATTACH,
-    LIBXL_DEVICE_DETACH,
-    LIBXL_DEVICE_UPDATE,
-};
 
 static int
-libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
+libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
-                             unsigned int flags, int action)
+                             unsigned int flags)
 {
     libxlDriverPrivatePtr driver = dom->conn->privateData;
     virDomainObjPtr vm = NULL;
@@ -3601,23 +3595,11 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
                                                     driver->xmlopt)))
             goto cleanup;
 
-        switch (action) {
+        if ((ret = libxlDomainAttachDeviceConfig(vmdef, dev)) < 0)
-            case LIBXL_DEVICE_ATTACH:
+            goto cleanup;
-                ret = libxlDomainAttachDeviceConfig(vmdef, dev);
+    } else {
-                break;
-            case LIBXL_DEVICE_DETACH:
-                ret = libxlDomainDetachDeviceConfig(vmdef, dev);
-                break;
-            case LIBXL_DEVICE_UPDATE:
-                ret = libxlDomainUpdateDeviceConfig(vmdef, dev);
-                break;
-            default:
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("unknown domain modify action %d"), action);
-                break;
-        }
-    } else
         ret = 0;
+    }
 
     if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
         /* If dev exists it was created to modify the domain config. Free it. */
@@ -3627,21 +3609,9 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
                                             VIR_DOMAIN_XML_INACTIVE)))
             goto cleanup;
 
-        switch (action) {
+        if ((ret = libxlDomainAttachDeviceLive(priv, vm, dev)) < 0)
-            case LIBXL_DEVICE_ATTACH:
+            goto cleanup;
-                ret = libxlDomainAttachDeviceLive(priv, vm, dev);
+
-                break;
-            case LIBXL_DEVICE_DETACH:
-                ret = libxlDomainDetachDeviceLive(priv, vm, dev);
-                break;
-            case LIBXL_DEVICE_UPDATE:
-                ret = libxlDomainUpdateDeviceLive(priv, vm, dev);
-                break;
-            default:
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("unknown domain modify action %d"), action);
-                break;
-        }
         /*
          * update domain status forcibly because the domain status may be
          * changed even if we attach the device failed.
@@ -3668,13 +3638,6 @@ cleanup:
     return ret;
 }
 
-static int
-libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
-                             unsigned int flags)
-{
-    return libxlDomainModifyDeviceFlags(dom, xml, flags, LIBXL_DEVICE_ATTACH);
-}
-
 static int
 libxlDomainAttachDevice(virDomainPtr dom, const char *xml)
 {
@@ -3686,7 +3649,98 @@ static int
 libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
                              unsigned int flags)
 {
-    return libxlDomainModifyDeviceFlags(dom, xml, flags, LIBXL_DEVICE_DETACH);
+    libxlDriverPrivatePtr driver = dom->conn->privateData;
+    virDomainObjPtr vm = NULL;
+    virDomainDefPtr vmdef = NULL;
+    virDomainDeviceDefPtr dev = NULL;
+    libxlDomainObjPrivatePtr priv;
+    int ret = -1;
+
+    virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+                  VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
+
+    libxlDriverLock(driver);
+    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
+
+    if (!vm) {
+        virReportError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid"));
+        goto cleanup;
+    }
+
+    if (virDomainObjIsActive(vm)) {
+        if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)
+            flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+    } else {
+        if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)
+            flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
+        /* check consistency between flags and the vm state */
+        if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+            virReportError(VIR_ERR_OPERATION_INVALID,
+                           "%s", _("Domain is not running"));
+            goto cleanup;
+        }
+    }
+
+    if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) {
+         virReportError(VIR_ERR_OPERATION_INVALID,
+                        "%s", _("cannot modify device on transient domain"));
+         goto cleanup;
+    }
+
+    priv = vm->privateData;
+
+    if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
+        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
+                                            driver->caps, driver->xmlopt,
+                                            VIR_DOMAIN_XML_INACTIVE)))
+            goto cleanup;
+
+        /* Make a copy for updated domain. */
+        if (!(vmdef = virDomainObjCopyPersistentDef(vm, driver->caps,
+                                                    driver->xmlopt)))
+            goto cleanup;
+
+        if ((ret = libxlDomainDetachDeviceConfig(vmdef, dev)) < 0)
+            goto cleanup;
+    } else {
+        ret = 0;
+    }
+
+    if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+        /* If dev exists it was created to modify the domain config. Free it. */
+        virDomainDeviceDefFree(dev);
+        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
+                                            driver->caps, driver->xmlopt,
+                                            VIR_DOMAIN_XML_INACTIVE)))
+            goto cleanup;
+
+        if ((ret = libxlDomainDetachDeviceLive(priv, vm, dev)) < 0)
+            goto cleanup;
+
+        /*
+         * update domain status forcibly because the domain status may be
+         * changed even if we attach the device failed.
+         */
+        if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0)
+            ret = -1;
+    }
+
+    /* Finally, if no error until here, we can save config. */
+    if (!ret && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
+        ret = virDomainSaveConfig(driver->configDir, vmdef);
+        if (!ret) {
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
+            vmdef = NULL;
+        }
+    }
+
+cleanup:
+    virDomainDefFree(vmdef);
+    virDomainDeviceDefFree(dev);
+    if (vm)
+        virObjectUnlock(vm);
+    libxlDriverUnlock(driver);
+    return ret;
 }
 
 static int
@@ -3700,7 +3754,98 @@ static int
 libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
                              unsigned int flags)
 {
-    return libxlDomainModifyDeviceFlags(dom, xml, flags, LIBXL_DEVICE_UPDATE);
+    libxlDriverPrivatePtr driver = dom->conn->privateData;
+    virDomainObjPtr vm = NULL;
+    virDomainDefPtr vmdef = NULL;
+    virDomainDeviceDefPtr dev = NULL;
+    libxlDomainObjPrivatePtr priv;
+    int ret = -1;
+
+    virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+                  VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
+
+    libxlDriverLock(driver);
+    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
+
+    if (!vm) {
+        virReportError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid"));
+        goto cleanup;
+    }
+
+    if (virDomainObjIsActive(vm)) {
+        if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)
+            flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+    } else {
+        if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)
+            flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
+        /* check consistency between flags and the vm state */
+        if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+            virReportError(VIR_ERR_OPERATION_INVALID,
+                           "%s", _("Domain is not running"));
+            goto cleanup;
+        }
+    }
+
+    if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) {
+         virReportError(VIR_ERR_OPERATION_INVALID,
+                        "%s", _("cannot modify device on transient domain"));
+         goto cleanup;
+    }
+
+    priv = vm->privateData;
+
+    if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
+        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
+                                            driver->caps, driver->xmlopt,
+                                            VIR_DOMAIN_XML_INACTIVE)))
+            goto cleanup;
+
+        /* Make a copy for updated domain. */
+        if (!(vmdef = virDomainObjCopyPersistentDef(vm, driver->caps,
+                                                    driver->xmlopt)))
+            goto cleanup;
+
+        if ((ret = libxlDomainUpdateDeviceConfig(vmdef, dev)) < 0)
+            goto cleanup;
+    } else {
+        ret = 0;
+    }
+
+    if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+        /* If dev exists it was created to modify the domain config. Free it. */
+        virDomainDeviceDefFree(dev);
+        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
+                                            driver->caps, driver->xmlopt,
+                                            VIR_DOMAIN_XML_INACTIVE)))
+            goto cleanup;
+
+        if ((ret = libxlDomainUpdateDeviceLive(priv, vm, dev)) < 0)
+            goto cleanup;
+
+        /*
+         * update domain status forcibly because the domain status may be
+         * changed even if we attach the device failed.
+         */
+        if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0)
+            ret = -1;
+    }
+
+    /* Finally, if no error until here, we can save config. */
+    if (!ret && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
+        ret = virDomainSaveConfig(driver->configDir, vmdef);
+        if (!ret) {
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
+            vmdef = NULL;
+        }
+    }
+
+cleanup:
+    virDomainDefFree(vmdef);
+    virDomainDeviceDefFree(dev);
+    if (vm)
+        virObjectUnlock(vm);
+    libxlDriverUnlock(driver);
+    return ret;
 }
 
 static unsigned long long

src/lxc/lxc_driver.c

@@ -4131,17 +4131,9 @@ lxcDomainDetachDeviceLive(virLXCDriverPtr driver,
 }
 
 
-/* Actions for lxcDomainModifyDeviceFlags */
+static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
-enum {
+                                      const char *xml,
-    LXC_DEVICE_ATTACH,
+                                      unsigned int flags)
-    LXC_DEVICE_UPDATE,
-    LXC_DEVICE_DETACH,
-};
-
-
-static int
-lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
-                           unsigned int flags, int action)
 {
     virLXCDriverPtr driver = dom->conn->privateData;
     virDomainObjPtr vm = NULL;
@@ -4151,9 +4143,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
     unsigned int affect;
 
     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
-                  VIR_DOMAIN_AFFECT_CONFIG |
+                  VIR_DOMAIN_AFFECT_CONFIG, -1);
-                  (action == LXC_DEVICE_UPDATE ?
-                   VIR_DOMAIN_DEVICE_MODIFY_FORCE : 0), -1);
 
     affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
 
@@ -4215,23 +4205,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
         vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt);
         if (!vmdef)
             goto cleanup;
-        switch (action) {
+        if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)
-        case LXC_DEVICE_ATTACH:
-            ret = lxcDomainAttachDeviceConfig(vmdef, dev);
-            break;
-        case LXC_DEVICE_DETACH:
-            ret = lxcDomainDetachDeviceConfig(vmdef, dev);
-            break;
-        case LXC_DEVICE_UPDATE:
-            ret = lxcDomainUpdateDeviceConfig(vmdef, dev);
-            break;
-        default:
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unknown domain modify action %d"), action);
-            break;
-        }
-
-        if (ret == -1)
             goto cleanup;
     }
 
@@ -4239,21 +4213,7 @@ lxcDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
         if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
             goto cleanup;
 
-        switch (action) {
+        if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) < 0)
-        case LXC_DEVICE_ATTACH:
-            ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy);
-            break;
-        case LXC_DEVICE_DETACH:
-            ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy);
-            break;
-        default:
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unknown domain modify action %d"), action);
-            ret = -1;
-            break;
-        }
-
-        if (ret == -1)
             goto cleanup;
         /*
          * update domain status forcibly because the domain status may be
@@ -4287,15 +4247,6 @@ cleanup:
 }
 
 
-static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
-                                      const char *xml,
-                                      unsigned int flags)
-{
-    return lxcDomainModifyDeviceFlags(dom, xml, flags,
-                                       LXC_DEVICE_ATTACH);
-}
-
-
 static int lxcDomainAttachDevice(virDomainPtr dom,
                                  const char *xml)
 {
@@ -4308,8 +4259,109 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
                                       const char *xml,
                                       unsigned int flags)
 {
-    return lxcDomainModifyDeviceFlags(dom, xml, flags,
+    virLXCDriverPtr driver = dom->conn->privateData;
-                                       LXC_DEVICE_UPDATE);
+    virDomainObjPtr vm = NULL;
+    virDomainDefPtr vmdef = NULL;
+    virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
+    int ret = -1;
+    unsigned int affect;
+
+    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+                  VIR_DOMAIN_AFFECT_CONFIG |
+                  VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
+
+    affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
+
+    lxcDriverLock(driver);
+    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
+
+    if (!vm) {
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+        virUUIDFormat(dom->uuid, uuidstr);
+        virReportError(VIR_ERR_NO_DOMAIN,
+                       _("no domain with matching uuid '%s'"), uuidstr);
+        goto cleanup;
+    }
+
+    if (virDomainObjIsActive(vm)) {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_LIVE;
+    } else {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_CONFIG;
+        /* check consistency between flags and the vm state */
+        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                           _("cannot do live update a device on "
+                             "inactive domain"));
+            goto cleanup;
+        }
+    }
+
+    if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
+         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                        _("cannot modify device on transient domain"));
+         goto cleanup;
+    }
+
+    dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
+                                             driver->caps, driver->xmlopt,
+                                             VIR_DOMAIN_XML_INACTIVE);
+    if (dev == NULL)
+        goto cleanup;
+
+    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,
+                                          driver->caps, driver->xmlopt);
+        if (!dev_copy)
+            goto cleanup;
+    }
+
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        if (virDomainDefCompatibleDevice(vm->def, dev) < 0)
+            goto cleanup;
+
+        /* Make a copy for updated domain. */
+        vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt);
+        if (!vmdef)
+            goto cleanup;
+        if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0)
+            goto cleanup;
+    }
+
+    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+        if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
+            goto cleanup;
+
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("Unable to modify live devices"));
+
+        goto cleanup;
+    }
+
+    /* Finally, if no error until here, we can save config. */
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        ret = virDomainSaveConfig(driver->configDir, vmdef);
+        if (!ret) {
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
+            vmdef = NULL;
+        }
+    }
+
+cleanup:
+    virDomainDefFree(vmdef);
+    if (dev != dev_copy)
+        virDomainDeviceDefFree(dev_copy);
+    virDomainDeviceDefFree(dev);
+    if (vm)
+        virObjectUnlock(vm);
+    lxcDriverUnlock(driver);
+    return ret;
 }
 
 
@@ -4317,8 +4369,116 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
                                       const char *xml,
                                       unsigned int flags)
 {
-    return lxcDomainModifyDeviceFlags(dom, xml, flags,
+    virLXCDriverPtr driver = dom->conn->privateData;
-                                      LXC_DEVICE_DETACH);
+    virDomainObjPtr vm = NULL;
+    virDomainDefPtr vmdef = NULL;
+    virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
+    int ret = -1;
+    unsigned int affect;
+
+    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+                  VIR_DOMAIN_AFFECT_CONFIG, -1);
+
+    affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
+
+    lxcDriverLock(driver);
+    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
+
+    if (!vm) {
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+        virUUIDFormat(dom->uuid, uuidstr);
+        virReportError(VIR_ERR_NO_DOMAIN,
+                       _("no domain with matching uuid '%s'"), uuidstr);
+        goto cleanup;
+    }
+
+    if (virDomainObjIsActive(vm)) {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_LIVE;
+    } else {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_CONFIG;
+        /* check consistency between flags and the vm state */
+        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                           _("cannot do live update a device on "
+                             "inactive domain"));
+            goto cleanup;
+        }
+    }
+
+    if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
+         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                        _("cannot modify device on transient domain"));
+         goto cleanup;
+    }
+
+    dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
+                                             driver->caps, driver->xmlopt,
+                                             VIR_DOMAIN_XML_INACTIVE);
+    if (dev == NULL)
+        goto cleanup;
+
+    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,
+                                          driver->caps, driver->xmlopt);
+        if (!dev_copy)
+            goto cleanup;
+    }
+
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        if (virDomainDefCompatibleDevice(vm->def, dev) < 0)
+            goto cleanup;
+
+        /* Make a copy for updated domain. */
+        vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt);
+        if (!vmdef)
+            goto cleanup;
+
+        if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0)
+            goto cleanup;
+    }
+
+    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+        if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
+            goto cleanup;
+
+        if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy)) < 0)
+            goto cleanup;
+        /*
+         * update domain status forcibly because the domain status may be
+         * changed even if we failed to attach the device. For example,
+         * a new controller may be created.
+         */
+        if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) {
+            ret = -1;
+            goto cleanup;
+        }
+    }
+
+    /* Finally, if no error until here, we can save config. */
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        ret = virDomainSaveConfig(driver->configDir, vmdef);
+        if (!ret) {
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
+            vmdef = NULL;
+        }
+    }
+
+cleanup:
+    virDomainDefFree(vmdef);
+    if (dev != dev_copy)
+        virDomainDeviceDefFree(dev_copy);
+    virDomainDeviceDefFree(dev);
+    if (vm)
+        virObjectUnlock(vm);
+    lxcDriverUnlock(driver);
+    return ret;
 }
 
 

src/qemu/qemu_driver.c

@@ -6339,23 +6339,14 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
     return 0;
 }
 
-/* Actions for qemuDomainModifyDeviceFlags */
-enum {
-    QEMU_DEVICE_ATTACH,
-    QEMU_DEVICE_DETACH,
-    QEMU_DEVICE_UPDATE,
-};
 
-
+static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
-static int
+                                       unsigned int flags)
-qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
-                            unsigned int flags, int action)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     virDomainObjPtr vm = NULL;
     virDomainDefPtr vmdef = NULL;
     virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
-    bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
     int ret = -1;
     unsigned int affect;
     virQEMUCapsPtr qemuCaps = NULL;
@@ -6364,9 +6355,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
     virCapsPtr caps = NULL;
 
     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
-                  VIR_DOMAIN_AFFECT_CONFIG |
+                  VIR_DOMAIN_AFFECT_CONFIG, -1);
-                  (action == QEMU_DEVICE_UPDATE ?
-                   VIR_DOMAIN_DEVICE_MODIFY_FORCE : 0), -1);
 
     cfg = virQEMUDriverGetConfig(driver);
 
@@ -6434,23 +6423,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
         vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
         if (!vmdef)
             goto endjob;
-        switch (action) {
+        if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0)
-        case QEMU_DEVICE_ATTACH:
-            ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev);
-            break;
-        case QEMU_DEVICE_DETACH:
-            ret = qemuDomainDetachDeviceConfig(vmdef, dev);
-            break;
-        case QEMU_DEVICE_UPDATE:
-            ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev);
-            break;
-        default:
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unknown domain modify action %d"), action);
-            break;
-        }
-
-        if (ret == -1)
             goto endjob;
     }
 
@@ -6458,24 +6431,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
         if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
             goto endjob;
 
-        switch (action) {
+        if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0)
-        case QEMU_DEVICE_ATTACH:
-            ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom);
-            break;
-        case QEMU_DEVICE_DETACH:
-            ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom);
-            break;
-        case QEMU_DEVICE_UPDATE:
-            ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force);
-            break;
-        default:
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unknown domain modify action %d"), action);
-            ret = -1;
-            break;
-        }
-
-        if (ret == -1)
             goto endjob;
         /*
          * update domain status forcibly because the domain status may be
@@ -6514,12 +6470,6 @@ cleanup:
     return ret;
 }
 
-static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
-                                       unsigned int flags)
-{
-    return qemuDomainModifyDeviceFlags(dom, xml, flags, QEMU_DEVICE_ATTACH);
-}
-
 static int qemuDomainAttachDevice(virDomainPtr dom, const char *xml)
 {
     return qemuDomainAttachDeviceFlags(dom, xml,
@@ -6531,13 +6481,265 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
                                        const char *xml,
                                        unsigned int flags)
 {
-    return qemuDomainModifyDeviceFlags(dom, xml, flags, QEMU_DEVICE_UPDATE);
+    virQEMUDriverPtr driver = dom->conn->privateData;
+    virDomainObjPtr vm = NULL;
+    virDomainDefPtr vmdef = NULL;
+    virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
+    bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
+    int ret = -1;
+    unsigned int affect;
+    virQEMUCapsPtr qemuCaps = NULL;
+    qemuDomainObjPrivatePtr priv;
+    virQEMUDriverConfigPtr cfg = NULL;
+    virCapsPtr caps = NULL;
+
+    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+                  VIR_DOMAIN_AFFECT_CONFIG |
+                  VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
+
+    cfg = virQEMUDriverGetConfig(driver);
+
+    affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
+
+    if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
+        goto cleanup;
+
+    if (!(vm = qemuDomObjFromDomain(dom)))
+        goto cleanup;
+
+    priv = vm->privateData;
+
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+        goto cleanup;
+
+    if (virDomainObjIsActive(vm)) {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_LIVE;
+    } else {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_CONFIG;
+        /* check consistency between flags and the vm state */
+        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                           _("cannot do live update a device on "
+                             "inactive domain"));
+            goto endjob;
+        }
+    }
+
+    if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
+         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                        _("cannot modify device on transient domain"));
+         goto endjob;
+    }
+
+    dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
+                                             caps, driver->xmlopt,
+                                             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(dev, vm->def, caps, driver->xmlopt);
+        if (!dev_copy)
+            goto endjob;
+    }
+
+    if (priv->qemuCaps)
+        qemuCaps = virObjectRef(priv->qemuCaps);
+    else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator)))
+        goto cleanup;
+
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        if (virDomainDefCompatibleDevice(vm->def, dev) < 0)
+            goto endjob;
+
+        /* Make a copy for updated domain. */
+        vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
+        if (!vmdef)
+            goto endjob;
+
+        if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev)) < 0)
+            goto endjob;
+    }
+
+    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+        if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
+            goto endjob;
+
+        if ((ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force)) < 0)
+            goto endjob;
+        /*
+         * update domain status forcibly because the domain status may be
+         * changed even if we failed to attach the device. For example,
+         * a new controller may be created.
+         */
+        if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
+            ret = -1;
+            goto endjob;
+        }
+    }
+
+    /* Finally, if no error until here, we can save config. */
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        ret = virDomainSaveConfig(cfg->configDir, vmdef);
+        if (!ret) {
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
+            vmdef = NULL;
+        }
+    }
+
+endjob:
+    if (qemuDomainObjEndJob(driver, vm) == 0)
+        vm = NULL;
+
+cleanup:
+    virObjectUnref(qemuCaps);
+    virDomainDefFree(vmdef);
+    if (dev != dev_copy)
+        virDomainDeviceDefFree(dev_copy);
+    virDomainDeviceDefFree(dev);
+    if (vm)
+        virObjectUnlock(vm);
+    virObjectUnref(caps);
+    virObjectUnref(cfg);
+    return ret;
 }
 
+
 static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
                                        unsigned int flags)
 {
-    return qemuDomainModifyDeviceFlags(dom, xml, flags, QEMU_DEVICE_DETACH);
+    virQEMUDriverPtr driver = dom->conn->privateData;
+    virDomainObjPtr vm = NULL;
+    virDomainDefPtr vmdef = NULL;
+    virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
+    int ret = -1;
+    unsigned int affect;
+    virQEMUCapsPtr qemuCaps = NULL;
+    qemuDomainObjPrivatePtr priv;
+    virQEMUDriverConfigPtr cfg = NULL;
+    virCapsPtr caps = NULL;
+
+    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+                  VIR_DOMAIN_AFFECT_CONFIG, -1);
+
+    cfg = virQEMUDriverGetConfig(driver);
+
+    affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
+
+    if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
+        goto cleanup;
+
+    if (!(vm = qemuDomObjFromDomain(dom)))
+        goto cleanup;
+
+    priv = vm->privateData;
+
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+        goto cleanup;
+
+    if (virDomainObjIsActive(vm)) {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_LIVE;
+    } else {
+        if (affect == VIR_DOMAIN_AFFECT_CURRENT)
+            flags |= VIR_DOMAIN_AFFECT_CONFIG;
+        /* check consistency between flags and the vm state */
+        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                           _("cannot do live update a device on "
+                             "inactive domain"));
+            goto endjob;
+        }
+    }
+
+    if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
+         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                        _("cannot modify device on transient domain"));
+         goto endjob;
+    }
+
+    dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
+                                             caps, driver->xmlopt,
+                                             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(dev, vm->def, caps, driver->xmlopt);
+        if (!dev_copy)
+            goto endjob;
+    }
+
+    if (priv->qemuCaps)
+        qemuCaps = virObjectRef(priv->qemuCaps);
+    else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator)))
+        goto cleanup;
+
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        if (virDomainDefCompatibleDevice(vm->def, dev) < 0)
+            goto endjob;
+
+        /* Make a copy for updated domain. */
+        vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
+        if (!vmdef)
+            goto endjob;
+        if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev)) < 0)
+            goto endjob;
+    }
+
+    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+        if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
+            goto endjob;
+
+        if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0)
+            goto endjob;
+        /*
+         * update domain status forcibly because the domain status may be
+         * changed even if we failed to attach the device. For example,
+         * a new controller may be created.
+         */
+        if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
+            ret = -1;
+            goto endjob;
+        }
+    }
+
+    /* Finally, if no error until here, we can save config. */
+    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+        ret = virDomainSaveConfig(cfg->configDir, vmdef);
+        if (!ret) {
+            virDomainObjAssignDef(vm, vmdef, false, NULL);
+            vmdef = NULL;
+        }
+    }
+
+endjob:
+    if (qemuDomainObjEndJob(driver, vm) == 0)
+        vm = NULL;
+
+cleanup:
+    virObjectUnref(qemuCaps);
+    virDomainDefFree(vmdef);
+    if (dev != dev_copy)
+        virDomainDeviceDefFree(dev_copy);
+    virDomainDeviceDefFree(dev);
+    if (vm)
+        virObjectUnlock(vm);
+    virObjectUnref(caps);
+    virObjectUnref(cfg);
+    return ret;
 }
 
 static int qemuDomainDetachDevice(virDomainPtr dom, const char *xml)