Re: [Qemu-devel] [PATCH] numa: fixup parsed NumaNodeOptions earlier

[Eduardo Habkost <ehabkost@redhat.com>]

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 <imammedo@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 <ehabkost@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