[Qemu-devel] [PATCH 0/8] -numa option parsing fixes (v2)

[Qemu-devel] [PATCH 0/8] -numa option parsing fixes (v2)

[Eduardo Habkost]

This series contains only the most important fixes from the previous "-numa
option parsing fixes & improvements" series I have submitted.

I have introduced parse_uint*() helpers that can be reused by other code, later.
I plan to submit parse_int*() (for signed integers) and parse_double*()
functions too, later, and change string-input-visitor.c and opts-visitor.c to
use those common functions instead of duplicating the number parsing code.

Eduardo Habkost (8):
  cutils: unsigned int parsing functions
  vl.c: Fix off-by-one bug when handling "-numa node" argument
  vl.c: Abort on unknown -numa option type
  vl.c: Check for NUMA node limit inside numa_add()
  vl.c: numa_add(): Validate nodeid before using it
  vl.c: Use parse_uint_full() for NUMA nodeid
  vl.c: Extract -numa "cpus" parsing to separate function
  vl.c: validate -numa "cpus" parameter properly

 include/qemu-common.h |   3 +
 tests/Makefile        |   3 +
 tests/test-cutils.c   | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++
 util/cutils.c         |  76 +++++++++++++++++++++
 vl.c                  |  93 ++++++++++++++++++-------
 5 files changed, 333 insertions(+), 25 deletions(-)
 create mode 100644 tests/test-cutils.c

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions

[Eduardo Habkost]

There are lots of duplicate parsing code using strto*() in QEMU, and
most of that code is broken in one way or another. Even the visitors
code have duplicate integer parsing code[1]. This introduces functions
to help parsing unsigned int values: parse_uint() and parse_uint_full().

Parsing functions for signed ints and floats will be submitted later.

parse_uint_full() has all the checks made by opts_type_uint64() at
opts-visitor.c:

 - Check for NULL (returns -EINVAL)
 - Check for negative numbers (returns -ERANGE)
 - Check for empty string (returns -EINVAL)
 - Check for overflow or other errno values set by strtoll() (returns
   -errno)
 - Check for end of string (reject invalid characters after number)
   (returns -EINVAL)

parse_uint() does everything above except checking for the end of the
string, so callers can continue parsing the remainder of string after
the number.

Unit tests included.

[1] string-input-visitor.c:parse_int() could use the same parsing code
    used by opts-visitor.c:opts_type_int(), instead of duplicating that
    logic.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
---
 include/qemu-common.h |   3 +
 tests/Makefile        |   3 +
 tests/test-cutils.c   | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++
 util/cutils.c         |  76 +++++++++++++++++++++
 4 files changed, 265 insertions(+)
 create mode 100644 tests/test-cutils.c

diff --git a/include/qemu-common.h b/include/qemu-common.h
index ca464bb..83f89fd 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -170,6 +170,9 @@ int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
 int qemu_parse_fd(const char *param);
 
+int parse_uint(const char *s, unsigned long long *value, char **endptr);
+int parse_uint_full(const char *s, unsigned long long *value);
+
 /*
  * strtosz() suffixes used to specify the default treatment of an
  * argument passed to strtosz() without an explicit suffix.
diff --git a/tests/Makefile b/tests/Makefile
index d97a571..2dba3e5 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -45,6 +45,8 @@ gcov-files-test-aio-$(CONFIG_WIN32) = aio-win32.c
 gcov-files-test-aio-$(CONFIG_POSIX) = aio-posix.c
 check-unit-y += tests/test-thread-pool$(EXESUF)
 gcov-files-test-thread-pool-y = thread-pool.c
+check-unit-y += tests/test-cutils$(EXESUF)
+gcov-files-test-cutils-y += util/cutils.c
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -84,6 +86,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil
 tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-iov$(EXESUF): tests/test-iov.o libqemuutil.a
+tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
new file mode 100644
index 0000000..d573eb1
--- /dev/null
+++ b/tests/test-cutils.c
@@ -0,0 +1,183 @@
+/*
+ * cutils.c unit-tests
+ *
+ * Copyright (C) 2013 Red Hat Inc.
+ *
+ * Authors:
+ *  Eduardo Habkost <ehabkost@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <glib.h>
+#include <errno.h>
+#include <string.h>
+
+#include "qemu-common.h"
+
+
+static void test_parse_uint_null(void)
+{
+    unsigned long long i = 999;
+    char f = 'X';
+    char *endptr = &f;
+    int r;
+
+    r = parse_uint(NULL, &i, &endptr);
+
+    g_assert_cmpint(r, ==, -EINVAL);
+    g_assert_cmpint(i, ==, 0);
+    g_assert(endptr == NULL);
+}
+
+static void test_parse_uint_empty(void)
+{
+    unsigned long long i = 999;
+    char f = 'X';
+    char *endptr = &f;
+    const char *str = "";
+    int r;
+
+    r = parse_uint(str, &i, &endptr);
+
+    g_assert_cmpint(r, ==, -EINVAL);
+    g_assert_cmpint(i, ==, 0);
+    g_assert(endptr == str);
+}
+
+static void test_parse_uint_trailing(void)
+{
+    unsigned long long i = 999;
+    char f = 'X';
+    char *endptr = &f;
+    const char *str = "123xxx";
+    int r;
+
+    r = parse_uint(str, &i, &endptr);
+
+    g_assert_cmpint(r, ==, 0);
+    g_assert_cmpint(i, ==, 123);
+    g_assert(endptr == str + 3);
+}
+
+static void test_parse_uint_correct(void)
+{
+    unsigned long long i = 999;
+    char f = 'X';
+    char *endptr = &f;
+    const char *str = "123";
+    int r;
+
+    r = parse_uint(str, &i, &endptr);
+
+    g_assert_cmpint(r, ==, 0);
+    g_assert_cmpint(i, ==, 123);
+    g_assert(endptr == str + strlen(str));
+}
+
+static void test_parse_uint_llong_max(void)
+{
+    unsigned long long i = 999;
+    char f = 'X';
+    char *endptr = &f;
+    char *str = g_strdup_printf("%llu", (unsigned long long)LLONG_MAX + 1);
+    int r;
+
+    r = parse_uint(str, &i, &endptr);
+
+    g_assert_cmpint(r, ==, 0);
+    g_assert_cmpint(i, ==, (unsigned long long)LLONG_MAX + 1);
+    g_assert(endptr == str + strlen(str));
+
+    g_free(str);
+}
+
+static void test_parse_uint_overflow(void)
+{
+    unsigned long long i = 999;
+    char f = 'X';
+    char *endptr = &f;
+    const char *str = "99999999999999999999999999999999999999";
+    int r;
+
+    r = parse_uint(str, &i, &endptr);
+
+    g_assert_cmpint(r, ==, -ERANGE);
+    g_assert_cmpint(i, ==, ULLONG_MAX);
+    g_assert(endptr == str + strlen(str));
+}
+
+static void test_parse_uint_negative(void)
+{
+    unsigned long long i = 999;
+    char f = 'X';
+    char *endptr = &f;
+    const char *str = " \t -321";
+    int r;
+
+    r = parse_uint(str, &i, &endptr);
+
+    g_assert_cmpint(r, ==, -ERANGE);
+    g_assert_cmpint(i, ==, 0);
+    g_assert(endptr == str + 3);
+}
+
+
+static void test_parse_uint_full_trailing(void)
+{
+    unsigned long long i = 999;
+    const char *str = "123xxx";
+    int r;
+
+    r = parse_uint_full(str, &i);
+
+    g_assert_cmpint(r, ==, -EINVAL);
+    g_assert_cmpint(i, ==, 123);
+}
+
+static void test_parse_uint_full_correct(void)
+{
+    unsigned long long i = 999;
+    const char *str = "123";
+    int r;
+
+    r = parse_uint_full(str, &i);
+
+    g_assert_cmpint(r, ==, 0);
+    g_assert_cmpint(i, ==, 123);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/cutils/parse_uint/null", test_parse_uint_null);
+    g_test_add_func("/cutils/parse_uint/empty", test_parse_uint_empty);
+    g_test_add_func("/cutils/parse_uint/trailing", test_parse_uint_trailing);
+    g_test_add_func("/cutils/parse_uint/correct", test_parse_uint_correct);
+    g_test_add_func("/cutils/parse_uint/llong_max", test_parse_uint_llong_max);
+    g_test_add_func("/cutils/parse_uint/overflow", test_parse_uint_overflow);
+    g_test_add_func("/cutils/parse_uint/negative", test_parse_uint_negative);
+    g_test_add_func("/cutils/parse_uint_full/trailing",
+                    test_parse_uint_full_trailing);
+    g_test_add_func("/cutils/parse_uint_full/correct",
+                    test_parse_uint_full_correct);
+
+    return g_test_run();
+}
diff --git a/util/cutils.c b/util/cutils.c
index 80bb1dc..b5c4b3d 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -270,6 +270,82 @@ int64_t strtosz(const char *nptr, char **end)
     return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
 }
 
+/* Try to parse an unsigned integer
+ *
+ * Error checks done by the function:
+ * - NULL pointer will return -EINVAL.
+ * - Empty strings will return -EINVAL.
+ * - Overflow errors or other errno values  set by strtoull() will
+ *   return -errno (-ERANGE in case of overflow).
+ * - Differently from strtoull(), values starting with a minus sign are
+ *   rejected (returning -ERANGE).
+ *
+ * Sets endptr to point to the first invalid character. Callers may rely
+ * on *value and *endptr to be always set by the function, even in case of
+ * errors.
+ *
+ * Returns 0 on success, negative errno value on error.
+ */
+int parse_uint(const char *s, unsigned long long *value, char **endptr)
+{
+    int r = 0;
+    char *endp = (char *)s;
+    unsigned long long val = 0;
+
+    if (!s) {
+        r = -EINVAL;
+        goto out;
+    }
+
+    /* make sure we reject negative numbers: */
+    while (isspace((unsigned char)*s)) {
+        ++s; endp++;
+    }
+    if (*s == '-') {
+        r = -ERANGE;
+        goto out;
+    }
+
+    errno = 0;
+    val = strtoull(s, &endp, 0);
+    if (errno) {
+        r =  -errno;
+        goto out;
+    }
+
+    if (endp == s) {
+        r =  -EINVAL;
+        goto out;
+    }
+
+out:
+    *value = val;
+    *endptr = endp;
+    return r;
+}
+
+/* Try to parse an unsigned integer, making sure the whole string is parsed
+ *
+ * Have the same behavior of parse_uint(), but with an additional check
+ * for additional data after the parsed number (in that case, the function
+ * will return -EINVAL).
+ */
+int parse_uint_full(const char *s, unsigned long long *value)
+{
+    char *endp;
+    int r;
+
+    r = parse_uint(s, value, &endp);
+    if (r < 0) {
+        return r;
+    }
+    if (*endp) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 int qemu_parse_fd(const char *param)
 {
     int fd;
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> There are lots of duplicate parsing code using strto*() in QEMU, and
> most of that code is broken in one way or another. Even the visitors
> code have duplicate integer parsing code[1]. This introduces functions
> to help parsing unsigned int values: parse_uint() and parse_uint_full().
> 
> Parsing functions for signed ints and floats will be submitted later.
> 
> parse_uint_full() has all the checks made by opts_type_uint64() at
> opts-visitor.c:
> 
>  - Check for NULL (returns -EINVAL)
>  - Check for negative numbers (returns -ERANGE)
>  - Check for empty string (returns -EINVAL)
>  - Check for overflow or other errno values set by strtoll() (returns
>    -errno)
>  - Check for end of string (reject invalid characters after number)
>    (returns -EINVAL)
> 
> parse_uint() does everything above except checking for the end of the
> string, so callers can continue parsing the remainder of string after
> the number.
> 
> Unit tests included.
> 
> [1] string-input-visitor.c:parse_int() could use the same parsing code
>     used by opts-visitor.c:opts_type_int(), instead of duplicating that
>     logic.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---

> +++ b/tests/test-cutils.c

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy

Interesting that you chose a BSD license instead of GPL, but doesn't
affect my review.

Your test case lacks test of octal or hexadecimal input strings; is that
worth adding?

> +++ b/util/cutils.c
> @@ -270,6 +270,82 @@ int64_t strtosz(const char *nptr, char **end)
>      return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
>  }
>  
> +/* Try to parse an unsigned integer
> + *
> + * Error checks done by the function:
> + * - NULL pointer will return -EINVAL.
> + * - Empty strings will return -EINVAL.
> + * - Overflow errors or other errno values  set by strtoull() will
> + *   return -errno (-ERANGE in case of overflow).
> + * - Differently from strtoull(), values starting with a minus sign are
> + *   rejected (returning -ERANGE).

Interesting that you chose to reject negative numbers, even though
strtoull() is required to accept them.  But you documented it and tested
for it, so I can live with it.

> +    errno = 0;
> +    val = strtoull(s, &endp, 0);
> +    if (errno) {
> +        r =  -errno;

Why two spaces?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions

[Eduardo Habkost]

On Wed, Jan 16, 2013 at 10:10:42AM -0700, Eric Blake wrote:
> On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> > There are lots of duplicate parsing code using strto*() in QEMU, and
> > most of that code is broken in one way or another. Even the visitors
> > code have duplicate integer parsing code[1]. This introduces functions
> > to help parsing unsigned int values: parse_uint() and parse_uint_full().
> > 
> > Parsing functions for signed ints and floats will be submitted later.
> > 
> > parse_uint_full() has all the checks made by opts_type_uint64() at
> > opts-visitor.c:
> > 
> >  - Check for NULL (returns -EINVAL)
> >  - Check for negative numbers (returns -ERANGE)
> >  - Check for empty string (returns -EINVAL)
> >  - Check for overflow or other errno values set by strtoll() (returns
> >    -errno)
> >  - Check for end of string (reject invalid characters after number)
> >    (returns -EINVAL)
> > 
> > parse_uint() does everything above except checking for the end of the
> > string, so callers can continue parsing the remainder of string after
> > the number.
> > 
> > Unit tests included.
> > 
> > [1] string-input-visitor.c:parse_int() could use the same parsing code
> >     used by opts-visitor.c:opts_type_int(), instead of duplicating that
> >     logic.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Eric Blake <eblake@redhat.com>
> > ---
> 
> > +++ b/tests/test-cutils.c
> 
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> 
> Interesting that you chose a BSD license instead of GPL, but doesn't
> affect my review.

It's a personal preference, but by coincidence is also the license of
cutils.c.

> 
> Your test case lacks test of octal or hexadecimal input strings; is that
> worth adding?

I believe I trust strtoll() enough to not require tests for those cases.
Just like I didn't add tests involving trailing spaces or tabs, except
on the negative-number test case.


> > +++ b/util/cutils.c
> > @@ -270,6 +270,82 @@ int64_t strtosz(const char *nptr, char **end)
> >      return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
> >  }
> >  
> > +/* Try to parse an unsigned integer
> > + *
> > + * Error checks done by the function:
> > + * - NULL pointer will return -EINVAL.
> > + * - Empty strings will return -EINVAL.
> > + * - Overflow errors or other errno values  set by strtoull() will
> > + *   return -errno (-ERANGE in case of overflow).
> > + * - Differently from strtoull(), values starting with a minus sign are
> > + *   rejected (returning -ERANGE).
> 
> Interesting that you chose to reject negative numbers, even though
> strtoull() is required to accept them.  But you documented it and tested
> for it, so I can live with it.

I find this behavior of strtoull() confusing for users. If an
option/field requires positive numbers, I expect QEMU to reject "-1"
instead of silently converting it to ULLONG_MAX.

> 
> > +    errno = 0;
> > +    val = strtoull(s, &endp, 0);
> > +    if (errno) {
> > +        r =  -errno;
> 
> Why two spaces?

Typo. I will send a fixed version (keeping your Reviewed-by, if you
don't mind).

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On 01/16/2013 10:33 AM, Eduardo Habkost wrote:
> On Wed, Jan 16, 2013 at 10:10:42AM -0700, Eric Blake wrote:
>> On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
>>> There are lots of duplicate parsing code using strto*() in QEMU, and
>>> most of that code is broken in one way or another. Even the visitors
>>> code have duplicate integer parsing code[1]. This introduces functions
>>> to help parsing unsigned int values: parse_uint() and parse_uint_full().
>>>
>>
>> Your test case lacks test of octal or hexadecimal input strings; is that
>> worth adding?
> 
> I believe I trust strtoll() enough to not require tests for those cases.

But see my comments in 8/8 about whether it makes sense to add a 'base'
parameter to let the caller choose whether they want to allow octal or
require strict decimal parsing.

>>> +        r =  -errno;
>>
>> Why two spaces?
> 
> Typo. I will send a fixed version (keeping your Reviewed-by, if you
> don't mind).

No problem - I wouldn't have left a reviewed-by if I didn't think the
changes couldn't be trivially fixed.  On the other hand, you may have a
non-trivial fix in the form of adding a base parameter...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions

[Eduardo Habkost]

On Wed, Jan 16, 2013 at 10:50:18AM -0700, Eric Blake wrote:
> On 01/16/2013 10:33 AM, Eduardo Habkost wrote:
> > On Wed, Jan 16, 2013 at 10:10:42AM -0700, Eric Blake wrote:
> >> On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> >>> There are lots of duplicate parsing code using strto*() in QEMU, and
> >>> most of that code is broken in one way or another. Even the visitors
> >>> code have duplicate integer parsing code[1]. This introduces functions
> >>> to help parsing unsigned int values: parse_uint() and parse_uint_full().
> >>>
> >>
> >> Your test case lacks test of octal or hexadecimal input strings; is that
> >> worth adding?
> > 
> > I believe I trust strtoll() enough to not require tests for those cases.
> 
> But see my comments in 8/8 about whether it makes sense to add a 'base'
> parameter to let the caller choose whether they want to allow octal or
> require strict decimal parsing.

In that case, I will add at least a test cases to check if "010" gives
the expected result depending on the 'base' parameter.

> 
> >>> +        r =  -errno;
> >>
> >> Why two spaces?
> > 
> > Typo. I will send a fixed version (keeping your Reviewed-by, if you
> > don't mind).
> 
> No problem - I wouldn't have left a reviewed-by if I didn't think the
> changes couldn't be trivially fixed.  On the other hand, you may have a
> non-trivial fix in the form of adding a base parameter...

I was planning to keep the reviewed-by line only if fixing the trivial
whitespace issue above. I won't keep it when adding the 'base'
parameter, don't worry. :-)

-- 
Eduardo

[Qemu-devel] [PATCH 2/8] vl.c: Fix off-by-one bug when handling "-numa node" argument

[Eduardo Habkost]

The numa_add() code was unconditionally adding 1 to the get_opt_name()
return value, making it point after the end of the string if no ','
separator is present.

Example of weird behavior caused by the bug:

  $ qemu-img create -f qcow2 this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2 5G
  Formatting 'this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2', fmt=qcow2 size=5368709120 encryption=off cluster_size=65536
  $ ./x86_64-softmmu/qemu-system-x86_64 -S -monitor stdio -numa node 'this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2'
  QEMU 1.3.50 monitor - type 'help' for more information
  (qemu) info numa
  1 nodes
  node 0 cpus: 0
  node 0 size: 1000 MB
  (qemu)

This changes the code to nove the pointer only if ',' is found.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 15e0280..f29d926 100644
--- a/vl.c
+++ b/vl.c
@@ -1250,7 +1250,10 @@ static void numa_add(const char *optarg)
 
     value = endvalue = 0ULL;
 
-    optarg = get_opt_name(option, 128, optarg, ',') + 1;
+    optarg = get_opt_name(option, 128, optarg, ',');
+    if (*optarg == ',') {
+        optarg++;
+    }
     if (!strcmp(option, "node")) {
         if (get_param_value(option, 128, "nodeid", optarg) == 0) {
             nodenr = nb_numa_nodes;
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 2/8] vl.c: Fix off-by-one bug when handling "-numa node" argument

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> The numa_add() code was unconditionally adding 1 to the get_opt_name()
> return value, making it point after the end of the string if no ','
> separator is present.
> 
> Example of weird behavior caused by the bug:
> 
>   $ qemu-img create -f qcow2 this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2 5G
>   Formatting 'this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2', fmt=qcow2 size=5368709120 encryption=off cluster_size=65536
>   $ ./x86_64-softmmu/qemu-system-x86_64 -S -monitor stdio -numa node 'this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2'
>   QEMU 1.3.50 monitor - type 'help' for more information
>   (qemu) info numa
>   1 nodes
>   node 0 cpus: 0
>   node 0 size: 1000 MB
>   (qemu)
> 
> This changes the code to nove the pointer only if ',' is found.

s/nove/move/

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 3/8] vl.c: Abort on unknown -numa option type

[Eduardo Habkost]

Abort in case an invalid -numa option is provided, instead of silently
ignoring it.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/vl.c b/vl.c
index f29d926..1cf8ba6 100644
--- a/vl.c
+++ b/vl.c
@@ -1290,6 +1290,9 @@ static void numa_add(const char *optarg)
             bitmap_set(node_cpumask[nodenr], value, endvalue-value+1);
         }
         nb_numa_nodes++;
+    } else {
+        fprintf(stderr, "Invalid -numa option: %s\n", option);
+        exit(1);
     }
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 4/8] vl.c: Check for NUMA node limit inside numa_add()

[Eduardo Habkost]

Instead of checking the limit before calling numa_add(), check the limit
only when we already know we're going to add a new node.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Implement the change without adding numa_node_add() function
---
 vl.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 1cf8ba6..dabbba1 100644
--- a/vl.c
+++ b/vl.c
@@ -1255,6 +1255,12 @@ static void numa_add(const char *optarg)
         optarg++;
     }
     if (!strcmp(option, "node")) {
+
+        if (nb_numa_nodes >= MAX_NODES) {
+            fprintf(stderr, "qemu: too many NUMA nodes\n");
+            exit(1);
+        }
+
         if (get_param_value(option, 128, "nodeid", optarg) == 0) {
             nodenr = nb_numa_nodes;
         } else {
@@ -2960,10 +2966,6 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_numa:
-                if (nb_numa_nodes >= MAX_NODES) {
-                    fprintf(stderr, "qemu: too many NUMA nodes\n");
-                    exit(1);
-                }
                 numa_add(optarg);
                 break;
             case QEMU_OPTION_display:
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 4/8] vl.c: Check for NUMA node limit inside numa_add()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> Instead of checking the limit before calling numa_add(), check the limit
> only when we already know we're going to add a new node.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v2:
>  - Implement the change without adding numa_node_add() function
> ---
>  vl.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 5/8] vl.c: numa_add(): Validate nodeid before using it

[Eduardo Habkost]

Without this check, QEMU will corrupt memory if a too-large nodeid is
provided in the command-line. e.g.:

  -numa node,mem=...,cpus=...,nodeid=65

This changes nodenr to unsigned long long, to avoid integer conversion
issues when converting the strtoull() result to int.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Implement change without creation of numa_node_add() function
---
 vl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index dabbba1..3637a39 100644
--- a/vl.c
+++ b/vl.c
@@ -1246,7 +1246,7 @@ static void numa_add(const char *optarg)
     char option[128];
     char *endptr;
     unsigned long long value, endvalue;
-    int nodenr;
+    unsigned long long nodenr;
 
     value = endvalue = 0ULL;
 
@@ -1267,6 +1267,11 @@ static void numa_add(const char *optarg)
             nodenr = strtoull(option, NULL, 10);
         }
 
+        if (nodenr >= MAX_NODES) {
+            fprintf(stderr, "qemu: invalid NUMA nodeid: %d\n", nodenr);
+            exit(1);
+        }
+
         if (get_param_value(option, 128, "mem", optarg) == 0) {
             node_mem[nodenr] = 0;
         } else {
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 5/8] vl.c: numa_add(): Validate nodeid before using it

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> Without this check, QEMU will corrupt memory if a too-large nodeid is
> provided in the command-line. e.g.:
> 
>   -numa node,mem=...,cpus=...,nodeid=65
> 
> This changes nodenr to unsigned long long, to avoid integer conversion
> issues when converting the strtoull() result to int.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v2:
>  - Implement change without creation of numa_node_add() function

>  
> +        if (nodenr >= MAX_NODES) {
> +            fprintf(stderr, "qemu: invalid NUMA nodeid: %d\n", nodenr);

%lld (I see you later fixed that in 6/8, but you should rebase that hunk
into the patch that first needs it).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eduardo Habkost]

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(-)

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);
         }
 
-- 
1.7.11.7

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

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?

>  
>          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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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 <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.

-- 
Eduardo

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

On 01/16/2013 10:42 AM, Eduardo Habkost wrote:
> 
> 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?

I'm not a maintainer, so anyone is free to overrule me; but I'm
personally fine with just documenting that the semantic change is
intentional.  It's enough of a corner case that probably no one was
relying on leading 0 still giving decimal parsing (libvirt certainly
wasn't relying on it], and the flexibility of allowing hex parsing is
useful.  But do document it in the commit message; a silent change is
not fair.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eduardo Habkost]

On Wed, Jan 16, 2013 at 10:54:34AM -0700, Eric Blake wrote:
> On 01/16/2013 10:42 AM, Eduardo Habkost wrote:
> > 
> > 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?
> 
> I'm not a maintainer, so anyone is free to overrule me; but I'm
> personally fine with just documenting that the semantic change is
> intentional.  It's enough of a corner case that probably no one was
> relying on leading 0 still giving decimal parsing (libvirt certainly
> wasn't relying on it], and the flexibility of allowing hex parsing is
> useful.  But do document it in the commit message; a silent change is
> not fair.

I will add a 'base' parameter to parse_uint*() anyway, as it may be
useful somewhere else. Then keeping the existing behavior will be
trivial, so I will keep it by now.

Later, when changing the code to use QemuOpts, we may decide to change
the behavior (to keep the code simpler) and document the semantic
change.

-- 
Eduardo

[Qemu-devel] [PATCH 7/8] vl.c: Extract -numa "cpus" parsing to separate function

[Eduardo Habkost]

This will make it easier to refactor that code later.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Implemented change without creation of numa_node_add() function
---
 vl.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/vl.c b/vl.c
index 43ce607..98cf9f0 100644
--- a/vl.c
+++ b/vl.c
@@ -1241,15 +1241,34 @@ char *get_boot_devices_list(uint32_t *size)
     return list;
 }
 
+static void numa_node_parse_cpus(int nodenr, const char *cpus)
+{
+    char *endptr;
+    unsigned long long value, endvalue;
+
+    value = strtoull(cpus, &endptr, 10);
+    if (*endptr == '-') {
+        endvalue = strtoull(endptr+1, &endptr, 10);
+    } else {
+        endvalue = value;
+    }
+
+    if (!(endvalue < MAX_CPUMASK_BITS)) {
+        endvalue = MAX_CPUMASK_BITS - 1;
+        fprintf(stderr,
+            "A max of %d CPUs are supported in a guest\n",
+             MAX_CPUMASK_BITS);
+    }
+
+    bitmap_set(node_cpumask[nodenr], value, endvalue-value+1);
+}
+
 static void numa_add(const char *optarg)
 {
     char option[128];
     char *endptr;
-    unsigned long long value, endvalue;
     unsigned long long nodenr;
 
-    value = endvalue = 0ULL;
-
     optarg = get_opt_name(option, 128, optarg, ',');
     if (*optarg == ',') {
         optarg++;
@@ -1287,21 +1306,7 @@ static void numa_add(const char *optarg)
             node_mem[nodenr] = sval;
         }
         if (get_param_value(option, 128, "cpus", optarg) != 0) {
-            value = strtoull(option, &endptr, 10);
-            if (*endptr == '-') {
-                endvalue = strtoull(endptr+1, &endptr, 10);
-            } else {
-                endvalue = value;
-            }
-
-            if (!(endvalue < MAX_CPUMASK_BITS)) {
-                endvalue = MAX_CPUMASK_BITS - 1;
-                fprintf(stderr,
-                    "A max of %d CPUs are supported in a guest\n",
-                     MAX_CPUMASK_BITS);
-            }
-
-            bitmap_set(node_cpumask[nodenr], value, endvalue-value+1);
+            numa_node_parse_cpus(nodenr, option);
         }
         nb_numa_nodes++;
     } else {
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 7/8] vl.c: Extract -numa "cpus" parsing to separate function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> This will make it easier to refactor that code later.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v2:
>  - Implemented change without creation of numa_node_add() function
> ---
>  vl.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eduardo Habkost]

- 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(-)

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)
-- 
1.7.11.7

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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 <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.

-- 
Eduardo

[Qemu-devel] [PATCH 0/8] -numa option parsing fixes (v3)

[Eduardo Habkost]

Changes v2 -> v3:
 - Add 'base' parameter to parse_uint*() (patch 1/8)
 - Keep existing base=10 behavior when parsing "nodeid" and "cpus"
   (patches 6/8, 8/8)
 - Trivial whitespace change on patch 1/8
 - Fix fprintf() format string on patch 5/8

This series contains only the most important fixes from the previous "-numa
option parsing fixes & improvements" series I have submitted.

I have introduced parse_uint*() helpers that can be reused by other code, later.
I plan to submit parse_int*() (for signed integers) and parse_double*()
functions too, later, and change string-input-visitor.c and opts-visitor.c to
use those common functions instead of duplicating the number parsing code.


Eduardo Habkost (8):
  cutils: unsigned int parsing functions
  vl.c: Fix off-by-one bug when handling "-numa node" argument
  vl.c: Abort on unknown -numa option type
  vl.c: Check for NUMA node limit inside numa_add()
  vl.c: numa_add(): Validate nodeid before using it
  vl.c: Use parse_uint_full() for NUMA nodeid
  vl.c: Extract -numa "cpus" parsing to separate function
  vl.c: validate -numa "cpus" parameter properly

 include/qemu-common.h |   4 +
 tests/Makefile        |   3 +
 tests/test-cutils.c   | 216 ++++++++++++++++++++++++++++++++++++++++++++++++++
 util/cutils.c         |  79 ++++++++++++++++++
 vl.c                  |  93 ++++++++++++++++------
 5 files changed, 370 insertions(+), 25 deletions(-)
 create mode 100644 tests/test-cutils.c

-- 
1.7.11.7

[Qemu-devel] [PATCH 2/8] vl.c: Fix off-by-one bug when handling "-numa node" argument

[Eduardo Habkost]

The numa_add() code was unconditionally adding 1 to the get_opt_name()
return value, making it point after the end of the string if no ','
separator is present.

Example of weird behavior caused by the bug:

  $ qemu-img create -f qcow2 this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2 5G
  Formatting 'this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2', fmt=qcow2 size=5368709120 encryption=off cluster_size=65536
  $ ./x86_64-softmmu/qemu-system-x86_64 -S -monitor stdio -numa node 'this-file-image-has,cpus=5,mem=1000,in-its-name.qcow2'
  QEMU 1.3.50 monitor - type 'help' for more information
  (qemu) info numa
  1 nodes
  node 0 cpus: 0
  node 0 size: 1000 MB
  (qemu)

This changes the code to nove the pointer only if ',' is found.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 15e0280..f29d926 100644
--- a/vl.c
+++ b/vl.c
@@ -1250,7 +1250,10 @@ static void numa_add(const char *optarg)
 
     value = endvalue = 0ULL;
 
-    optarg = get_opt_name(option, 128, optarg, ',') + 1;
+    optarg = get_opt_name(option, 128, optarg, ',');
+    if (*optarg == ',') {
+        optarg++;
+    }
     if (!strcmp(option, "node")) {
         if (get_param_value(option, 128, "nodeid", optarg) == 0) {
             nodenr = nb_numa_nodes;
-- 
1.7.11.7