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@xxxxxxxxx>
> > ---
> > 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
