1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] test_driver: implement virDomainGetMemoryParameters

Ilias Stamatis posted 1 patch 4 years, 9 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190630221919.18640-1-stamatis.iliass@gmail.com
src/test/test_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
[libvirt] [PATCH v2] test_driver: implement virDomainGetMemoryParameters
Posted by Ilias Stamatis 4 years, 9 months ago 
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
---

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;
+
+    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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetMemoryParameters
Posted by Erik Skultety 4 years, 9 months ago 
On Mon, Jul 01, 2019 at 12:19:19AM +0200, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> ---
>
> 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

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetMemoryParameters
Posted by Ilias Stamatis 4 years, 9 months ago 
On Mon, Jul 1, 2019 at 5:50 PM Erik Skultety <eskultet@redhat.com> wrote:
>
> On Mon, Jul 01, 2019 at 12:19:19AM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > ---
> >
> > 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@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetMemoryParameters
Posted by Erik Skultety 4 years, 9 months ago 
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@redhat.com> wrote:
> >
> > On Mon, Jul 01, 2019 at 12:19:19AM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > > ---
> > >
> > > 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@redhat.com
> > > https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetMemoryParameters
Posted by Ilias Stamatis 4 years, 9 months ago 
On Tue, Jul 2, 2019 at 12:07 PM Erik Skultety <eskultet@redhat.com> wrote:
>
> 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@redhat.com> wrote:
> > >
> > > On Mon, Jul 01, 2019 at 12:19:19AM +0200, Ilias Stamatis wrote:
> > > > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > > > ---
> > > >
> > > > 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);

Apart from not loving macros in general there's another thing that I
don't like in this one.

If I place the cursor over VIR_DOMAIN_MEMORY_HARD_LIMIT or
VIR_TYPED_PARAM_ULLONG etc. it is easy with vim or whatever editor to
jump directly to the definition and check the enum. If we use this
macro string concatenation technique navigating through the source
code becomes a bit more difficult.

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

the only "drawback" is a few more keystrokes / slightly longer lines,
but I think we can live with that

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

Sure, I'm editing and re-sending everything then.

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

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.