libxl / lxc / qemu drivers share some common codes in their
DomainDetachDeviceConfig functions, so extract them to domain_driver and
reuse them.
At the same time, this will enable test driver to test these functions
with virshtest in the future.
Signed-off-by: Luke Yue <lukedyue@gmail.com>
---
Not pretty sure whether this is a proper way to make these functions
reusable, maybe there is a more elegant choice.
---
src/hypervisor/domain_driver.c | 266 +++++++++++++++++++++++++++++++++
src/hypervisor/domain_driver.h | 41 +++++
src/libvirt_private.syms | 14 ++
src/libxl/libxl_driver.c | 41 +----
src/lxc/lxc_driver.c | 37 +----
src/qemu/qemu_driver.c | 123 +++------------
6 files changed, 356 insertions(+), 166 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
index 31737b0f4a..01ecb4e30e 100644
--- a/src/hypervisor/domain_driver.c
+++ b/src/hypervisor/domain_driver.c
@@ -644,3 +644,269 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
return ret;
}
+
+
+int
+virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainDiskDef *disk;
+ virDomainDiskDef *det_disk;
+
+ disk = dev->data.disk;
+ if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
+ virReportError(VIR_ERR_DEVICE_MISSING, _("no target device %s"),
+ disk->dst);
+ return -1;
+ }
+ virDomainDiskDefFree(det_disk);
+
+ return 0;
+}
+
+int
+virDomainDriverDetachNetDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainNetDef *net;
+ int idx;
+
+ net = dev->data.net;
+ if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ return -1;
+
+ /* this is guaranteed to succeed */
+ virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachSoundDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainSoundDef *sound;
+ int idx;
+
+ sound = dev->data.sound;
+ if ((idx = virDomainSoundDefFind(vmdef, sound)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("device not present in domain configuration"));
+ return -1;
+ }
+ virDomainSoundDefFree(virDomainSoundDefRemove(vmdef, idx));
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachHostdevDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainHostdevDef *hostdev;
+ virDomainHostdevDef *det_hostdev;
+ int idx;
+
+ hostdev = dev->data.hostdev;
+ if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("device not present in domain configuration"));
+ return -1;
+ }
+ virDomainHostdevRemove(vmdef, idx);
+ virDomainHostdevDefFree(det_hostdev);
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachLeaseDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainLeaseDef *lease;
+ virDomainLeaseDef *det_lease;
+
+ lease = dev->data.lease;
+ if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) {
+ virReportError(VIR_ERR_DEVICE_MISSING,
+ _("Lease %s in lockspace %s does not exist"),
+ lease->key, NULLSTR(lease->lockspace));
+ return -1;
+ }
+ virDomainLeaseDefFree(det_lease);
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachControllerDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainControllerDef *cont;
+ virDomainControllerDef *det_cont;
+ int idx;
+
+ cont = dev->data.controller;
+ if ((idx = virDomainControllerFind(vmdef, cont->type,
+ cont->idx)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("device not present in domain configuration"));
+ return -1;
+ }
+ det_cont = virDomainControllerRemove(vmdef, idx);
+ virDomainControllerDefFree(det_cont);
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachFSDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainFSDef *fs;
+ int idx;
+
+ fs = dev->data.fs;
+ idx = virDomainFSIndexByName(vmdef, fs->dst);
+ if (idx < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("no matching filesystem device was found"));
+ return -1;
+ }
+
+ fs = virDomainFSRemove(vmdef, idx);
+ virDomainFSDefFree(fs);
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachRNGDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ int idx;
+
+ if ((idx = virDomainRNGFind(vmdef, dev->data.rng)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("no matching RNG device was found"));
+ return -1;
+ }
+
+ virDomainRNGDefFree(virDomainRNGRemove(vmdef, idx));
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachMemoryDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ virDomainMemoryDef *mem;
+ int idx;
+
+ if ((idx = virDomainMemoryFindInactiveByDef(vmdef,
+ dev->data.memory)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("matching memory device was not found"));
+ return -1;
+ }
+ mem = virDomainMemoryRemove(vmdef, idx);
+ vmdef->mem.cur_balloon -= mem->size;
+ virDomainMemoryDefFree(mem);
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachRedirdevDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ int idx;
+
+ if ((idx = virDomainRedirdevDefFind(vmdef,
+ dev->data.redirdev)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("no matching redirdev was not found"));
+ return -1;
+ }
+
+ virDomainRedirdevDefFree(virDomainRedirdevDefRemove(vmdef, idx));
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachShmemDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ int idx;
+
+ if ((idx = virDomainShmemDefFind(vmdef, dev->data.shmem)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("matching shmem device was not found"));
+ return -1;
+ }
+
+ virDomainShmemDefFree(virDomainShmemDefRemove(vmdef, idx));
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachWatchdogDeviceConfig(virDomainDef *vmdef)
+{
+ if (!vmdef->watchdog) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("domain has no watchdog"));
+ return -1;
+ }
+ virDomainWatchdogDefFree(vmdef->watchdog);
+ vmdef->watchdog = NULL;
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachInputDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ int idx;
+
+ if ((idx = virDomainInputDefFind(vmdef, dev->data.input)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+ _("matching input device not found"));
+ return -1;
+ }
+
+ virDomainInputDefFree(virDomainInputDefRemove(vmdef, idx));
+
+ return 0;
+}
+
+
+int
+virDomainDriverDetachVsockDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev)
+{
+ if (!vmdef->vsock ||
+ !virDomainVsockDefEquals(dev->data.vsock, vmdef->vsock)) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("matching vsock device not found"));
+ return -1;
+ }
+ virDomainVsockDefFree(vmdef->vsock);
+ vmdef->vsock = NULL;
+
+ return 0;
+}
diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h
index 7b0fbae2fd..e7fbd70d7b 100644
--- a/src/hypervisor/domain_driver.h
+++ b/src/hypervisor/domain_driver.h
@@ -70,3 +70,44 @@ int virDomainDriverDelIOThreadCheck(virDomainDef *def,
int virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
virDomainIOThreadInfoPtr **info,
unsigned int bitmap_size);
+
+int virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachNetDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachSoundDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachHostdevDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachLeaseDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachControllerDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachFSDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachRNGDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachMemoryDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachRedirdevDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachShmemDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachWatchdogDeviceConfig(virDomainDef *vmdef);
+
+int virDomainDriverDetachInputDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
+
+int virDomainDriverDetachVsockDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 000c9893f0..0252f7c9d6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1551,6 +1551,20 @@ virDomainCgroupSetupMemtune;
# hypervisor/domain_driver.h
virDomainDriverAddIOThreadCheck;
virDomainDriverDelIOThreadCheck;
+virDomainDriverDetachControllerDeviceConfig;
+virDomainDriverDetachDiskDeviceConfig;
+virDomainDriverDetachFSDeviceConfig;
+virDomainDriverDetachHostdevDeviceConfig;
+virDomainDriverDetachInputDeviceConfig;
+virDomainDriverDetachLeaseDeviceConfig;
+virDomainDriverDetachMemoryDeviceConfig;
+virDomainDriverDetachNetDeviceConfig;
+virDomainDriverDetachRedirdevDeviceConfig;
+virDomainDriverDetachRNGDeviceConfig;
+virDomainDriverDetachShmemDeviceConfig;
+virDomainDriverDetachSoundDeviceConfig;
+virDomainDriverDetachVsockDeviceConfig;
+virDomainDriverDetachWatchdogDeviceConfig;
virDomainDriverGenerateMachineName;
virDomainDriverGenerateRootHash;
virDomainDriverGetIOThreadsConfig;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 7ea157f9c4..1e3d40f448 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3912,56 +3912,29 @@ libxlDomainDetachDeviceLive(libxlDriverPrivate *driver,
static int
libxlDomainDetachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev)
{
- virDomainDiskDef *disk;
- virDomainDiskDef *detach;
- virDomainHostdevDef *hostdev;
- virDomainHostdevDef *det_hostdev;
- virDomainControllerDef *cont;
- virDomainControllerDef *det_cont;
- virDomainNetDef *net;
- int idx;
-
switch (dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- disk = dev->data.disk;
- if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("no target device %s"), disk->dst);
+ if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainDiskDefFree(detach);
+
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- cont = dev->data.controller;
- if ((idx = virDomainControllerFind(vmdef, cont->type,
- cont->idx)) < 0) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("device not present in domain configuration"));
+ if (virDomainDriverDetachControllerDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- det_cont = virDomainControllerRemove(vmdef, idx);
- virDomainControllerDefFree(det_cont);
+
break;
case VIR_DOMAIN_DEVICE_NET:
- net = dev->data.net;
- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0)
return -1;
- /* this is guaranteed to succeed */
- virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
break;
case VIR_DOMAIN_DEVICE_HOSTDEV: {
- hostdev = dev->data.hostdev;
- if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("device not present in domain configuration"));
+ if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainHostdevRemove(vmdef, idx);
- virDomainHostdevDefFree(det_hostdev);
+
break;
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e2720a6f89..485683fe3a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3129,56 +3129,33 @@ static int
lxcDomainDetachDeviceConfig(virDomainDef *vmdef,
virDomainDeviceDef *dev)
{
- int ret = -1;
- virDomainDiskDef *disk;
- virDomainDiskDef *det_disk;
- virDomainNetDef *net;
- virDomainHostdevDef *hostdev;
- virDomainHostdevDef *det_hostdev;
- int idx;
-
switch (dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- disk = dev->data.disk;
- if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("no target device %s"), disk->dst);
+ if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainDiskDefFree(det_disk);
- ret = 0;
+
break;
case VIR_DOMAIN_DEVICE_NET:
- net = dev->data.net;
- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0)
return -1;
- /* this is guaranteed to succeed */
- virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
- ret = 0;
break;
case VIR_DOMAIN_DEVICE_HOSTDEV: {
- hostdev = dev->data.hostdev;
- if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("device not present in domain configuration"));
+ if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainHostdevRemove(vmdef, idx);
- virDomainHostdevDefFree(det_hostdev);
- ret = 0;
+
break;
}
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("persistent detach of device is not supported"));
- break;
+ return -1;
}
- return ret;
+ return 0;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cf9407c358..d149cd22b2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7517,84 +7517,43 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef,
unsigned int parse_flags,
virDomainXMLOption *xmlopt)
{
- virDomainDiskDef *disk;
- virDomainDiskDef *det_disk;
- virDomainNetDef *net;
- virDomainSoundDef *sound;
- virDomainHostdevDef *hostdev;
- virDomainHostdevDef *det_hostdev;
- virDomainLeaseDef *lease;
- virDomainLeaseDef *det_lease;
- virDomainControllerDef *cont;
- virDomainControllerDef *det_cont;
virDomainChrDef *chr;
- virDomainFSDef *fs;
- virDomainMemoryDef *mem;
- int idx;
switch ((virDomainDeviceType)dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- disk = dev->data.disk;
- if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
- virReportError(VIR_ERR_DEVICE_MISSING,
- _("no target device %s"), disk->dst);
+ if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainDiskDefFree(det_disk);
+
break;
case VIR_DOMAIN_DEVICE_NET:
- net = dev->data.net;
- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0)
return -1;
- /* this is guaranteed to succeed */
- virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
break;
case VIR_DOMAIN_DEVICE_SOUND:
- sound = dev->data.sound;
- if ((idx = virDomainSoundDefFind(vmdef, sound)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("device not present in domain configuration"));
+ if (virDomainDriverDetachSoundDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainSoundDefFree(virDomainSoundDefRemove(vmdef, idx));
+
break;
case VIR_DOMAIN_DEVICE_HOSTDEV: {
- hostdev = dev->data.hostdev;
- if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("device not present in domain configuration"));
+ if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainHostdevRemove(vmdef, idx);
- virDomainHostdevDefFree(det_hostdev);
+
break;
}
case VIR_DOMAIN_DEVICE_LEASE:
- lease = dev->data.lease;
- if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) {
- virReportError(VIR_ERR_DEVICE_MISSING,
- _("Lease %s in lockspace %s does not exist"),
- lease->key, NULLSTR(lease->lockspace));
+ if (virDomainDriverDetachLeaseDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainLeaseDefFree(det_lease);
+
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- cont = dev->data.controller;
- if ((idx = virDomainControllerFind(vmdef, cont->type,
- cont->idx)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("device not present in domain configuration"));
+ if (virDomainDriverDetachControllerDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- det_cont = virDomainControllerRemove(vmdef, idx);
- virDomainControllerDefFree(det_cont);
break;
@@ -7606,91 +7565,51 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef,
break;
case VIR_DOMAIN_DEVICE_FS:
- fs = dev->data.fs;
- idx = virDomainFSIndexByName(vmdef, fs->dst);
- if (idx < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("no matching filesystem device was found"));
+ if (virDomainDriverDetachFSDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- fs = virDomainFSRemove(vmdef, idx);
- virDomainFSDefFree(fs);
break;
case VIR_DOMAIN_DEVICE_RNG:
- if ((idx = virDomainRNGFind(vmdef, dev->data.rng)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("no matching RNG device was found"));
+ if (virDomainDriverDetachRNGDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainRNGDefFree(virDomainRNGRemove(vmdef, idx));
break;
case VIR_DOMAIN_DEVICE_MEMORY:
- if ((idx = virDomainMemoryFindInactiveByDef(vmdef,
- dev->data.memory)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("matching memory device was not found"));
+ if (virDomainDriverDetachMemoryDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- mem = virDomainMemoryRemove(vmdef, idx);
- vmdef->mem.cur_balloon -= mem->size;
- virDomainMemoryDefFree(mem);
+
break;
case VIR_DOMAIN_DEVICE_REDIRDEV:
- if ((idx = virDomainRedirdevDefFind(vmdef,
- dev->data.redirdev)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("no matching redirdev was not found"));
+ if (virDomainDriverDetachRedirdevDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainRedirdevDefFree(virDomainRedirdevDefRemove(vmdef, idx));
break;
case VIR_DOMAIN_DEVICE_SHMEM:
- if ((idx = virDomainShmemDefFind(vmdef, dev->data.shmem)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("matching shmem device was not found"));
+ if (virDomainDriverDetachShmemDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainShmemDefFree(virDomainShmemDefRemove(vmdef, idx));
break;
-
case VIR_DOMAIN_DEVICE_WATCHDOG:
- if (!vmdef->watchdog) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("domain has no watchdog"));
+ if (virDomainDriverDetachWatchdogDeviceConfig(vmdef) < 0)
return -1;
- }
- virDomainWatchdogDefFree(vmdef->watchdog);
- vmdef->watchdog = NULL;
+
break;
case VIR_DOMAIN_DEVICE_INPUT:
- if ((idx = virDomainInputDefFind(vmdef, dev->data.input)) < 0) {
- virReportError(VIR_ERR_DEVICE_MISSING, "%s",
- _("matching input device not found"));
+ if (virDomainDriverDetachInputDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainInputDefFree(virDomainInputDefRemove(vmdef, idx));
break;
case VIR_DOMAIN_DEVICE_VSOCK:
- if (!vmdef->vsock ||
- !virDomainVsockDefEquals(dev->data.vsock, vmdef->vsock)) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("matching vsock device not found"));
+ if (virDomainDriverDetachVsockDeviceConfig(vmdef, dev) < 0)
return -1;
- }
- virDomainVsockDefFree(vmdef->vsock);
- vmdef->vsock = NULL;
+
break;
case VIR_DOMAIN_DEVICE_VIDEO:
--
2.33.1
On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
>libxl / lxc / qemu drivers share some common codes in their
>DomainDetachDeviceConfig functions, so extract them to domain_driver and
>reuse them.
>
I would argue that these specific functions are actually dealing just
with the domain definition which is done mostly in domain_conf.c, so I
would pick that place.
>At the same time, this will enable test driver to test these functions
>with virshtest in the future.
>
>Signed-off-by: Luke Yue <lukedyue@gmail.com>
>---
>Not pretty sure whether this is a proper way to make these functions
>reusable, maybe there is a more elegant choice.
The patch does a good job in deduplicating some of the code and except
the file placement I think the overall approach is fine.
However unfortunately the functions are just copy-paste from various
places and they could be cleaned up to look the same. For example...
>---
> src/hypervisor/domain_driver.c | 266 +++++++++++++++++++++++++++++++++
> src/hypervisor/domain_driver.h | 41 +++++
> src/libvirt_private.syms | 14 ++
> src/libxl/libxl_driver.c | 41 +----
> src/lxc/lxc_driver.c | 37 +----
> src/qemu/qemu_driver.c | 123 +++------------
> 6 files changed, 356 insertions(+), 166 deletions(-)
>
>diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
>index 31737b0f4a..01ecb4e30e 100644
>--- a/src/hypervisor/domain_driver.c
>+++ b/src/hypervisor/domain_driver.c
>@@ -644,3 +644,269 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
>
> return ret;
> }
>+
>+
>+int
>+virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef,
>+ virDomainDeviceDef *dev)
>+{
>+ virDomainDiskDef *disk;
>+ virDomainDiskDef *det_disk;
>+
>+ disk = dev->data.disk;
>+ if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
>+ virReportError(VIR_ERR_DEVICE_MISSING, _("no target device %s"),
>+ disk->dst);
>+ return -1;
>+ }
>+ virDomainDiskDefFree(det_disk);
>+
>+ return 0;
This function uses temporary variables for everything and adds an error
message (which could be moved to the virDomainDiskRemoveByName() as all
callers do report the same error message when it is not found. Other
functions do essentially the same job, but have random comments, some
have extra helper variables. All that could be nicely unified and it
would read so much nicer.
>diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>index 7ea157f9c4..1e3d40f448 100644
>--- a/src/libxl/libxl_driver.c
>+++ b/src/libxl/libxl_driver.c
>@@ -3912,56 +3912,29 @@ libxlDomainDetachDeviceLive(libxlDriverPrivate *driver,
> static int
> libxlDomainDetachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev)
> {
>- virDomainDiskDef *disk;
>- virDomainDiskDef *detach;
>- virDomainHostdevDef *hostdev;
>- virDomainHostdevDef *det_hostdev;
>- virDomainControllerDef *cont;
>- virDomainControllerDef *det_cont;
>- virDomainNetDef *net;
>- int idx;
>-
> switch (dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
>- disk = dev->data.disk;
>- if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) {
>- virReportError(VIR_ERR_INVALID_ARG,
>- _("no target device %s"), disk->dst);
>+ if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0)
> return -1;
>- }
>- virDomainDiskDefFree(detach);
>+
> break;
>
> case VIR_DOMAIN_DEVICE_CONTROLLER:
>- cont = dev->data.controller;
>- if ((idx = virDomainControllerFind(vmdef, cont->type,
>- cont->idx)) < 0) {
>- virReportError(VIR_ERR_INVALID_ARG, "%s",
>- _("device not present in domain configuration"));
>+ if (virDomainDriverDetachControllerDeviceConfig(vmdef, dev) < 0)
> return -1;
>- }
>- det_cont = virDomainControllerRemove(vmdef, idx);
>- virDomainControllerDefFree(det_cont);
>+
> break;
>
> case VIR_DOMAIN_DEVICE_NET:
>- net = dev->data.net;
>- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
>+ if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0)
> return -1;
>
>- /* this is guaranteed to succeed */
>- virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
> break;
>
> case VIR_DOMAIN_DEVICE_HOSTDEV: {
>- hostdev = dev->data.hostdev;
>- if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) {
>- virReportError(VIR_ERR_INVALID_ARG, "%s",
>- _("device not present in domain configuration"));
>+ if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0)
> return -1;
>- }
>- virDomainHostdevRemove(vmdef, idx);
>- virDomainHostdevDefFree(det_hostdev);
>+
> break;
> }
>
>diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
>index e2720a6f89..485683fe3a 100644
>--- a/src/lxc/lxc_driver.c
>+++ b/src/lxc/lxc_driver.c
>@@ -3129,56 +3129,33 @@ static int
> lxcDomainDetachDeviceConfig(virDomainDef *vmdef,
> virDomainDeviceDef *dev)
> {
>- int ret = -1;
>- virDomainDiskDef *disk;
>- virDomainDiskDef *det_disk;
>- virDomainNetDef *net;
>- virDomainHostdevDef *hostdev;
>- virDomainHostdevDef *det_hostdev;
>- int idx;
>-
> switch (dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
>- disk = dev->data.disk;
>- if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
>- virReportError(VIR_ERR_INVALID_ARG,
>- _("no target device %s"), disk->dst);
>+ if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0)
> return -1;
>- }
>- virDomainDiskDefFree(det_disk);
>- ret = 0;
>+
> break;
>
> case VIR_DOMAIN_DEVICE_NET:
>- net = dev->data.net;
>- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
>+ if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0)
> return -1;
>
>- /* this is guaranteed to succeed */
>- virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
>- ret = 0;
> break;
>
> case VIR_DOMAIN_DEVICE_HOSTDEV: {
>- hostdev = dev->data.hostdev;
>- if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) {
>- virReportError(VIR_ERR_INVALID_ARG, "%s",
>- _("device not present in domain configuration"));
>+ if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0)
> return -1;
>- }
>- virDomainHostdevRemove(vmdef, idx);
>- virDomainHostdevDefFree(det_hostdev);
>- ret = 0;
>+
> break;
> }
>
> default:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("persistent detach of device is not supported"));
>- break;
>+ return -1;
> }
>
>- return ret;
>+ return 0;
> }
>
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index cf9407c358..d149cd22b2 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -7517,84 +7517,43 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef,
> unsigned int parse_flags,
> virDomainXMLOption *xmlopt)
What I really like about this change is that apart from roughly two
device types, like video and chardev, this function could be
de-duplicated as well. Adding a new functionality to that de-duplicated
function would then add that functionality into all the drivers using
this function. Of course the special devices would need some xmlopt
callbacks or something that drivers can modify themselves for such
reasons, but such change would be really, really helpful, especially in
the long term.
On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
> On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
> > libxl / lxc / qemu drivers share some common codes in their
> > DomainDetachDeviceConfig functions, so extract them to
> > domain_driver and
> > reuse them.
> >
>
> I would argue that these specific functions are actually dealing just
> with the domain definition which is done mostly in domain_conf.c, so
> I
> would pick that place.
>
Thanks for the review! No problem, I would move the code to
domain_conf.c in next version.
> > At the same time, this will enable test driver to test these
> > functions
> > with virshtest in the future.
> >
> > Signed-off-by: Luke Yue <lukedyue@gmail.com>
> > ---
> > Not pretty sure whether this is a proper way to make these
> > functions
> > reusable, maybe there is a more elegant choice.
>
> The patch does a good job in deduplicating some of the code and
> except
> the file placement I think the overall approach is fine.
>
> However unfortunately the functions are just copy-paste from various
> places and they could be cleaned up to look the same. For example...
>
> > ---
> > src/hypervisor/domain_driver.c | 266
> > +++++++++++++++++++++++++++++++++
> > src/hypervisor/domain_driver.h | 41 +++++
> > src/libvirt_private.syms | 14 ++
> > src/libxl/libxl_driver.c | 41 +----
> > src/lxc/lxc_driver.c | 37 +----
> > src/qemu/qemu_driver.c | 123 +++------------
> > 6 files changed, 356 insertions(+), 166 deletions(-)
> >
> > diff --git a/src/hypervisor/domain_driver.c
> > b/src/hypervisor/domain_driver.c
> > index 31737b0f4a..01ecb4e30e 100644
> > --- a/src/hypervisor/domain_driver.c
> > +++ b/src/hypervisor/domain_driver.c
> > @@ -644,3 +644,269 @@
> > virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
> >
> > return ret;
> > }
> > +
> > +
> > +int
> > +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef,
> > + virDomainDeviceDef *dev)
> > +{
> > + virDomainDiskDef *disk;
> > + virDomainDiskDef *det_disk;
> > +
> > + disk = dev->data.disk;
> > + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst)))
> > {
> > + virReportError(VIR_ERR_DEVICE_MISSING, _("no target device
> > %s"),
> > + disk->dst);
> > + return -1;
> > + }
> > + virDomainDiskDefFree(det_disk);
> > +
> > + return 0;
>
> This function uses temporary variables for everything and adds an
> error
> message (which could be moved to the virDomainDiskRemoveByName() as
> all
> callers do report the same error message when it is not found. Other
> functions do essentially the same job, but have random comments, some
> have extra helper variables. All that could be nicely unified and it
> would read so much nicer.
>
> What I really like about this change is that apart from roughly two
> device types, like video and chardev, this function could be
> de-duplicated as well. Adding a new functionality to that de-
> duplicated
> function would then add that functionality into all the drivers using
> this function. Of course the special devices would need some xmlopt
> callbacks or something that drivers can modify themselves for such
> reasons, but such change would be really, really helpful, especially
> in
> the long term.
So if I understand you correctly, I will unified all these functions
into one, just like the original ones for QEMU / lxc / libxl, but this
one is for all, and maybe I should create a new enum for flags of
detachable device that different hypervisor support, and xmlopt etc for
special devices. I will do that in next version.
Thanks,
Luke
On Mon, Nov 29, 2021 at 09:06:57PM +0800, Luke Yue wrote:
>On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
>> On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
>> > libxl / lxc / qemu drivers share some common codes in their
>> > DomainDetachDeviceConfig functions, so extract them to
>> > domain_driver and
>> > reuse them.
>> >
>>
>> I would argue that these specific functions are actually dealing just
>> with the domain definition which is done mostly in domain_conf.c, so
>> I
>> would pick that place.
>>
>
>Thanks for the review! No problem, I would move the code to
>domain_conf.c in next version.
>
>> > At the same time, this will enable test driver to test these
>> > functions
>> > with virshtest in the future.
>> >
>> > Signed-off-by: Luke Yue <lukedyue@gmail.com>
>> > ---
>> > Not pretty sure whether this is a proper way to make these
>> > functions
>> > reusable, maybe there is a more elegant choice.
>>
>> The patch does a good job in deduplicating some of the code and
>> except
>> the file placement I think the overall approach is fine.
>>
>> However unfortunately the functions are just copy-paste from various
>> places and they could be cleaned up to look the same. For example...
>>
>> > ---
>> > src/hypervisor/domain_driver.c | 266
>> > +++++++++++++++++++++++++++++++++
>> > src/hypervisor/domain_driver.h | 41 +++++
>> > src/libvirt_private.syms | 14 ++
>> > src/libxl/libxl_driver.c | 41 +----
>> > src/lxc/lxc_driver.c | 37 +----
>> > src/qemu/qemu_driver.c | 123 +++------------
>> > 6 files changed, 356 insertions(+), 166 deletions(-)
>> >
>> > diff --git a/src/hypervisor/domain_driver.c
>> > b/src/hypervisor/domain_driver.c
>> > index 31737b0f4a..01ecb4e30e 100644
>> > --- a/src/hypervisor/domain_driver.c
>> > +++ b/src/hypervisor/domain_driver.c
>> > @@ -644,3 +644,269 @@
>> > virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
>> >
>> > return ret;
>> > }
>> > +
>> > +
>> > +int
>> > +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef,
>> > + virDomainDeviceDef *dev)
>> > +{
>> > + virDomainDiskDef *disk;
>> > + virDomainDiskDef *det_disk;
>> > +
>> > + disk = dev->data.disk;
>> > + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst)))
>> > {
>> > + virReportError(VIR_ERR_DEVICE_MISSING, _("no target device
>> > %s"),
>> > + disk->dst);
>> > + return -1;
>> > + }
>> > + virDomainDiskDefFree(det_disk);
>> > +
>> > + return 0;
>>
>> This function uses temporary variables for everything and adds an
>> error
>> message (which could be moved to the virDomainDiskRemoveByName() as
>> all
>> callers do report the same error message when it is not found. Other
>> functions do essentially the same job, but have random comments, some
>> have extra helper variables. All that could be nicely unified and it
>> would read so much nicer.
>>
>> What I really like about this change is that apart from roughly two
>> device types, like video and chardev, this function could be
>> de-duplicated as well. Adding a new functionality to that de-
>> duplicated
>> function would then add that functionality into all the drivers using
>> this function. Of course the special devices would need some xmlopt
>> callbacks or something that drivers can modify themselves for such
>> reasons, but such change would be really, really helpful, especially
>> in
>> the long term.
>
>So if I understand you correctly, I will unified all these functions
>into one,
Not really into one, although if you figure out a way to make that
easily possible feel free to go on, what I meant make them look and read
the same.
>just like the original ones for QEMU / lxc / libxl, but this
>one is for all, and maybe I should create a new enum for flags of
>detachable device that different hypervisor support, and xmlopt etc for
>special devices. I will do that in next version.
>
Deduplicating the bigger function would be nice to have for the future,
it does not need to happen together in this one. It will require some
more work to make it "modular" enough to be used by various drivers.
On Mon, 2021-11-29 at 15:33 +0100, Martin Kletzander wrote:
> On Mon, Nov 29, 2021 at 09:06:57PM +0800, Luke Yue wrote:
> > On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
> > > On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
> > > > libxl / lxc / qemu drivers share some common codes in their
> > > > DomainDetachDeviceConfig functions, so extract them to
> > > > domain_driver and
> > > > reuse them.
> > > >
> > >
> > > I would argue that these specific functions are actually dealing
> > > just
> > > with the domain definition which is done mostly in domain_conf.c,
> > > so
> > > I
> > > would pick that place.
> > >
> >
> > Thanks for the review! No problem, I would move the code to
> > domain_conf.c in next version.
> >
> > > > At the same time, this will enable test driver to test these
> > > > functions
> > > > with virshtest in the future.
> > > >
> > > > Signed-off-by: Luke Yue <lukedyue@gmail.com>
> > > > ---
> > > > Not pretty sure whether this is a proper way to make these
> > > > functions
> > > > reusable, maybe there is a more elegant choice.
> > >
> > > The patch does a good job in deduplicating some of the code and
> > > except
> > > the file placement I think the overall approach is fine.
> > >
> > > However unfortunately the functions are just copy-paste from
> > > various
> > > places and they could be cleaned up to look the same. For
> > > example...
> > >
> > > > ---
> > > > src/hypervisor/domain_driver.c | 266
> > > > +++++++++++++++++++++++++++++++++
> > > > src/hypervisor/domain_driver.h | 41 +++++
> > > > src/libvirt_private.syms | 14 ++
> > > > src/libxl/libxl_driver.c | 41 +----
> > > > src/lxc/lxc_driver.c | 37 +----
> > > > src/qemu/qemu_driver.c | 123 +++------------
> > > > 6 files changed, 356 insertions(+), 166 deletions(-)
> > > >
> > > > diff --git a/src/hypervisor/domain_driver.c
> > > > b/src/hypervisor/domain_driver.c
> > > > index 31737b0f4a..01ecb4e30e 100644
> > > > --- a/src/hypervisor/domain_driver.c
> > > > +++ b/src/hypervisor/domain_driver.c
> > > > @@ -644,3 +644,269 @@
> > > > virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
> > > >
> > > > return ret;
> > > > }
> > > > +
> > > > +
> > > > +int
> > > > +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef,
> > > > + virDomainDeviceDef *dev)
> > > > +{
> > > > + virDomainDiskDef *disk;
> > > > + virDomainDiskDef *det_disk;
> > > > +
> > > > + disk = dev->data.disk;
> > > > + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk-
> > > > >dst)))
> > > > {
> > > > + virReportError(VIR_ERR_DEVICE_MISSING, _("no target
> > > > device
> > > > %s"),
> > > > + disk->dst);
> > > > + return -1;
> > > > + }
> > > > + virDomainDiskDefFree(det_disk);
> > > > +
> > > > + return 0;
> > >
> > > This function uses temporary variables for everything and adds an
> > > error
> > > message (which could be moved to the virDomainDiskRemoveByName()
> > > as
> > > all
> > > callers do report the same error message when it is not found.
> > > Other
> > > functions do essentially the same job, but have random comments,
> > > some
> > > have extra helper variables. All that could be nicely unified and
> > > it
> > > would read so much nicer.
> > >
> > > What I really like about this change is that apart from roughly
> > > two
> > > device types, like video and chardev, this function could be
> > > de-duplicated as well. Adding a new functionality to that de-
> > > duplicated
> > > function would then add that functionality into all the drivers
> > > using
> > > this function. Of course the special devices would need some
> > > xmlopt
> > > callbacks or something that drivers can modify themselves for
> > > such
> > > reasons, but such change would be really, really helpful,
> > > especially
> > > in
> > > the long term.
> >
> > So if I understand you correctly, I will unified all these
> > functions
> > into one,
>
> Not really into one, although if you figure out a way to make that
> easily possible feel free to go on, what I meant make them look and
> read
> the same.
>
> > just like the original ones for QEMU / lxc / libxl, but this
> > one is for all, and maybe I should create a new enum for flags of
> > detachable device that different hypervisor support, and xmlopt etc
> > for
> > special devices. I will do that in next version.
> >
>
> Deduplicating the bigger function would be nice to have for the
> future,
> it does not need to happen together in this one. It will require
> some
> more work to make it "modular" enough to be used by various drivers.
OK, thanks, now I see.
© 2016 - 2026 Red Hat, Inc.