Re: [PATCH v3 03/12] test_driver: Implement virDomainDetachDeviceFlags

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(a)gmail.com&gt; > --- > 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.

[...] > + > +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.