| Message ID | 1358349851-20960-9-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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)
- 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(-)