[Qemu-devel] [PATCH 00/10] -numa option parsing fixes & improvements

[Qemu-devel] [PATCH 00/10] -numa option parsing fixes & improvements

[Eduardo Habkost]

This series contains the following:

 * Patches 1-7 are multiple bug fixes to the current code
 * Patch 8 introduce a feature that libvirt requires since a long time,
   and even tries to use it today (in a way that doesn't work,
   using the "-numa node,cpus=1,2,3,4" format): having non-contiguous CPU
   ranges assigned to a NUMA node.

The last 2 patches I am sending as RFCs:

 * Patch 9 makes the "-numa" option deprecated and introduces a "-numa-node"
   command-line and config file option.
 * Patch 10 adds a small hack to the (now deprecated) "-numa" option, that
   makes the "cpus=1,2,3,4" format currently used by libvirt work.


Eduardo Habkost (10):
  vl.c: Fix off-by-one bug when handling "-numa node" argument
  vl.c: Abort on unknown -numa option type
  vl.c: Isolate code specific to "-numa node" option type
  vl.c: Check for NUMA node limit inside numa_node_add()
  vl.c: Extract -numa "cpus" parsing to separate function
  vl.c: handle invalid NUMA CPU ranges properly
  vl.c: numa_add_node(): Validate nodeid before using it
  vl.c: Support multiple CPU ranges on -numa option
  vl.c: Introduce QemuOpts-friendly "-numa-node" config option
  vl.c: Handle legacy "-numa node,cpus=A,B,C,D" format

 qemu-config.c   |  25 +++++++
 qemu-options.hx |  50 +++++++++++++-
 vl.c            | 204 ++++++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 240 insertions(+), 39 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 01/10] 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 e5da31c..93fde36 100644
--- a/vl.c
+++ b/vl.c
@@ -1061,7 +1061,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

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

[Eduardo Habkost]

Instead of silently ignoring them, abort in case an invalid -numa option
is provided.

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 93fde36..042cc7f 100644
--- a/vl.c
+++ b/vl.c
@@ -1101,6 +1101,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 03/10] vl.c: Isolate code specific to "-numa node" option type

[Eduardo Habkost]

Extract the code that's specific for the "node" -numa option type (the
only one, today) to a separate function.

The extracted code will eventually become a function specific for a
"numa-node" config section, independent from the numa_add() code.

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

diff --git a/vl.c b/vl.c
index 042cc7f..94cc6fd 100644
--- a/vl.c
+++ b/vl.c
@@ -1052,7 +1052,7 @@ char *get_boot_devices_list(uint32_t *size)
     return list;
 }
 
-static void numa_add(const char *optarg)
+static void numa_node_add(const char *optarg)
 {
     char option[128];
     char *endptr;
@@ -1061,46 +1061,53 @@ static void numa_add(const char *optarg)
 
     value = endvalue = 0ULL;
 
-    optarg = get_opt_name(option, 128, optarg, ',');
-    if (*optarg == ',') {
-        optarg++;
+    if (get_param_value(option, 128, "nodeid", optarg) == 0) {
+        nodenr = nb_numa_nodes;
+    } else {
+        nodenr = strtoull(option, NULL, 10);
     }
-    if (!strcmp(option, "node")) {
-        if (get_param_value(option, 128, "nodeid", optarg) == 0) {
-            nodenr = nb_numa_nodes;
+
+    if (get_param_value(option, 128, "mem", optarg) == 0) {
+        node_mem[nodenr] = 0;
+    } else {
+        int64_t sval;
+        sval = strtosz(option, &endptr);
+        if (sval < 0 || *endptr) {
+            fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+            exit(1);
+        }
+        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 {
-            nodenr = strtoull(option, NULL, 10);
+            endvalue = value;
         }
 
-        if (get_param_value(option, 128, "mem", optarg) == 0) {
-            node_mem[nodenr] = 0;
-        } else {
-            int64_t sval;
-            sval = strtosz(option, &endptr);
-            if (sval < 0 || *endptr) {
-                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
-                exit(1);
-            }
-            node_mem[nodenr] = sval;
+        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);
         }
-        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);
+    }
+    nb_numa_nodes++;
+}
 
-            bitmap_set(node_cpumask[nodenr], value, endvalue-value+1);
-        }
-        nb_numa_nodes++;
+static void numa_add(const char *optarg)
+{
+    char option[128];
+
+    optarg = get_opt_name(option, 128, optarg, ',');
+    if (*optarg == ',') {
+        optarg++;
+    }
+    if (!strcmp(option, "node")) {
+        numa_node_add(optarg);
     } else {
         fprintf(stderr, "Invalid -numa option: %s\n", option);
         exit(1);
-- 
1.7.11.7

[Qemu-devel] [PATCH 04/10] vl.c: Check for NUMA node limit inside numa_node_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>
---
 vl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 94cc6fd..2c3bbb9 100644
--- a/vl.c
+++ b/vl.c
@@ -1061,6 +1061,11 @@ static void numa_node_add(const char *optarg)
 
     value = endvalue = 0ULL;
 
+    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 {
@@ -2762,10 +2767,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

[Qemu-devel] [PATCH 05/10] 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>
---
 vl.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/vl.c b/vl.c
index 2c3bbb9..03a826e 100644
--- a/vl.c
+++ b/vl.c
@@ -1052,15 +1052,33 @@ 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_node_add(const char *optarg)
 {
     char option[128];
     char *endptr;
-    unsigned long long value, endvalue;
     int nodenr;
 
-    value = endvalue = 0ULL;
-
     if (nb_numa_nodes >= MAX_NODES) {
         fprintf(stderr, "qemu: too many NUMA nodes\n");
         exit(1);
@@ -1084,21 +1102,7 @@ static void numa_node_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++;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 06/10] vl.c: handle invalid NUMA CPU ranges properly

[Eduardo Habkost]

Add checks for the following cases:

* Empty string: will be ignored and won't set any CPU bitmap,
  parser won't abort.
* Missing end value after "-": parser will abort.
* Extra characters after a valid CPU range: parser will abort.
* "N-M" string where M < N: parser will abort.

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

diff --git a/vl.c b/vl.c
index 03a826e..19010fa 100644
--- a/vl.c
+++ b/vl.c
@@ -1057,13 +1057,30 @@ static void numa_node_parse_cpus(int nodenr, const char *cpus)
     char *endptr;
     unsigned long long value, endvalue;
 
+    /* Empty strings will be ignored, and not considered an error */
+    if (!*cpus) {
+        return;
+    }
+
     value = strtoull(cpus, &endptr, 10);
     if (*endptr == '-') {
-        endvalue = strtoull(endptr+1, &endptr, 10);
+        endptr++;
+        if (!*endptr) {
+            goto error;
+        }
+        endvalue = strtoull(endptr, &endptr, 10);
     } else {
         endvalue = value;
     }
 
+    if (*endptr != '\0')  {
+        goto error;
+    }
+
+    if (endvalue < value) {
+        goto error;
+    }
+
     if (!(endvalue < MAX_CPUMASK_BITS)) {
         endvalue = MAX_CPUMASK_BITS - 1;
         fprintf(stderr, "A max of %d CPUs are supported in a guest\n",
@@ -1071,6 +1088,11 @@ static void numa_node_parse_cpus(int nodenr, const char *cpus)
     }
 
     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_node_add(const char *optarg)
-- 
1.7.11.7

[Qemu-devel] [PATCH 07/10] vl.c: numa_add_node(): Validate nodeid before using it

[Eduardo Habkost]

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

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

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

diff --git a/vl.c b/vl.c
index 19010fa..31175f6 100644
--- a/vl.c
+++ b/vl.c
@@ -1112,6 +1112,11 @@ static void numa_node_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

[Qemu-devel] [PATCH 08/10] vl.c: Support multiple CPU ranges on -numa option

[Eduardo Habkost]

This allows "," or ";" to be used a separator between each CPU range.
Note that commas inside key=value command-line options have to be
escaped using ",,", so the command-line will look like:

  -numa node,cpus=A,,B,,C,,D

or:

  -numa node,cpus=A;B;C;D

Note that the following format, currently used by libvirt:

  -numa nodes,cpus=A,B,C,D

will _not_ work yet, as "," is the option separator for the command-line
option parser, and it will require changing the -numa option parsing
code to handle "cpus" as a special case.

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

diff --git a/vl.c b/vl.c
index 31175f6..d7337c1 100644
--- a/vl.c
+++ b/vl.c
@@ -1052,7 +1052,7 @@ char *get_boot_devices_list(uint32_t *size)
     return list;
 }
 
-static void numa_node_parse_cpus(int nodenr, const char *cpus)
+static void numa_node_parse_cpu_range(int nodenr, const char *cpus)
 {
     char *endptr;
     unsigned long long value, endvalue;
@@ -1095,6 +1095,18 @@ error:
     exit(1);
 }
 
+static void numa_node_parse_cpus(int nodenr, const char *option)
+{
+    char **parts;
+    int i;
+
+    parts = g_strsplit_set(option, ",;", 0);
+    for (i = 0; parts[i]; i++) {
+        numa_node_parse_cpu_range(nodenr, parts[i]);
+    }
+    g_strfreev(parts);
+}
+
 static void numa_node_add(const char *optarg)
 {
     char option[128];
-- 
1.7.11.7

[Qemu-devel] [RFC 09/10] vl.c: Introduce QemuOpts-friendly "-numa-node" config option

[Eduardo Habkost]

This introduces both a "-numa-node" command-line option that is parsed
using a pure qemu_opts_parse() call, and a "numa-node" config file
section.

As the -numa option has a non-standard syntax, parse it manually and
translate it "numa-node" config options.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-config.c   |  25 ++++++++++++
 qemu-options.hx |  50 ++++++++++++++++++++++-
 vl.c            | 120 ++++++++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 164 insertions(+), 31 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 2188c3e..601dcda 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -647,6 +647,30 @@ static QemuOptsList qemu_object_opts = {
     },
 };
 
+static QemuOptsList qemu_numa_node_opts = {
+    .name = "numa-node",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_numa_node_opts.head),
+    .implied_opt_name = "type",
+    .desc = {
+        {
+            .name = "cpus",
+            .type = QEMU_OPT_STRING,
+            .help = "CPU range in the format N[-M]",
+        },
+        {
+            .name = "mem",
+            .type = QEMU_OPT_STRING,
+            .help = "RAM size for node, in MB",
+        },
+        {
+            .name = "nodeid",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Node ID",
+        },
+        { /* end of list */ }
+    },
+};
+
 static QemuOptsList *vm_config_groups[32] = {
     &qemu_drive_opts,
     &qemu_chardev_opts,
@@ -664,6 +688,7 @@ static QemuOptsList *vm_config_groups[32] = {
     &qemu_sandbox_opts,
     &qemu_add_fd_opts,
     &qemu_object_opts,
+    &qemu_numa_node_opts,
     NULL,
 };
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 9df0cde..a1c1a87 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -99,8 +99,54 @@ DEF("numa", HAS_ARG, QEMU_OPTION_numa,
 STEXI
 @item -numa @var{opts}
 @findex -numa
-Simulate a multi node NUMA system. If mem and cpus are omitted, resources
-are split equally.
+Deprecated. See the @option{-numa-node} option.
+ETEXI
+
+DEF("numa-node", HAS_ARG, QEMU_OPTION_numa_node,
+    "-numa-node [nodeid=@var{nodeid}][,mem=@var{size}][,cpus=@var{cpuranges}\n", QEMU_ARCH_ALL)
+STEXI
+@item -numa-node @var{opts}
+@findex -numa-node
+
+Define a NUMA node.
+
+@table @option
+@item nodeid=@var{nodeid}
+Index of the NUMA node, starting with 0. If omitted, NUMA nodes will be defined
+in the order they appear.
+
+@item mem=@var{size}
+
+Sets the RAM size of the NUMA node (in MB, of no unit is specified). If size
+of all nodes is omitted, memory is split equally.
+
+@item cpus=@var{cpus}
+
+@var{cpus} is a list of CPU indexes or CPU index ranges in the format:
+@samp{@var{start}[-@var{end}]}, separated by commas or semicolons.
+
+Note that commas used in values in key=value options have to be escaped, using
+@samp{,,}.
+
+The @option{cpus} option may appear multiple times, to assign multiple CPUs or
+CPU ranges to a node.
+
+If no node has CPU ranges assigned, CPUs will be split equally between the
+nodes.
+@end table
+
+Examples:
+
+@example
+-numa-node nodeid=1,mem=1024,cpus=0,,2,,4,,6 \
+-numa-node nodeid=0,mem=1024,cpus=1,,3,,5,,7
+@end example
+
+@example
+-numa-node mem=1024,cpus=0-3,,8-11 \
+-numa-node mem=1024,cpus=4-7,cpus=12-15
+@end example
+
 ETEXI
 
 DEF("fda", HAS_ARG, QEMU_OPTION_fda,
diff --git a/vl.c b/vl.c
index d7337c1..14bf9b6 100644
--- a/vl.c
+++ b/vl.c
@@ -1052,7 +1052,7 @@ char *get_boot_devices_list(uint32_t *size)
     return list;
 }
 
-static void numa_node_parse_cpu_range(int nodenr, const char *cpus)
+static int numa_node_parse_cpu_range(int nodenr, const char *cpus)
 {
     char *endptr;
     unsigned long long value, endvalue;
@@ -1088,62 +1088,113 @@ static void numa_node_parse_cpu_range(int nodenr, const char *cpus)
     }
 
     bitmap_set(node_cpumask[nodenr], value, endvalue-value+1);
-    return;
+    return 0;
 
 error:
     fprintf(stderr, "qemu: Invalid NUMA CPU range: %s\n", cpus);
-    exit(1);
+    return -1;
 }
 
-static void numa_node_parse_cpus(int nodenr, const char *option)
+static int numa_node_parse_cpus(int nodenr, const char *option)
 {
     char **parts;
-    int i;
+    int i, r = 0;
 
     parts = g_strsplit_set(option, ",;", 0);
     for (i = 0; parts[i]; i++) {
-        numa_node_parse_cpu_range(nodenr, parts[i]);
+        r = numa_node_parse_cpu_range(nodenr, parts[i]);
+        if (r < 0) {
+            break;
+        }
     }
     g_strfreev(parts);
+    return r;
 }
 
-static void numa_node_add(const char *optarg)
+static int parse_numa_node_opt(const char *name, const char *value,
+                                void *opaque)
 {
-    char option[128];
-    char *endptr;
-    int nodenr;
+    uint64_t nodenr = *(uint64_t *)opaque;
+    if (!strcmp(name, "cpus")) {
+        return numa_node_parse_cpus(nodenr, value);
+    }
+    return 0;
+}
+
+static int parse_numa_node(QemuOpts *opts, void *opaque)
+{
+    uint64_t nodenr;
+    const char *memstr;
+    int64_t mem;
 
     if (nb_numa_nodes >= MAX_NODES) {
         fprintf(stderr, "qemu: too many NUMA nodes\n");
-        exit(1);
+        return -1;
+    }
+
+    nodenr = qemu_opt_get_number(opts, "nodeid", nb_numa_nodes);
+
+    if (nodenr >= MAX_NODES) {
+        fprintf(stderr, "qemu: invalid NUMA nodeid: %" PRIu64 "\n", nodenr);
+        return -1;
     }
 
-    if (get_param_value(option, 128, "nodeid", optarg) == 0) {
-        nodenr = nb_numa_nodes;
+    memstr = qemu_opt_get(opts, "mem");
+    if (memstr) {
+        char *endptr;
+        mem = strtosz(memstr, &endptr);
+        if (mem < 0 || *endptr) {
+            fprintf(stderr, "qemu: invalid numa mem size: %s\n", memstr);
+            return -1;
+        }
     } else {
-        nodenr = strtoull(option, NULL, 10);
+        mem = 0;
     }
 
-    if (nodenr >= MAX_NODES) {
-        fprintf(stderr, "qemu: invalid NUMA nodeid: %d\n", nodenr);
+    node_mem[nodenr] = mem;
+
+    /* There may be multiple "cpus" options set */
+    if (qemu_opt_foreach(opts, parse_numa_node_opt, &nodenr, 1) < 0) {
+        return -1;
+    }
+
+    nb_numa_nodes++;
+
+    return 0;
+}
+
+/* Parse legacy -numa option
+ *
+ * The option will be translated to a numa-node config option.
+ */
+static void parse_legacy_numa_node(const char *optarg)
+{
+    char option[128];
+    char value[128];
+    const char *p = optarg;
+    Error *error = NULL;
+    QemuOpts *opts = qemu_opts_create(qemu_find_opts("numa-node"),
+                                      NULL, 0, &error);
+    if (error) {
+        error_report("%s", error_get_pretty(error));
         exit(1);
     }
 
-    if (get_param_value(option, 128, "mem", optarg) == 0) {
-        node_mem[nodenr] = 0;
-    } else {
-        int64_t sval;
-        sval = strtosz(option, &endptr);
-        if (sval < 0 || *endptr) {
-            fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+    while (*p) {
+        p = get_opt_name(option, 128, p, '=');
+        if (*p == '=') {
+            p++;
+        } else {
+            fprintf(stderr, "Invalid -numa option: %s\n", optarg);
             exit(1);
         }
-        node_mem[nodenr] = sval;
-    }
-    if (get_param_value(option, 128, "cpus", optarg) != 0) {
-        numa_node_parse_cpus(nodenr, option);
+
+        p = get_opt_value(value, 128, p);
+        if (*p == ',') {
+            p++;
+        }
+        qemu_opt_set(opts, option, value);
     }
-    nb_numa_nodes++;
 }
 
 static void numa_add(const char *optarg)
@@ -1155,7 +1206,7 @@ static void numa_add(const char *optarg)
         optarg++;
     }
     if (!strcmp(option, "node")) {
-        numa_node_add(optarg);
+        parse_legacy_numa_node(optarg);
     } else {
         fprintf(stderr, "Invalid -numa option: %s\n", option);
         exit(1);
@@ -2812,6 +2863,12 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_numa:
                 numa_add(optarg);
                 break;
+            case QEMU_OPTION_numa_node:
+                opts = qemu_opts_parse(qemu_find_opts("numa-node"), optarg, 0);
+                if (!opts) {
+                    exit(1);
+                }
+                break;
             case QEMU_OPTION_display:
                 display_type = select_display(optarg);
                 break;
@@ -3856,6 +3913,11 @@ int main(int argc, char **argv, char **envp)
 
     register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
 
+    if (qemu_opts_foreach(qemu_find_opts("numa-node"),
+                          parse_numa_node, NULL, 1) < 0) {
+        exit(1);
+    }
+
     if (nb_numa_nodes > 0) {
         int i;
 
-- 
1.7.11.7

[Qemu-devel] [RFC 10/10] vl.c: Handle legacy "-numa node, cpus=A, B, C, D" format

[Eduardo Habkost]

As libvirt already uses this format and expects it to work, add a small
hack to the legacy -numa option parsing code to make the "cpus=A,B,C,D"
format work.

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

diff --git a/vl.c b/vl.c
index 14bf9b6..cf30d44 100644
--- a/vl.c
+++ b/vl.c
@@ -1194,6 +1194,17 @@ static void parse_legacy_numa_node(const char *optarg)
             p++;
         }
         qemu_opt_set(opts, option, value);
+
+        /* special case for "cpus", as it can contain "," */
+        if (!strcmp(option, "cpus")) {
+            while (isdigit(*p)) {
+                p = get_opt_value(value, 128, p);
+                if (*p == ',') {
+                    p++;
+                }
+                qemu_opt_set(opts, "cpus", value);
+            }
+        }
     }
 }
 
-- 
1.7.11.7

Re: [Qemu-devel] [libvirt] [PATCH 03/10] vl.c: Isolate code specific to "-numa node" option type

[Eric Blake]

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

On 01/11/2013 11:15 AM, Eduardo Habkost wrote:
> Extract the code that's specific for the "node" -numa option type (the
> only one, today) to a separate function.
> 
> The extracted code will eventually become a function specific for a
> "numa-node" config section, independent from the numa_add() code.

> +    if (get_param_value(option, 128, "nodeid", optarg) == 0) {
> +        nodenr = nb_numa_nodes;
> +    } else {
> +        nodenr = strtoull(option, NULL, 10);
>      }

strtoull() needs additional error checking after the fact, to make sure
I didn't pass an empty string, trailing garbage, or so many digits I
triggered overflow.


> +    if (get_param_value(option, 128, "cpus", optarg) != 0) {
> +        value = strtoull(option, &endptr, 10);
> +        if (*endptr == '-') {
> +            endvalue = strtoull(endptr+1, &endptr, 10);
>          } else {
> -            nodenr = strtoull(option, NULL, 10);
> +            endvalue = value;
>          }

More uses of strtoull() that aren't guarding against all possible errors.

-- 
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: 619 bytes --]

Re: [Qemu-devel] [libvirt] [PATCH 03/10] vl.c: Isolate code specific to "-numa node" option type

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 02:06:05PM -0700, Eric Blake wrote:
> On 01/11/2013 11:15 AM, Eduardo Habkost wrote:
> > Extract the code that's specific for the "node" -numa option type (the
> > only one, today) to a separate function.
> > 
> > The extracted code will eventually become a function specific for a
> > "numa-node" config section, independent from the numa_add() code.
> 
> > +    if (get_param_value(option, 128, "nodeid", optarg) == 0) {
> > +        nodenr = nb_numa_nodes;
> > +    } else {
> > +        nodenr = strtoull(option, NULL, 10);
> >      }
> 
> strtoull() needs additional error checking after the fact, to make sure
> I didn't pass an empty string, trailing garbage, or so many digits I
> triggered overflow.
> 
> 
> > +    if (get_param_value(option, 128, "cpus", optarg) != 0) {
> > +        value = strtoull(option, &endptr, 10);
> > +        if (*endptr == '-') {
> > +            endvalue = strtoull(endptr+1, &endptr, 10);
> >          } else {
> > -            nodenr = strtoull(option, NULL, 10);
> > +            endvalue = value;
> >          }
> 
> More uses of strtoull() that aren't guarding against all possible errors.

All this code gets replaced in patch 09/10. The only remaining
strtoull() call gets error-checking at patch 06/10.

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 06/10] vl.c: handle invalid NUMA CPU ranges properly

[Eric Blake]

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

On 01/11/2013 11:15 AM, Eduardo Habkost wrote:
> Add checks for the following cases:
> 
> * Empty string: will be ignored and won't set any CPU bitmap,
>   parser won't abort.
> * Missing end value after "-": parser will abort.
> * Extra characters after a valid CPU range: parser will abort.
> * "N-M" string where M < N: parser will abort.


>      value = strtoull(cpus, &endptr, 10);
>      if (*endptr == '-') {
> -        endvalue = strtoull(endptr+1, &endptr, 10);
> +        endptr++;
> +        if (!*endptr) {
> +            goto error;
> +        }
> +        endvalue = strtoull(endptr, &endptr, 10);
>      } else {
>          endvalue = value;
>      }

Still missing a check for '-numa=-2' with no number on the left of '-',
as well as missing a check for overflow for -numa=999999999999999999999999

-- 
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: 619 bytes --]

Re: [Qemu-devel] [libvirt] [PATCH 06/10] vl.c: handle invalid NUMA CPU ranges properly

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 02:32:48PM -0700, Eric Blake wrote:
> On 01/11/2013 11:15 AM, Eduardo Habkost wrote:
> > Add checks for the following cases:
> > 
> > * Empty string: will be ignored and won't set any CPU bitmap,
> >   parser won't abort.
> > * Missing end value after "-": parser will abort.
> > * Extra characters after a valid CPU range: parser will abort.
> > * "N-M" string where M < N: parser will abort.
> 
> 
> >      value = strtoull(cpus, &endptr, 10);
> >      if (*endptr == '-') {
> > -        endvalue = strtoull(endptr+1, &endptr, 10);
> > +        endptr++;
> > +        if (!*endptr) {
> > +            goto error;
> > +        }
> > +        endvalue = strtoull(endptr, &endptr, 10);
> >      } else {
> >          endvalue = value;
> >      }
> 
> Still missing a check for '-numa=-2' with no number on the left of '-',
> as well as missing a check for overflow for -numa=999999999999999999999999

Thanks!

I will fix and submit v2 of this patch.

-- 
Eduardo