[PATCH v3 02/12] domain_driver: extract DetachXXXDeviceConfig related functions and use them
Martin Kletzander
mkletzan at redhat.com
Fri Nov 26 15:43:17 UTC 2021
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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211126/91759665/attachment-0001.sig>
More information about the libvir-list
mailing list