Re: [Qemu-devel] [PATCH 6/8] vl.c: Use parse_uint_full() for NUMA nodeid

[Eduardo Habkost]

On Wed, Jan 16, 2013 at 10:25:31AM -0700, Eric Blake wrote:
> On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> > This should catch many kinds of errors that the current code wasn't
> > checking for:
> > 
> >  - Values that can't be parsed as a number
> >  - Negative values
> >  - Overflow
> >  - Empty string
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Cc: Eric Blake <address@hidden>
> > ---
> >  vl.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> 
> > +++ b/vl.c
> > @@ -1264,11 +1264,14 @@ static void numa_add(const char *optarg)
> >          if (get_param_value(option, 128, "nodeid", optarg) == 0) {
> >              nodenr = nb_numa_nodes;
> >          } else {
> > -            nodenr = strtoull(option, NULL, 10);
> > +            if (parse_uint_full(option, &nodenr) < 0) {
> 
> This allows a user to pass octal or hex numbers, where previously it was
> forced to be decimal.  That means 'nodeid=010' is now '8' instead of
> '10'; is that intentional?

It wasn't intentional, thanks for catching it!

However, the existing option parsing infra-structure
(parse_option_number() at qemu-option.c and opts_type_uint64() at
opts-visitor.c) use 0 as the 'base' parameter. So when we convert this
code to use that infra-structure, it will end up accepting hex and octal
numbers as well.

So I believe I will fix it this way:

 - Add 'base' parameter to parse_uint*()
 - Use base=10 when parsing "-numa" (keeping "nodeid=010" working, just
   in case)
 - Use the QemuOpts number parser when introducing the "numa-node"
   config section, and manually parse/convert the (then obsolete)
   "-numa node,..." option to "numa-node"

Or, we could simply change the meaning of "nodeid=010" and document it
in the commit message and QEMU ChageLog. Which option do the QEMU
maintainers prefer?

> 
> >  
> >          if (nodenr >= MAX_NODES) {
> > -            fprintf(stderr, "qemu: invalid NUMA nodeid: %d\n", nodenr);
> > +            fprintf(stderr, "qemu: invalid NUMA nodeid: %llu\n", nodenr);
> 
> Already mentions that this part belongs in 5/8.

I will fix it. Thanks.

-- 
Eduardo