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

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

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.