Re: [PATCH 1/5] test_driver: Implement virDomainDetachDeviceFlags

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Tue, Aug 17, 2021 at 08:50:59PM +0800, Luke Yue wrote:
> On Tue, 2021-08-17 at 14:13 +0200, Martin Kletzander wrote:
> > On Mon, Aug 16, 2021 at 07:19:45PM +0800, Luke Yue wrote:
> > > Introduce testDomainChgDevice for further development (just like
> > > what we
> > > did for IOThread). And introduce
> > > testDomainDetachDeviceLiveAndConfig for
> > > detaching devices.
> > >
> > > Signed-off-by: Luke Yue <lukedyue@xxxxxxxxx>
> > > ---
> > > src/test/test_driver.c | 270
> > > +++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 270 insertions(+)
> > >

[...]

> > > +
> > > +    case VIR_DOMAIN_DEVICE_VSOCK:
> > > +        if (!vmdef->vsock ||
> > > +            !virDomainVsockDefEquals(dev->data.vsock, vmdef-
> > > >vsock)) {
> > > +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > > +                           _("matching vsock device not found"));
> > > +            return -1;
> > > +        }
> > > +        virDomainVsockDefFree(vmdef->vsock);
> > > +        vmdef->vsock = NULL;
> > > +        break;
> > > +
> >
> > There is lot of duplication here.  These are already used in multiple
> > places and de-duplicating would help us tremendously, even more than
> > in
> > the previous case with virDomainGetMessages.  The lxc, libxl, qemu
> > and
> > possibly other drivers would benefit from many of these, some of them
> > could then easily add more functionality and it would be even more
> > tested once we start testing these properly.  It is not going to be a
> > trivial task to properlyd ecide where to split this and how, but I
> > believe you can figure out a nice, clean way.  But do not hesitate to
> > let know if you need help with that.
>
> Yes, I was thinking if I can extract them for these drivers, but I
> noticed that there may be some differences, like we have to check ACL
> for some devices when using other drivers, and at least for QEMU
> driver, there is no 
>
> virDomainInputDefFree(vmdef->inputs[idx]);
>
> line in `case VIR_DOMAIN_DEVICE_INPUT:` in QEMU driver, I'm not pretty
> sure whether this will lead to memory leak for QEMU or not, or it's by
> design, but at least it do leak memory when using test driver. So we
> may need to handle them for different drivers' case, and I am not
> pretty sure whether I should extract or not, so I decide to send the
> patches first. I was wondering if we can add a flag to indicate which
> driver is using the function, will give it a try.

This whole thing does not have to be extracted as one function.  Or it
can and there can be either flags or some callbacks, for example passed
inside xmlopt.  Whatever makes it more readable, nicer etc.  There will
inevitably be numerous ways to approach this and the real work is
figuring out which is the cleaner one.

> > > +    case VIR_DOMAIN_DEVICE_CHR:
> > > +    case VIR_DOMAIN_DEVICE_VIDEO:
> > > +    case VIR_DOMAIN_DEVICE_GRAPHICS:
> > > +    case VIR_DOMAIN_DEVICE_HUB:
> > > +    case VIR_DOMAIN_DEVICE_SMARTCARD:
> > > +    case VIR_DOMAIN_DEVICE_MEMBALLOON:
> > > +    case VIR_DOMAIN_DEVICE_NVRAM:
> > > +    case VIR_DOMAIN_DEVICE_NONE:
> > > +    case VIR_DOMAIN_DEVICE_TPM:
> > > +    case VIR_DOMAIN_DEVICE_PANIC:
> > > +    case VIR_DOMAIN_DEVICE_IOMMU:
> > > +    case VIR_DOMAIN_DEVICE_AUDIO:
> >
> > What's good about the test driver is that we can support attach and
> > detach for all the devices.  It would be nice to also have a way to
> > test
> > something that is not supported, but there is no need to do it with
> > all
> > of the above.  It makes for example for an IOMMU as that is something
> > I
> > cannot imagine being hot(un)plugged.  For --config anything is
> > possible
> > because it is just the XML that is being changed.
>
> OK, I will try to add more of them.

Of course first figuring out how to approach the above may make it way
easier to then add more of these.

[...]

> > > +    switch (action) {
> > > +    case VIR_DOMAIN_DEVICE_ACTION_ATTACH:
> > > +        break;
> >
> > So Attach will always succeed, but nothing will show up in the XML.
> >
> > > +
> > > +    case VIR_DOMAIN_DEVICE_ACTION_DETACH:
> > > +        if (testDomainDetachDeviceLiveAndConfig(def, dev) < 0)
> > > +            goto cleanup;
> > > +        break;
> > > +
> > > +    case VIR_DOMAIN_DEVICE_ACTION_UPDATE:
> > > +        break;
> >
> > Same here
>
> I thought there is no function would pass
> VIR_DOMAIN_DEVICE_ACTION_ATTACH or UPDATE to this function at the
> moment, so I just leave it there.

Well yes, but then it can make it hard to find an error if someone wants
to add support for attaching and they use the function, based on the
name it sounds alright and then there is no error, it just does not
work.  It's not that it is bad now, but for the purpose of
future-proofing it is beneficial to handle those cases clearly in some
way.

Attachment: signature.asc
Description: PGP signature