Re: [Qemu-devel] [PATCH 8/8] vl.c: validate -numa "cpus" parameter properly

[Eduardo Habkost]

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 <address@hidden>
> > ---
> > Cc: Eric Blake <address@hidden>
> > ---
> >  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.

-- 
Eduardo