On Thu, Oct 12, 2017 at 11:39:58AM +0200, Igor Mammedov wrote:
> numa 'mem' option with suffix or without one is possible
> only on CLI/HMP. Instead of fixing up special suffix less
> CLI case deep in parse_numa_node() do it earlier right
> after option is parsed into NumaNodeOptions with OptVisistor
> so that the rest of the code would use valid values in
> NumaNodeOptions and won't have to reparse QemuOpts.
>
> It will help to isolate CLI/HMP parts in parse_numa() and
> split out parsed NumaNodeOptions processing into separate
> function that could be reused by QMP handler where we have
> only NumaNodeOptions and don't need any fixups.
>
> While at it reuse qemu_strtosz_MiB() instead of manually
> checking for suffixes.
>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> numa.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/numa.c b/numa.c
> index 17ea514..fb4c35a 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -38,6 +38,7 @@
> #include "hw/mem/pc-dimm.h"
> #include "qemu/option.h"
> #include "qemu/config-file.h"
> +#include "qemu/cutils.h"
>
> QemuOptsList qemu_numa_opts = {
> .name = "numa",
> @@ -142,7 +143,7 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp)
> }
>
> static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
> - QemuOpts *opts, Error **errp)
> + Error **errp)
> {
> uint16_t nodenr;
> uint16List *cpus = NULL;
> @@ -199,13 +200,7 @@ static void parse_numa_node(MachineState *ms,
> NumaNodeOptions *node,
> }
>
> if (node->has_mem) {
> - uint64_t mem_size = node->mem;
> - const char *mem_str = qemu_opt_get(opts, "mem");
> - /* Fix up legacy suffix-less format */
> - if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> - mem_size <<= 20;
> - }
> - numa_info[nodenr].node_mem = mem_size;
> + numa_info[nodenr].node_mem = node->mem;
> }
> if (node->has_memdev) {
> Object *o;
> @@ -290,9 +285,15 @@ int parse_numa(void *opaque, QemuOpts *opts, Error
> **errp)
> goto end;
> }
>
> + /* Fix up legacy suffix-less format */
> + if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
> + const char *mem_str = qemu_opt_get(opts, "mem");
> + qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
> + }
> +
I would like to have a way to represent this behavior in the QAPI
schema somehow. Markus, do you see a solution for that?
But this patch is an improvement, anyway, so:
Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
> switch (object->type) {
> case NUMA_OPTIONS_TYPE_NODE:
> - parse_numa_node(ms, &object->u.node, opts, &err);
> + parse_numa_node(ms, &object->u.node, &err);
> if (err) {
> goto end;
> }
> --
> 2.7.4
>
--
Eduardo
