Re: [PATCH v2] test_driver: implement virDomainGetMemoryParameters

[Erik Skultety <eskultet@xxxxxxxxxx>]

On Tue, Jul 02, 2019 at 10:43:08AM +0200, Ilias Stamatis wrote:
> On Mon, Jul 1, 2019 at 5:50 PM Erik Skultety <eskultet@xxxxxxxxxx> wrote:
> >
> > On Mon, Jul 01, 2019 at 12:19:19AM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis <stamatis.iliass@xxxxxxxxx>
> > > ---
> > >
> > > 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?

I don't like long ids and long lines and because we couldn't hide
VIR_TYPED_PARAM_X within the macro, my original suggestion (which I admitted to
be ugly) was doing:

do { MYMACRO(name, type, val)
    virTypedParameterAssign(&params[index],
                            VIR_DOMAIN_ ## name,
                            VIR_TYPED_PARAM_ ## type,
                            val);
} while(0);

MYMACRO(0, MEMORY_HARD_LIMIT,
        ULLONG, value);

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

... but let's do it ^this way...

>
> 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?

Definitely.

Erik

>
> 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@xxxxxxxxxx
> > > https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list