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