Patchwork [6/8] vl.c: Use parse_uint_full() for NUMA nodeid

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

Comments

Eduardo Habkost - Jan. 16, 2013, 3:24 p.m.
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 <ehabkost@redhat.com>
---
Cc: Eric Blake <eblake@redhat.com>
---
 vl.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Eduardo Habkost - Jan. 16, 2013, 5:42 p.m.
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 <ehabkost@redhat.com>
> > ---
> > Cc: Eric Blake <eblake@redhat.com>
> > ---
> >  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.

Patch

diff --git a/vl.c b/vl.c
index 3637a39..43ce607 100644
--- a/vl.c
+++ 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) {
+                fprintf(stderr, "qemu: Invalid NUMA nodeid: %s\n", option);
+                exit(1);
+            }
         }
 
         if (nodenr >= MAX_NODES) {
-            fprintf(stderr, "qemu: invalid NUMA nodeid: %d\n", nodenr);
+            fprintf(stderr, "qemu: invalid NUMA nodeid: %llu\n", nodenr);
             exit(1);
         }