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