> -----Original Message----- > From: Michal Prívozník <mprivozn@xxxxxxxxxx> > Sent: Tuesday, November 28, 2023 1:19 PM > To: Thanos Makatos <thanos.makatos@xxxxxxxxxxx>; devel@xxxxxxxxxxxxxxxxx > 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@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://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@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
