[libvirt] [PATCH v2] virsh: freecell --all getting wrong NUMA nodes count

[Jiri Denemark]

> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 22d53e5..ba2f793 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -468,7 +468,8 @@ nodeGetCellsFreeMemory(virConnectPtr conn ATTRIBUTE_UNUSED,
>          long long mem;
>          if (numa_node_size64(n, &mem) < 0) {
>              nodeReportError(VIR_ERR_INTERNAL_ERROR,
> -                            "%s", _("Failed to query NUMA free memory"));
> +                            "%s: %d", _("Failed to query NUMA free memory "
> +                                    "for node"), n);

This is hard to translate and it doesn't work well if the number needs to be
somewhere else than at the end in some language. It should rather look like

    _("Failed to query NUMA free memory for node: %d"), n

>              goto cleanup;
>          }
>          freeMems[numCells++] = mem;
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 50d5e33..165a791 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -2301,32 +2307,60 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
>      }
>  
>      if (all_given) {
> -        if (virNodeGetInfo(ctl->conn, &info) < 0) {
> -            vshError(ctl, "%s", _("failed to get NUMA nodes count"));
> +        cap_xml = virConnectGetCapabilities(ctl->conn);
> +        if (!cap_xml) {
> +            vshError(ctl, "%s", _("unable to get node capabilities"));
>              goto cleanup;
>          }
>  
> -        if (!info.nodes) {
> -            vshError(ctl, "%s", _("no NUMA nodes present"));
> +        xml = xmlReadDoc((const xmlChar *) cap_xml, "node.xml", NULL,
> +                          XML_PARSE_NOENT | XML_PARSE_NONET | 
Space at the end of the line above.

> +                          XML_PARSE_NOWARNING);
> +
> +        if (!xml) {
> +            vshError(ctl, "%s", _("unable to get node capabilities"));
>              goto cleanup;
>          }
>  
> -        if (VIR_ALLOC_N(nodes, info.nodes) < 0) {
> -            vshError(ctl, "%s", _("could not allocate memory"));
> +        ctxt = xmlXPathNewContext(xml);
> +        nodes_cnt = virXPathNodeSet("/capabilities/host/topology/cells/cell",
> +                            ctxt, &nodes);
> +
> +        if (nodes_cnt == -1) {
> +            vshError(ctl, "%s", _("could not get information about "
> +                                  "NUMA topology"));
>              goto cleanup;
>          }
>  
> -        ret = virNodeGetCellsFreeMemory(ctl->conn, nodes, 0, info.nodes);
> -        if (ret != info.nodes) {
> -            vshError(ctl, "%s", _("could not get information about "
> -                                  "all NUMA nodes"));
> +        if ((VIR_ALLOC_N(nodes_free, nodes_cnt) < 0) ||
> +            (VIR_ALLOC_N(nodes_id, nodes_cnt) < 0)){
> +            vshError(ctl, "%s", _("could not allocate memory"));
>              goto cleanup;
>          }
You can use vshCalloc, which already prints an error and fails. Yeah, virsh
code is a mess and doesn't really fit in general libvirt coding standards. But
that's a different story.

>  
> +        for (i = 0; i < nodes_cnt; i++) {
> +            unsigned long id;
> +            char *val = virXMLPropString(nodes[i], "id");
> +            char *conv = NULL;
> +            id = strtoul(val, &conv, 10);
> +             if (*conv !='\0') {
> +                 vshError(ctl, "%s", _("conversion from string failed"));
> +                 goto cleanup;
> +             }
Apart from the funky indentation, you can use virStrToLong_ul() to do the job.
Also val is leaked here, you should free it just after converting it to
integer.

> +            nodes_id[i]=id;
> +            ret = virNodeGetCellsFreeMemory(ctl->conn, &(nodes_free[i]), id, 1);
> +            if (ret != 1) {
> +                vshError(ctl, "%s %lu", _("failed to get free memory for "
> +                                         "NUMA node nuber:"), id);
Move %lu inside the translatable string here as well.

> +                goto cleanup;
> +            }
> +        }
> +
>          memory = 0;
> -        for (cell = 0; cell < info.nodes; cell++) {
> -            vshPrint(ctl, "%5d: %10llu kB\n", cell, (nodes[cell]/1024));
> -            memory += nodes[cell];
> +        for (cell = 0; cell < nodes_cnt; cell++) {
> +            vshPrint(ctl, "%5lu: %10llu kB\n", nodes_id[cell],
> +                    (nodes_free[cell]/1024));
> +            memory += nodes_free[cell];
>          }
>  
>          vshPrintExtra(ctl, "--------------------\n");

Otherwise it looks good.

Jirka