1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] qemu: Enforce vCPU hotplug granularity constraints
  4. View patch

[libvirt] [PATCH 1/2] qemu: Invert condition nesting in qemuDomainDefValidate()

Andrea Bolognani posted 2 patches 7 years, 5 months ago
There is a newer version of this series
[libvirt] [PATCH 1/2] qemu: Invert condition nesting in qemuDomainDefValidate()
Posted by Andrea Bolognani 7 years, 5 months ago 
While at the moment we're only performing a single check that is
connected to vCPU hotplugging, we're going to introduce a second
one soon. Move the topology check underneath the capability check
to make that easier; as a bonus, doing so allows us to reduce the
scope of the 'topologycpus' variable.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 74b82450b..e93333669 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3299,7 +3299,6 @@ qemuDomainDefValidate(const virDomainDef *def,
 {
     virQEMUDriverPtr driver = opaque;
     virQEMUCapsPtr qemuCaps = NULL;
-    unsigned int topologycpus;
     int ret = -1;
 
     if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
@@ -3359,11 +3358,15 @@ qemuDomainDefValidate(const virDomainDef *def,
         }
     }
 
-    /* qemu as of 2.5.0 rejects SMP topologies that don't match the cpu count */
-    if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
-        topologycpus != virDomainDefGetVcpusMax(def)) {
-        /* presence of query-hotpluggable-cpus should be a good enough witness */
-        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
+    /* QEMU 2.7 (detected via the availability of query-hotpluggable-cpus)
+     * enforces stricter rules than previous versions when it comes to guest
+     * CPU topology. Verify known constraints are respected */
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
+        unsigned int topologycpus;
+
+        /* Max vCPU count and overall vCPU topology must agree */
+        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
+            topologycpus != virDomainDefGetVcpusMax(def)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("CPU topology doesn't match maximum vcpu count"));
             goto cleanup;
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Invert condition nesting in qemuDomainDefValidate()
Posted by Peter Krempa 7 years, 5 months ago 
On Thu, Dec 14, 2017 at 17:29:50 +0100, Andrea Bolognani wrote:
> While at the moment we're only performing a single check that is
> connected to vCPU hotplugging, we're going to introduce a second
> one soon. Move the topology check underneath the capability check
> to make that easier; as a bonus, doing so allows us to reduce the
> scope of the 'topologycpus' variable.

You know that generally we prefer variables declared at the top scope?

Also you change the version of qemu in the comment without explanation
here. Rather than bragging how you reduced scope, please document that
change.

> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 74b82450b..e93333669 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3299,7 +3299,6 @@ qemuDomainDefValidate(const virDomainDef *def,
>  {
>      virQEMUDriverPtr driver = opaque;
>      virQEMUCapsPtr qemuCaps = NULL;
> -    unsigned int topologycpus;
>      int ret = -1;
>  
>      if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
> @@ -3359,11 +3358,15 @@ qemuDomainDefValidate(const virDomainDef *def,
>          }
>      }
>  
> -    /* qemu as of 2.5.0 rejects SMP topologies that don't match the cpu count */
> -    if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
> -        topologycpus != virDomainDefGetVcpusMax(def)) {
> -        /* presence of query-hotpluggable-cpus should be a good enough witness */
> -        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
> +    /* QEMU 2.7 (detected via the availability of query-hotpluggable-cpus)
> +     * enforces stricter rules than previous versions when it comes to guest
> +     * CPU topology. Verify known constraints are respected */
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
> +        unsigned int topologycpus;
> +
> +        /* Max vCPU count and overall vCPU topology must agree */
> +        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
> +            topologycpus != virDomainDefGetVcpusMax(def)) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                             _("CPU topology doesn't match maximum vcpu count"));
>              goto cleanup;

The code looks okay, but I want to see a justification on the changed
version.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Invert condition nesting in qemuDomainDefValidate()
Posted by Andrea Bolognani 7 years, 5 months ago 
On Fri, 2017-12-15 at 13:18 +0100, Peter Krempa wrote:
> On Thu, Dec 14, 2017 at 17:29:50 +0100, Andrea Bolognani wrote:
> > While at the moment we're only performing a single check that is
> > connected to vCPU hotplugging, we're going to introduce a second
> > one soon. Move the topology check underneath the capability check
> > to make that easier; as a bonus, doing so allows us to reduce the
> > scope of the 'topologycpus' variable.
> 
> You know that generally we prefer variables declared at the top scope?

Is that so? I've seen several instances of the opposite, and it
makes to me not to pollute the function scope with one-use
variables when there are usually enough variables that actually
need to be accessed throughout the function. But I can leave the
declaration where it is if you like it better that way.

> Also you change the version of qemu in the comment without explanation
> here. Rather than bragging how you reduced scope, please document that
> change.

Right, I meant to include the explanation but forgot :)

It's very simple, anyway: the version number was wrong, since
QEMU introduced query-hotpluggable-cpus in 2.7 rather than 2.5.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Invert condition nesting in qemuDomainDefValidate()
Posted by Peter Krempa 7 years, 5 months ago 
On Fri, Dec 15, 2017 at 14:11:41 +0100, Andrea Bolognani wrote:
> On Fri, 2017-12-15 at 13:18 +0100, Peter Krempa wrote:
> > On Thu, Dec 14, 2017 at 17:29:50 +0100, Andrea Bolognani wrote:
> > > While at the moment we're only performing a single check that is
> > > connected to vCPU hotplugging, we're going to introduce a second
> > > one soon. Move the topology check underneath the capability check
> > > to make that easier; as a bonus, doing so allows us to reduce the
> > > scope of the 'topologycpus' variable.
> > 
> > You know that generally we prefer variables declared at the top scope?
> 
> Is that so? I've seen several instances of the opposite, and it
> makes to me not to pollute the function scope with one-use
> variables when there are usually enough variables that actually
> need to be accessed throughout the function. But I can leave the
> declaration where it is if you like it better that way.

Fair enough. We only state that we should declare it at the beginning of
a scope, so while generally most are declared at function scoep, this
does not violate the contributor guidelines.

> 
> > Also you change the version of qemu in the comment without explanation
> > here. Rather than bragging how you reduced scope, please document that
> > change.
> 
> Right, I meant to include the explanation but forgot :)
> 
> It's very simple, anyway: the version number was wrong, since
> QEMU introduced query-hotpluggable-cpus in 2.7 rather than 2.5.

While this is true for the presence of query-hotpluggable-cpus. The
paragraph is specifically saying that QEMU rejects such configurations
starting from 2.5., but the code checks them only since
query-hotpluggable-cpus (which was introduced in 2.7).

Your modification is thus incorrect. You can add statement that
query-hotpluggable-cpus was added in 2.7. but mangling it as you did is
not correct in my opinion.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Invert condition nesting in qemuDomainDefValidate()
Posted by Andrea Bolognani 7 years, 5 months ago 
On Fri, 2017-12-15 at 14:20 +0100, Peter Krempa wrote:
> > > You know that generally we prefer variables declared at the top scope?
> > 
> > Is that so? I've seen several instances of the opposite, and it
> > makes to me not to pollute the function scope with one-use
> > variables when there are usually enough variables that actually
> > need to be accessed throughout the function. But I can leave the
> > declaration where it is if you like it better that way.
> 
> Fair enough. We only state that we should declare it at the beginning of
> a scope, so while generally most are declared at function scoep, this
> does not violate the contributor guidelines.

Cool, I'll still move it to the inner scope then.

> > It's very simple, anyway: the version number was wrong, since
> > QEMU introduced query-hotpluggable-cpus in 2.7 rather than 2.5.
> 
> While this is true for the presence of query-hotpluggable-cpus. The
> paragraph is specifically saying that QEMU rejects such configurations
> starting from 2.5., but the code checks them only since
> query-hotpluggable-cpus (which was introduced in 2.7).
> 
> Your modification is thus incorrect. You can add statement that
> query-hotpluggable-cpus was added in 2.7. but mangling it as you did is
> not correct in my opinion.

I see, that's a nuance I was not aware of. I'll amend the comment
so that it tells the whole story.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.