[libvirt] [PATCH 09/10] qemu: monitor: Return structures from qemuMonitorGetCPUInfo

[John Ferlan]

On 08/03/2016 04:11 AM, Peter Krempa wrote:
> The function will gradually add more returned data. Return a struct for
> every vCPU containing the data.
> ---
>  src/qemu/qemu_domain.c  | 26 ++++++++++-------------
>  src/qemu/qemu_monitor.c | 56 ++++++++++++++++++++++++++++++++++++++++++-------
>  src/qemu/qemu_monitor.h | 13 +++++++++++-
>  3 files changed, 72 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4cdd012..9d389f7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5682,10 +5682,11 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>                            int asyncJob)
>  {
>      virDomainVcpuDefPtr vcpu;
> +    qemuDomainVcpuPrivatePtr vcpupriv;
> +    qemuMonitorCPUInfoPtr info = NULL;
>      size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
> -    pid_t *cpupids = NULL;
> -    int ncpupids;
>      size_t i;
> +    int rc;
>      int ret = -1;
> 
>      /*
> @@ -5721,31 +5722,26 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> 
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>          return -1;
> -    ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids);
> +
> +    rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus);
> +
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;
> 
> -    /* failure to get the VCPU <-> PID mapping or to execute the query
> -     * command will not be treated fatal as some versions of qemu don't
> -     * support this command */
> -    if (ncpupids <= 0) {
> -        virResetLastError();
> -        ret = 0;
> +    if (rc < 0)
>          goto cleanup;
> -    }
> 
>      for (i = 0; i < maxvcpus; i++) {
>          vcpu = virDomainDefGetVcpu(vm->def, i);
> +        vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
> 
> -        if (i < ncpupids)
> -            QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = cpupids[i];
> -        else
> -            QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
> +        vcpupriv->tid = info[i].tid;
>      }

FWIW:
Once we reach this point the VIR_DOMAIN_VIRT_QEMU check is the only way
qemuDomainHasVcpuPids can return 0 in qemuDomainValidateVcpuInfo... So I
wonder - would it be useful to alter that function too so that we're
sure things match up properly vis-a-vis the 'tid' and online checks?
And my alter, I was thinking along the lines of a similar check for
VIR_DOMAIN_VIRT_QEMU...

> 
>      ret = 0;
> 
>   cleanup:
> -    VIR_FREE(cpupids);
> +    qemuMonitorCPUInfoFree(info, maxvcpus);
>      return ret;
>  }
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 4403bdd..0011ceb 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1656,25 +1656,67 @@ qemuMonitorSystemReset(qemuMonitorPtr mon)
>  }
> 
> 
> +void
> +qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
> +                       size_t ncpus ATTRIBUTE_UNUSED)
> +{
> +    if (!cpus)
> +        return;
> +
> +    VIR_FREE(cpus);
> +}
> +
> +
>  /**
>   * qemuMonitorGetCPUInfo:
>   * @mon: monitor
> - * @pids: returned array of thread ids corresponding to the vCPUs
> + * @cpus: pointer filled by array of qemuMonitorCPUInfo structures
> + * @maxvcpus: total possible number of vcpus
> + *
> + * Detects VCPU information. If qemu doesn't support or fails reporting
> + * information this function will return success as other parts of libvirt
> + * are able to cope with that.
>   *
> - * Detects the vCPU thread ids. Returns count of detected vCPUs on success,
> - * 0 if qemu didn't report thread ids (does not report libvirt error),
> - * -1 on error (reports libvirt error).
> + * Returns 0 on success (including if qemu didn't report any data) and
> + *  -1 on error (reports libvirt error).
>   */
>  int
>  qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> -                      int **pids)
> +                      qemuMonitorCPUInfoPtr *vcpus,
> +                      size_t maxvcpus)
>  {
> +    qemuMonitorCPUInfoPtr info = NULL;
> +    int *pids = NULL;
> +    size_t i;
> +    int ret = -1;
> +    int rc;
> +
>      QEMU_CHECK_MONITOR(mon);
> 
> +    if (VIR_ALLOC_N(info, maxvcpus) < 0)
> +        return -1;
> +
>      if (mon->json)
> -        return qemuMonitorJSONQueryCPUs(mon, pids);
> +        rc = qemuMonitorJSONQueryCPUs(mon, &pids);
>      else
> -        return qemuMonitorTextQueryCPUs(mon, pids);
> +        rc = qemuMonitorTextQueryCPUs(mon, &pids);
> +
> +    if (rc < 0) {

If qemuMonitorJSONExtractCPUInfo() finds ncpus == 0, then it returns an
error which won't be "cleaned up" unless rc <= 0

> +        virResetLastError();
> +        ret = 0;
> +        goto cleanup;

So, ret == 0, we go to cleanup, it cleans up 'info', then our caller
finds rc == 0, so it can fall into that for maxvcpus loop looking 'info'
and then of course freeing 'info' again.

> +    }
> +
> +    for (i = 0; i < rc; i++)
> +        info[i].tid = pids[i];

Still no "gaps" in our "internal" pids list which is fine, but I assume
could change in the future... Just keeping it fresh in my mind ;-)


ACK with the necessary adjustments for the above rc check condition
which I think would be along the lines of:

    if (rc <= 0)
        virResetLastError();

    if (rc < 0)
        goto cleanup;

But I could be wrong...

John

> +
> +    VIR_STEAL(*vcpus, info);
> +    ret = 0;
> +
> + cleanup:
> +    qemuMonitorCPUInfoFree(info, maxvcpus);
> +    VIR_FREE(pids);
> +    return ret;
>  }
> 
> 
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 805656b..1ef002a 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -390,8 +390,19 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
>  int qemuMonitorSystemReset(qemuMonitorPtr mon);
>  int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
> 
> +
> +struct _qemuMonitorCPUInfo {
> +    pid_t tid;
> +};
> +typedef struct _qemuMonitorCPUInfo qemuMonitorCPUInfo;
> +typedef qemuMonitorCPUInfo *qemuMonitorCPUInfoPtr;
> +
> +void qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr list,
> +                            size_t nitems);
>  int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> -                          int **pids);
> +                          qemuMonitorCPUInfoPtr *vcpus,
> +                          size_t maxvcpus);
> +
>  int qemuMonitorGetVirtType(qemuMonitorPtr mon,
>                             virDomainVirtType *virtType);
>  int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
>