Patchwork [8/8] vl.c: validate -numa "cpus" parameter properly

login
register
mail settings
Submitter Eduardo Habkost
Date Jan. 16, 2013, 3:24 p.m.
Message ID <1358349851-20960-9-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/212879/
State New
Headers show

Comments

Eduardo Habkost - Jan. 16, 2013, 3:24 p.m.
- Accept empty strings without aborting
- Use parse_uint*() to parse numbers
- Abort if anything except '-' or end-of-string is found after the first
  number.
- Check for endvalue < value

Also change the MAX_CPUMASK_BITS warning message from "A max of %d CPUs
are supported in a guest" to "qemu: NUMA: A max of %d VCPUs are
supported".

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Eric Blake <eblake@redhat.com>
---
 vl.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)
Eric Blake - Jan. 16, 2013, 5:30 p.m.
On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> - Accept empty strings without aborting
> - Use parse_uint*() to parse numbers
> - Abort if anything except '-' or end-of-string is found after the first
>   number.
> - Check for endvalue < value
> 
> Also change the MAX_CPUMASK_BITS warning message from "A max of %d CPUs
> are supported in a guest" to "qemu: NUMA: A max of %d VCPUs are
> supported".
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Eric Blake <eblake@redhat.com>
> ---
>  vl.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 

>  
> -    value = strtoull(cpus, &endptr, 10);
> +    /* Empty CPU range strings will be considered valid, they will simply
> +     * not set any bit in the CPU bitmap.
> +     */
> +    if (!*cpus) {
> +        return;

Does the code behave correctly when there are no bits in the CPU bitmap,
or do you require that at least one bit be set?

> +    }
> +
> +    if (parse_uint(cpus, &value, &endptr) < 0) {
> +        goto error;

Again, another case of accepting octal where you used to only accept
binary; if the change of interpretation of 010 is intentional, it would
be worth documenting in the commit message.  Otherwise, it might be
worth refactoring 1/8 to add a 'base' parameter to parse_uint[_full] to
allow the caller to control whether they want base 0 or base 10.
Eduardo Habkost - Jan. 16, 2013, 5:50 p.m.
On Wed, Jan 16, 2013 at 10:30:25AM -0700, Eric Blake wrote:
> On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> > - Accept empty strings without aborting
> > - Use parse_uint*() to parse numbers
> > - Abort if anything except '-' or end-of-string is found after the first
> >   number.
> > - Check for endvalue < value
> > 
> > Also change the MAX_CPUMASK_BITS warning message from "A max of %d CPUs
> > are supported in a guest" to "qemu: NUMA: A max of %d VCPUs are
> > supported".
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: Eric Blake <eblake@redhat.com>
> > ---
> >  vl.c | 32 +++++++++++++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 5 deletions(-)
> > 
> 
> >  
> > -    value = strtoull(cpus, &endptr, 10);
> > +    /* Empty CPU range strings will be considered valid, they will simply
> > +     * not set any bit in the CPU bitmap.
> > +     */
> > +    if (!*cpus) {
> > +        return;
> 
> Does the code behave correctly when there are no bits in the CPU bitmap,
> or do you require that at least one bit be set?

If no bits are set on any node, QEMU will divide the VCPUs equally
through all nodes. More exactly, each VCPU (i) would go to NUMA node
(i % nb_numa_nodes).

It is also possible to build ACPI tables where a node have no CPUs, just
RAM regions. It doesn't mean it is a good idea or that guest OSes will
be able to handle it properly, but QEMU will do exactly what you asked
for.


> 
> > +    }
> > +
> > +    if (parse_uint(cpus, &value, &endptr) < 0) {
> > +        goto error;
> 
> Again, another case of accepting octal where you used to only accept
> binary; if the change of interpretation of 010 is intentional, it would
> be worth documenting in the commit message.  Otherwise, it might be
> worth refactoring 1/8 to add a 'base' parameter to parse_uint[_full] to
> allow the caller to control whether they want base 0 or base 10.

True. Thanks for catching it, again. I plan to take the same approach
proposed on my reply to patch 6/8.

Patch

diff --git a/vl.c b/vl.c
index 98cf9f0..2d156e2 100644
--- a/vl.c
+++ b/vl.c
@@ -1246,21 +1246,43 @@  static void numa_node_parse_cpus(int nodenr, const char *cpus)
     char *endptr;
     unsigned long long value, endvalue;
 
-    value = strtoull(cpus, &endptr, 10);
+    /* Empty CPU range strings will be considered valid, they will simply
+     * not set any bit in the CPU bitmap.
+     */
+    if (!*cpus) {
+        return;
+    }
+
+    if (parse_uint(cpus, &value, &endptr) < 0) {
+        goto error;
+    }
     if (*endptr == '-') {
-        endvalue = strtoull(endptr+1, &endptr, 10);
-    } else {
+        if (parse_uint_full(endptr + 1, &endvalue) < 0) {
+            goto error;
+        }
+    } else if (*endptr == '\0') {
         endvalue = value;
+    } else {
+        goto error;
     }
 
-    if (!(endvalue < MAX_CPUMASK_BITS)) {
+    if (endvalue >= MAX_CPUMASK_BITS) {
         endvalue = MAX_CPUMASK_BITS - 1;
         fprintf(stderr,
-            "A max of %d CPUs are supported in a guest\n",
+            "qemu: NUMA: A max of %d VCPUs are supported\n",
              MAX_CPUMASK_BITS);
     }
 
+    if (endvalue < value) {
+        goto error;
+    }
+
     bitmap_set(node_cpumask[nodenr], value, endvalue-value+1);
+    return;
+
+error:
+    fprintf(stderr, "qemu: Invalid NUMA CPU range: %s\n", cpus);
+    exit(1);
 }
 
 static void numa_add(const char *optarg)