[PATCH v3 03/12] test_driver: Implement virDomainDetachDeviceFlags

[Luke Yue]

On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
> On Wed, Nov 10, 2021 at 10:24:22PM +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 at gmail.com>
> > ---
> > src/test/test_driver.c | 202
> > +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 202 insertions(+)
> > 
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index ea474d55ac..6a7eb12f77 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -10051,6 +10051,207 @@
> > testConnectGetAllDomainStats(virConnectPtr conn,
> >     return ret;
> > }
> > 
> > +static int
> > +testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef,
> > +                                    virDomainDeviceDef *dev)
> 
> My comment from the previous patch would be nicely usable here as we
> could easily just make use of it.
> 
> Also I see no reason for splitting the next two patches from this
> one.
> 

OK, I will squash them in next version.

> [...]
> 
> > +
> > +static int
> > +testDomainChgDevice(virDomainPtr dom,
> > +                    virDomainDeviceAction action,
> > +                    const char *xml,
> > +                    const char *alias,
> > +                    unsigned int flags)
> > +{
> > +    testDriver *driver = dom->conn->privateData;
> > +    virDomainObj *vm = NULL;
> > +    virDomainDef *def;
> > +    virDomainDeviceDef *dev = NULL;
> > +    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> > +    int ret = -1;
> > +
> > +    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > +                  VIR_DOMAIN_AFFECT_CONFIG, -1);
> 
> So here you check for both live and config, saying both of them are
> supported, ...
> 
> > +
> > +    if (!(vm = testDomObjFromDomain(dom)))
> > +        goto cleanup;
> > +
> > +    if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> > +        goto cleanup;
> > +
> > +    if (!(def = virDomainObjGetOneDef(vm, flags)))
> > +        goto cleanup;
> > +
> 
> But here it would fail with both since you are explicitly saying you
> want just one definition.
> 
> > +    if (action == VIR_DOMAIN_DEVICE_ACTION_DETACH)
> > +        parse_flags |= VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
> > +
> > +    if (xml) {
> > +        if (!(dev = virDomainDeviceDefParse(xml, def, driver-
> > >xmlopt,
> > +                                            driver->caps,
> > parse_flags)))
> > +            goto cleanup;
> > +    } else if (alias) {
> > +        dev = g_new0(virDomainDeviceDef, 1);
> > +        if (virDomainDefFindDevice(def, alias, dev, true) < 0)
> > +            goto cleanup;
> > +    }
> > +
> > +    if (dev == NULL)
> > +        goto cleanup;
> > +
> > +    switch (action) {
> > +    case VIR_DOMAIN_DEVICE_ACTION_ATTACH:
> > +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > +                       _("attaching devices is not supported"));
> > +        goto cleanup;
> > +        break;
> > +
> > +    case VIR_DOMAIN_DEVICE_ACTION_DETACH:
> > +        if (testDomainDetachDeviceLiveAndConfig(def, dev) < 0)
> 
> And I kind of see now why you call the function "LiveAndConfig" since
> it
> can do both based on what DomainDef you call it with.  This function
> could be very easily modified to do both live and config properly.

Sorry, I missed that situation, will fix it in next version.

Thanks,
Luke