On 10/30/23 17:02, Thanos Makatos wrote:
> Signed-off-by: Thanos Makatos <thanos.makatos@xxxxxxxxxxx>
> ---
> 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
