> -----Original Message-----
> From: Michal Prívozník <mprivozn@xxxxxxxxxx>
> Sent: Tuesday, October 31, 2023 9:50 AM
> To: Thanos Makatos <thanos.makatos@xxxxxxxxxxx>; devel@xxxxxxxxxxxxxxxxx
> 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@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().
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
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
