[libvirt] [PATCH] Fix to list online cpus using virsh capabilities

[Michal Privoznik]

On 18.05.2015 12:28, Kothapally Madhu Pavan wrote:
> Virsh capabilities will list offline cpus as online when
> libvirt is compiled with numactl option disabled. This
> fix will list correct set of online cpus.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp at linux.vnet.ibm.com>
> ---
>  src/nodeinfo.c |   36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 22df95c..602c76c 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -1639,28 +1639,38 @@ nodeCapsInitNUMAFake(virCapsPtr caps ATTRIBUTE_UNUSED)
>  {
>      virNodeInfo nodeinfo;
>      virCapsHostNUMACellCPUPtr cpus;
> -    int ncpus;
> +    int ncpus, onlinecpus;
>      int s, c, t;
> -    int id;
> +    int id, cid;
> +    char *sysfs_cpudir = NULL;
>  
>      if (nodeGetInfo(&nodeinfo) < 0)
>          return -1;
>  
>      ncpus = VIR_NODEINFO_MAXCPUS(nodeinfo);
> +    onlinecpus = nodeinfo.cpus;
> +
> +    if (VIR_ALLOC_N(cpus, onlinecpus) < 0)
> +        return -1;
>  
> -    if (VIR_ALLOC_N(cpus, ncpus) < 0)
> +    if (virAsprintf(&sysfs_cpudir, SYSFS_CPU_PATH) < 0)

If this failed, @cpus are leaked. Moreover, why do you even need to copy
@SYSFS_CPU_PATH? Just use it directly where needed. Then, SYSFS_CPU_PATH
is declared only on linux system. So this breaks build on other systems.

>          return -1;
>  
>      id = 0;
> +    cid = 0;
>      for (s = 0; s < nodeinfo.sockets; s++) {
>          for (c = 0; c < nodeinfo.cores; c++) {
>              for (t = 0; t < nodeinfo.threads; t++) {
> -                cpus[id].id = id;
> -                cpus[id].socket_id = s;
> -                cpus[id].core_id = c;
> -                if (!(cpus[id].siblings = virBitmapNew(ncpus)))
> -                    goto error;
> -                ignore_value(virBitmapSetBit(cpus[id].siblings, id));
> +                if (virNodeGetCpuValue(sysfs_cpudir, id, "online", 0)) {

Unfortunately, this function is defined on linux only. Then, what about
cpu0? Just until recently, it wasn't possible to put it to offline mode,
so on most kernels, there's no "online" file under
/sys/devices/system/cpu/cpu0/. This results in virNodeGetCpuValue()
return the default value passed by caller (0 in this case). So even
thought the CPU#0 is online, it's reported as offline.

> +                    cpus[cid].id = id;
> +                    cpus[cid].socket_id = s;
> +                    cpus[cid].core_id = c;
> +                    if (!(cpus[cid].siblings = virBitmapNew(ncpus)))
> +                        goto error;
> +                    ignore_value(virBitmapSetBit(cpus[cid].siblings, id));
> +                    cid++;
> +                }
> +
>                  id++;
>              }
>          }
> @@ -1668,17 +1678,19 @@ nodeCapsInitNUMAFake(virCapsPtr caps ATTRIBUTE_UNUSED)
>  
>      if (virCapabilitiesAddHostNUMACell(caps, 0,
>                                         nodeinfo.memory,
> -                                       ncpus, cpus,
> +                                       onlinecpus, cpus,

I believe you wanted s/onlinecpus/cid/ as the @cpus array contains only
that much valid items (the rest are zeros).

>                                         0, NULL,
>                                         0, NULL) < 0)
>          goto error;
>  
> +    VIR_FREE(sysfs_cpudir);
>      return 0;
>  
>   error:
> -    for (; id >= 0; id--)
> -        virBitmapFree(cpus[id].siblings);
> +    for (; cid >= 0; cid--)
> +        virBitmapFree(cpus[cid].siblings);
>      VIR_FREE(cpus);
> +    VIR_FREE(sysfs_cpudir);
>      return -1;
>  }
>  

Otherwise looking good.

Michal