src/test/test_driver.c | 382 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 380 insertions(+), 2 deletions(-)
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
---
Changed since v4:
* removed inadvertent calls to functions virNWFilterReadLockFilterUpdates/virNWFilterUnlockFilterUpdates
(original patch was based on v8.0.0)
---
src/test/test_driver.c | 382 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 380 insertions(+), 2 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index e87d7cfd44..effb7f9880 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -26,8 +26,6 @@
#include <unistd.h>
#include <sys/stat.h>
#include <libxml/xmlsave.h>
-
-
#include "virerror.h"
#include "datatypes.h"
#include "test_driver.h"
@@ -38,9 +36,12 @@
#include "virnetworkobj.h"
#include "interface_conf.h"
#include "checkpoint_conf.h"
+#include "domain_addr.h"
+#include "domain_audit.h"
#include "domain_conf.h"
#include "domain_driver.h"
#include "domain_event.h"
+#include "domain_validate.h"
#include "network_event.h"
#include "snapshot_conf.h"
#include "virfdstream.h"
@@ -50,6 +51,7 @@
#include "node_device_conf.h"
#include "virnodedeviceobj.h"
#include "node_device_event.h"
+#include "vircgroup.h"
#include "virxml.h"
#include "virthread.h"
#include "virlog.h"
@@ -10035,6 +10037,379 @@ testConnectGetDomainCapabilities(virConnectPtr conn G_GNUC_UNUSED,
return virDomainCapsFormat(domCaps);
}
+static int
+testDomainAttachHostPCIDevice(testDriver *driver G_GNUC_UNUSED,
+ virDomainObj *vm,
+ virDomainHostdevDef *hostdev)
+{
+ int backend = hostdev->source.subsys.u.pci.backend;
+
+ switch ((virDomainHostdevSubsysPCIBackendType)backend) {
+ case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
+ case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
+ case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
+ break;
+
+ case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
+ case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("test hypervisor does not support device assignment mode '%s'"),
+ virDomainHostdevSubsysPCIBackendTypeToString(backend));
+ return -1;
+ }
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit during hotplug"));
+ return -1;
+ }
+
+ VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1);
+
+ virDomainAuditHostdev(vm, hostdev, "attach", true);
+
+ vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
+
+ return 0;
+}
+
+static int
+testDomainAttachHostDevice(testDriver *driver,
+ virDomainObj *vm,
+ virDomainHostdevDef *hostdev)
+{
+ if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("hotplug is not supported for hostdev mode '%s'"),
+ virDomainHostdevModeTypeToString(hostdev->mode));
+ return -1;
+ }
+
+ if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("hotplug is not supported for hostdev subsys type '%s'"),
+ virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
+ return -1;
+ }
+
+ return testDomainAttachHostPCIDevice(driver, vm, hostdev);
+}
+
+static int
+testDomainAttachDeviceLive(virDomainObj *vm,
+ virDomainDeviceDef *dev,
+ testDriver *driver)
+{
+ int ret = -1;
+ const char *alias = NULL;
+
+ if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("live attach of device '%s' is not supported"),
+ virDomainDeviceTypeToString(dev->type));
+ return -1;
+ }
+
+ testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
+ if ((ret = testDomainAttachHostDevice(driver, vm, dev->data.hostdev)) != 0)
+ return ret;
+
+ alias = dev->data.hostdev->info->alias;
+ dev->data.hostdev = NULL;
+
+ if (alias) {
+ virObjectEvent *event;
+ event = virDomainEventDeviceAddedNewFromObj(vm, alias);
+ virObjectEventStateQueue(driver->eventState, event);
+ }
+
+ return 0;
+}
+
+static int
+testDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
+ testDriver *driver,
+ const char *xml,
+ unsigned int flags)
+{
+ g_autoptr(virDomainDeviceDef) devConf = NULL;
+ g_autoptr(virDomainDeviceDef) devLive = NULL;
+ unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+ VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
+
+ virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
+
+ if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+ if (!(devLive = virDomainDeviceDefParse(xml, vm->def,
+ driver->xmlopt, NULL,
+ parse_flags)))
+ return -1;
+
+ if (virDomainDeviceValidateAliasForHotplug(vm, devLive,
+ VIR_DOMAIN_AFFECT_LIVE) < 0)
+ return -1;
+
+ if (virDomainDefCompatibleDevice(vm->def, devLive, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH,
+ true) < 0)
+ return -1;
+
+ if (testDomainAttachDeviceLive(vm, devLive, driver) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
+static int
+testDomainAttachDeviceFlags(virDomainPtr domain,
+ const char *xml,
+ unsigned int flags) {
+
+ testDriver *driver = domain->conn->privateData;
+ virDomainObj *vm = NULL;
+ int ret = -1;
+
+ if (!(vm = testDomObjFromDomain(domain)))
+ return -1;
+
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto cleanup;
+
+ if (testDomainAttachDeviceLiveAndConfig(vm, driver, xml, flags) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ virDomainObjEndAPI(&vm);
+ return ret;
+}
+
+static int
+testDomainAttachDevice(virDomainPtr domain, const char *xml)
+{
+ return testDomainAttachDeviceFlags(domain, xml, 0);
+}
+
+/* search for a hostdev matching dev and detach it */
+static int
+testDomainDetachPrepHostdev(virDomainObj *vm,
+ virDomainHostdevDef *match,
+ virDomainHostdevDef **detach)
+{
+ virDomainHostdevSubsys *subsys = &match->source.subsys;
+ virDomainHostdevSubsysPCI *pcisrc = &subsys->u.pci;
+ virDomainHostdevDef *hostdev = NULL;
+
+ if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("hot unplug is not supported for hostdev mode '%s'"),
+ virDomainHostdevModeTypeToString(match->mode));
+ return -1;
+ }
+
+ if (virDomainHostdevFind(vm->def, match, &hostdev) < 0) {
+ if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
+ virReportError(VIR_ERR_DEVICE_MISSING,
+ _("host pci device " VIR_PCI_DEVICE_ADDRESS_FMT
+ " not found"),
+ pcisrc->addr.domain, pcisrc->addr.bus,
+ pcisrc->addr.slot, pcisrc->addr.function);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected hostdev type %d"), subsys->type);
+ }
+ return -1;
+ }
+
+ *detach = hostdev;
+
+ return 0;
+}
+
+static int
+testDomainRemoveHostDevice(testDriver *driver G_GNUC_UNUSED,
+ virDomainObj *vm,
+ virDomainHostdevDef *hostdev)
+{
+ virDomainNetDef *net = NULL;
+ size_t i;
+
+ VIR_DEBUG("Removing host device %s from domain %p %s",
+ hostdev->info->alias, vm, vm->def->name);
+
+ if (hostdev->parentnet) {
+ net = hostdev->parentnet;
+ for (i = 0; i < vm->def->nnets; i++) {
+ if (vm->def->nets[i] == hostdev->parentnet) {
+ virDomainNetRemove(vm->def, i);
+ break;
+ }
+ }
+ }
+
+ for (i = 0; i < vm->def->nhostdevs; i++) {
+ if (vm->def->hostdevs[i] == hostdev) {
+ virDomainHostdevRemove(vm->def, i);
+ break;
+ }
+ }
+
+ virDomainAuditHostdev(vm, hostdev, "detach", true);
+
+ virDomainHostdevDefFree(hostdev);
+
+ if (net) {
+ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
+ g_autoptr(virConnect) conn = virGetConnectNetwork();
+ if (conn)
+ virDomainNetReleaseActualDevice(conn, vm->def, net);
+ else
+ VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname));
+ }
+ virDomainNetDefFree(net);
+ }
+
+ return 0;
+}
+
+static int
+testDomainRemoveDevice(testDriver *driver,
+ virDomainObj *vm,
+ virDomainDeviceDef *dev)
+{
+ virDomainDeviceInfo *info;
+ virObjectEvent *event;
+ g_autofree char *alias = NULL;
+
+ /*
+ * save the alias to use when sending a DEVICE_REMOVED event after
+ * all other teardown is complete
+ */
+ if ((info = virDomainDeviceGetInfo(dev)))
+ alias = g_strdup(info->alias);
+ info = NULL;
+
+ if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("don't know how to remove a %s device"),
+ virDomainDeviceTypeToString(dev->type));
+ goto out;
+ }
+
+ if (testDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
+ return -1;
+
+out:
+ event = virDomainEventDeviceRemovedNewFromObj(vm, alias);
+ virObjectEventStateQueue(driver->eventState, event);
+
+ return 0;
+}
+
+static int
+testDomainDetachDeviceLive(virDomainObj *vm,
+ virDomainDeviceDef *match,
+ testDriver *driver)
+{
+ virDomainDeviceDef detach = { .type = match->type };
+ virDomainDeviceInfo *info = NULL;
+
+ if ((virDomainDeviceType)match->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("live detach of device '%s' is not supported"),
+ virDomainDeviceTypeToString(match->type));
+ return -1;
+ }
+
+ if (testDomainDetachPrepHostdev(vm, match->data.hostdev,
+ &detach.data.hostdev) < 0)
+ return -1;
+
+ /* "detach" now points to the actual device we want to detach */
+
+ if (!(info = virDomainDeviceGetInfo(&detach))) {
+ /*
+ * This should never happen, since all of the device types in
+ * the switch cases that end with a "break" instead of a
+ * return have a virDeviceInfo in them.
+ */
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("device of type '%s' has no device info"),
+ virDomainDeviceTypeToString(detach.type));
+ return -1;
+ }
+
+ /* Make generic validation checks common to all device types */
+
+ if (!info->alias) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot detach %s device with no alias"),
+ virDomainDeviceTypeToString(detach.type));
+ return -1;
+ }
+
+ return testDomainRemoveDevice(driver, vm, &detach);
+}
+
+static int
+testDomainDetachDeviceAliasLiveAndConfig(testDriver *driver,
+ virDomainObj *vm,
+ const char *alias,
+ unsigned int flags)
+{
+ virDomainDef *def = NULL;
+ g_autoptr(virDomainDef) vmdef = NULL;
+
+ virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
+
+ if (virDomainObjGetDefs(vm, flags, &def, NULL) < 0)
+ return -1;
+
+ if (def) {
+ virDomainDeviceDef dev;
+
+ if (virDomainDefFindDevice(def, alias, &dev, true) < 0)
+ return -1;
+
+ if (testDomainDetachDeviceLive(vm, &dev, driver) < 0)
+ return -1;
+ }
+
+ if (vmdef) {
+ if (virDomainDefSave(vmdef, driver->xmlopt, NULL) < 0)
+ return -1;
+ virDomainObjAssignDef(vm, &vmdef, false, NULL);
+ }
+
+ return 0;
+}
+
+static int
+testDomainDetachDeviceAlias(virDomainPtr dom,
+ const char *alias,
+ unsigned int flags)
+{
+ testDriver *driver = dom->conn->privateData;
+ virDomainObj *vm = NULL;
+ int ret = -1;
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ return -1;
+
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto cleanup;
+
+ if (testDomainDetachDeviceAliasLiveAndConfig(driver, vm, alias, flags) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ virDomainObjEndAPI(&vm);
+ return ret;
+}
/*
* Test driver
@@ -10058,6 +10433,9 @@ static virHypervisorDriver testHypervisorDriver = {
.connectListDomains = testConnectListDomains, /* 0.1.1 */
.connectNumOfDomains = testConnectNumOfDomains, /* 0.1.1 */
.connectListAllDomains = testConnectListAllDomains, /* 0.9.13 */
+ .domainAttachDevice = testDomainAttachDevice, /* 9.10.0 */
+ .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 9.10.0 */
+ .domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 9.10.0 */
.domainCreateXML = testDomainCreateXML, /* 0.1.4 */
.domainCreateXMLWithFiles = testDomainCreateXMLWithFiles, /* 5.7.0 */
.domainLookupByID = testDomainLookupByID, /* 0.1.1 */
--
2.27.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
ping
> -----Original Message-----
> From: Thanos Makatos <thanos.makatos@nutanix.com>
> Sent: Thursday, November 9, 2023 11:15 AM
> To: devel@lists.libvirt.org
> Cc: Thanos Makatos <thanos.makatos@nutanix.com>
> Subject: [PATCH V5] support for hotplug/hotunplug in test hypervisor
>
> Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
>
> ---
>
> Changed since v4:
> * removed inadvertent calls to functions
> virNWFilterReadLockFilterUpdates/virNWFilterUnlockFilterUpdates
> (original patch was based on v8.0.0)
>
> ---
> src/test/test_driver.c | 382 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 380 insertions(+), 2 deletions(-)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index e87d7cfd44..effb7f9880 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -26,8 +26,6 @@
> #include <unistd.h>
> #include <sys/stat.h>
> #include <libxml/xmlsave.h>
> -
> -
> #include "virerror.h"
> #include "datatypes.h"
> #include "test_driver.h"
> @@ -38,9 +36,12 @@
> #include "virnetworkobj.h"
> #include "interface_conf.h"
> #include "checkpoint_conf.h"
> +#include "domain_addr.h"
> +#include "domain_audit.h"
> #include "domain_conf.h"
> #include "domain_driver.h"
> #include "domain_event.h"
> +#include "domain_validate.h"
> #include "network_event.h"
> #include "snapshot_conf.h"
> #include "virfdstream.h"
> @@ -50,6 +51,7 @@
> #include "node_device_conf.h"
> #include "virnodedeviceobj.h"
> #include "node_device_event.h"
> +#include "vircgroup.h"
> #include "virxml.h"
> #include "virthread.h"
> #include "virlog.h"
> @@ -10035,6 +10037,379 @@
> testConnectGetDomainCapabilities(virConnectPtr conn G_GNUC_UNUSED,
> return virDomainCapsFormat(domCaps);
> }
>
> +static int
> +testDomainAttachHostPCIDevice(testDriver *driver G_GNUC_UNUSED,
> + virDomainObj *vm,
> + virDomainHostdevDef *hostdev)
> +{
> + int backend = hostdev->source.subsys.u.pci.backend;
> +
> + switch ((virDomainHostdevSubsysPCIBackendType)backend) {
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> + break;
> +
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("test hypervisor does not support device assignment mode
> '%s'"),
> + virDomainHostdevSubsysPCIBackendTypeToString(backend));
> + return -1;
> + }
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest unexpectedly quit during hotplug"));
> + return -1;
> + }
> +
> + VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1);
> +
> + virDomainAuditHostdev(vm, hostdev, "attach", true);
> +
> + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> +
> + return 0;
> +}
> +
> +static int
> +testDomainAttachHostDevice(testDriver *driver,
> + virDomainObj *vm,
> + virDomainHostdevDef *hostdev)
> +{
> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("hotplug is not supported for hostdev mode '%s'"),
> + virDomainHostdevModeTypeToString(hostdev->mode));
> + return -1;
> + }
> +
> + if (hostdev->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("hotplug is not supported for hostdev subsys type '%s'"),
> + virDomainHostdevSubsysTypeToString(hostdev-
> >source.subsys.type));
> + return -1;
> + }
> +
> + return testDomainAttachHostPCIDevice(driver, vm, hostdev);
> +}
> +
> +static int
> +testDomainAttachDeviceLive(virDomainObj *vm,
> + virDomainDeviceDef *dev,
> + testDriver *driver)
> +{
> + int ret = -1;
> + const char *alias = NULL;
> +
> + if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("live attach of device '%s' is not supported"),
> + virDomainDeviceTypeToString(dev->type));
> + return -1;
> + }
> +
> + testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
> + if ((ret = testDomainAttachHostDevice(driver, vm, dev->data.hostdev)) !=
> 0)
> + return ret;
> +
> + alias = dev->data.hostdev->info->alias;
> + dev->data.hostdev = NULL;
> +
> + if (alias) {
> + virObjectEvent *event;
> + event = virDomainEventDeviceAddedNewFromObj(vm, alias);
> + virObjectEventStateQueue(driver->eventState, event);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +testDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
> + testDriver *driver,
> + const char *xml,
> + unsigned int flags)
> +{
> + g_autoptr(virDomainDeviceDef) devConf = NULL;
> + g_autoptr(virDomainDeviceDef) devLive = NULL;
> + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> + if (!(devLive = virDomainDeviceDefParse(xml, vm->def,
> + driver->xmlopt, NULL,
> + parse_flags)))
> + return -1;
> +
> + if (virDomainDeviceValidateAliasForHotplug(vm, devLive,
> + VIR_DOMAIN_AFFECT_LIVE) < 0)
> + return -1;
> +
> + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL,
> + VIR_DOMAIN_DEVICE_ACTION_ATTACH,
> + true) < 0)
> + return -1;
> +
> + if (testDomainAttachDeviceLive(vm, devLive, driver) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +testDomainAttachDeviceFlags(virDomainPtr domain,
> + const char *xml,
> + unsigned int flags) {
> +
> + testDriver *driver = domain->conn->privateData;
> + virDomainObj *vm = NULL;
> + int ret = -1;
> +
> + if (!(vm = testDomObjFromDomain(domain)))
> + return -1;
> +
> + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> + goto cleanup;
> +
> + if (testDomainAttachDeviceLiveAndConfig(vm, driver, xml, flags) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
> +static int
> +testDomainAttachDevice(virDomainPtr domain, const char *xml)
> +{
> + return testDomainAttachDeviceFlags(domain, xml, 0);
> +}
> +
> +/* search for a hostdev matching dev and detach it */
> +static int
> +testDomainDetachPrepHostdev(virDomainObj *vm,
> + virDomainHostdevDef *match,
> + virDomainHostdevDef **detach)
> +{
> + virDomainHostdevSubsys *subsys = &match->source.subsys;
> + virDomainHostdevSubsysPCI *pcisrc = &subsys->u.pci;
> + virDomainHostdevDef *hostdev = NULL;
> +
> + if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("hot unplug is not supported for hostdev mode '%s'"),
> + virDomainHostdevModeTypeToString(match->mode));
> + return -1;
> + }
> +
> + if (virDomainHostdevFind(vm->def, match, &hostdev) < 0) {
> + if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> + virReportError(VIR_ERR_DEVICE_MISSING,
> + _("host pci device " VIR_PCI_DEVICE_ADDRESS_FMT
> + " not found"),
> + pcisrc->addr.domain, pcisrc->addr.bus,
> + pcisrc->addr.slot, pcisrc->addr.function);
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected hostdev type %d"), subsys->type);
> + }
> + return -1;
> + }
> +
> + *detach = hostdev;
> +
> + return 0;
> +}
> +
> +static int
> +testDomainRemoveHostDevice(testDriver *driver G_GNUC_UNUSED,
> + virDomainObj *vm,
> + virDomainHostdevDef *hostdev)
> +{
> + virDomainNetDef *net = NULL;
> + size_t i;
> +
> + VIR_DEBUG("Removing host device %s from domain %p %s",
> + hostdev->info->alias, vm, vm->def->name);
> +
> + if (hostdev->parentnet) {
> + net = hostdev->parentnet;
> + for (i = 0; i < vm->def->nnets; i++) {
> + if (vm->def->nets[i] == hostdev->parentnet) {
> + virDomainNetRemove(vm->def, i);
> + break;
> + }
> + }
> + }
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + if (vm->def->hostdevs[i] == hostdev) {
> + virDomainHostdevRemove(vm->def, i);
> + break;
> + }
> + }
> +
> + virDomainAuditHostdev(vm, hostdev, "detach", true);
> +
> + virDomainHostdevDefFree(hostdev);
> +
> + if (net) {
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + g_autoptr(virConnect) conn = virGetConnectNetwork();
> + if (conn)
> + virDomainNetReleaseActualDevice(conn, vm->def, net);
> + else
> + VIR_WARN("Unable to release network device '%s'", NULLSTR(net-
> >ifname));
> + }
> + virDomainNetDefFree(net);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +testDomainRemoveDevice(testDriver *driver,
> + virDomainObj *vm,
> + virDomainDeviceDef *dev)
> +{
> + virDomainDeviceInfo *info;
> + virObjectEvent *event;
> + g_autofree char *alias = NULL;
> +
> + /*
> + * save the alias to use when sending a DEVICE_REMOVED event after
> + * all other teardown is complete
> + */
> + if ((info = virDomainDeviceGetInfo(dev)))
> + alias = g_strdup(info->alias);
> + info = NULL;
> +
> + if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("don't know how to remove a %s device"),
> + virDomainDeviceTypeToString(dev->type));
> + goto out;
> + }
> +
> + if (testDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
> + return -1;
> +
> +out:
> + event = virDomainEventDeviceRemovedNewFromObj(vm, alias);
> + virObjectEventStateQueue(driver->eventState, event);
> +
> + return 0;
> +}
> +
> +static int
> +testDomainDetachDeviceLive(virDomainObj *vm,
> + virDomainDeviceDef *match,
> + testDriver *driver)
> +{
> + virDomainDeviceDef detach = { .type = match->type };
> + virDomainDeviceInfo *info = NULL;
> +
> + if ((virDomainDeviceType)match->type !=
> VIR_DOMAIN_DEVICE_HOSTDEV) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("live detach of device '%s' is not supported"),
> + virDomainDeviceTypeToString(match->type));
> + return -1;
> + }
> +
> + if (testDomainDetachPrepHostdev(vm, match->data.hostdev,
> + &detach.data.hostdev) < 0)
> + return -1;
> +
> + /* "detach" now points to the actual device we want to detach */
> +
> + if (!(info = virDomainDeviceGetInfo(&detach))) {
> + /*
> + * This should never happen, since all of the device types in
> + * the switch cases that end with a "break" instead of a
> + * return have a virDeviceInfo in them.
> + */
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("device of type '%s' has no device info"),
> + virDomainDeviceTypeToString(detach.type));
> + return -1;
> + }
> +
> + /* Make generic validation checks common to all device types */
> +
> + if (!info->alias) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Cannot detach %s device with no alias"),
> + virDomainDeviceTypeToString(detach.type));
> + return -1;
> + }
> +
> + return testDomainRemoveDevice(driver, vm, &detach);
> +}
> +
> +static int
> +testDomainDetachDeviceAliasLiveAndConfig(testDriver *driver,
> + virDomainObj *vm,
> + const char *alias,
> + unsigned int flags)
> +{
> + virDomainDef *def = NULL;
> + g_autoptr(virDomainDef) vmdef = NULL;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
> +
> + if (virDomainObjGetDefs(vm, flags, &def, NULL) < 0)
> + return -1;
> +
> + if (def) {
> + virDomainDeviceDef dev;
> +
> + if (virDomainDefFindDevice(def, alias, &dev, true) < 0)
> + return -1;
> +
> + if (testDomainDetachDeviceLive(vm, &dev, driver) < 0)
> + return -1;
> + }
> +
> + if (vmdef) {
> + if (virDomainDefSave(vmdef, driver->xmlopt, NULL) < 0)
> + return -1;
> + virDomainObjAssignDef(vm, &vmdef, false, NULL);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +testDomainDetachDeviceAlias(virDomainPtr dom,
> + const char *alias,
> + unsigned int flags)
> +{
> + testDriver *driver = dom->conn->privateData;
> + virDomainObj *vm = NULL;
> + int ret = -1;
> +
> + if (!(vm = testDomObjFromDomain(dom)))
> + return -1;
> +
> + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> + goto cleanup;
> +
> + if (testDomainDetachDeviceAliasLiveAndConfig(driver, vm, alias, flags) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
>
> /*
> * Test driver
> @@ -10058,6 +10433,9 @@ static virHypervisorDriver testHypervisorDriver =
> {
> .connectListDomains = testConnectListDomains, /* 0.1.1 */
> .connectNumOfDomains = testConnectNumOfDomains, /* 0.1.1 */
> .connectListAllDomains = testConnectListAllDomains, /* 0.9.13 */
> + .domainAttachDevice = testDomainAttachDevice, /* 9.10.0 */
> + .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 9.10.0 */
> + .domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 9.10.0 */
> .domainCreateXML = testDomainCreateXML, /* 0.1.4 */
> .domainCreateXMLWithFiles = testDomainCreateXMLWithFiles, /* 5.7.0 */
> .domainLookupByID = testDomainLookupByID, /* 0.1.1 */
> --
> 2.27.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 11/9/23 12:14, Thanos Makatos wrote:
> Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
>
> ---
>
> Changed since v4:
> * removed inadvertent calls to functions virNWFilterReadLockFilterUpdates/virNWFilterUnlockFilterUpdates
> (original patch was based on v8.0.0)
>
> ---
> src/test/test_driver.c | 382 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 380 insertions(+), 2 deletions(-)
Ooops, this has missed my attention and unfortunately upcoming release
too as we are in freeze. Sorry. To make it up to you, I'm fixing all the
small issues I'm raising below and will push once after the release.
I've uploaded the fixup patch (that I'll squash into yours before
merging) here, if you want to take a look:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/test_driver_hotplug/?ref_type=heads
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index e87d7cfd44..effb7f9880 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -26,8 +26,6 @@
> #include <unistd.h>
> #include <sys/stat.h>
> #include <libxml/xmlsave.h>
> -
> -
We like to keep these separate to make it more obvious where public
header section ends and private header section starts.
> #include "virerror.h"
> #include "datatypes.h"
> #include "test_driver.h"
> @@ -38,9 +36,12 @@
> #include "virnetworkobj.h"
> #include "interface_conf.h"
> #include "checkpoint_conf.h"
> +#include "domain_addr.h"
> +#include "domain_audit.h"
This shouldn't be needed. domain_audit contains functions to audit real
resource auditing (e.g. assigning a real device to a guest) - test
driver should not do such thing.
> #include "domain_conf.h"
> #include "domain_driver.h"
> #include "domain_event.h"
> +#include "domain_validate.h"
> #include "network_event.h"
> #include "snapshot_conf.h"
> #include "virfdstream.h"
> @@ -50,6 +51,7 @@
> #include "node_device_conf.h"
> #include "virnodedeviceobj.h"
> #include "node_device_event.h"
> +#include "vircgroup.h"
And this too shouldn't be here. We do not set up any cgroups for domains
under the test driver.
> #include "virxml.h"
> #include "virthread.h"
> #include "virlog.h"
> @@ -10035,6 +10037,379 @@ testConnectGetDomainCapabilities(virConnectPtr conn G_GNUC_UNUSED,
> return virDomainCapsFormat(domCaps);
> }
>
> +static int
> +testDomainAttachHostPCIDevice(testDriver *driver G_GNUC_UNUSED,
> + virDomainObj *vm,
> + virDomainHostdevDef *hostdev)
> +{
> + int backend = hostdev->source.subsys.u.pci.backend;
> +
> + switch ((virDomainHostdevSubsysPCIBackendType)backend) {
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> + break;
> +
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("test hypervisor does not support device assignment mode '%s'"),
I'll raise it here: for translatable strings, we've switched to using
more robust format: %1$s because it bit us in the past when one of
locales contained wrong data causing us to crash. This should have been
caught by 'ninja test'.
> + virDomainHostdevSubsysPCIBackendTypeToString(backend));
> + return -1;
> + }
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("guest unexpectedly quit during hotplug"));
> + return -1;
> + }
> +
This check makes sense in QEMU world, because we unlock the domain
object (virDomainObj) whilst talking to QEMU on monitor. But in the test
driver it's needless - the object is locked throughough whole API run.
> + VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1);
> +
> + virDomainAuditHostdev(vm, hostdev, "attach", true);
This shouldn't be here.
> +
> + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> +
> + return 0;
> +}
> +
> +static int
> +testDomainAttachHostDevice(testDriver *driver,
> + virDomainObj *vm,
> + virDomainHostdevDef *hostdev)
> +{
> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("hotplug is not supported for hostdev mode '%s'"),
> + virDomainHostdevModeTypeToString(hostdev->mode));
> + return -1;
> + }
> +
> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("hotplug is not supported for hostdev subsys type '%s'"),
> + virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
> + return -1;
> + }
> +
> + return testDomainAttachHostPCIDevice(driver, vm, hostdev);
> +}
> +
> +static int
> +testDomainAttachDeviceLive(virDomainObj *vm,
> + virDomainDeviceDef *dev,
> + testDriver *driver)
> +{
> + int ret = -1;
> + const char *alias = NULL;
> +
> + if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
The ->type member is already of the correct type, no need to type cast it.
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("live attach of device '%s' is not supported"),
> + virDomainDeviceTypeToString(dev->type));
> + return -1;
> + }
> +
> + testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
> + if ((ret = testDomainAttachHostDevice(driver, vm, dev->data.hostdev)) != 0)
> + return ret;
> +
> + alias = dev->data.hostdev->info->alias;
> + dev->data.hostdev = NULL;
> +
> + if (alias) {
> + virObjectEvent *event;
> + event = virDomainEventDeviceAddedNewFromObj(vm, alias);
> + virObjectEventStateQueue(driver->eventState, event);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +testDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
> + testDriver *driver,
> + const char *xml,
> + unsigned int flags)
> +{
> + g_autoptr(virDomainDeviceDef) devConf = NULL;
> + g_autoptr(virDomainDeviceDef) devLive = NULL;
> + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> + if (!(devLive = virDomainDeviceDefParse(xml, vm->def,
> + driver->xmlopt, NULL,
> + parse_flags)))
> + return -1;
> +
> + if (virDomainDeviceValidateAliasForHotplug(vm, devLive,
> + VIR_DOMAIN_AFFECT_LIVE) < 0)
> + return -1;
> +
> + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL,
> + VIR_DOMAIN_DEVICE_ACTION_ATTACH,
> + true) < 0)
> + return -1;
> +
> + if (testDomainAttachDeviceLive(vm, devLive, driver) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +testDomainAttachDeviceFlags(virDomainPtr domain,
> + const char *xml,
> + unsigned int flags) {
> +
> + testDriver *driver = domain->conn->privateData;
> + virDomainObj *vm = NULL;
> + int ret = -1;
> +
> + if (!(vm = testDomObjFromDomain(domain)))
> + return -1;
> +
> + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> + goto cleanup;
> +
> + if (testDomainAttachDeviceLiveAndConfig(vm, driver, xml, flags) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
> +static int
> +testDomainAttachDevice(virDomainPtr domain, const char *xml)
> +{
> + return testDomainAttachDeviceFlags(domain, xml, 0);
> +}
> +
> +/* search for a hostdev matching dev and detach it */
> +static int
> +testDomainDetachPrepHostdev(virDomainObj *vm,
> + virDomainHostdevDef *match,
> + virDomainHostdevDef **detach)
> +{
> + virDomainHostdevSubsys *subsys = &match->source.subsys;
> + virDomainHostdevSubsysPCI *pcisrc = &subsys->u.pci;
> + virDomainHostdevDef *hostdev = NULL;
> +
> + if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("hot unplug is not supported for hostdev mode '%s'"),
> + virDomainHostdevModeTypeToString(match->mode));
> + return -1;
> + }
> +
> + if (virDomainHostdevFind(vm->def, match, &hostdev) < 0) {
> + if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> + virReportError(VIR_ERR_DEVICE_MISSING,
> + _("host pci device " VIR_PCI_DEVICE_ADDRESS_FMT
> + " not found"),
We prefer error messages to be on a single line. Again, we have a
syntax-check rule for that. And in this specific case,
virPCIDeviceAddressAsString() looks like the way to format PCI address.
> + pcisrc->addr.domain, pcisrc->addr.bus,
> + pcisrc->addr.slot, pcisrc->addr.function);
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected hostdev type %d"), subsys->type);
> + }
> + return -1;
> + }
> +
> + *detach = hostdev;
> +
> + return 0;
> +}
> +
> +static int
> +testDomainRemoveHostDevice(testDriver *driver G_GNUC_UNUSED,
> + virDomainObj *vm,
> + virDomainHostdevDef *hostdev)
> +{
> + virDomainNetDef *net = NULL;
> + size_t i;
> +
> + VIR_DEBUG("Removing host device %s from domain %p %s",
> + hostdev->info->alias, vm, vm->def->name);
> +
> + if (hostdev->parentnet) {
> + net = hostdev->parentnet;
> + for (i = 0; i < vm->def->nnets; i++) {
> + if (vm->def->nets[i] == hostdev->parentnet) {
> + virDomainNetRemove(vm->def, i);
> + break;
> + }
> + }
> + }
Fortunately, we do not implement <interface type="hostdev/> the same way
as in QEMU driver. In QEMU driver, such <interface/> is also added to
<hostdev/> (well, vm->def->hostdevs[]), which is not the case in test
driver. Thus, this is not needed.
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + if (vm->def->hostdevs[i] == hostdev) {
> + virDomainHostdevRemove(vm->def, i);
> + break;
> + }
> + }
> +
> + virDomainAuditHostdev(vm, hostdev, "detach", true);
> +
> + virDomainHostdevDefFree(hostdev);
> +
> + if (net) {
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + g_autoptr(virConnect) conn = virGetConnectNetwork();
> + if (conn)
> + virDomainNetReleaseActualDevice(conn, vm->def, net);
> + else
> + VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname));
> + }
> + virDomainNetDefFree(net);
> + }
This is also not needed. The test driver network does not implement port
APIs, but firstly, we don't have that aforementioned shadowing of
<interface/> and <hostdev/> devices.
> +
> + return 0;
> +}
> +
> +static int
> +testDomainRemoveDevice(testDriver *driver,
> + virDomainObj *vm,
> + virDomainDeviceDef *dev)
> +{
> + virDomainDeviceInfo *info;
> + virObjectEvent *event;
> + g_autofree char *alias = NULL;
> +
> + /*
> + * save the alias to use when sending a DEVICE_REMOVED event after
> + * all other teardown is complete
> + */
> + if ((info = virDomainDeviceGetInfo(dev)))
> + alias = g_strdup(info->alias);
> + info = NULL;
> +
> + if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("don't know how to remove a %s device"),
> + virDomainDeviceTypeToString(dev->type));
> + goto out;
So if this is called with a device != _HOSTDEV but an alias set, an
event is emitted? That doesn't feel right.
> + }
> +
> + if (testDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
> + return -1;
> +
> +out:
> + event = virDomainEventDeviceRemovedNewFromObj(vm, alias);
> + virObjectEventStateQueue(driver->eventState, event);
> +
> + return 0;
> +}
> +
> +static int
> +testDomainDetachDeviceLive(virDomainObj *vm,
> + virDomainDeviceDef *match,
> + testDriver *driver)
Nitpick, arguments should merely follow their importance: driver > vm >
device.
> +{
> + virDomainDeviceDef detach = { .type = match->type };
> + virDomainDeviceInfo *info = NULL;
> +
> + if ((virDomainDeviceType)match->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("live detach of device '%s' is not supported"),
> + virDomainDeviceTypeToString(match->type));
> + return -1;
> + }
> +
> + if (testDomainDetachPrepHostdev(vm, match->data.hostdev,
> + &detach.data.hostdev) < 0)
> + return -1;
> +
> + /* "detach" now points to the actual device we want to detach */
> +
> + if (!(info = virDomainDeviceGetInfo(&detach))) {
> + /*
> + * This should never happen, since all of the device types in
> + * the switch cases that end with a "break" instead of a
> + * return have a virDeviceInfo in them.
> + */
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("device of type '%s' has no device info"),
> + virDomainDeviceTypeToString(detach.type));
> + return -1;
> + }
> +
> + /* Make generic validation checks common to all device types */
> +
> + if (!info->alias) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Cannot detach %s device with no alias"),
> + virDomainDeviceTypeToString(detach.type));
> + return -1;
> + }
> +
> + return testDomainRemoveDevice(driver, vm, &detach);
> +}
> +
> +static int
> +testDomainDetachDeviceAliasLiveAndConfig(testDriver *driver,
> + virDomainObj *vm,
> + const char *alias,
> + unsigned int flags)
> +{
> + virDomainDef *def = NULL;
> + g_autoptr(virDomainDef) vmdef = NULL;
This variable is initalized to NULL ...
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
> +
> + if (virDomainObjGetDefs(vm, flags, &def, NULL) < 0)
> + return -1;
> +
> + if (def) {
> + virDomainDeviceDef dev;
> +
> + if (virDomainDefFindDevice(def, alias, &dev, true) < 0)
> + return -1;
> +
> + if (testDomainDetachDeviceLive(vm, &dev, driver) < 0)
> + return -1;
> + }
> +
> + if (vmdef) {
and never set really, rendering this if to be a dead code.
> + if (virDomainDefSave(vmdef, driver->xmlopt, NULL) < 0)
> + return -1;
> + virDomainObjAssignDef(vm, &vmdef, false, NULL);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +testDomainDetachDeviceAlias(virDomainPtr dom,
> + const char *alias,
> + unsigned int flags)
> +{
> + testDriver *driver = dom->conn->privateData;
> + virDomainObj *vm = NULL;
> + int ret = -1;
> +
> + if (!(vm = testDomObjFromDomain(dom)))
> + return -1;
> +
> + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> + goto cleanup;
> +
> + if (testDomainDetachDeviceAliasLiveAndConfig(driver, vm, alias, flags) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
>
> /*
> * Test driver
> @@ -10058,6 +10433,9 @@ static virHypervisorDriver testHypervisorDriver = {
> .connectListDomains = testConnectListDomains, /* 0.1.1 */
> .connectNumOfDomains = testConnectNumOfDomains, /* 0.1.1 */
> .connectListAllDomains = testConnectListAllDomains, /* 0.9.13 */
> + .domainAttachDevice = testDomainAttachDevice, /* 9.10.0 */
> + .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 9.10.0 */
> + .domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 9.10.0 */
These will have to be 10.0.0 becasue this is going to miss 9.10.0
release.
> .domainCreateXML = testDomainCreateXML, /* 0.1.4 */
> .domainCreateXMLWithFiles = testDomainCreateXMLWithFiles, /* 5.7.0 */
> .domainLookupByID = testDomainLookupByID, /* 0.1.1 */
Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
> -----Original Message-----
> From: Michal Prívozník <mprivozn@redhat.com>
> Sent: Tuesday, November 28, 2023 1:19 PM
> To: Thanos Makatos <thanos.makatos@nutanix.com>; devel@lists.libvirt.org
> Subject: Re: [PATCH V5] support for hotplug/hotunplug in test hypervisor
>
> On 11/9/23 12:14, Thanos Makatos wrote:
> > Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
> >
> > ---
> >
> > Changed since v4:
> > * removed inadvertent calls to functions
> virNWFilterReadLockFilterUpdates/virNWFilterUnlockFilterUpdates
> > (original patch was based on v8.0.0)
> >
> > ---
> > src/test/test_driver.c | 382
> ++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 380 insertions(+), 2 deletions(-)
>
> Ooops, this has missed my attention and unfortunately upcoming release
> too as we are in freeze. Sorry. To make it up to you, I'm fixing all the
> small issues I'm raising below and will push once after the release.
>
> I've uploaded the fixup patch (that I'll squash into yours before
> merging) here, if you want to take a look:
>
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__gitlab.com_MichalPrivoznik_libvirt_-2D_commits_test-5Fdriver-
> 5Fhotplug_-3Fref-5Ftype-
> 3Dheads&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=XTpYsh5Ps2zJvtw6ogtti
> 46atk736SI4vgsJiUKIyDE&m=o5_V_Q03_HbW5P4oVf5Tez4K2mYV_jf35yvol4O5
> KioGaofPI3B07QN9CvepCKAc&s=kiKbd_XHWn3_p3luPq89G0pku0iApEIs18MAtL
> u_d88&e=
>
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index e87d7cfd44..effb7f9880 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -26,8 +26,6 @@
> > #include <unistd.h>
> > #include <sys/stat.h>
> > #include <libxml/xmlsave.h>
> > -
> > -
>
> We like to keep these separate to make it more obvious where public
> header section ends and private header section starts.
>
> > #include "virerror.h"
> > #include "datatypes.h"
> > #include "test_driver.h"
> > @@ -38,9 +36,12 @@
> > #include "virnetworkobj.h"
> > #include "interface_conf.h"
> > #include "checkpoint_conf.h"
> > +#include "domain_addr.h"
> > +#include "domain_audit.h"
>
> This shouldn't be needed. domain_audit contains functions to audit real
> resource auditing (e.g. assigning a real device to a guest) - test
> driver should not do such thing.
>
> > #include "domain_conf.h"
> > #include "domain_driver.h"
> > #include "domain_event.h"
> > +#include "domain_validate.h"
> > #include "network_event.h"
> > #include "snapshot_conf.h"
> > #include "virfdstream.h"
> > @@ -50,6 +51,7 @@
> > #include "node_device_conf.h"
> > #include "virnodedeviceobj.h"
> > #include "node_device_event.h"
> > +#include "vircgroup.h"
>
> And this too shouldn't be here. We do not set up any cgroups for domains
> under the test driver.
>
> > #include "virxml.h"
> > #include "virthread.h"
> > #include "virlog.h"
> > @@ -10035,6 +10037,379 @@
> testConnectGetDomainCapabilities(virConnectPtr conn G_GNUC_UNUSED,
> > return virDomainCapsFormat(domCaps);
> > }
> >
> > +static int
> > +testDomainAttachHostPCIDevice(testDriver *driver G_GNUC_UNUSED,
> > + virDomainObj *vm,
> > + virDomainHostdevDef *hostdev)
> > +{
> > + int backend = hostdev->source.subsys.u.pci.backend;
> > +
> > + switch ((virDomainHostdevSubsysPCIBackendType)backend) {
> > + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
> > + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
> > + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> > + break;
> > +
> > + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
> > + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("test hypervisor does not support device assignment mode
> '%s'"),
>
> I'll raise it here: for translatable strings, we've switched to using
> more robust format: %1$s because it bit us in the past when one of
> locales contained wrong data causing us to crash. This should have been
> caught by 'ninja test'.
>
> > + virDomainHostdevSubsysPCIBackendTypeToString(backend));
> > + return -1;
> > + }
> > +
> > + if (!virDomainObjIsActive(vm)) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("guest unexpectedly quit during hotplug"));
> > + return -1;
> > + }
> > +
>
> This check makes sense in QEMU world, because we unlock the domain
> object (virDomainObj) whilst talking to QEMU on monitor. But in the test
> driver it's needless - the object is locked throughough whole API run.
>
> > + VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1);
> > +
> > + virDomainAuditHostdev(vm, hostdev, "attach", true);
>
> This shouldn't be here.
>
> > +
> > + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testDomainAttachHostDevice(testDriver *driver,
> > + virDomainObj *vm,
> > + virDomainHostdevDef *hostdev)
> > +{
> > + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("hotplug is not supported for hostdev mode '%s'"),
> > + virDomainHostdevModeTypeToString(hostdev->mode));
> > + return -1;
> > + }
> > +
> > + if (hostdev->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("hotplug is not supported for hostdev subsys type '%s'"),
> > + virDomainHostdevSubsysTypeToString(hostdev-
> >source.subsys.type));
> > + return -1;
> > + }
> > +
> > + return testDomainAttachHostPCIDevice(driver, vm, hostdev);
> > +}
> > +
> > +static int
> > +testDomainAttachDeviceLive(virDomainObj *vm,
> > + virDomainDeviceDef *dev,
> > + testDriver *driver)
> > +{
> > + int ret = -1;
> > + const char *alias = NULL;
> > +
> > + if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
>
> The ->type member is already of the correct type, no need to type cast it.
>
> > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > + _("live attach of device '%s' is not supported"),
> > + virDomainDeviceTypeToString(dev->type));
> > + return -1;
> > + }
> > +
> > + testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
> > + if ((ret = testDomainAttachHostDevice(driver, vm, dev->data.hostdev)) !=
> 0)
> > + return ret;
> > +
> > + alias = dev->data.hostdev->info->alias;
> > + dev->data.hostdev = NULL;
> > +
> > + if (alias) {
> > + virObjectEvent *event;
> > + event = virDomainEventDeviceAddedNewFromObj(vm, alias);
> > + virObjectEventStateQueue(driver->eventState, event);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
> > + testDriver *driver,
> > + const char *xml,
> > + unsigned int flags)
> > +{
> > + g_autoptr(virDomainDeviceDef) devConf = NULL;
> > + g_autoptr(virDomainDeviceDef) devLive = NULL;
> > + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> > + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
> > +
> > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
> > +
> > + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> > + if (!(devLive = virDomainDeviceDefParse(xml, vm->def,
> > + driver->xmlopt, NULL,
> > + parse_flags)))
> > + return -1;
> > +
> > + if (virDomainDeviceValidateAliasForHotplug(vm, devLive,
> > + VIR_DOMAIN_AFFECT_LIVE) < 0)
> > + return -1;
> > +
> > + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL,
> > + VIR_DOMAIN_DEVICE_ACTION_ATTACH,
> > + true) < 0)
> > + return -1;
> > +
> > + if (testDomainAttachDeviceLive(vm, devLive, driver) < 0)
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testDomainAttachDeviceFlags(virDomainPtr domain,
> > + const char *xml,
> > + unsigned int flags) {
> > +
> > + testDriver *driver = domain->conn->privateData;
> > + virDomainObj *vm = NULL;
> > + int ret = -1;
> > +
> > + if (!(vm = testDomObjFromDomain(domain)))
> > + return -1;
> > +
> > + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> > + goto cleanup;
> > +
> > + if (testDomainAttachDeviceLiveAndConfig(vm, driver, xml, flags) < 0)
> > + goto cleanup;
> > +
> > + ret = 0;
> > +
> > + cleanup:
> > + virDomainObjEndAPI(&vm);
> > + return ret;
> > +}
> > +
> > +static int
> > +testDomainAttachDevice(virDomainPtr domain, const char *xml)
> > +{
> > + return testDomainAttachDeviceFlags(domain, xml, 0);
> > +}
> > +
> > +/* search for a hostdev matching dev and detach it */
> > +static int
> > +testDomainDetachPrepHostdev(virDomainObj *vm,
> > + virDomainHostdevDef *match,
> > + virDomainHostdevDef **detach)
> > +{
> > + virDomainHostdevSubsys *subsys = &match->source.subsys;
> > + virDomainHostdevSubsysPCI *pcisrc = &subsys->u.pci;
> > + virDomainHostdevDef *hostdev = NULL;
> > +
> > + if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("hot unplug is not supported for hostdev mode '%s'"),
> > + virDomainHostdevModeTypeToString(match->mode));
> > + return -1;
> > + }
> > +
> > + if (virDomainHostdevFind(vm->def, match, &hostdev) < 0) {
> > + if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> > + virReportError(VIR_ERR_DEVICE_MISSING,
> > + _("host pci device " VIR_PCI_DEVICE_ADDRESS_FMT
> > + " not found"),
>
> We prefer error messages to be on a single line. Again, we have a
> syntax-check rule for that. And in this specific case,
> virPCIDeviceAddressAsString() looks like the way to format PCI address.
>
> > + pcisrc->addr.domain, pcisrc->addr.bus,
> > + pcisrc->addr.slot, pcisrc->addr.function);
> > + } else {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("unexpected hostdev type %d"), subsys->type);
> > + }
> > + return -1;
> > + }
> > +
> > + *detach = hostdev;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testDomainRemoveHostDevice(testDriver *driver G_GNUC_UNUSED,
> > + virDomainObj *vm,
> > + virDomainHostdevDef *hostdev)
> > +{
> > + virDomainNetDef *net = NULL;
> > + size_t i;
> > +
> > + VIR_DEBUG("Removing host device %s from domain %p %s",
> > + hostdev->info->alias, vm, vm->def->name);
> > +
> > + if (hostdev->parentnet) {
> > + net = hostdev->parentnet;
> > + for (i = 0; i < vm->def->nnets; i++) {
> > + if (vm->def->nets[i] == hostdev->parentnet) {
> > + virDomainNetRemove(vm->def, i);
> > + break;
> > + }
> > + }
> > + }
>
> Fortunately, we do not implement <interface type="hostdev/> the same way
> as in QEMU driver. In QEMU driver, such <interface/> is also added to
> <hostdev/> (well, vm->def->hostdevs[]), which is not the case in test
> driver. Thus, this is not needed.
>
> > +
> > + for (i = 0; i < vm->def->nhostdevs; i++) {
> > + if (vm->def->hostdevs[i] == hostdev) {
> > + virDomainHostdevRemove(vm->def, i);
> > + break;
> > + }
> > + }
> > +
> > + virDomainAuditHostdev(vm, hostdev, "detach", true);
> > +
> > + virDomainHostdevDefFree(hostdev);
> > +
> > + if (net) {
> > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> > + g_autoptr(virConnect) conn = virGetConnectNetwork();
> > + if (conn)
> > + virDomainNetReleaseActualDevice(conn, vm->def, net);
> > + else
> > + VIR_WARN("Unable to release network device '%s'", NULLSTR(net-
> >ifname));
> > + }
> > + virDomainNetDefFree(net);
> > + }
>
> This is also not needed. The test driver network does not implement port
> APIs, but firstly, we don't have that aforementioned shadowing of
> <interface/> and <hostdev/> devices.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testDomainRemoveDevice(testDriver *driver,
> > + virDomainObj *vm,
> > + virDomainDeviceDef *dev)
> > +{
> > + virDomainDeviceInfo *info;
> > + virObjectEvent *event;
> > + g_autofree char *alias = NULL;
> > +
> > + /*
> > + * save the alias to use when sending a DEVICE_REMOVED event after
> > + * all other teardown is complete
> > + */
> > + if ((info = virDomainDeviceGetInfo(dev)))
> > + alias = g_strdup(info->alias);
> > + info = NULL;
> > +
> > + if ((virDomainDeviceType)dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
> > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > + _("don't know how to remove a %s device"),
> > + virDomainDeviceTypeToString(dev->type));
> > + goto out;
>
> So if this is called with a device != _HOSTDEV but an alias set, an
> event is emitted? That doesn't feel right.
>
> > + }
> > +
> > + if (testDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
> > + return -1;
> > +
> > +out:
> > + event = virDomainEventDeviceRemovedNewFromObj(vm, alias);
> > + virObjectEventStateQueue(driver->eventState, event);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testDomainDetachDeviceLive(virDomainObj *vm,
> > + virDomainDeviceDef *match,
> > + testDriver *driver)
>
> Nitpick, arguments should merely follow their importance: driver > vm >
> device.
>
> > +{
> > + virDomainDeviceDef detach = { .type = match->type };
> > + virDomainDeviceInfo *info = NULL;
> > +
> > + if ((virDomainDeviceType)match->type !=
> VIR_DOMAIN_DEVICE_HOSTDEV) {
> > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > + _("live detach of device '%s' is not supported"),
> > + virDomainDeviceTypeToString(match->type));
> > + return -1;
> > + }
> > +
> > + if (testDomainDetachPrepHostdev(vm, match->data.hostdev,
> > + &detach.data.hostdev) < 0)
> > + return -1;
> > +
> > + /* "detach" now points to the actual device we want to detach */
> > +
> > + if (!(info = virDomainDeviceGetInfo(&detach))) {
> > + /*
> > + * This should never happen, since all of the device types in
> > + * the switch cases that end with a "break" instead of a
> > + * return have a virDeviceInfo in them.
> > + */
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("device of type '%s' has no device info"),
> > + virDomainDeviceTypeToString(detach.type));
> > + return -1;
> > + }
> > +
> > + /* Make generic validation checks common to all device types */
> > +
> > + if (!info->alias) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Cannot detach %s device with no alias"),
> > + virDomainDeviceTypeToString(detach.type));
> > + return -1;
> > + }
> > +
> > + return testDomainRemoveDevice(driver, vm, &detach);
> > +}
> > +
> > +static int
> > +testDomainDetachDeviceAliasLiveAndConfig(testDriver *driver,
> > + virDomainObj *vm,
> > + const char *alias,
> > + unsigned int flags)
> > +{
> > + virDomainDef *def = NULL;
> > + g_autoptr(virDomainDef) vmdef = NULL;
>
> This variable is initalized to NULL ...
>
> > +
> > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
> > +
> > + if (virDomainObjGetDefs(vm, flags, &def, NULL) < 0)
> > + return -1;
> > +
> > + if (def) {
> > + virDomainDeviceDef dev;
> > +
> > + if (virDomainDefFindDevice(def, alias, &dev, true) < 0)
> > + return -1;
> > +
> > + if (testDomainDetachDeviceLive(vm, &dev, driver) < 0)
> > + return -1;
> > + }
> > +
> > + if (vmdef) {
>
> and never set really, rendering this if to be a dead code.
>
> > + if (virDomainDefSave(vmdef, driver->xmlopt, NULL) < 0)
> > + return -1;
> > + virDomainObjAssignDef(vm, &vmdef, false, NULL);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +testDomainDetachDeviceAlias(virDomainPtr dom,
> > + const char *alias,
> > + unsigned int flags)
> > +{
> > + testDriver *driver = dom->conn->privateData;
> > + virDomainObj *vm = NULL;
> > + int ret = -1;
> > +
> > + if (!(vm = testDomObjFromDomain(dom)))
> > + return -1;
> > +
> > + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> > + goto cleanup;
> > +
> > + if (testDomainDetachDeviceAliasLiveAndConfig(driver, vm, alias, flags) < 0)
> > + goto cleanup;
> > +
> > + ret = 0;
> > +
> > + cleanup:
> > + virDomainObjEndAPI(&vm);
> > + return ret;
> > +}
> >
> > /*
> > * Test driver
> > @@ -10058,6 +10433,9 @@ static virHypervisorDriver testHypervisorDriver =
> {
> > .connectListDomains = testConnectListDomains, /* 0.1.1 */
> > .connectNumOfDomains = testConnectNumOfDomains, /* 0.1.1 */
> > .connectListAllDomains = testConnectListAllDomains, /* 0.9.13 */
> > + .domainAttachDevice = testDomainAttachDevice, /* 9.10.0 */
> > + .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 9.10.0 */
> > + .domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 9.10.0 */
>
> These will have to be 10.0.0 becasue this is going to miss 9.10.0
> release.
>
> > .domainCreateXML = testDomainCreateXML, /* 0.1.4 */
> > .domainCreateXMLWithFiles = testDomainCreateXMLWithFiles, /* 5.7.0 */
> > .domainLookupByID = testDomainLookupByID, /* 0.1.1 */
>
>
> Michal
Thanks a lot!
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2024 Red Hat, Inc.