7:18 a.m.
On 11/9/23 12:14, Thanos Makatos wrote:
Signed-off-by: Thanos Makatos <thanos.makatos(a)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/...
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