[PATCH V3] support for hotplug/hotunplug in test hypervisor
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com> --- src/test/test_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e87d7cfd44..d605649262 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10035,6 +10035,62 @@ testConnectGetDomainCapabilities(virConnectPtr conn G_GNUC_UNUSED, return virDomainCapsFormat(domCaps); } +static int +testVirDomainAttachDeviceFlags(virDomainPtr domain, + const char *xml, + unsigned int flags G_GNUC_UNUSED) { + + int ret = -1; + virDomainObj *vm; + testDriver *driver; + virDomainDeviceDef *devConf; + + if (!(vm = testDomObjFromDomain(domain))) + return -1; + + driver = domain->conn->privateData; + + if (!(devConf = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt, + NULL, 0))) + goto out; + + VIR_APPEND_ELEMENT(vm->def->hostdevs, vm->def->nhostdevs, + devConf->data.hostdev); + virDomainDeviceDefFree(devConf); + ret = 0; +out: + virDomainObjEndAPI(&vm); + return ret; +} + +static int +testVirDomainAttachDevice(virDomainPtr domain, const char *xml) +{ + return testVirDomainAttachDeviceFlags(domain, xml, 0); +} + +static int +testVirDomainDetachDeviceAlias(virDomainPtr domain, + const char *alias, + unsigned int flags G_GNUC_UNUSED) +{ + virDomainObj *vm; + size_t i; + bool found = false; + + if (!(vm = testDomObjFromDomain(domain))) + return -1; + + for (i = 0; i < vm->def->nhostdevs && !found; i++) { + if (!strcmp(vm->def->hostdevs[i]->info->alias, alias)) { + virDomainHostdevDefFree(vm->def->hostdevs[i]); + VIR_DELETE_ELEMENT(vm->def->hostdevs, i, vm->def->nhostdevs); + found = true; + } + } + virDomainObjEndAPI(&vm); + return found ? 0 : -1; +} /* * Test driver @@ -10058,6 +10114,9 @@ static virHypervisorDriver testHypervisorDriver = { .connectListDomains = testConnectListDomains, /* 0.1.1 */ .connectNumOfDomains = testConnectNumOfDomains, /* 0.1.1 */ .connectListAllDomains = testConnectListAllDomains, /* 0.9.13 */ + .domainAttachDevice = testVirDomainAttachDevice, /* 9.9.0 */ + .domainAttachDeviceFlags = testVirDomainAttachDeviceFlags, /* 9.9.0 */ + .domainDetachDeviceAlias = testVirDomainDetachDeviceAlias, /* 9.9.0 */ .domainCreateXML = testDomainCreateXML, /* 0.1.4 */ .domainCreateXMLWithFiles = testDomainCreateXMLWithFiles, /* 5.7.0 */ .domainLookupByID = testDomainLookupByID, /* 0.1.1 */ -- 2.27.0
On 10/30/23 17:02, Thanos Makatos wrote:
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com> --- src/test/test_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
We're getting there.
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e87d7cfd44..d605649262 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10035,6 +10035,62 @@ testConnectGetDomainCapabilities(virConnectPtr conn G_GNUC_UNUSED, return virDomainCapsFormat(domCaps); }
+static int +testVirDomainAttachDeviceFlags(virDomainPtr domain,
We construct names a bit differently. For virDomainSomething() public API, respective driver implementations should be named: s/^vir/${driver}/ for instance qemuDomainSomething() for the qemu driver, testDomainSomething() for the test driver. Therefore, this should have been: testDomainAttachDeviceFlags().
+ const char *xml, + unsigned int flags G_GNUC_UNUSED) {
Nope, flags must be checked. Always. We have a handy function for that (virCheckFlags()) but you'll need a slightly different approach anyway.
+ + int ret = -1; + virDomainObj *vm; + testDriver *driver; + virDomainDeviceDef *devConf; + + if (!(vm = testDomObjFromDomain(domain))) + return -1; + + driver = domain->conn->privateData; + + if (!(devConf = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt, + NULL, 0))) + goto out; + + VIR_APPEND_ELEMENT(vm->def->hostdevs, vm->def->nhostdevs, + devConf->data.hostdev); + virDomainDeviceDefFree(devConf);
This can't work. There are plenty of more devices that can be hotplugged than just <hostdev>. In fact, if an <interface/> was on the input, then devConf->data.hostdev points to some garbage. But anyway, I think we want more elaborate approach. The whole point of the test driver is to mimic real guest(s) without resources needed. IOW, it's just a stub that developers can use to write/test their code against and as such it should mimic real world as close as possible/practical. For instance, in real world this API accepts basically two flags: VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG which can be specified at the same time or independently of each other. But this code does not account for that. What you can do is take inspiration in some real world driver, copy it over to test driver and then remove actual talking to hypervisor. For instance, you can see how attach is done in QEMU driver (which is our most mature driver and thus code there is usually maintained the best), keep all vir...() calls and remove qemu...() calls, basically. Oh, and do not forget to emit an event after successful hotplug. It contributes to "real world feeling".
+ ret = 0; +out: + virDomainObjEndAPI(&vm); + return ret; +} + +static int +testVirDomainAttachDevice(virDomainPtr domain, const char *xml) +{ + return testVirDomainAttachDeviceFlags(domain, xml, 0); +} + +static int +testVirDomainDetachDeviceAlias(virDomainPtr domain, + const char *alias, + unsigned int flags G_GNUC_UNUSED) +{ + virDomainObj *vm; + size_t i; + bool found = false; + + if (!(vm = testDomObjFromDomain(domain))) + return -1; + + for (i = 0; i < vm->def->nhostdevs && !found; i++) { + if (!strcmp(vm->def->hostdevs[i]->info->alias, alias)) { + virDomainHostdevDefFree(vm->def->hostdevs[i]); + VIR_DELETE_ELEMENT(vm->def->hostdevs, i, vm->def->nhostdevs); + found = true; + } + }
Same comments here.
+ virDomainObjEndAPI(&vm); + return found ? 0 : -1; +}
/* * Test driver @@ -10058,6 +10114,9 @@ static virHypervisorDriver testHypervisorDriver = { .connectListDomains = testConnectListDomains, /* 0.1.1 */ .connectNumOfDomains = testConnectNumOfDomains, /* 0.1.1 */ .connectListAllDomains = testConnectListAllDomains, /* 0.9.13 */ + .domainAttachDevice = testVirDomainAttachDevice, /* 9.9.0 */ + .domainAttachDeviceFlags = testVirDomainAttachDeviceFlags, /* 9.9.0 */ + .domainDetachDeviceAlias = testVirDomainDetachDeviceAlias, /* 9.9.0 */
Unfortunately, 9.9.0 is on it's way (we have entered feature freeze ~ a week ago). The soonest this can land in is 9.10.0. If you feel this has become bigger task than you anticipated then I can post patches instead. Michal
On Tue, Oct 31, 2023 at 10:49:31AM +0100, Michal Prívozník wrote:
+ VIR_APPEND_ELEMENT(vm->def->hostdevs, vm->def->nhostdevs,
But anyway, I think we want more elaborate approach. The whole point of the test driver is to mimic real guest(s) without resources needed. IOW,
Michal, point taken, but are you saying you wouldn't take such a change unless it implemented attach for all possible devices? Or would you be OK with a partial implementation that handles hostdev, but returns the equivalent of ENOTSUP for everything else? Reason I ask is that right now we only have a need for hostdev testing. regards john
On Tue, Oct 31, 2023 at 10:12:51AM +0000, John Levon wrote:
On Tue, Oct 31, 2023 at 10:49:31AM +0100, Michal Prívozník wrote:
+ VIR_APPEND_ELEMENT(vm->def->hostdevs, vm->def->nhostdevs,
But anyway, I think we want more elaborate approach. The whole point of the test driver is to mimic real guest(s) without resources needed. IOW,
Michal, point taken, but are you saying you wouldn't take such a change unless it implemented attach for all possible devices?
Or would you be OK with a partial implementation that handles hostdev, but returns the equivalent of ENOTSUP for everything else?
The minimum bar required is "doesn't crash". Right now it'll crash due to accessing union branches without checking the type first. Checking the device type and returning ENOTSUP for things which are not hostdevs will be sufficient if you don't want to spend time wiring up other types of device. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/31/23 11:12, John Levon wrote:
On Tue, Oct 31, 2023 at 10:49:31AM +0100, Michal Prívozník wrote:
+ VIR_APPEND_ELEMENT(vm->def->hostdevs, vm->def->nhostdevs,
But anyway, I think we want more elaborate approach. The whole point of the test driver is to mimic real guest(s) without resources needed. IOW,
Michal, point taken, but are you saying you wouldn't take such a change unless it implemented attach for all possible devices?
Or would you be OK with a partial implementation that handles hostdev, but returns the equivalent of ENOTSUP for everything else?
Yeah, that's perfectly acceptable. If you go with what I suggested and basically copy qemuDomainAttachDeviceConfig() over to the test driver, it's perfectly acceptable when in the big switch() only hostdevs are implemented and for everything else an error is reported. Meanwhile, I could move parts of the qemu function into say virDomainDeviceDefAdd(virDomainDef *def, virDomainDeviceDef *dev) which would then handle adding a device into corresponding array in domain definition (maybe with duplicate detection?). Same for virDomainDeviceDefRemove().
Reason I ask is that right now we only have a need for hostdev testing.
That much I figured. And indeed I too am guilty of implementing bare minimum (fairly recently: v9.8.0-rc1~11) but the API I implemented behaves just like in other drivers. Michal
-----Original Message----- From: Michal Prívozník <mprivozn@redhat.com> Sent: Tuesday, October 31, 2023 9:50 AM To: Thanos Makatos <thanos.makatos@nutanix.com>; devel@lists.libvirt.org Subject: Re: [PATCH V3] support for hotplug/hotunplug in test hypervisor
On 10/30/23 17:02, Thanos Makatos wrote:
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com> --- src/test/test_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
We're getting there.
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e87d7cfd44..d605649262 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10035,6 +10035,62 @@ testConnectGetDomainCapabilities(virConnectPtr conn G_GNUC_UNUSED, return virDomainCapsFormat(domCaps); }
+static int +testVirDomainAttachDeviceFlags(virDomainPtr domain,
We construct names a bit differently. For virDomainSomething() public API, respective driver implementations should be named: s/^vir/${driver}/ for instance qemuDomainSomething() for the qemu driver, testDomainSomething() for the test driver. Therefore, this should have been: testDomainAttachDeviceFlags().
Ack.
+ const char *xml, + unsigned int flags G_GNUC_UNUSED) {
Nope, flags must be checked. Always. We have a handy function for that (virCheckFlags()) but you'll need a slightly different approach anyway.
Ack.
+ + int ret = -1; + virDomainObj *vm; + testDriver *driver; + virDomainDeviceDef *devConf; + + if (!(vm = testDomObjFromDomain(domain))) + return -1; + + driver = domain->conn->privateData; + + if (!(devConf = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt, + NULL, 0))) + goto out; + + VIR_APPEND_ELEMENT(vm->def->hostdevs, vm->def->nhostdevs, + devConf->data.hostdev); + virDomainDeviceDefFree(devConf);
This can't work. There are plenty of more devices that can be hotplugged than just <hostdev>. In fact, if an <interface/> was on the input, then devConf->data.hostdev points to some garbage.
But anyway, I think we want more elaborate approach. The whole point of the test driver is to mimic real guest(s) without resources needed. IOW, it's just a stub that developers can use to write/test their code against and as such it should mimic real world as close as possible/practical. For instance, in real world this API accepts basically two flags: VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG which can be specified at the same time or independently of each other. But this code does not account for that.
We care about VIR_DOMAIN_AFFECT_LIVE so I'll support only this, for now.
What you can do is take inspiration in some real world driver, copy it over to test driver and then remove actual talking to hypervisor. For instance, you can see how attach is done in QEMU driver (which is our most mature driver and thus code there is usually maintained the best), keep all vir...() calls and remove qemu...() calls, basically.
I followed your suggestion and ended up with similar but substantially smaller code as I had to omit any code that eventually looks at the actual system (e.g. checking that the device sysfs entry exists, setting up cgroups, etc.). One thing I didn't clarify in the original patch is that we don’t care whether the physical device exists, we just want the test hypervisor to pretend it does. I suppose we can support this by manually creating a mock sysfs directory which the test hypervisor can use, however such functionality isn't necessary for our immediate needs so I'd be inclined to skip it for now.
Oh, and do not forget to emit an event after successful hotplug. It contributes to "real world feeling".
Are you referring to virDomainAuditHostdev?
+ ret = 0; +out: + virDomainObjEndAPI(&vm); + return ret; +} + +static int +testVirDomainAttachDevice(virDomainPtr domain, const char *xml) +{ + return testVirDomainAttachDeviceFlags(domain, xml, 0); +} + +static int +testVirDomainDetachDeviceAlias(virDomainPtr domain, + const char *alias, + unsigned int flags G_GNUC_UNUSED) +{ + virDomainObj *vm; + size_t i; + bool found = false; + + if (!(vm = testDomObjFromDomain(domain))) + return -1; + + for (i = 0; i < vm->def->nhostdevs && !found; i++) { + if (!strcmp(vm->def->hostdevs[i]->info->alias, alias)) { + virDomainHostdevDefFree(vm->def->hostdevs[i]); + VIR_DELETE_ELEMENT(vm->def->hostdevs, i, vm->def->nhostdevs); + found = true; + } + }
Same comments here.
Ack.
+ virDomainObjEndAPI(&vm); + return found ? 0 : -1; +}
/* * Test driver @@ -10058,6 +10114,9 @@ static virHypervisorDriver testHypervisorDriver = { .connectListDomains = testConnectListDomains, /* 0.1.1 */ .connectNumOfDomains = testConnectNumOfDomains, /* 0.1.1 */ .connectListAllDomains = testConnectListAllDomains, /* 0.9.13 */ + .domainAttachDevice = testVirDomainAttachDevice, /* 9.9.0 */ + .domainAttachDeviceFlags = testVirDomainAttachDeviceFlags, /* 9.9.0 */ + .domainDetachDeviceAlias = testVirDomainDetachDeviceAlias, /* 9.9.0 */
Unfortunately, 9.9.0 is on it's way (we have entered feature freeze ~ a week ago). The soonest this can land in is 9.10.0.
Ack.
If you feel this has become bigger task than you anticipated then I can post patches instead.
Michal
participants (4)
-
Daniel P. Berrangé -
John Levon -
Michal Prívozník -
Thanos Makatos