[libvirt] [PATCH 1/2] Add NUMA support to virshAllocpagesPagesizeCompleter.

[Michal Privoznik]

On 05/22/2018 11:54 AM, Roland Schulz wrote:
> Signed-off-by: Roland Schulz <schullzroll at gmail.com>
> ---
>  tools/virsh-completer.c | 15 ++++++++++++++-
>  tools/virsh-host.c      |  2 +-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
> index 2816e7b76..21c73f048 100644
> --- a/tools/virsh-completer.c
> +++ b/tools/virsh-completer.c
> @@ -579,6 +579,9 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl,
>      double size = 0;
>      size_t i = 0;
>      const char *suffix = NULL;
> +    const char *cellnum = NULL;
> +    bool cellno = vshCommandOptBool(cmd, "cellno");
> +    char *path = NULL;
>      char *pagesize = NULL;
>      char *cap_xml = NULL;
>      char **ret = NULL;
> @@ -595,7 +598,17 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl,
>      if (!(virXMLParseStringCtxt(cap_xml, _("capabilities"), &ctxt)))
>          goto error;
>  
> -    npages = virXPathNodeSet("/capabilities/host/cpu/pages", ctxt, &pages);
> +    if (cellno && vshCommandOptStringQuiet(ctl, cmd, "cellno", &cellnum) > 0) {
> +        if (virAsprintf(&path,
> +                        "/capabilities/host/topology/cells/cell[@id=\"%s\"]/pages",
> +                        cellnum) < 0)
> +            goto error;
> +    }
> +    else
> +        if (virAsprintf(&path, "/capabilities/host/cpu/pages") < 0)
> +            goto error;
> +

What an innovative way of avoiding syntax-check rule ;-)
This should be wrapped by curly braces too.

> +    npages = virXPathNodeSet(path, ctxt, &pages);
>      if (npages <= 0)
>          goto error;

So you're allocating @path but never free it.

>  
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index 293f06e9e..793a10452 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -473,7 +473,7 @@ static const vshCmdOptDef opts_allocpages[] = {
>       .type = VSH_OT_INT,
>       .flags = VSH_OFLAG_REQ,
>       .completer = virshAllocpagesPagesizeCompleter,
> -     .help = N_("page size (in kibibytes)")
> +     .help = N_("page size")

This change is not necessary. 'allocpages --pagesize 2048' just works,
just like 'allocpages --pagesize 2MiB'.

I'm fixing the curly braces and mem leak problems, ACKing and pushing.
However, there are couple of problems that deserve fixing:

1) @pages shouldn't be freed individually like they are under the
cleanup label.
2) There's still one memleak (retval of virXMLParseStringCtxt)
3) The misordering of variable declaration and free calls makes it hard
to follow which variables are freed and which are forgotten.

Michal