Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetMemoryParameters

On Mon, Jul 1, 2019 at 5:50 PM Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > On Mon, Jul 01, 2019 at 12:19:19AM +0200, Ilias Stamatis wrote: > > Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com&gt; > > --- > > > > Things changed since v1: > > > > - virDomainObjGetOneDef is used in order to get the appropriate def > > - the TEST_ASSIGN_MEM_PARAM macro got removed > > > > src/test/test_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 52 insertions(+) > > > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c > > index 4b1f2724a0..01b3ed88f3 100755 > > --- a/src/test/test_driver.c > > +++ b/src/test/test_driver.c > > @@ -2514,6 +2514,57 @@ testDomainGetMaxVcpus(virDomainPtr domain) > > VIR_DOMAIN_VCPU_MAXIMUM)); > > } > > > > + > > +static int > > +testDomainGetMemoryParameters(virDomainPtr dom, > > + virTypedParameterPtr params, > > + int *nparams, > > + unsigned int flags) > > +{ > > + int ret = -1; > > + virDomainObjPtr vm = NULL; > > + virDomainDefPtr def = NULL; > > + > > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > > + VIR_DOMAIN_AFFECT_CONFIG | > > + VIR_TYPED_PARAM_STRING_OKAY, -1); > > + > > + if ((*nparams) == 0) { > > + *nparams = 3; > > + return 0; > > + } > > + > > + if (!(vm = testDomObjFromDomain(dom))) > > + goto cleanup; > > + > > + if (!(def = virDomainObjGetOneDef(vm, flags))) > > + goto cleanup; > > + > > + if (*nparams > 0 && > > + virTypedParameterAssign(&params[0], VIR_DOMAIN_MEMORY_HARD_LIMIT, > > + VIR_TYPED_PARAM_ULLONG, def->mem.hard_limit) < 0) > > + goto cleanup; > > + > > + if (*nparams > 1 && > > + virTypedParameterAssign(&params[1], VIR_DOMAIN_MEMORY_SOFT_LIMIT, > > + VIR_TYPED_PARAM_ULLONG, def->mem.soft_limit) < 0) > > + goto cleanup; > > + > > + if (*nparams > 2 && > > + virTypedParameterAssign(&params[2], VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, > > + VIR_TYPED_PARAM_ULLONG, def->mem.swap_hard_limit) < 0) > > + goto cleanup; > > It didn't occur to me in the NumaParameters patch, but seeing ^this, we > definitely do want to introduce a similar macro to QEMU_ASSIGN_MEM_PARAM. Not > very pleasing to the eye, but I'd maybe go as far as make it generic enough to > be used with GetNumaParameters, which of course will require preprocessor > string concatenation in the macro definition, but I won't insist on the latter > unless we could make use of the macro in GetInterfaceParameters too (which I > haven't looked at yet). > > Otherwise the changes look okay. > > Erik Well, in the v1 of the patch I had a TEST_ASSIGN_MEM_PARAM macro which was a copy-paste of the qemu macro. But then I felt that maybe it is a bit ugly and also not so good to introduce small macros in random locations. Plus it was used only for this function, so I thought it's better if I remove it. For which part does it need preprocessor string concatenation?

Something more generic could be: #define TEST_ASSIGN_PARAM(index, name, type, value) \ if (index < *nparams && \ virTypedParameterAssign(&params[index], name, type, value) < 0) \ goto cleanup

So in that case we could "call" it as: TEST_ASSIGN_PARAM(0, VIR_DOMAIN_MEMORY_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, def->mem.hard_limit); Would you like me to introduce this and then use it in all the relevant APIs?

Ilias > > > + > > + if (*nparams > 3) > > + *nparams = 3; > > + > > + ret = 0; > > + cleanup: > > + virDomainObjEndAPI(&vm); > > + return ret; > > +} > > + > > + > > static int > > testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, > > unsigned int flags) > > @@ -7275,6 +7326,7 @@ static virHypervisorDriver testHypervisorDriver = { > > .domainGetVcpus = testDomainGetVcpus, /* 0.7.3 */ > > .domainGetVcpuPinInfo = testDomainGetVcpuPinInfo, /* 1.2.18 */ > > .domainGetMaxVcpus = testDomainGetMaxVcpus, /* 0.7.3 */ > > + .domainGetMemoryParameters = testDomainGetMemoryParameters, /* 5.6.0 */ > > .domainGetXMLDesc = testDomainGetXMLDesc, /* 0.1.4 */ > > .connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */ > > .connectNumOfDefinedDomains = testConnectNumOfDefinedDomains, /* 0.1.11 */ > > -- > > 2.22.0 > > > > -- > > libvir-list mailing list > > libvir-list(a)redhat.com > > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list