[libvirt] [PATCH 0/4] prepare for qemu 1.5 command-line probing via QMP
This series sets up the groundwork to make it easy to add a capability bit in qemu_capabilities based on whether a particular qemu command-line option has a given parameter. Future patches, such as Osier's work to use '-machine mem-merge' when it exists, will benefit from this new probing ability. Since this missed rc1 freeze, I'm a bit reluctant to include it in libvirt 1.0.5 without a client that actually needs it. Hmm, in writing up this cover letter, I think I should work on a patch 5/4 that enhances qemu_capabilities to make it easy to search for a given option and capability whether parsing -help output (hello RHEL qemu) or using QMP (upstream). Eric Blake (4): json: support removing a value from an object qemu: use bool in monitor struct qemu: simplify string cleanup qemu: query command line options in QMP src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 7 +- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_monitor.c | 59 ++++++++++++--- src/qemu/qemu_monitor.h | 13 +++- src/qemu/qemu_monitor_json.c | 174 +++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_monitor_json.h | 4 + src/qemu/qemu_process.c | 8 +- src/util/virjson.c | 34 ++++++++- src/util/virjson.h | 9 ++- tests/jsontest.c | 83 ++++++++++++++++++++- tests/qemumonitorjsontest.c | 103 +++++++++++++++++++++++++ tests/qemumonitortestutils.c | 2 +- 13 files changed, 437 insertions(+), 62 deletions(-) -- 1.8.1.4
In an upcoming patch, I need the way to safely transfer a nested virJSON object out of its parent container for independent use, even after the parent is freed. * src/util/virjson.h (virJSONValueObjectRemoveKey): New function. (_virJSONObject, _virJSONArray): Use correct type. * src/util/virjson.c (virJSONValueObjectRemoveKey): Implement it. * src/libvirt_private.syms (virjson.h): Export it. * tests/jsontest.c (mymain): Test it. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 34 +++++++++++++++++++- src/util/virjson.h | 9 ++++-- tests/jsontest.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 120 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bb6f5f..b502ed8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1392,6 +1392,7 @@ virJSONValueObjectGetValue; virJSONValueObjectHasKey; virJSONValueObjectIsNull; virJSONValueObjectKeysNumber; +virJSONValueObjectRemoveKey; virJSONValueToString; diff --git a/src/util/virjson.c b/src/util/virjson.c index e6a3b1b..10afe88 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1,7 +1,7 @@ /* * virjson.c: JSON object parsing/formatting * - * Copyright (C) 2009-2010, 2012 Red Hat, Inc. + * Copyright (C) 2009-2010, 2012-2013 Red Hat, Inc. * Copyright (C) 2009 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -447,6 +447,38 @@ const char *virJSONValueObjectGetKey(virJSONValuePtr object, unsigned int n) return object->data.object.pairs[n].key; } +/* Remove the key-value pair tied to KEY out of OBJECT. If VALUE is + * not NULL, the dropped value object is returned instead of freed. + * Returns 1 on success, 0 if no key was found, and -1 on error. */ +int +virJSONValueObjectRemoveKey(virJSONValuePtr object, const char *key, + virJSONValuePtr *value) +{ + int i; + + if (value) + *value = NULL; + + if (object->type != VIR_JSON_TYPE_OBJECT) + return -1; + + for (i = 0 ; i < object->data.object.npairs ; i++) { + if (STREQ(object->data.object.pairs[i].key, key)) { + if (value) { + *value = object->data.object.pairs[i].value; + object->data.object.pairs[i].value = NULL; + } + VIR_FREE(object->data.object.pairs[i].key); + virJSONValueFree(object->data.object.pairs[i].value); + VIR_DELETE_ELEMENT(object->data.object.pairs, i, + object->data.object.npairs); + return 1; + } + } + + return 0; +} + virJSONValuePtr virJSONValueObjectGetValue(virJSONValuePtr object, unsigned int n) { if (object->type != VIR_JSON_TYPE_OBJECT) diff --git a/src/util/virjson.h b/src/util/virjson.h index 67f4398..ff0fe75 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -1,7 +1,7 @@ /* * virjson.h: JSON object parsing/formatting * - * Copyright (C) 2009, 2012 Red Hat, Inc. + * Copyright (C) 2009, 2012-2013 Red Hat, Inc. * Copyright (C) 2009 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -55,12 +55,12 @@ struct _virJSONObjectPair { }; struct _virJSONObject { - unsigned int npairs; + size_t npairs; virJSONObjectPairPtr pairs; }; struct _virJSONArray { - unsigned int nvalues; + size_t nvalues; virJSONValuePtr *values; }; @@ -131,6 +131,9 @@ int virJSONValueObjectAppendNumberDouble(virJSONValuePtr object, const char *key int virJSONValueObjectAppendBoolean(virJSONValuePtr object, const char *key, int boolean); int virJSONValueObjectAppendNull(virJSONValuePtr object, const char *key); +int virJSONValueObjectRemoveKey(virJSONValuePtr object, const char *key, + virJSONValuePtr *value); + virJSONValuePtr virJSONValueFromString(const char *jsonstring); char *virJSONValueToString(virJSONValuePtr object, bool pretty); diff --git a/tests/jsontest.c b/tests/jsontest.c index 98a6069..a37a980 100644 --- a/tests/jsontest.c +++ b/tests/jsontest.c @@ -11,6 +11,7 @@ struct testInfo { const char *doc; + const char *expect; bool pass; }; @@ -55,19 +56,88 @@ cleanup: static int +testJSONAddRemove(const void *data) +{ + const struct testInfo *info = data; + virJSONValuePtr json; + virJSONValuePtr name; + char *result = NULL; + int ret = -1; + + json = virJSONValueFromString(info->doc); + + switch (virJSONValueObjectRemoveKey(json, "name", &name)) { + case 1: + if (!info->pass) { + if (virTestGetVerbose()) + fprintf(stderr, "should not remove from non-object %s\n", + info->doc); + goto cleanup; + } + break; + case -1: + if (!info->pass) + ret = 0; + else if (virTestGetVerbose()) + fprintf(stderr, "Fail to recognize non-object %s\n", info->doc); + goto cleanup; + default: + if (virTestGetVerbose()) + fprintf(stderr, "unexpected result when removing from %s\n", + info->doc); + goto cleanup; + } + if (STRNEQ_NULLABLE(virJSONValueGetString(name), "sample")) { + if (virTestGetVerbose()) + fprintf(stderr, "unexpected value after removing name: %s\n", + NULLSTR(virJSONValueGetString(name))); + goto cleanup; + } + if (virJSONValueObjectRemoveKey(json, "name", NULL)) { + if (virTestGetVerbose()) + fprintf(stderr, "%s", + "unexpected success when removing missing key\n"); + goto cleanup; + } + if (virJSONValueObjectAppendString(json, "newname", "foo") < 0) { + if (virTestGetVerbose()) + fprintf(stderr, "%s", "unexpected failure adding new key\n"); + goto cleanup; + } + if (!(result = virJSONValueToString(json, false))) { + if (virTestGetVerbose()) + fprintf(stderr, "%s", "failed to stringize result\n"); + goto cleanup; + } + if (STRNEQ(info->expect, result)) { + if (virTestGetVerbose()) + virtTestDifference(stderr, info->expect, result); + goto cleanup; + } + ret = 0; + +cleanup: + virJSONValueFree(json); + virJSONValueFree(name); + VIR_FREE(result); + return ret; +} + + +static int mymain(void) { int ret = 0; -#define DO_TEST_FULL(name, cmd, doc, pass) \ +#define DO_TEST_FULL(name, cmd, doc, expect, pass) \ do { \ - struct testInfo info = { doc, pass }; \ + struct testInfo info = { doc, expect, pass }; \ if (virtTestRun(name, 1, testJSON ## cmd, &info) < 0) \ ret = -1; \ } while (0) #define DO_TEST_PARSE(name, doc) \ - DO_TEST_FULL(name, FromString, doc, true) + DO_TEST_FULL(name, FromString, doc, NULL, true) DO_TEST_PARSE("Simple", "{\"return\": {}, \"id\": \"libvirt-1\"}"); DO_TEST_PARSE("NotSoSimple", "{\"QMP\": {\"version\": {\"qemu\":" @@ -105,6 +175,13 @@ mymain(void) "\"query-uuid\"}, {\"name\": \"query-migrate\"}, {\"name\": " "\"query-balloon\"}], \"id\": \"libvirt-2\"}"); + DO_TEST_FULL("add and remove", AddRemove, + "{\"name\": \"sample\", \"value\": true}", + "{\"value\":true,\"newname\":\"foo\"}", + true); + DO_TEST_FULL("add and remove", AddRemove, + "[ 1 ]", NULL, false); + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.1.4
On 27/04/13 05:01, Eric Blake wrote:
In an upcoming patch, I need the way to safely transfer a nested virJSON object out of its parent container for independent use, even after the parent is freed.
* src/util/virjson.h (virJSONValueObjectRemoveKey): New function. (_virJSONObject, _virJSONArray): Use correct type. * src/util/virjson.c (virJSONValueObjectRemoveKey): Implement it. * src/libvirt_private.syms (virjson.h): Export it. * tests/jsontest.c (mymain): Test it.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 34 +++++++++++++++++++- src/util/virjson.h | 9 ++++-- tests/jsontest.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 120 insertions(+), 7 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bb6f5f..b502ed8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1392,6 +1392,7 @@ virJSONValueObjectGetValue; virJSONValueObjectHasKey; virJSONValueObjectIsNull; virJSONValueObjectKeysNumber; +virJSONValueObjectRemoveKey; virJSONValueToString;
diff --git a/src/util/virjson.c b/src/util/virjson.c index e6a3b1b..10afe88 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1,7 +1,7 @@ /* * virjson.c: JSON object parsing/formatting * - * Copyright (C) 2009-2010, 2012 Red Hat, Inc. + * Copyright (C) 2009-2010, 2012-2013 Red Hat, Inc. * Copyright (C) 2009 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -447,6 +447,38 @@ const char *virJSONValueObjectGetKey(virJSONValuePtr object, unsigned int n) return object->data.object.pairs[n].key; }
+/* Remove the key-value pair tied to KEY out of OBJECT. If VALUE is + * not NULL, the dropped value object is returned instead of freed.
How about: s/KEY/@key/, s/OBJECT/@object/, s/VALUE/@value/, ?
+ * Returns 1 on success, 0 if no key was found, and -1 on error. */ +int +virJSONValueObjectRemoveKey(virJSONValuePtr object, const char *key, + virJSONValuePtr *value) +{ + int i; + + if (value) + *value = NULL; + + if (object->type != VIR_JSON_TYPE_OBJECT) + return -1; + + for (i = 0 ; i < object->data.object.npairs ; i++) { + if (STREQ(object->data.object.pairs[i].key, key)) { + if (value) { + *value = object->data.object.pairs[i].value; + object->data.object.pairs[i].value = NULL; + } + VIR_FREE(object->data.object.pairs[i].key); + virJSONValueFree(object->data.object.pairs[i].value); + VIR_DELETE_ELEMENT(object->data.object.pairs, i, + object->data.object.npairs); + return 1; + } + } + + return 0; +} + virJSONValuePtr virJSONValueObjectGetValue(virJSONValuePtr object, unsigned int n) { if (object->type != VIR_JSON_TYPE_OBJECT) diff --git a/src/util/virjson.h b/src/util/virjson.h index 67f4398..ff0fe75 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -1,7 +1,7 @@ /* * virjson.h: JSON object parsing/formatting * - * Copyright (C) 2009, 2012 Red Hat, Inc. + * Copyright (C) 2009, 2012-2013 Red Hat, Inc. * Copyright (C) 2009 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -55,12 +55,12 @@ struct _virJSONObjectPair { };
struct _virJSONObject { - unsigned int npairs; + size_t npairs; virJSONObjectPairPtr pairs; };
struct _virJSONArray { - unsigned int nvalues; + size_t nvalues; virJSONValuePtr *values; };
@@ -131,6 +131,9 @@ int virJSONValueObjectAppendNumberDouble(virJSONValuePtr object, const char *key int virJSONValueObjectAppendBoolean(virJSONValuePtr object, const char *key, int boolean); int virJSONValueObjectAppendNull(virJSONValuePtr object, const char *key);
+int virJSONValueObjectRemoveKey(virJSONValuePtr object, const char *key,
Worth to add ATTRIBUTE_NONNULL for "object" and "key"? with the questions. ACK with the two questions resolved.
On 05/13/2013 02:05 AM, Osier Yang wrote:
On 27/04/13 05:01, Eric Blake wrote:
In an upcoming patch, I need the way to safely transfer a nested virJSON object out of its parent container for independent use, even after the parent is freed.
* src/util/virjson.h (virJSONValueObjectRemoveKey): New function. (_virJSONObject, _virJSONArray): Use correct type. * src/util/virjson.c (virJSONValueObjectRemoveKey): Implement it. * src/libvirt_private.syms (virjson.h): Export it. * tests/jsontest.c (mymain): Test it.
+/* Remove the key-value pair tied to KEY out of OBJECT. If VALUE is + * not NULL, the dropped value object is returned instead of freed.
How about:
s/KEY/@key/, s/OBJECT/@object/, s/VALUE/@value/, ?
Sure thing. Some of my old habits slipping through (not every project uses doxygen-style markup :)
+int virJSONValueObjectRemoveKey(virJSONValuePtr object, const char *key,
Worth to add ATTRIBUTE_NONNULL for "object" and "key"?
Good idea. Consider it done :)
with the questions.
ACK with the two questions resolved.
Will push shortly as amended. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Follows on the heels of other bool cleanups, such as commit 93002b98. * src/qemu/qemu_monitor.h (qemuMonitorOpen, qemuMonitorOpenFD): Update json parameter type. * src/qemu/qemu_monitor.c (qemuMonitorOpen, qemuMonitorOpenFD): Likewise. (_qemuMonitor): Adjust field type. * src/qemu/qemu_domain.h (_qemuDomainObjPrivate): Likewise. * src/qemu/qemu_domain.c (qemuDomainObjPrivateXMLParse): Adjust client. * src/qemu/qemu_process.c (qemuProcessStart): Likewise. * tests/qemumonitortestutils.c (qemuMonitorTestNew): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_domain.c | 7 ++----- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_monitor.c | 18 +++++++++--------- src/qemu/qemu_monitor.h | 4 ++-- src/qemu/qemu_process.c | 8 ++------ tests/qemumonitortestutils.c | 2 +- 6 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d927716..8a476e6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -363,11 +363,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) priv->monConfig->type = VIR_DOMAIN_CHR_TYPE_PTY; VIR_FREE(tmp); - if (virXPathBoolean("count(./monitor[@json = '1']) > 0", ctxt)) { - priv->monJSON = 1; - } else { - priv->monJSON = 0; - } + priv->monJSON = virXPathBoolean("count(./monitor[@json = '1']) > 0", + ctxt) > 0; switch (priv->monConfig->type) { case VIR_DOMAIN_CHR_TYPE_PTY: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index da04377..81e2f91 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -131,7 +131,7 @@ struct _qemuDomainObjPrivate { qemuMonitorPtr mon; virDomainChrSourceDefPtr monConfig; - int monJSON; + bool monJSON; bool monError; unsigned long long monStart; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1b1d4a1..5f714d6 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -76,8 +76,8 @@ struct _qemuMonitor { int nextSerial; - unsigned json: 1; - unsigned wait_greeting: 1; + bool json; + bool waitGreeting; }; static virClassPtr qemuMonitorClass; @@ -356,8 +356,8 @@ qemuMonitorIOProcess(qemuMonitorPtr mon) if (len < 0) return -1; - if (len && mon->wait_greeting) - mon->wait_greeting = 0; + if (len && mon->waitGreeting) + mon->waitGreeting = false; if (len < mon->bufferOffset) { memmove(mon->buffer, mon->buffer + len, mon->bufferOffset - len); @@ -536,7 +536,7 @@ static void qemuMonitorUpdateWatch(qemuMonitorPtr mon) events |= VIR_EVENT_HANDLE_READABLE; if ((mon->msg && mon->msg->txOffset < mon->msg->txLength) && - !mon->wait_greeting) + !mon->waitGreeting) events |= VIR_EVENT_HANDLE_WRITABLE; } @@ -682,7 +682,7 @@ static qemuMonitorPtr qemuMonitorOpenInternal(virDomainObjPtr vm, int fd, bool hasSendFD, - int json, + bool json, qemuMonitorCallbacksPtr cb) { qemuMonitorPtr mon; @@ -715,7 +715,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm, mon->vm = vm; mon->json = json; if (json) - mon->wait_greeting = 1; + mon->waitGreeting = true; mon->cb = cb; if (virSetCloseExec(mon->fd) < 0) { @@ -769,7 +769,7 @@ cleanup: qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, virDomainChrSourceDefPtr config, - int json, + bool json, qemuMonitorCallbacksPtr cb) { int fd; @@ -804,7 +804,7 @@ qemuMonitorOpen(virDomainObjPtr vm, qemuMonitorPtr qemuMonitorOpenFD(virDomainObjPtr vm, int sockfd, - int json, + bool json, qemuMonitorCallbacksPtr cb) { return qemuMonitorOpenInternal(vm, sockfd, true, json, cb); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f39f009..527da0a 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -147,12 +147,12 @@ char *qemuMonitorUnescapeArg(const char *in); qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, virDomainChrSourceDefPtr config, - int json, + bool json, qemuMonitorCallbacksPtr cb) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); qemuMonitorPtr qemuMonitorOpenFD(virDomainObjPtr vm, int sockfd, - int json, + bool json, qemuMonitorCallbacksPtr cb) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f12d7d5..3bf9443 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3550,11 +3550,7 @@ int qemuProcessStart(virConnectPtr conn, if (qemuProcessPrepareMonitorChr(cfg, priv->monConfig, vm->def->name) < 0) goto cleanup; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON)) - priv->monJSON = 1; - else - priv->monJSON = 0; - + priv->monJSON = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON); priv->monError = false; priv->monStart = 0; priv->gotShutdown = false; @@ -3594,7 +3590,7 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, - priv->monJSON != 0, priv->qemuCaps, + priv->monJSON, priv->qemuCaps, migrateFrom, stdin_fd, snapshot, vmop))) goto cleanup; diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 43e7cb9..11d264e 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -502,7 +502,7 @@ qemuMonitorTestPtr qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) if (!(test->mon = qemuMonitorOpen(test->vm, &src, - json ? 1 : 0, + json, &qemuCallbacks))) goto error; virObjectLock(test->mon); -- 1.8.1.4
On 27/04/13 05:01, Eric Blake wrote:
Follows on the heels of other bool cleanups, such as commit 93002b98.
* src/qemu/qemu_monitor.h (qemuMonitorOpen, qemuMonitorOpenFD): Update json parameter type. * src/qemu/qemu_monitor.c (qemuMonitorOpen, qemuMonitorOpenFD): Likewise. (_qemuMonitor): Adjust field type. * src/qemu/qemu_domain.h (_qemuDomainObjPrivate): Likewise. * src/qemu/qemu_domain.c (qemuDomainObjPrivateXMLParse): Adjust client. * src/qemu/qemu_process.c (qemuProcessStart): Likewise. * tests/qemumonitortestutils.c (qemuMonitorTestNew): Likewise.
Signed-off-by: Eric Blake<eblake@redhat.com> ACK
No need to open code a string list cleanup, if we are nice to the caller by guaranteeing a NULL-terminated result. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONGetCPUDefinitions) (qemuMonitorJSONGetCommands, qemuMonitorJSONGetEvents) (qemuMonitorJSONGetObjectTypes, qemuMonitorJSONGetObjectProps): Use simpler cleanup. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_monitor_json.c | 53 ++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6fdd650..341b295 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3978,7 +3978,8 @@ int qemuMonitorJSONGetMachines(qemuMonitorPtr mon, goto cleanup; } - if (VIR_ALLOC_N(infolist, n) < 0) { + /* null-terminated list */ + if (VIR_ALLOC_N(infolist, n + 1) < 0) { virReportOOMError(); goto cleanup; } @@ -4091,7 +4092,8 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon, goto cleanup; } - if (VIR_ALLOC_N(cpulist, n) < 0) { + /* null-terminated list */ + if (VIR_ALLOC_N(cpulist, n + 1) < 0) { virReportOOMError(); goto cleanup; } @@ -4116,11 +4118,8 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon, *cpus = cpulist; cleanup: - if (ret < 0 && cpulist) { - for (i = 0 ; i < n ; i++) - VIR_FREE(cpulist[i]); - VIR_FREE(cpulist); - } + if (ret < 0) + virStringFreeList(cpulist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -4165,7 +4164,8 @@ int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, goto cleanup; } - if (VIR_ALLOC_N(commandlist, n) < 0) { + /* null-terminated list */ + if (VIR_ALLOC_N(commandlist, n + 1) < 0) { virReportOOMError(); goto cleanup; } @@ -4190,11 +4190,8 @@ int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, *commands = commandlist; cleanup: - if (ret < 0 && commandlist) { - for (i = 0 ; i < n ; i++) - VIR_FREE(commandlist[i]); - VIR_FREE(commandlist); - } + if (ret < 0) + virStringFreeList(commandlist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -4244,7 +4241,8 @@ int qemuMonitorJSONGetEvents(qemuMonitorPtr mon, goto cleanup; } - if (VIR_ALLOC_N(eventlist, n) < 0) { + /* null-terminated list */ + if (VIR_ALLOC_N(eventlist, n + 1) < 0) { virReportOOMError(); goto cleanup; } @@ -4269,11 +4267,8 @@ int qemuMonitorJSONGetEvents(qemuMonitorPtr mon, *events = eventlist; cleanup: - if (ret < 0 && eventlist) { - for (i = 0 ; i < n ; i++) - VIR_FREE(eventlist[i]); - VIR_FREE(eventlist); - } + if (ret < 0) + virStringFreeList(eventlist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -4369,7 +4364,8 @@ int qemuMonitorJSONGetObjectTypes(qemuMonitorPtr mon, goto cleanup; } - if (VIR_ALLOC_N(typelist, n) < 0) { + /* null-terminated list */ + if (VIR_ALLOC_N(typelist, n + 1) < 0) { virReportOOMError(); goto cleanup; } @@ -4394,11 +4390,8 @@ int qemuMonitorJSONGetObjectTypes(qemuMonitorPtr mon, *types = typelist; cleanup: - if (ret < 0 && typelist) { - for (i = 0 ; i < n ; i++) - VIR_FREE(typelist[i]); - VIR_FREE(typelist); - } + if (ret < 0) + virStringFreeList(typelist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -4451,7 +4444,8 @@ int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, goto cleanup; } - if (VIR_ALLOC_N(proplist, n) < 0) { + /* null-terminated list */ + if (VIR_ALLOC_N(proplist, n + 1) < 0) { virReportOOMError(); goto cleanup; } @@ -4476,11 +4470,8 @@ int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon, *props = proplist; cleanup: - if (ret < 0 && proplist) { - for (i = 0 ; i < n ; i++) - VIR_FREE(proplist[i]); - VIR_FREE(proplist); - } + if (ret < 0) + virStringFreeList(proplist); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; -- 1.8.1.4
On 27/04/13 05:01, Eric Blake wrote:
No need to open code a string list cleanup, if we are nice to the caller by guaranteeing a NULL-terminated result.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONGetCPUDefinitions) (qemuMonitorJSONGetCommands, qemuMonitorJSONGetEvents) (qemuMonitorJSONGetObjectTypes, qemuMonitorJSONGetObjectProps): Use simpler cleanup.
Signed-off-by: Eric Blake <eblake@redhat.com>
ACK
Ever since the conversion to using only QMP for probing features of qemu 1.2 and newer, we have been unable to detect features that are added only by additional command line options. For example, we'd like to know if '-machine mem-merge=on' (added in qemu 1.5) is present. To do this, we will take advantage of qemu 1.5's query-command-line-parameters QMP call [1]. This patch wires up the framework for probing the command results; if the QMP command is missing, or if a particular command line option does not output any parameters (for example, -net uses an old-style parser, so it shows up with no parameters), we silently treat that command as having no results. [1] https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg05180.html * src/qemu/qemu_monitor.h (qemuMonitorGetOptions) (qemuMonitorSetOptions) (qemuMonitorGetCommandLineOptionParameters): New functions. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONGetCommandLineOptionParameters): Likewise. * src/qemu/qemu_monitor.c (_qemuMonitor): Add cache field. (qemuMonitorDispose): Clean it. (qemuMonitorGetCommandLineOptionParameters): Implement new function. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONGetCommandLineOptionParameters): Likewise. (testQemuMonitorJSONGetCommandLineParameters): Test it. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_monitor.c | 41 +++++++++++++++ src/qemu/qemu_monitor.h | 9 +++- src/qemu/qemu_monitor_json.c | 123 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++ tests/qemumonitorjsontest.c | 103 ++++++++++++++++++++++++++++++++++++ 5 files changed, 279 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5f714d6..fffd4cd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -78,6 +78,9 @@ struct _qemuMonitor { bool json; bool waitGreeting; + + /* cache of query-command-line-options results */ + virJSONValuePtr options; }; static virClassPtr qemuMonitorClass; @@ -242,6 +245,7 @@ static void qemuMonitorDispose(void *obj) (mon->cb->destroy)(mon, mon->vm); virCondDestroy(&mon->notify); VIR_FREE(mon->buffer); + virJSONValueFree(mon->options); } @@ -911,6 +915,18 @@ cleanup: } +virJSONValuePtr +qemuMonitorGetOptions(qemuMonitorPtr mon) +{ + return mon->options; +} + +void +qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) +{ + mon->options = options; +} + int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, @@ -3333,6 +3349,31 @@ int qemuMonitorGetEvents(qemuMonitorPtr mon, } +/* Collect the parameters associated with a given command line option. + * Return count of known parameters or -1 on error. */ +int +qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params) +{ + VIR_DEBUG("mon=%p option=%s params=%p", mon, option, params); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + return qemuMonitorJSONGetCommandLineOptionParameters(mon, option, params); +} + + int qemuMonitorGetKVMState(qemuMonitorPtr mon, bool *enabled, bool *present) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 527da0a..8f9c182 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -162,12 +162,16 @@ int qemuMonitorSetCapabilities(qemuMonitorPtr mon); int qemuMonitorSetLink(qemuMonitorPtr mon, const char *name, - enum virDomainNetInterfaceLinkState state) ; + enum virDomainNetInterfaceLinkState state); /* These APIs are for use by the internal Text/JSON monitor impl code only */ char *qemuMonitorNextCommandID(qemuMonitorPtr mon); int qemuMonitorSend(qemuMonitorPtr mon, qemuMonitorMessagePtr msg); +virJSONValuePtr qemuMonitorGetOptions(qemuMonitorPtr mon) + ATTRIBUTE_NONNULL(1); +void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) + ATTRIBUTE_NONNULL(1); int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, @@ -664,6 +668,9 @@ int qemuMonitorGetCommands(qemuMonitorPtr mon, char ***commands); int qemuMonitorGetEvents(qemuMonitorPtr mon, char ***events); +int qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params); int qemuMonitorGetKVMState(qemuMonitorPtr mon, bool *enabled, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 341b295..0423ec7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4275,6 +4275,129 @@ cleanup: } +int +qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data = NULL; + virJSONValuePtr array = NULL; + char **paramlist = NULL; + int n = 0; + size_t i; + + *params = NULL; + + /* query-command-line-options has fixed output for a given qemu + * binary; but since callers want to query parameters for one + * option at a time, we cache the option list from qemu. */ + if (!qemuMonitorGetOptions(mon)) { + if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options", + NULL))) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) { + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) + goto cleanup; + ret = qemuMonitorJSONCheckError(cmd, reply); + } + + if (ret < 0) + goto cleanup; + + if (virJSONValueObjectRemoveKey(reply, "return", &array) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options reply was missing " + "return data")); + goto cleanup; + } + qemuMonitorSetOptions(mon, array); + } + + ret = -1; + + array = qemuMonitorGetOptions(mon); + if ((n = virJSONValueArraySize(array)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options reply data was not " + "an array")); + goto cleanup; + } + + for (i = 0 ; i < n ; i++) { + virJSONValuePtr child = virJSONValueArrayGet(array, i); + const char *tmp; + + if (!(tmp = virJSONValueObjectGetString(child, "option"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options reply data was " + "missing 'option'")); + goto cleanup; + } + if (STREQ(tmp, option)) { + data = virJSONValueObjectGet(child, "parameters"); + break; + } + } + + if (!data) { + /* Option not found; return 0 parameters rather than an error. */ + ret = 0; + goto cleanup; + } + + if ((n = virJSONValueArraySize(data)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options parameter data was not " + "an array")); + goto cleanup; + } + + /* null-terminated list */ + if (VIR_ALLOC_N(paramlist, n + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0 ; i < n ; i++) { + virJSONValuePtr child = virJSONValueArrayGet(data, i); + const char *tmp; + + if (!(tmp = virJSONValueObjectGetString(child, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options parameter data was " + "missing 'name'")); + goto cleanup; + } + + if (!(paramlist[i] = strdup(tmp))) { + virReportOOMError(); + goto cleanup; + } + } + + ret = n; + *params = paramlist; + +cleanup: + /* If we failed before getting the JSON array of options, we (try) + * to cache an empty array to speed up the next failure. */ + if (!qemuMonitorGetOptions(mon)) + qemuMonitorSetOptions(mon, virJSONValueNewArray()); + + if (ret < 0) + virStringFreeList(paramlist); + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, bool *enabled, bool *present) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 5ee0d84..74e2476 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -319,6 +319,10 @@ int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, int qemuMonitorJSONGetEvents(qemuMonitorPtr mon, char ***events) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, bool *enabled, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 6f42598..c7f0536 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -494,6 +494,108 @@ cleanup: static int +testQemuMonitorJSONGetCommandLineOptionParameters(const void *data) +{ + const virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNew(true, xmlopt); + int ret = -1; + char **params = NULL; + int nparams = 0; + + if (!test) + return -1; + + if (qemuMonitorTestAddItem(test, "query-command-line-options", + "{ " + " \"return\": [ " + " {\"parameters\": [], \"option\": \"acpi\" }," + " {\"parameters\": [" + " {\"name\": \"romfile\", " + " \"type\": \"string\"}, " + " {\"name\": \"bootindex\", " + " \"type\": \"number\"}], " + " \"option\": \"option-rom\"}" + " ]" + "}") < 0) + goto cleanup; + + /* present with params */ + if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), + "option-rom", + ¶ms)) < 0) + goto cleanup; + + if (nparams != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "nparams %d is not 2", nparams); + goto cleanup; + } + +#define CHECK(i, wantname) \ + do { \ + if (STRNEQ(params[i], (wantname))) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "name %s is not %s", \ + params[i], (wantname)); \ + goto cleanup; \ + } \ + } while (0) + + CHECK(0, "romfile"); + CHECK(1, "bootindex"); + +#undef CHECK + + virStringFreeList(params); + params = NULL; + + /* present but empty */ + if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), + "acpi", + ¶ms)) < 0) + goto cleanup; + + if (nparams != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "nparams %d is not 0", nparams); + goto cleanup; + } + if (params && params[0]) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected array contents"); + goto cleanup; + } + + virStringFreeList(params); + params = NULL; + + /* no such option */ + if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), + "foobar", + ¶ms)) < 0) + goto cleanup; + + if (nparams != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "nparams %d is not 0", nparams); + goto cleanup; + } + if (params && params[0]) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected array contents"); + goto cleanup; + } + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + virStringFreeList(params); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -520,6 +622,7 @@ mymain(void) DO_TEST(GetCPUDefinitions); DO_TEST(GetCommands); DO_TEST(GetTPMModels); + DO_TEST(GetCommandLineOptionParameters); virObjectUnref(xmlopt); -- 1.8.1.4
Il 26/04/2013 23:01, Eric Blake ha scritto:
(for example, -net uses an old-style parser, so it shows up with no parameters)
This is not really correct. -net (and in 1.6, -drive will too) uses the same parser, but is a "polymorphic" option so it uses a different kind of validation. This should be fixed in the commit message. For -net, we can add the list of options (even in 1.5). For -drive we will have to invent something along the lines of "-device foo,?". Paolo
On 27/04/13 05:01, Eric Blake wrote:
Ever since the conversion to using only QMP for probing features of qemu 1.2 and newer, we have been unable to detect features that are added only by additional command line options. For example, we'd like to know if '-machine mem-merge=on' (added in qemu 1.5) is present. To do this, we will take advantage of qemu 1.5's query-command-line-parameters QMP call [1].
This patch wires up the framework for probing the command results; if the QMP command is missing, or if a particular command line option does not output any parameters (for example, -net uses an old-style parser, so it shows up with no parameters), we silently treat that command as having no results.
[1] https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg05180.html
* src/qemu/qemu_monitor.h (qemuMonitorGetOptions) (qemuMonitorSetOptions) (qemuMonitorGetCommandLineOptionParameters): New functions. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONGetCommandLineOptionParameters): Likewise. * src/qemu/qemu_monitor.c (_qemuMonitor): Add cache field. (qemuMonitorDispose): Clean it. (qemuMonitorGetCommandLineOptionParameters): Implement new function. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONGetCommandLineOptionParameters): Likewise. (testQemuMonitorJSONGetCommandLineParameters): Test it.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_monitor.c | 41 +++++++++++++++ src/qemu/qemu_monitor.h | 9 +++- src/qemu/qemu_monitor_json.c | 123 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++ tests/qemumonitorjsontest.c | 103 ++++++++++++++++++++++++++++++++++++ 5 files changed, 279 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5f714d6..fffd4cd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -78,6 +78,9 @@ struct _qemuMonitor {
bool json; bool waitGreeting; + + /* cache of query-command-line-options results */ + virJSONValuePtr options; };
static virClassPtr qemuMonitorClass; @@ -242,6 +245,7 @@ static void qemuMonitorDispose(void *obj) (mon->cb->destroy)(mon, mon->vm); virCondDestroy(&mon->notify); VIR_FREE(mon->buffer); + virJSONValueFree(mon->options); }
@@ -911,6 +915,18 @@ cleanup: }
+virJSONValuePtr +qemuMonitorGetOptions(qemuMonitorPtr mon) +{ + return mon->options; +} + +void +qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) +{ + mon->options = options; +} + int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, @@ -3333,6 +3349,31 @@ int qemuMonitorGetEvents(qemuMonitorPtr mon, }
+/* Collect the parameters associated with a given command line option. + * Return count of known parameters or -1 on error. */ +int +qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params) +{ + VIR_DEBUG("mon=%p option=%s params=%p", mon, option, params); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + return qemuMonitorJSONGetCommandLineOptionParameters(mon, option, params); +} + + int qemuMonitorGetKVMState(qemuMonitorPtr mon, bool *enabled, bool *present) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 527da0a..8f9c182 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -162,12 +162,16 @@ int qemuMonitorSetCapabilities(qemuMonitorPtr mon);
int qemuMonitorSetLink(qemuMonitorPtr mon, const char *name, - enum virDomainNetInterfaceLinkState state) ; + enum virDomainNetInterfaceLinkState state);
/* These APIs are for use by the internal Text/JSON monitor impl code only */ char *qemuMonitorNextCommandID(qemuMonitorPtr mon); int qemuMonitorSend(qemuMonitorPtr mon, qemuMonitorMessagePtr msg); +virJSONValuePtr qemuMonitorGetOptions(qemuMonitorPtr mon) + ATTRIBUTE_NONNULL(1); +void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) + ATTRIBUTE_NONNULL(1); int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, @@ -664,6 +668,9 @@ int qemuMonitorGetCommands(qemuMonitorPtr mon, char ***commands); int qemuMonitorGetEvents(qemuMonitorPtr mon, char ***events); +int qemuMonitorGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params);
int qemuMonitorGetKVMState(qemuMonitorPtr mon, bool *enabled, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 341b295..0423ec7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4275,6 +4275,129 @@ cleanup: }
+int +qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data = NULL; + virJSONValuePtr array = NULL; + char **paramlist = NULL; + int n = 0; + size_t i; + + *params = NULL; + + /* query-command-line-options has fixed output for a given qemu + * binary; but since callers want to query parameters for one + * option at a time, we cache the option list from qemu. */ + if (!qemuMonitorGetOptions(mon)) { + if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options", + NULL))) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) { + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) + goto cleanup; + ret = qemuMonitorJSONCheckError(cmd, reply); + } + + if (ret < 0) + goto cleanup; + + if (virJSONValueObjectRemoveKey(reply, "return", &array) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options reply was missing " + "return data")); + goto cleanup; + } + qemuMonitorSetOptions(mon, array); + } + + ret = -1; + + array = qemuMonitorGetOptions(mon);
No need to get it when the options is cached. How about: if (!(array = qemuMonitorGetOptions(mon))) { ..... }
+ if ((n = virJSONValueArraySize(array)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options reply data was not "
s/was/is/, ? I'm sure that I don't want to talk about English with you though. :-)
+ "an array")); + goto cleanup; + } + + for (i = 0 ; i < n ; i++) { + virJSONValuePtr child = virJSONValueArrayGet(array, i); + const char *tmp; + + if (!(tmp = virJSONValueObjectGetString(child, "option"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options reply data was "
Likewise.
+ "missing 'option'")); + goto cleanup; + } + if (STREQ(tmp, option)) { + data = virJSONValueObjectGet(child, "parameters"); + break; + } + } + + if (!data) { + /* Option not found; return 0 parameters rather than an error. */ + ret = 0; + goto cleanup; + } + + if ((n = virJSONValueArraySize(data)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options parameter data was not "
Likewise.
+ "an array")); + goto cleanup; + } + + /* null-terminated list */ + if (VIR_ALLOC_N(paramlist, n + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0 ; i < n ; i++) { + virJSONValuePtr child = virJSONValueArrayGet(data, i); + const char *tmp; + + if (!(tmp = virJSONValueObjectGetString(child, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options parameter data was "
Likwise.
+ "missing 'name'")); + goto cleanup; + } + + if (!(paramlist[i] = strdup(tmp))) {
This has to be changed to use VIR_STRDUP.
+ virReportOOMError(); + goto cleanup; + } + } + + ret = n; + *params = paramlist; + +cleanup: + /* If we failed before getting the JSON array of options, we (try) + * to cache an empty array to speed up the next failure. */ + if (!qemuMonitorGetOptions(mon)) + qemuMonitorSetOptions(mon, virJSONValueNewArray()); + + if (ret < 0) + virStringFreeList(paramlist); + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, bool *enabled, bool *present) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 5ee0d84..74e2476 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -319,6 +319,10 @@ int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, int qemuMonitorJSONGetEvents(qemuMonitorPtr mon, char ***events) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, + const char *option, + char ***params) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
int qemuMonitorJSONGetKVMState(qemuMonitorPtr mon, bool *enabled, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 6f42598..c7f0536 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -494,6 +494,108 @@ cleanup:
static int +testQemuMonitorJSONGetCommandLineOptionParameters(const void *data) +{ + const virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNew(true, xmlopt); + int ret = -1; + char **params = NULL; + int nparams = 0; + + if (!test) + return -1; + + if (qemuMonitorTestAddItem(test, "query-command-line-options", + "{ " + " \"return\": [ " + " {\"parameters\": [], \"option\": \"acpi\" }," + " {\"parameters\": [" + " {\"name\": \"romfile\", " + " \"type\": \"string\"}, " + " {\"name\": \"bootindex\", " + " \"type\": \"number\"}], " + " \"option\": \"option-rom\"}" + " ]" + "}") < 0) + goto cleanup; + + /* present with params */ + if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), + "option-rom", + ¶ms)) < 0) + goto cleanup; + + if (nparams != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "nparams %d is not 2", nparams);
This will produce something like "4 is not 2", which is obvious and confused. :/ "'nparams' is not 2" might be better.
+ goto cleanup; + } + +#define CHECK(i, wantname) \ + do { \ + if (STRNEQ(params[i], (wantname))) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "name %s is not %s", \ + params[i], (wantname)); \ + goto cleanup; \ + } \ + } while (0) + + CHECK(0, "romfile"); + CHECK(1, "bootindex"); + +#undef CHECK + + virStringFreeList(params); + params = NULL; + + /* present but empty */ + if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), + "acpi", + ¶ms)) < 0) + goto cleanup; + + if (nparams != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "nparams %d is not 0", nparams);
Likewise.
+ goto cleanup; + } + if (params && params[0]) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "unexpected array contents"); + goto cleanup; + } + + virStringFreeList(params); + params = NULL; + + /* no such option */ + if ((nparams = qemuMonitorGetCommandLineOptionParameters(qemuMonitorTestGetMonitor(test), + "foobar", + ¶ms)) < 0) + goto cleanup; + + if (nparams != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "nparams %d is not 0", nparams);
Likewise. ACK with the nits fixed.
On 05/13/2013 02:35 AM, Osier Yang wrote:
+ + array = qemuMonitorGetOptions(mon);
No need to get it when the options is cached. How about:
if (!(array = qemuMonitorGetOptions(mon))) { ..... }
Nicer indeed.
+ if ((n = virJSONValueArraySize(array)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-command-line-options reply data was not "
s/was/is/, ? I'm sure that I don't want to talk about English with you though. :-)
I was just copying existing messages, such as: qemuMonitorJSONGetKVMState: _("query-kvm reply was missing return data")); At any rate, the error is internal, because we only expect to see it if something goes really wrong with the version of qemu we were talking to compared to what they documented as their interface, so it will probably never trigger, and I don't think it's worth changing.
+ } + + if (!(paramlist[i] = strdup(tmp))) {
This has to be changed to use VIR_STRDUP.
Yep.
+ + if (nparams != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "nparams %d is not 2", nparams);
This will produce something like "4 is not 2", which is obvious and confused. :/
"'nparams' is not 2" might be better.
Looking at other tests, it's nice to know how the test failed; I'll try: nparams was %d, expected 2 for this and the other places you called out.
ACK with the nits fixed.
Here's what I'm squashing in before pushing 1-4, along with a tweak to the commit message requested by Paolo. I'll let you take over 5/4 as part of your series. diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c index 0423ec7..0c011d3 100644 --- i/src/qemu/qemu_monitor_json.c +++ w/src/qemu/qemu_monitor_json.c @@ -4294,7 +4294,7 @@ qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, /* query-command-line-options has fixed output for a given qemu * binary; but since callers want to query parameters for one * option at a time, we cache the option list from qemu. */ - if (!qemuMonitorGetOptions(mon)) { + if (!(array = qemuMonitorGetOptions(mon))) { if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options", NULL))) return -1; @@ -4321,7 +4321,6 @@ qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, ret = -1; - array = qemuMonitorGetOptions(mon); if ((n = virJSONValueArraySize(array)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-command-line-options reply data was not " @@ -4375,10 +4374,8 @@ qemuMonitorJSONGetCommandLineOptionParameters(qemuMonitorPtr mon, goto cleanup; } - if (!(paramlist[i] = strdup(tmp))) { - virReportOOMError(); + if (VIR_STRDUP(paramlist[i], tmp) < 0) goto cleanup; - } } ret = n; diff --git i/tests/qemumonitorjsontest.c w/tests/qemumonitorjsontest.c index c7f0536..acc94ca 100644 --- i/tests/qemumonitorjsontest.c +++ w/tests/qemumonitorjsontest.c @@ -527,7 +527,7 @@ testQemuMonitorJSONGetCommandLineOptionParameters(const void *data) if (nparams != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, - "nparams %d is not 2", nparams); + "nparams was %d, expected 2", nparams); goto cleanup; } @@ -535,7 +535,7 @@ testQemuMonitorJSONGetCommandLineOptionParameters(const void *data) do { \ if (STRNEQ(params[i], (wantname))) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ - "name %s is not %s", \ + "name was %s, expected %s", \ params[i], (wantname)); \ goto cleanup; \ } \ @@ -557,7 +557,7 @@ testQemuMonitorJSONGetCommandLineOptionParameters(const void *data) if (nparams != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "nparams %d is not 0", nparams); + "nparams was %d, expected 0", nparams); goto cleanup; } if (params && params[0]) { @@ -577,7 +577,7 @@ testQemuMonitorJSONGetCommandLineOptionParameters(const void *data) if (nparams != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "nparams %d is not 0", nparams); + "nparams was %d, expected 0", nparams); goto cleanup; } if (params && params[0]) { -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
* src/qemu/qemu_capabilities.h: New capability bit. * src/qemu/qemu_capabilities.c (virQEMUCapsProbeQMPCommandLine): New function; use it to set new capability bit. (virQEMUCapsInitQMP): Use new function. --- As promised, here is how I would set the new capability for use in Osier's series here: https://www.redhat.com/archives/libvir-list/2013-April/msg01833.html src/qemu/qemu_capabilities.c | 46 ++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2acf535..40b824c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -222,9 +222,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "tpm-tis", "nvram", /* 140 */ - "pci-bridge", /* 141 */ - "vfio-pci", /* 142 */ - "vfio-pci.bootindex", /* 143 */ + "pci-bridge", + "vfio-pci", + "vfio-pci.bootindex", + "mem-merge", ); struct _virQEMUCaps { @@ -1441,7 +1442,6 @@ static struct virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { ARRAY_CARDINALITY(virQEMUCapsObjectPropsUsbHost) }, }; - static void virQEMUCapsProcessStringFlags(virQEMUCapsPtr qemuCaps, size_t nflags, @@ -2225,6 +2225,42 @@ virQEMUCapsProbeQMPKVMState(virQEMUCapsPtr qemuCaps, } +struct virQEMUCapsCommandLineProps { + const char *option; + const char *param; + int flag; +}; + +static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { + { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE }, +}; + +static int +virQEMUCapsProbeQMPCommandLine(virQEMUCapsPtr qemuCaps, + qemuMonitorPtr mon) +{ + int nvalues; + char **values; + size_t i, j; + + for (i = 0; i < ARRAY_CARDINALITY(virQEMUCapsCommandLine); i++) { + if ((nvalues = qemuMonitorGetCommandLineOptionParameters(mon, + virQEMUCapsCommandLine[i].option, + &values)) < 0) + return -1; + for (j = 0; j < nvalues; j++) { + if (STREQ(virQEMUCapsCommandLine[i].param, values[j])) { + virQEMUCapsSet(qemuCaps, virQEMUCapsCommandLine[i].flag); + break; + } + } + virStringFreeList(values); + } + + return 0; +} + + int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) { @@ -2552,6 +2588,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, goto cleanup; if (virQEMUCapsProbeQMPTPM(qemuCaps, mon) < 0) goto cleanup; + if (virQEMUCapsProbeQMPCommandLine(qemuCaps, mon) < 0) + goto cleanup; ret = 0; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 213f63c..c9c7fc9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -182,6 +182,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_PCI_BRIDGE = 141, /* -device pci-bridge */ QEMU_CAPS_DEVICE_VFIO_PCI = 142, /* -device vfio-pci */ QEMU_CAPS_VFIO_PCI_BOOTINDEX = 143, /* bootindex param for vfio-pci device */ + QEMU_CAPS_MEM_MERGE = 144, /* -machine mem-merge */ QEMU_CAPS_LAST, /* this must always be the last item */ }; -- 1.8.1.4
On 30/04/13 01:44, Eric Blake wrote:
* src/qemu/qemu_capabilities.h: New capability bit. * src/qemu/qemu_capabilities.c (virQEMUCapsProbeQMPCommandLine): New function; use it to set new capability bit. (virQEMUCapsInitQMP): Use new function. ---
As promised, here is how I would set the new capability for use in Osier's series here: https://www.redhat.com/archives/libvir-list/2013-April/msg01833.html
src/qemu/qemu_capabilities.c | 46 ++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2acf535..40b824c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -222,9 +222,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "tpm-tis",
"nvram", /* 140 */ - "pci-bridge", /* 141 */ - "vfio-pci", /* 142 */ - "vfio-pci.bootindex", /* 143 */ + "pci-bridge", + "vfio-pci", + "vfio-pci.bootindex", + "mem-merge", );
struct _virQEMUCaps { @@ -1441,7 +1442,6 @@ static struct virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { ARRAY_CARDINALITY(virQEMUCapsObjectPropsUsbHost) }, };
- static void virQEMUCapsProcessStringFlags(virQEMUCapsPtr qemuCaps, size_t nflags, @@ -2225,6 +2225,42 @@ virQEMUCapsProbeQMPKVMState(virQEMUCapsPtr qemuCaps, }
+struct virQEMUCapsCommandLineProps { + const char *option; + const char *param; + int flag; +}; + +static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { + { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE }, +}; + +static int +virQEMUCapsProbeQMPCommandLine(virQEMUCapsPtr qemuCaps, + qemuMonitorPtr mon) +{ + int nvalues; + char **values; + size_t i, j; + + for (i = 0; i < ARRAY_CARDINALITY(virQEMUCapsCommandLine); i++) { + if ((nvalues = qemuMonitorGetCommandLineOptionParameters(mon, + virQEMUCapsCommandLine[i].option, + &values)) < 0) + return -1; + for (j = 0; j < nvalues; j++) { + if (STREQ(virQEMUCapsCommandLine[i].param, values[j])) { + virQEMUCapsSet(qemuCaps, virQEMUCapsCommandLine[i].flag); + break; + } + } + virStringFreeList(values);
values is leaked if the loop breaks. ACK with this fixed, but it will need rebasing, feel free to leave it to me, I can do it when rebasing the mem-merge patch. Osier
participants (3)
-
Eric Blake -
Osier Yang -
Paolo Bonzini