Re: [Xen-devel] [PATCH v5 08/24] libxl: introduce libxl__vnuma_config_check

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2015-02-12 at 19:44 +0000, Wei Liu wrote:
> This function is used to check whether vNUMA configuration (be it
> auto-generated or supplied by user) is valid.
> 
> Define a new error code ERROR_VNUMA_CONFIG_INVALID.
> 
> The checks performed can be found in the comment of the function.
> 
> This vNUMA function (and future ones) is placed in a new file called
> libxl_vnuma.c
> 
I'm not sure whether having more files is a good or a bad thing. I would
say that libxl_numa.c is rather small, and can certainly accept more
code, if this kind of consolidation is desirable.

If it were me doing this, I'd put things there, but I don't have a super
strong opinion, and I'm also aware that I'm saying this pretty late, so
I'll just state this as a preference, and leave it to you and the
(other) maintainers.

> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: Elena Ufimtseva <ufimtseva@xxxxxxxxx>
> ---
> Changes in v5:
> 1. Define and use new error code.
> 2. Use LOG macro.
> 3. Fix hard tabs.
> 
> Changes in v4:
> 1. Adapt to new interface.
> 
> Changes in v3:
> 1. Rewrite commit log.
> 2. Shorten two error messages.
> ---
>  tools/libxl/Makefile         |   2 +-
>  tools/libxl/libxl_internal.h |   7 +++
>  tools/libxl/libxl_types.idl  |   1 +
>  tools/libxl/libxl_vnuma.c    | 131 
> +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 140 insertions(+), 1 deletion(-)
>  create mode 100644 tools/libxl/libxl_vnuma.c
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 7329521..1b16598 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -93,7 +93,7 @@ LIBXL_LIBS += -lyajl
>  LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
>                       libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
>                       libxl_internal.o libxl_utils.o libxl_uuid.o \
> -                     libxl_json.o libxl_aoutils.o libxl_numa.o \
> +                     libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o 
> \
>                       libxl_save_callout.o _libxl_save_msgs_callout.o \
>                       libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6d3ac58..258be0d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3394,6 +3394,13 @@ void libxl__numa_candidate_put_nodemap(libxl__gc *gc,
>      libxl_bitmap_copy(CTX, &cndt->nodemap, nodemap);
>  }
>  
> +/* Check if vNUMA config is valid. Returns 0 if valid,
> + * ERROR_VNUMA_CONFIG_INVALID otherwise.
> + */
> +int libxl__vnuma_config_check(libxl__gc *gc,
> +                              const libxl_domain_build_info *b_info,
> +                              const libxl__domain_build_state *state);
> +
>  _hidden int libxl__ms_vm_genid_set(libxl__gc *gc, uint32_t domid,
>                                     const libxl_ms_vm_genid *id);
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 14c7e7c..23951fc 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -63,6 +63,7 @@ libxl_error = Enumeration("error", [
>      (-17, "DEVICE_EXISTS"),
>      (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"),
>      (-19, "REMUS_DEVICE_NOT_SUPPORTED"),
> +    (-20, "VNUMA_CONFIG_INVALID"),
>      ], value_namespace = "")
>  
>  libxl_domain_type = Enumeration("domain_type", [
> diff --git a/tools/libxl/libxl_vnuma.c b/tools/libxl/libxl_vnuma.c
> new file mode 100644
> index 0000000..fa5aa8d
> --- /dev/null
> +++ b/tools/libxl/libxl_vnuma.c
> @@ -0,0 +1,131 @@
> +/*
> + * Copyright (C) 2014      Citrix Ltd.
> + * Author Wei Liu <wei.liu2@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxl_internal.h"
> +#include <stdlib.h>
> +
> +/* Sort vmemranges in ascending order with "start" */
> +static int compare_vmemrange(const void *a, const void *b)
> +{
> +    const xen_vmemrange_t *x = a, *y = b;
> +    if (x->start < y->start)
> +        return -1;
> +    if (x->start > y->start)
> +        return 1;
> +    return 0;
> +}
> +
> +/* Check if vNUMA configuration is valid:
> + *  1. all pnodes inside vnode_to_pnode array are valid
> + *  2. one vcpu belongs to and only belongs to one vnode
>
something like "each vcpu belongs to one and only one vnode" sounds
better here...

> + *  3. each vmemrange is valid and doesn't overlap with each other
>
"doesn't overlap with any other one" perhaps (of course, I'm not a
native speaker, so I can be very wrong! :-D)

> + */
> +int libxl__vnuma_config_check(libxl__gc *gc,
> +                              const libxl_domain_build_info *b_info,
> +                              const libxl__domain_build_state *state)
> +{
> +    int i, j, rc = ERROR_VNUMA_CONFIG_INVALID, nr_nodes;
>
You should init nr_nodes to = 0 (see below).

> +    libxl_numainfo *ninfo = NULL;
> +    uint64_t total_memkb = 0;
> +    libxl_bitmap cpumap;
> +    libxl_vnode_info *p;
> +
*v, or *vnode, or *vinfo, all sounds better than *p.

> +    rc = 0;
> +out:
> +    if (ninfo) libxl_numainfo_dispose(ninfo);
>
What you want here, to free a libxl_numainfo, is
libxl_numainfo_list_free(ninfo,nr_nodes), which, BTW, can be called
without checking whether ninfo is NULL, _provided_ you initialize
nr_nodes to 0.

Regards,
Dario

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel