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,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list