[libvirt] [PATCH 20/34] conf: Store cpu pinning data in def->vcpus

[John Ferlan]

On 01/14/2016 11:27 AM, Peter Krempa wrote:
> Now with the new struct the data can be stored in a much saner place.
> ---
>  src/conf/domain_conf.c   | 131 ++++++++++++++++++--------------------------
>  src/conf/domain_conf.h   |   3 +-
>  src/libxl/libxl_domain.c |  17 +++---
>  src/libxl/libxl_driver.c |  39 ++++++--------
>  src/qemu/qemu_cgroup.c   |  15 ++----
>  src/qemu/qemu_driver.c   | 138 ++++++++++++++++++++++-------------------------
>  src/qemu/qemu_process.c  |  38 +++++++------
>  src/test/test_driver.c   |  43 ++++++---------
>  8 files changed, 179 insertions(+), 245 deletions(-)
> 

As noted from my review of 10/34, I'm just noting something Coverity
found - will review more as I get to this later.

[...]

> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 5986749..ed4de12 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -2455,15 +2455,14 @@ static int testDomainGetVcpus(virDomainPtr domain,
>      memset(cpumaps, 0, maxinfo * maplen);
> 
>      for (i = 0; i < maxinfo; i++) {
> -        virDomainPinDefPtr pininfo;
> +        virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i);
>          virBitmapPtr bitmap = NULL;
> 
> -        pininfo = virDomainPinFind(def->cputune.vcpupin,
> -                                   def->cputune.nvcpupin,
> -                                   i);
> +        if (vcpu && !vcpu->online)
> +            continue;


Coverity notes that by comparing vcpu to NULL here, but not doing so in
the following line could cause a NULL deref in the following lines.

> 
> -        if (pininfo && pininfo->cpumask)
> -            bitmap = pininfo->cpumask;
> +        if (vcpu->cpumask)
> +            bitmap = vcpu->cpumask;
>          else if (def->cpumask)
>              bitmap = def->cpumask;
>          else
> @@ -2492,6 +2491,7 @@ static int testDomainPinVcpu(virDomainPtr domain,
>                               unsigned char *cpumap,
>                               int maplen)
>  {
> +    virDomainVcpuInfoPtr vcpuinfo;
>      virDomainObjPtr privdom;
>      virDomainDefPtr def;
>      int ret = -1;
> @@ -2507,29 +2507,21 @@ static int testDomainPinVcpu(virDomainPtr domain,
>          goto cleanup;
>      }
> 
> -    if (vcpu > virDomainDefGetVcpus(privdom->def)) {
> +    if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu)) ||
> +        !vcpuinfo->online) {
>          virReportError(VIR_ERR_INVALID_ARG,
>                         _("requested vcpu '%d' is not present in the domain"),
>                         vcpu);
>          goto cleanup;
>      }
> 
> -    if (!def->cputune.vcpupin) {
> -        if (VIR_ALLOC(def->cputune.vcpupin) < 0)
> -            goto cleanup;
> -        def->cputune.nvcpupin = 0;
> -    }
> -    if (virDomainPinAdd(&def->cputune.vcpupin,
> -                        &def->cputune.nvcpupin,
> -                        cpumap,
> -                        maplen,
> -                        vcpu) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("failed to update or add vcpupin"));
> +    virBitmapFree(vcpuinfo->cpumask);
> +
> +    if (!(vcpuinfo->cpumask = virBitmapNewData(cpumap, maplen)))
>          goto cleanup;
> -    }
> 
>      ret = 0;
> +
>   cleanup:
>      virDomainObjEndAPI(&privdom);
>      return ret;
> @@ -2566,15 +2558,14 @@ testDomainGetVcpuPinInfo(virDomainPtr dom,
>          ncpumaps = virDomainDefGetVcpus(def);
> 
>      for (vcpu = 0; vcpu < ncpumaps; vcpu++) {
> -        virDomainPinDefPtr pininfo;
> +        virDomainVcpuInfoPtr vcpuinfo = virDomainDefGetVcpu(def, vcpu);
>          virBitmapPtr bitmap = NULL;
> 
> -        pininfo = virDomainPinFind(def->cputune.vcpupin,
> -                                   def->cputune.nvcpupin,
> -                                   vcpu);
> +        if (vcpuinfo && !vcpuinfo->online)
> +            continue;

Coverity notes that by comparing vcpuinfo to NULL here, but not doing so
in the following line could cause a NULL deref in the following lines.

> 
> -        if (pininfo && pininfo->cpumask)
> -            bitmap = pininfo->cpumask;
> +        if (vcpuinfo->cpumask)
> +            bitmap = vcpuinfo->cpumask;
>          else if (def->cpumask)
>              bitmap = def->cpumask;
>          else
>