Re: [Qemu-devel] [PATCH V3 06/10] NUMA: split out the common range parser

[Bandan Das]

Wanlong Gao <address@hidden> writes:

> Since cpus parser and hostnode parser have the common range parser
> part, split it out to the common range parser to avoid the duplicate
> code.
>
> Signed-off-by: Wanlong Gao <address@hidden>
> ---
>  vl.c | 89 
> ++++++++++++++++++++++++++++----------------------------------------
>  1 file changed, 37 insertions(+), 52 deletions(-)

For the patch order, I was thinking along the lines of introducing the common
function first followed by the ones that use it, but I guess this is fine too :)

Thanks for taking care of this!

Reviewed-by: Bandan Das <address@hidden>

> diff --git a/vl.c b/vl.c
> index 79c39b9..c9d25bd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1338,47 +1338,55 @@ char *get_boot_devices_list(size_t *size)
>      return list;
>  }
>  
> -static void numa_node_parse_cpus(int nodenr, const char *cpus, Error **errp)
> +static int numa_node_parse_common(const char *str,
> +                                  unsigned long long *value,
> +                                  unsigned long long *endvalue)
>  {
>      char *endptr;
> -    unsigned long long value, endvalue;
> -
> -    /* 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(str, value, &endptr, 10) < 0) {
> +        return -1;
>      }
>  
> -    if (parse_uint(cpus, &value, &endptr, 10) < 0) {
> -        goto error;
> -    }
>      if (*endptr == '-') {
> -        if (parse_uint_full(endptr + 1, &endvalue, 10) < 0) {
> -            goto error;
> +        if (parse_uint_full(endptr + 1, endvalue, 10) < 0) {
> +           return -1;
>          }
>      } else if (*endptr == '\0') {
> -        endvalue = value;
> +        *endvalue = *value;
>      } else {
> -        goto error;
> +        return -1;
>      }
>  
> -    if (endvalue >= MAX_CPUMASK_BITS) {
> -        endvalue = MAX_CPUMASK_BITS - 1;
> -        fprintf(stderr,
> -            "qemu: NUMA: A max of %d VCPUs are supported\n",
> -             MAX_CPUMASK_BITS);
> +    if (*endvalue >= MAX_CPUMASK_BITS) {
> +        *endvalue = MAX_CPUMASK_BITS - 1;
> +        fprintf(stderr, "qemu: NUMA: A max number %d is supported\n",
> +                MAX_CPUMASK_BITS);
>      }
>  
> -    if (endvalue < value) {
> -        goto error;
> +    if (*endvalue < *value) {
> +        return -1;
>      }
>  
> -    bitmap_set(numa_info[nodenr].node_cpu, value, endvalue-value+1);
> -    return;
> +    return 0;
> +}
>  
> -error:
> -    error_setg(errp, "Invalid NUMA CPU range: %s\n", cpus);
> +static void numa_node_parse_cpus(int nodenr, const char *cpus, Error **errp)
> +{
> +    unsigned long long value, endvalue;
> +
> +    /* Empty CPU range strings will be considered valid, they will simply
> +     * not set any bit in the CPU bitmap.
> +     */
> +    if (!*cpus) {
> +        return;
> +    }
> +
> +    if (numa_node_parse_common(cpus, &value, &endvalue) < 0) {
> +        error_setg(errp, "Invalid NUMA CPU range: %s", cpus);
> +        return;
> +    }
> +
> +    bitmap_set(numa_info[nodenr].node_cpu, value, endvalue-value+1);
>      return;
>  }
>  
> @@ -1403,7 +1411,6 @@ void numa_node_parse_mpol(int nodenr, const char *mpol, 
> Error **errp)
>  void numa_node_parse_hostnode(int nodenr, const char *hostnode, Error **errp)
>  {
>      unsigned long long value, endvalue;
> -    char *endptr;
>      bool clear = false;
>      unsigned long *bm = numa_info[nodenr].host_mem;
>  
> @@ -1422,27 +1429,9 @@ void numa_node_parse_hostnode(int nodenr, const char 
> *hostnode, Error **errp)
>          return;
>      }
>  
> -    if (parse_uint(hostnode, &value, &endptr, 10) < 0)
> -        goto error;
> -    if (*endptr == '-') {
> -        if (parse_uint_full(endptr + 1, &endvalue, 10) < 0) {
> -            goto error;
> -        }
> -    } else if (*endptr == '\0') {
> -        endvalue = value;
> -    } else {
> -        goto error;
> -    }
> -
> -    if (endvalue >= MAX_CPUMASK_BITS) {
> -        endvalue = MAX_CPUMASK_BITS - 1;
> -        fprintf(stderr,
> -            "qemu: NUMA: A max of %d host nodes are supported\n",
> -             MAX_CPUMASK_BITS);
> -    }
> -
> -    if (endvalue < value) {
> -        goto error;
> +    if (numa_node_parse_common(hostnode, &value, &endvalue) < 0) {
> +        error_setg(errp, "Invalid host NUMA ndoes range: %s", hostnode);
> +        return;
>      }
>  
>      if (clear)
> @@ -1451,10 +1440,6 @@ void numa_node_parse_hostnode(int nodenr, const char 
> *hostnode, Error **errp)
>          bitmap_set(bm, value, endvalue - value + 1);
>  
>      return;
> -
> -error:
> -    error_setg(errp, "Invalid host NUMA nodes range: %s", hostnode);
> -    return;
>  }
>  
>  static int numa_add_cpus(const char *name, const char *value, void *opaque)