On 11/9/23 12:14, Thanos Makatos wrote: > Signed-off-by: Thanos Makatos <thanos.makatos@xxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
