On Wed, Aug 03, 2016 at 12:43:48PM +0200, Pavel Hrdina wrote:

On Wed, Aug 03, 2016 at 10:10:53AM +0200, Peter Krempa wrote:

...

Rather than formating a string and splitting it back to a JSON object add API that will take a JSON object directly. --- src/qemu/qemu_monitor.c | 18 ++++++++++++++++++ src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 29 +++++++++++++++++++---------- src/qemu/qemu_monitor_json.h | 2 ++ 4 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 58c04d5..b58c412 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2771,6 +2771,24 @@ qemuMonitorAddDevice(qemuMonitorPtr mon,

/** + * qemuMonitorAddDeviceArgs: + * @mon: monitor object + * @args: arguments for device add, consumed on success or failure + * + * Adds a device described by @args. Requires JSON monitor. + * Returns 0 on success -1 on error. + */ +int +qemuMonitorAddDeviceArgs(qemuMonitorPtr mon, + virJSONValuePtr args) +{ + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONAddDeviceArgs(mon, args); +} + + +/** * qemuMonitorAddObject: * @mon: Pointer to monitor object * @type: Type name of object to add diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index ae0954d..805656b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -685,6 +685,8 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon, const char *bus, virPCIDeviceAddress *guestAddr);

+int qemuMonitorAddDeviceArgs(qemuMonitorPtr mon, + virJSONValuePtr args);

I've missed this one, wrong indentation.

On 08/03/2016 04:10 AM, Peter Krempa wrote:

VIR_STEAL copies the second argument into the first and then sets it to NULL. This is useful for stealing pointers. --- src/internal.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/src/internal.h b/src/internal.h index 0dc34c7..c633ee6 100644 --- a/src/internal.h +++ b/src/internal.h @@ -307,6 +307,18 @@ } while (0)

/** + * VIR_STEAL: + * + * Steals pointer passed as second argument into the first argument. Second + * argument must not have side effects. + */ +# define VIR_STEAL(a, b) \ + do { \ + (a) = (b); \ + (b) = NULL;\ + } while (0) + +/** * virCheckFlags: * @supported: an OR'ed set of supported flags * @retval: return value in case unsupported flags were passed

While one can look at the code and deduce what it does - the name is not that descriptive. VIR_ASSIGN_PTR or VIR_ASSIGN_POINTER? Just a thought... It's also not used until patch 9 - perhaps move it a bit closer to that. John

$SUBJ s/qemuDomainDetectVcpuPids/qemuDomainRefreshVcpuInfo/ On 08/03/2016 04:10 AM, Peter Krempa wrote:

Validate the presence of the thread id according to state of the vCPU rather than just checking the vCPU count. Additionally put the new validation code into a separate function so that the information retrieval can be split from the validation. --- src/qemu/qemu_domain.c | 49 +++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_domain.h | 1 + 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5cde841..f27f2f7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5624,6 +5624,48 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,

/** + * qemuDomainValidateVcpuInfo: + * + * Validates vcpu thread information. If vcpu thread IDs are reported by qemu, + * this function validates that online vcpus have thread info present and + * offline vcpus don't. + * + * Returns 0 on success -1 on error. + */ +int +qemuDomainValidateVcpuInfo(virDomainObjPtr vm) +{ + size_t maxvcpus = virDomainDefGetVcpusMax(vm->def); + virDomainVcpuDefPtr vcpu; + qemuDomainVcpuPrivatePtr vcpupriv; + size_t i; + + if (!qemuDomainHasVcpuPids(vm)) + return 0;

This is checking priv->nvcpupids > 0, which we know as a truth since a few lines before calling this function there's a check: /* 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) { Of course future patches "adjust" where this function is called to split the Refresh and Validate into two parts and by patch 9 those lines disappear. I guess it's just duplicitous for a few patches. Still, by the time we reach patch 9 other than the VIR_DOMAIN_VIRT_QEMU check - I wonder if we may want to make the subsequent checks since there'd be no other way to get here since if the Refresh function returns failure, this function wouldn't be called. ACK to what's here with the adjustment for the commit message... maybe I'm thinking too hard for this early in the morning ;-) John

+ + for (i = 0; i < maxvcpus; i++) { + vcpu = virDomainDefGetVcpu(vm->def, i); + vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); + + if (vcpu->online && vcpupriv->tid == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("qemu didn't report thread id for vcpu '%zu'"), i); + return -1; + } + + if (!vcpu->online && vcpupriv->tid != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("qemu reported thread if for inactive vcpu '%zu'"), + i); + return -1; + } + } + + return 0; +} + + +/** * qemuDomainRefreshVcpuInfo: * @driver: qemu driver data * @vm: domain object @@ -5703,13 +5745,8 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0; }

- if (ncpupids != virDomainDefGetVcpus(vm->def)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("got wrong number of vCPU pids from QEMU monitor. " - "got %d, wanted %d"), - ncpupids, virDomainDefGetVcpus(vm->def)); + if (qemuDomainValidateVcpuInfo(vm) < 0) goto cleanup; - }

ret = ncpupids;

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3193427..0613093 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -647,6 +647,7 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,

bool qemuDomainHasVcpuPids(virDomainObjPtr vm); pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid); +int qemuDomainValidateVcpuInfo(virDomainObjPtr vm); int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob);

On Wed, Aug 03, 2016 at 09:03:50AM -0400, John Ferlan wrote:

On 08/03/2016 04:11 AM, Peter Krempa wrote:

...

Call the vcpu thread info validation separately to decrease complexity of returned values by qemuDomainRefreshVcpuInfo.

This function now returns 0 on success and -1 on error. Certain failures of qemu to report data are still considered as success. Any error reported now is fatal. --- src/qemu/qemu_domain.c | 16 +++++----------- src/qemu/qemu_driver.c | 20 ++++++++++---------- src/qemu/qemu_process.c | 6 ++++++ 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f27f2f7..4cdd012 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm) * @vm: domain object * @asyncJob: current asynchronous job type * - * Updates vCPU information private data of @vm. + * Updates vCPU information private data of @vm. Due to historical reasons this + * function returns success even if some data were not reported by qemu. * - * Returns number of detected vCPU threads on success, -1 on error and reports - * an appropriate error, -2 if the domain doesn't exist any more. + * Returns 0 on success and -1 on fatal error. */ int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, @@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &cpupids); - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - ret = -2; - goto cleanup; - } + if (qemuDomainObjExitMonitor(driver, vm) < 0)

This is missing a "goto cleanup" which is added in the next patch, but needs to be here!

...

/* 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 @@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0; }

- if (qemuDomainValidateVcpuInfo(vm) < 0) - goto cleanup; - - ret = ncpupids; + ret = 0;

cleanup: VIR_FREE(cpupids); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 453572b..2199258 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu); + qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo); int ret = -1; int rc; int oldvcpus = virDomainDefGetVcpus(vm->def); @@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,

vcpuinfo->online = true;

- if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) { - /* vcpu pids were not detected, skip setting of affinity */ - if (rc == 0) - ret = 0; + if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup;

+ if (qemuDomainValidateVcpuInfo(vm) < 0) goto cleanup; - }

- if (qemuProcessSetupVcpu(vm, vcpu) < 0) + if (vcpupriv->tid > 0 && + qemuProcessSetupVcpu(vm, vcpu) < 0) goto cleanup;

ret = 0; @@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver, goto cleanup; }

- if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) { - /* rollback only if domain didn't exit */ - if (rc == -2) - goto cleanup; + if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup;

Typing what I'm thinking - just to be sure it matches your expectations of this code. So we no longer care to distinguish between failure and failure due to domain exit. This just seems to mean we'll get that audit after a possible domain exit and reset the vcpuinfo->online (won't really matter if the domain exited).

The logic is still the same as before this patch. Function qemuDomainRefreshVcpuInfo fails with error only in case the domain exited. It's safe to rollback only if qemuDomainValidateVcpuInfo fails, this is what was extracted from qemuDomainRefreshVcpuInfo.

ACK - with the goto cleanup added at least.

John

...

+ if (qemuDomainValidateVcpuInfo(vm) < 0) { + /* rollback vcpu count if the setting has failed */ virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", false); vcpuinfo->online = true; goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9136ba5..2e405af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5193,6 +5193,9 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0) goto cleanup;

+ if (qemuDomainValidateVcpuInfo(vm) < 0) + goto cleanup; + VIR_DEBUG("Detecting IOThread PIDs"); if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0) goto cleanup; @@ -5985,6 +5988,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto error;

+ if (qemuDomainValidateVcpuInfo(vm) < 0) + goto error; + VIR_DEBUG("Detecting IOThread PIDs"); if (qemuProcessDetectIOThreadPIDs(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto error;

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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,

On 08/03/2016 04:11 AM, Peter Krempa wrote:

Prepare to extract more data by returning a array of structs rather than just an array of thread ids. Additionally report fatal errors separately from qemu not being able to produce data. --- src/qemu/qemu_monitor.c | 31 ++++++++++++------- src/qemu/qemu_monitor.h | 6 ++++ src/qemu/qemu_monitor_json.c | 71 ++++++++++++++++++++++---------------------- src/qemu/qemu_monitor_json.h | 2 +- src/qemu/qemu_monitor_text.c | 37 +++++++++++------------ src/qemu/qemu_monitor_text.h | 2 +- tests/qemumonitorjsontest.c | 31 ++++++++++++++----- 7 files changed, 104 insertions(+), 76 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0011ceb..578b078 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus, VIR_FREE(cpus); }

+void +qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries, + size_t nentries ATTRIBUTE_UNUSED) +{ + if (!entries) + return;

[1] Maybe this should be a 'int' parameter and a <= 0 check...

+ + VIR_FREE(entries); +} +

/** * qemuMonitorGetCPUInfo: @@ -1686,10 +1696,10 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon, size_t maxvcpus) { qemuMonitorCPUInfoPtr info = NULL; - int *pids = NULL; + struct qemuMonitorQueryCpusEntry *cpuentries = NULL; size_t i; int ret = -1; - int rc; + int ncpuentries;

QEMU_CHECK_MONITOR(mon);

@@ -1697,25 +1707,26 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon, return -1;

if (mon->json) - rc = qemuMonitorJSONQueryCPUs(mon, &pids); + ncpuentries = qemuMonitorJSONQueryCPUs(mon, &cpuentries); else - rc = qemuMonitorTextQueryCPUs(mon, &pids); + ncpuentries = qemuMonitorTextQueryCPUs(mon, &cpuentries); + + if (ncpuentries < 0) { + if (ncpuentries == -2) + ret = 0;

Similar to previous patch w/r/t "ret = 0" and returning to caller

- if (rc < 0) { - virResetLastError(); - ret = 0; goto cleanup; }

- for (i = 0; i < rc; i++) - info[i].tid = pids[i]; + for (i = 0; i < ncpuentries; i++) + info[i].tid = cpuentries[i].tid;

VIR_STEAL(*vcpus, info); ret = 0;

cleanup: qemuMonitorCPUInfoFree(info, maxvcpus); - VIR_FREE(pids); + qemuMonitorQueryCpusFree(cpuentries, ncpuentries);

[1]Although not "currently" used, the ncpuentries is an int being passed to a function that wants size_t And yes, that's from a Coverity warning

return ret; }

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1ef002a..6022b9d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -390,6 +390,12 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon, int qemuMonitorSystemReset(qemuMonitorPtr mon); int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);

+struct qemuMonitorQueryCpusEntry { + pid_t tid; +}; +void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries, + size_t nentries); +

struct _qemuMonitorCPUInfo { pid_t tid; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 715e1c7..8018860 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1323,69 +1323,65 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon) * { "CPU": 1, "current": false, "halted": true, "pc": 7108165 } ] */ static int -qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, - int **pids) +qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, + struct qemuMonitorQueryCpusEntry **entries) { - virJSONValuePtr data; + struct qemuMonitorQueryCpusEntry *cpus = NULL; int ret = -1; size_t i; - int *threads = NULL; ssize_t ncpus;

- if (!(data = virJSONValueObjectGetArray(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cpu reply was missing return data")); - goto cleanup; - } - - if ((ncpus = virJSONValueArraySize(data)) <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cpu information was empty")); - goto cleanup; - } + if ((ncpus = virJSONValueArraySize(data)) <= 0) + return -2;

- if (VIR_ALLOC_N(threads, ncpus) < 0) + if (VIR_ALLOC_N(cpus, ncpus) < 0) goto cleanup;

for (i = 0; i < ncpus; i++) { virJSONValuePtr entry = virJSONValueArrayGet(data, i); - int thread; + int thread = 0; if (!entry) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cpu information was missing an array element")); + ret = -2; goto cleanup; }

- if (virJSONValueObjectGetNumberInt(entry, "thread_id", &thread) < 0) { - /* Some older qemu versions don't report the thread_id, - * so treat this as non-fatal, simply returning no data */ - ret = 0; - goto cleanup; - } + /* Some older qemu versions don't report the thread_id so treat this as + * non-fatal, simply returning no data */ + ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", &thread));

So we're going to still make 'ncpus' calls to get the same answer... Then we're going to return 5 elements with {0, 0, 0, 0, 0} and the caller is going to do what? I think eventually we'll end up failing in qemuDomainValidateVcpuInfo.

- threads[i] = thread; + cpus[i].tid = thread; }

- *pids = threads; - threads = NULL; + VIR_STEAL(*entries, cpus); ret = ncpus;

cleanup: - VIR_FREE(threads); + qemuMonitorQueryCpusFree(cpus, ncpus); return ret; }

+/** + * qemuMonitorJSONQueryCPUs: + * + * @mon: monitor object + * @entries: filled with detected entries on success + * + * Queries qemu for cpu-related information. Failure to execute the command or + * extract results does not produce an error as libvirt can continue without + * this information. + * + * Returns number of cpu data entries returned in @entries on success, -1 on a + * fatal error (oom ...) and -2 if the query failed gracefully. + */ int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon, - int **pids) + struct qemuMonitorQueryCpusEntry **entries) { int ret = -1; - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", - NULL); + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", NULL); virJSONValuePtr reply = NULL; - - *pids = NULL; + virJSONValuePtr data;

if (!cmd) return -1; @@ -1393,10 +1389,13 @@ qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup;

- if (qemuMonitorJSONCheckError(cmd, reply) < 0) + if (!(data = virJSONValueObjectGetArray(reply, "return"))) { + ret = -2; goto cleanup; + } + + ret = qemuMonitorJSONExtractCPUInfo(data, entries);

- ret = qemuMonitorJSONExtractCPUInfo(reply, pids); cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 174f0ef..24b23d2 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -59,7 +59,7 @@ int qemuMonitorJSONSystemPowerdown(qemuMonitorPtr mon); int qemuMonitorJSONSystemReset(qemuMonitorPtr mon);

int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon, - int **pids); + struct qemuMonitorQueryCpusEntry **entries); int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon, virDomainVirtType *virtType); int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index ca04965..648c7db 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -502,12 +502,14 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon)

int qemuMonitorTextQueryCPUs(qemuMonitorPtr mon, - int **pids) + struct qemuMonitorQueryCpusEntry **entries) { char *qemucpus = NULL; char *line; - pid_t *cpupids = NULL; - size_t ncpupids = 0; + struct qemuMonitorQueryCpusEntry *cpus = NULL; + size_t ncpus;

Need to initialize to 0 (surprised neither compiler nor Coverity picked up on it).

+ struct qemuMonitorQueryCpusEntry cpu = {0}; + int ret = -2; /* -2 denotes a non-fatal error to get the data */

if (qemuMonitorHMPCommand(mon, "info cpus", &qemucpus) < 0) return -1; @@ -529,15 +531,17 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,

/* Extract host Thread ID */ if ((offset = strstr(line, "thread_id=")) == NULL) - goto error; + goto cleanup;

if (virStrToLong_i(offset + strlen("thread_id="), &end, 10, &tid) < 0) - goto error; + goto cleanup; if (end == NULL || !c_isspace(*end)) - goto error; + goto cleanup;

- if (VIR_APPEND_ELEMENT_COPY(cpupids, ncpupids, tid) < 0) - goto error; + cpu.tid = tid; + + if (VIR_APPEND_ELEMENT_COPY(cpus, ncpus, cpu) < 0) + goto cleanup;

VIR_DEBUG("tid=%d", tid);

@@ -547,20 +551,13 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon, line = strchr(offset, '\n'); } while (line != NULL);

- /* Validate we got data for all VCPUs we expected */ - VIR_FREE(qemucpus); - *pids = cpupids; - return ncpupids; + VIR_STEAL(*entries, cpus); + ret = ncpus;

- error: + cleanup: + qemuMonitorQueryCpusFree(cpus, ncpus); VIR_FREE(qemucpus); - VIR_FREE(cpupids); - - /* Returning 0 to indicate non-fatal failure, since - * older QEMU does not have VCPU<->PID mapping and - * we don't want to fail on that - */ - return 0; + return ret; }

diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index b7f0cab..f9e0f82 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -50,7 +50,7 @@ int qemuMonitorTextSystemPowerdown(qemuMonitorPtr mon); int qemuMonitorTextSystemReset(qemuMonitorPtr mon);

int qemuMonitorTextQueryCPUs(qemuMonitorPtr mon, - int **pids); + struct qemuMonitorQueryCpusEntry **entries); int qemuMonitorTextGetVirtType(qemuMonitorPtr mon, virDomainVirtType *virtType); int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 544b569..f848d80 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c

Just a thought... should we have a test that returns "0" entries - does it make sense? I think this is close, but needs a few tweaks or explanation before ACK John

@@ -1201,6 +1201,16 @@ GEN_TEST_FUNC(qemuMonitorJSONNBDServerStart, "localhost", 12345) GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", true) GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")

+static bool +testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(struct qemuMonitorQueryCpusEntry *a, + struct qemuMonitorQueryCpusEntry *b) +{ + if (a->tid != b->tid) + return false; + + return true; +} +

static int testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data) @@ -1208,9 +1218,14 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data) virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; - pid_t *cpupids = NULL; - pid_t expected_cpupids[] = {17622, 17624, 17626, 17628}; - int ncpupids; + struct qemuMonitorQueryCpusEntry *cpudata = NULL; + struct qemuMonitorQueryCpusEntry expect[] = { + {17622}, + {17624}, + {17626}, + {17628}, + }; + int ncpupids = 0; size_t i;

if (!test) @@ -1252,7 +1267,7 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data) "}") < 0) goto cleanup;

- ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), &cpupids); + ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), &cpudata);

if (ncpupids != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1261,10 +1276,10 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data) }

for (i = 0; i < ncpupids; i++) { - if (cpupids[i] != expected_cpupids[i]) { + if (!testQemuMonitorJSONqemuMonitorJSONQueryCPUsEqual(cpudata + i, + expect + i)) { virReportError(VIR_ERR_INTERNAL_ERROR, - "Expecting cpupids[%zu] = %d but got %d", - i, expected_cpupids[i], cpupids[i]); + "vcpu entry %zu does not match expected data", i); goto cleanup; } } @@ -1272,7 +1287,7 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data) ret = 0;

cleanup: - VIR_FREE(cpupids); + qemuMonitorQueryCpusFree(cpudata, ncpupids); qemuMonitorTestFree(test); return ret; }

On 08/03/2016 01:00 PM, Peter Krempa wrote:

On Wed, Aug 03, 2016 at 12:04:02 -0400, John Ferlan wrote:

...

On 08/03/2016 04:11 AM, Peter Krempa wrote:

...

Prepare to extract more data by returning a array of structs rather than just an array of thread ids. Additionally report fatal errors separately from qemu not being able to produce data. --- src/qemu/qemu_monitor.c | 31 ++++++++++++------- src/qemu/qemu_monitor.h | 6 ++++ src/qemu/qemu_monitor_json.c | 71 ++++++++++++++++++++++---------------------- src/qemu/qemu_monitor_json.h | 2 +- src/qemu/qemu_monitor_text.c | 37 +++++++++++------------ src/qemu/qemu_monitor_text.h | 2 +- tests/qemumonitorjsontest.c | 31 ++++++++++++++----- 7 files changed, 104 insertions(+), 76 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0011ceb..578b078 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus, VIR_FREE(cpus); }

+void +qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries, + size_t nentries ATTRIBUTE_UNUSED) +{ + if (!entries) + return;

[1] Maybe this should be a 'int' parameter and a <= 0 check...

What?! That's a freeing function. That does not make any sense.

oh right - my eyes read nentries... and I even noted in the caller that nentries is not used.... Anyway, I suppose part of me was trying to forward think why you would add nentries and that passing a -2 or -1 here may not end quickly... John