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

[Peter Krempa]

On Wed, Aug 03, 2016 at 09:21:28 -0400, John Ferlan wrote:
> 
> 
> 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

[...]

> > -    }
> > 
> >      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 waya
> qemuDomainHasVcpuPids can return 0 in qemuDomainValidateVcpuInfo... So I

Not really. There are actually two options:

1) The thread id's were not set here due to the VIR_DOMAIN_VIRT_QEMU
   check

   This is necessary as qemu would return garbage thread ids as
   described by the rather huge comment above the check.

2) Qemu itself would not report the thread ids

    Some vintage versions were not doing it. I don't really know if it
    is possible since we support only certain versions of qemu now but
    I'm not really going to check at this point.

> 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

qemuMonitorJSONExtractCPUInfo does not set any error if the returned
value is 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.

The idea is that qemuMonitorGetCPUInfo returns the fully allocated array
of structures if it retuns success. This is including if there are no
threads reported by qemu.

There is a different bug there actually. In case when the error is reset
the returning pointer needs to be set.

I'll post a fixed version.

Peter