On Tue, Nov 12, 2013 at 10:04:09AM +0100, Peter Krempa wrote:

To allow returning more granullar errors, change the error type to an integer. --- src/qemu/qemu_monitor.c | 19 ++++++++++++------- src/qemu/qemu_monitor.h | 5 +++-- src/qemu/qemu_monitor_json.c | 26 ++++++++++++++++---------- src/qemu/qemu_monitor_json.h | 4 +++- src/qemu/qemu_process.c | 9 +++++++-- tests/qemumonitorjsontest.c | 5 +++-- 6 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 14e4e2d..30315b3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3933,28 +3933,33 @@ qemuMonitorSetDomainLog(qemuMonitorPtr mon, int logfd) * qemuMonitorJSONGetGuestCPU: * @mon: Pointer to the monitor * @arch: arch of the guest + * @data: returns the cpu data * * Retrieve the definition of the guest CPU from a running qemu instance. * - * Returns the cpu definition object. On error returns NULL. + * Returns 0 on success, -2 if the operation is not supported by the guest, + * -1 on other errors. */ -virCPUDataPtr +int qemuMonitorGetGuestCPU(qemuMonitorPtr mon, - virArch arch) + virArch arch, + virCPUDataPtr *data) { - VIR_DEBUG("mon=%p, arch='%s'", mon, virArchToString(arch)); + VIR_DEBUG("mon=%p, arch='%s' data='%p'", mon, virArchToString(arch), data);

if (!mon) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("monitor must not be NULL")); - return NULL; + return -1; }

if (!mon->json) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("JSON monitor is required")); - return NULL; + return -1; }

- return qemuMonitorJSONGetGuestCPU(mon, arch); + *data = NULL; + + return qemuMonitorJSONGetGuestCPU(mon, arch, data); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index ecc6d7b..f893b1f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -764,8 +764,9 @@ int qemuMonitorGetDeviceAliases(qemuMonitorPtr mon,

int qemuMonitorSetDomainLog(qemuMonitorPtr mon, int logfd);

-virCPUDataPtr qemuMonitorGetGuestCPU(qemuMonitorPtr mon, - virArch arch); +int qemuMonitorGetGuestCPU(qemuMonitorPtr mon, + virArch arch, + virCPUDataPtr *data);

/** * When running two dd process and using <> redirection, we need a diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 593c90f..87990e2 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5503,9 +5503,10 @@ qemuMonitorJSONParseCPUx86FeatureWord(virJSONValuePtr data, }

-static virCPUDataPtr +static int qemuMonitorJSONGetCPUx86Data(qemuMonitorPtr mon, - const char *property) + const char *property, + virCPUDataPtr *cpudata) { virJSONValuePtr cmd; virJSONValuePtr reply = NULL; @@ -5513,14 +5514,14 @@ qemuMonitorJSONGetCPUx86Data(qemuMonitorPtr mon, virCPUx86Data *x86Data = NULL; virCPUx86CPUID cpuid; size_t i; - virCPUDataPtr ret = NULL; int n; + int ret = -1;

if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", "s:path", QOM_CPU_PATH, "s:property", property, NULL))) - return NULL; + return -1;

if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; @@ -5551,9 +5552,11 @@ qemuMonitorJSONGetCPUx86Data(qemuMonitorPtr mon, goto cleanup; }

- if (!(ret = virCPUx86MakeData(VIR_ARCH_X86_64, &x86Data))) + if (!(*cpudata = virCPUx86MakeData(VIR_ARCH_X86_64, &x86Data))) goto cleanup;

+ ret = 0; + cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); @@ -5566,24 +5569,27 @@ cleanup: * qemuMonitorJSONGetGuestCPU: * @mon: Pointer to the monitor * @arch: arch of the guest + * @data: returns the cpu data of the guest * * Retrieve the definition of the guest CPU from a running qemu instance. * - * Returns the cpu definition object. On error returns NULL. + * Returns 0 on success, -2 if guest doesn't support this feature, + * -1 on other errors. */ -virCPUDataPtr +int qemuMonitorJSONGetGuestCPU(qemuMonitorPtr mon, - virArch arch) + virArch arch, + virCPUDataPtr *data) { switch (arch) { case VIR_ARCH_X86_64: case VIR_ARCH_I686: - return qemuMonitorJSONGetCPUx86Data(mon, "feature-words"); + return qemuMonitorJSONGetCPUx86Data(mon, "feature-words", data);

default: virReportError(VIR_ERR_INTERNAL_ERROR, _("CPU definition retrieval isn't supported for '%s'"), virArchToString(arch)); - return NULL; + return -1; } } diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 9e0a0c9..a93c51e 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -427,5 +427,7 @@ int qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, int qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon, char ***aliases);

-virCPUDataPtr qemuMonitorJSONGetGuestCPU(qemuMonitorPtr mon, virArch arch); +int qemuMonitorJSONGetGuestCPU(qemuMonitorPtr mon, + virArch arch, + virCPUDataPtr *data); #endif /* QEMU_MONITOR_JSON_H */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fcceedd..e34f542 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3472,17 +3472,22 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, virDomainObjPtr vm) virArch arch = def->os.arch; virCPUDataPtr guestcpu = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; bool ret = false;

switch (arch) { case VIR_ARCH_I686: case VIR_ARCH_X86_64: qemuDomainObjEnterMonitor(driver, vm); - guestcpu = qemuMonitorGetGuestCPU(priv->mon, arch); + rc = qemuMonitorGetGuestCPU(priv->mon, arch, &guestcpu); qemuDomainObjExitMonitor(driver, vm);

- if (!(guestcpu)) + if (rc < 0) { + if (rc == -2) + break; + goto cleanup; + }

if (def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK] == VIR_DOMAIN_FEATURE_STATE_ON) { if (!cpuHasFeature(guestcpu, VIR_CPU_x86_KVM_PV_UNHALT)) { diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index a6bd346..5636d95 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1997,8 +1997,9 @@ testQemuMonitorJSONGetCPUData(const void *opaque) if (qemuMonitorTestAddItem(test, "qom-get", jsonStr) < 0) goto cleanup;

- if (!(cpuData = qemuMonitorJSONGetGuestCPU(qemuMonitorTestGetMonitor(test), - VIR_ARCH_X86_64))) + if (qemuMonitorJSONGetGuestCPU(qemuMonitorTestGetMonitor(test), + VIR_ARCH_X86_64, + &cpuData) < 0) goto cleanup;

if (!(actual = cpuDataFormat(cpuData)))

ACK. -- Guido

-- 1.8.4.2

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

On 12.11.2013 10:04, Peter Krempa wrote:

The QOM path in qemu that contains the CPUID registers of a running VM may not be present (introduced in QEMU 1.5).

Since commit d94b7817719 we have a regression with QEMU that don't support reporting of the CPUID register state via the monitor as the process startup code expects the path to exist.

This patch adds code that checks with the monitor if the requested path already exists and uses it only in this case. --- src/qemu/qemu_monitor_json.c | 50 ++++++++++++++++++++++++++++++++++++++++++-- tests/qemumonitorjsontest.c | 16 ++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 87990e2..e716fb3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5508,20 +5508,66 @@ qemuMonitorJSONGetCPUx86Data(qemuMonitorPtr mon, const char *property, virCPUDataPtr *cpudata) { - virJSONValuePtr cmd; + virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr data; + virJSONValuePtr element; virCPUx86Data *x86Data = NULL; virCPUx86CPUID cpuid; size_t i; int n; int ret = -1;

+ /* look up if the property exists before asking */ + if (!(cmd = qemuMonitorJSONMakeCommand("qom-list", + "s:path", QOM_CPU_PATH, + NULL))) + goto cleanup; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + /* check if device exists */ + if ((data = virJSONValueObjectGet(reply, "error")) && + STREQ_NULLABLE(virJSONValueObjectGetString(data, "class"), + "DeviceNotFound")) { + ret = -2; + goto cleanup; + } + + if (qemuMonitorJSONCheckError(cmd, reply)) + goto cleanup; + + data = virJSONValueObjectGet(reply, "return"); + + if ((n = virJSONValueArraySize(data)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s CPU property did not return an array"), + property); + goto cleanup; + } + + for (i = 0; i < n; i++) { + element = virJSONValueArrayGet(data, i); + if (STREQ_NULLABLE(virJSONValueObjectGetString(element, "name"), + property)) + break; + } + + /* "property" was not found */ + if (i == n) { + ret = -2; + goto cleanup; + } + + virJSONValueFree(cmd); + virJSONValueFree(reply); + if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", "s:path", QOM_CPU_PATH, "s:property", property, NULL))) - return -1; + goto cleanup;

if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 5636d95..4cdb938 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1994,6 +1994,22 @@ testQemuMonitorJSONGetCPUData(const void *opaque) virtTestLoadFile(dataFile, &expected) < 0) goto cleanup;

+ if (qemuMonitorTestAddItem(test, "qom-list", + "{" + " \"return\": [" + " {" + " \"name\": \"filtered-features\"," + " \"type\": \"X86CPUFeatureWordInfo\"" + " }," + " {" + " \"name\": \"feature-words\"," + " \"type\": \"X86CPUFeatureWordInfo\"" + " }" + " ]," + " \"id\": \"libvirt-19\"" + "}") < 0) + goto cleanup; + if (qemuMonitorTestAddItem(test, "qom-get", jsonStr) < 0) goto cleanup;

It seems OK to me, but I'm not that familiar with the libvirt code so I'm giving only weak ACK. Pavel