[libvirt] [PATCH 00/20] tests: add qemu agent test
As promised earlier I'm sending a unit test for guest agent interaction. This series contains a few refactors and additions of monitor test utils and then add tests for all agent interaction functions. The refactors done in this series will allow to do a more thorough testing on the json monitor too. The small drawback of this test suite is a 6 second run time introduced to test timeout of a guest agent command. The test may be removed in case it's too much. Peter Krempa (20): conf: Export virDomainChrSourceDefClear() qemu_agent: Output newline at the end of the sync JSON message qemu_agent: Move updater function for VCPU hotplug into qemu_agent.c qemu_agent: Remove obvious comments qemumonitortestutils: Use consistent header style and line spacing qemumonitortestutils: Use VIR_DELETE_ELEMENT and VIR_APPEND_ELEMENT qemumonitortestutils: remove multiline function calls qemumonitortestutils: Don't crash on non fully initialized test qemumonitortestutils: Split up creation of the test to allow reuse qemumonitortestutils: Refactor the test helpers to allow reuse qemumonitortestutils: Split lines on \n instead of \r\n qemumonitortestutils: Add instrumentation for guest agent testing qemumonitortestutils: Improve error reporting from mock qemu monitor qemumonitortestutils: Add the ability to check arguments of commands tests: Add qemuagenttest qemuagenttest: Test the filesystem trimming qemuagenttest: Add testing of agent suspend modes qemuagenttest: Introduce testing of shutdown commands qemuagenttest: Test arbitrary qemu commands and timeouting of commands qemuagenttest: Add tests for CPU plug functions and helpers .gitignore | 1 + src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 73 ++++- src/qemu/qemu_agent.h | 3 + src/qemu/qemu_driver.c | 64 +--- tests/Makefile.am | 11 +- tests/qemuagenttest.c | 580 ++++++++++++++++++++++++++++++++++++ tests/qemumonitortestutils.c | 684 +++++++++++++++++++++++++++++++++---------- tests/qemumonitortestutils.h | 39 ++- 11 files changed, 1231 insertions(+), 229 deletions(-) create mode 100644 tests/qemuagenttest.c -- 1.8.3.2
--- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 783df96..e3aec69 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1332,7 +1332,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def) VIR_FREE(def); } -static void ATTRIBUTE_NONNULL(1) +void ATTRIBUTE_NONNULL(1) virDomainChrSourceDefClear(virDomainChrSourceDefPtr def) { switch (def->type) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index abf024c..de3b59c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2719,4 +2719,6 @@ int virDomainDefFindDevice(virDomainDefPtr def, bool virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def) ATTRIBUTE_NONNULL(1); +void virDomainChrSourceDefClear(virDomainChrSourceDefPtr def); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9615ea..7163350 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -123,6 +123,7 @@ virDomainChrInsert; virDomainChrRemove; virDomainChrSerialTargetTypeFromString; virDomainChrSerialTargetTypeToString; +virDomainChrSourceDefClear; virDomainChrSourceDefCopy; virDomainChrSourceDefFree; virDomainChrSpicevmcTypeFromString; -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
--- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 4 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Although this isn't apparently needed for the guest agent itself, the test I will be adding later depends on the newline as a separator of messages to process. --- src/qemu/qemu_agent.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 72bf211..1607e88 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -918,7 +918,7 @@ qemuAgentGuestSync(qemuAgentPtr mon) if (virAsprintf(&sync_msg.txBuffer, "{\"execute\":\"guest-sync\", " - "\"arguments\":{\"id\":%llu}}", id) < 0) + "\"arguments\":{\"id\":%llu}}\n", id) < 0) return -1; sync_msg.txLength = strlen(sync_msg.txBuffer); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Although this isn't apparently needed for the guest agent itself, the test I will be adding later depends on the newline as a separator of messages to process.
Agreed - absence of whitespace like newlines is inconsequential in strict JSON, but using it to simplify tests is worthwhile; it also makes it easier to see what is sent across a line when tracing debug messages.
--- src/qemu/qemu_agent.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 72bf211..1607e88 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -918,7 +918,7 @@ qemuAgentGuestSync(qemuAgentPtr mon)
if (virAsprintf(&sync_msg.txBuffer, "{\"execute\":\"guest-sync\", " - "\"arguments\":{\"id\":%llu}}", id) < 0) + "\"arguments\":{\"id\":%llu}}\n", id) < 0) return -1;
sync_msg.txLength = strlen(sync_msg.txBuffer);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow testing of the cpu updater function, this function needs to be available separately. Export it from qemu_agent.c where it should belong. --- src/qemu/qemu_agent.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 3 +++ src/qemu/qemu_driver.c | 64 +------------------------------------------------- 3 files changed, 67 insertions(+), 63 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 1607e88..fc85e3e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1597,3 +1597,66 @@ cleanup: virJSONValueFree(cpus); return ret; } + + +/* modify the cpu info structure to set the correct amount of cpus */ +int +qemuAgentUpdateCPUInfo(unsigned int nvcpus, + qemuAgentCPUInfoPtr cpuinfo, + int ncpuinfo) +{ + size_t i; + int nonline = 0; + int nofflinable = 0; + + /* count the active and offlinable cpus */ + for (i = 0; i < ncpuinfo; i++) { + if (cpuinfo[i].online) + nonline++; + + if (cpuinfo[i].offlinable && cpuinfo[i].online) + nofflinable++; + + /* This shouldn't happen, but we can't trust the guest agent */ + if (!cpuinfo[i].online && !cpuinfo[i].offlinable) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid data provided by guest agent")); + return -1; + } + } + + /* the guest agent reported less cpus than requested */ + if (nvcpus > ncpuinfo) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest agent reports less cpu than requested")); + return -1; + } + + /* not enough offlinable CPUs to support the request */ + if (nvcpus < nonline - nofflinable) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Cannot offline enough CPUs")); + return -1; + } + + for (i = 0; i < ncpuinfo; i++) { + if (nvcpus < nonline) { + /* unplug */ + if (cpuinfo[i].offlinable && cpuinfo[i].online) { + cpuinfo[i].online = false; + nonline--; + } + } else if (nvcpus > nonline) { + /* plug */ + if (!cpuinfo[i].online) { + cpuinfo[i].online = true; + nonline++; + } + } else { + /* done */ + break; + } + } + + return 0; +} diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index cf70653..5fbacdb 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -94,4 +94,7 @@ struct _qemuAgentCPUInfo { int qemuAgentGetVCPUs(qemuAgentPtr mon, qemuAgentCPUInfoPtr *info); int qemuAgentSetVCPUs(qemuAgentPtr mon, qemuAgentCPUInfoPtr cpus, size_t ncpus); +int qemuAgentUpdateCPUInfo(unsigned int nvcpus, + qemuAgentCPUInfoPtr cpuinfo, + int ncpuinfo); #endif /* __QEMU_AGENT_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5634abf..2daafa8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4090,68 +4090,6 @@ unsupported: static int -qemuDomainPrepareAgentVCPUs(unsigned int nvcpus, - qemuAgentCPUInfoPtr cpuinfo, - int ncpuinfo) -{ - size_t i; - int nonline = 0; - int nofflinable = 0; - - /* count the active and offlinable cpus */ - for (i = 0; i < ncpuinfo; i++) { - if (cpuinfo[i].online) - nonline++; - - if (cpuinfo[i].offlinable && cpuinfo[i].online) - nofflinable++; - - /* This shouldn't happen, but we can't trust the guest agent */ - if (!cpuinfo[i].online && !cpuinfo[i].offlinable) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Invalid data provided by guest agent")); - return -1; - } - } - - /* the guest agent reported less cpus than requested */ - if (nvcpus > ncpuinfo) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest agent reports less cpu than requested")); - return -1; - } - - /* not enough offlinable CPUs to support the request */ - if (nvcpus < nonline - nofflinable) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Cannot offline enough CPUs")); - return -1; - } - - for (i = 0; i < ncpuinfo; i++) { - if (nvcpus < nonline) { - /* unplug */ - if (cpuinfo[i].offlinable && cpuinfo[i].online) { - cpuinfo[i].online = false; - nonline--; - } - } else if (nvcpus > nonline) { - /* plug */ - if (!cpuinfo[i].online) { - cpuinfo[i].online = true; - nonline++; - } - } else { - /* done */ - break; - } - } - - return 0; -} - - -static int qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, unsigned int flags) { @@ -4243,7 +4181,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (ncpuinfo < 0) goto endjob; - if (qemuDomainPrepareAgentVCPUs(nvcpus, cpuinfo, ncpuinfo) < 0) + if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0) goto endjob; qemuDomainObjEnterAgent(vm); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
To allow testing of the cpu updater function, this function needs to be available separately. Export it from qemu_agent.c where it should belong. --- src/qemu/qemu_agent.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 3 +++ src/qemu/qemu_driver.c | 64 +------------------------------------------------- 3 files changed, 67 insertions(+), 63 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Most APIs in libvirt report errors, thus no need to state that explictly. --- src/qemu/qemu_agent.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index fc85e3e..2cd0ccc 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -930,10 +930,8 @@ qemuAgentGuestSync(qemuAgentPtr mon) VIR_DEBUG("qemuAgentSend returned: %d", send_ret); - if (send_ret < 0) { - /* error reported */ + if (send_ret < 0) goto cleanup; - } if (!sync_msg.rxObject) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -976,10 +974,8 @@ qemuAgentCommand(qemuAgentPtr mon, *reply = NULL; - if (qemuAgentGuestSync(mon) < 0) { - /* helper reported the error */ + if (qemuAgentGuestSync(mon) < 0) return -1; - } memset(&msg, 0, sizeof(msg)); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Most APIs in libvirt report errors, thus no need to state that explictly.
s/explictly/explicitly/ ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tests/qemumonitortestutils.c | 62 +++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 56368a2..ff90c5f 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -76,11 +76,13 @@ struct _qemuMonitorTest { static void qemuMonitorTestItemFree(qemuMonitorTestItemPtr item); + /* * Appends data for a reply to the outgoing buffer */ -static int qemuMonitorTestAddReponse(qemuMonitorTestPtr test, - const char *response) +static int +qemuMonitorTestAddReponse(qemuMonitorTestPtr test, + const char *response) { size_t want = strlen(response) + 2; size_t have = test->outgoingCapacity - test->outgoingLength; @@ -107,8 +109,9 @@ static int qemuMonitorTestAddReponse(qemuMonitorTestPtr test, * Processes a single line, looking for a matching expected * item to reply with, else replies with an error */ -static int qemuMonitorTestProcessCommandJSON(qemuMonitorTestPtr test, - const char *cmdstr) +static int +qemuMonitorTestProcessCommandJSON(qemuMonitorTestPtr test, + const char *cmdstr) { virJSONValuePtr val; const char *cmdname; @@ -150,8 +153,9 @@ cleanup: } -static int qemuMonitorTestProcessCommandText(qemuMonitorTestPtr test, - const char *cmdstr) +static int +qemuMonitorTestProcessCommandText(qemuMonitorTestPtr test, + const char *cmdstr) { char *tmp; char *cmdname; @@ -190,8 +194,10 @@ cleanup: return ret; } -static int qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, - const char *cmdstr) + +static int +qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, + const char *cmdstr) { if (test->json) return qemuMonitorTestProcessCommandJSON(test ,cmdstr); @@ -199,12 +205,14 @@ static int qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, return qemuMonitorTestProcessCommandText(test ,cmdstr); } + /* * Handles read/write of monitor data on the monitor server side */ -static void qemuMonitorTestIO(virNetSocketPtr sock, - int events, - void *opaque) +static void +qemuMonitorTestIO(virNetSocketPtr sock, + int events, + void *opaque) { qemuMonitorTestPtr test = opaque; bool err = false; @@ -298,7 +306,8 @@ cleanup: } -static void qemuMonitorTestWorker(void *opaque) +static void +qemuMonitorTestWorker(void *opaque) { qemuMonitorTestPtr test = opaque; @@ -322,7 +331,9 @@ static void qemuMonitorTestWorker(void *opaque) return; } -static void qemuMonitorTestItemFree(qemuMonitorTestItemPtr item) + +static void +qemuMonitorTestItemFree(qemuMonitorTestItemPtr item) { if (!item) return; @@ -335,13 +346,15 @@ static void qemuMonitorTestItemFree(qemuMonitorTestItemPtr item) static void -qemuMonitorTestFreeTimer(int timer ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) +qemuMonitorTestFreeTimer(int timer ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) { /* nothing to be done here */ } -void qemuMonitorTestFree(qemuMonitorTestPtr test) +void +qemuMonitorTestFree(qemuMonitorTestPtr test) { size_t i; int timer = -1; @@ -424,13 +437,16 @@ error: } -static void qemuMonitorTestEOFNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, - virDomainObjPtr vm ATTRIBUTE_UNUSED) +static void +qemuMonitorTestEOFNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) { } -static void qemuMonitorTestErrorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, - virDomainObjPtr vm ATTRIBUTE_UNUSED) + +static void +qemuMonitorTestErrorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) { } @@ -440,12 +456,14 @@ static qemuMonitorCallbacks qemuCallbacks = { .errorNotify = qemuMonitorTestErrorNotify, }; + #define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}" /* We skip the normal handshake reply of "{\"execute\":\"qmp_capabilities\"}" */ #define QEMU_TEXT_GREETING "QEMU 1.0,1 monitor - type 'help' for more information" -qemuMonitorTestPtr qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) +qemuMonitorTestPtr +qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) { qemuMonitorTestPtr test = NULL; virDomainChrSourceDef src; @@ -541,7 +559,9 @@ error: goto cleanup; } -qemuMonitorPtr qemuMonitorTestGetMonitor(qemuMonitorTestPtr test) + +qemuMonitorPtr +qemuMonitorTestGetMonitor(qemuMonitorTestPtr test) { return test->mon; } -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
--- tests/qemumonitortestutils.c | 62 +++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 21 deletions(-)
Thanks for separating cosmetic from functional changes. ACK.
@@ -440,12 +456,14 @@ static qemuMonitorCallbacks qemuCallbacks = { .errorNotify = qemuMonitorTestErrorNotify, };
+ #define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}"
Might be worth splitting this macro long line with \-newline, and relying on C's implicit concatenation of strings separated only by whitespace. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Simplify the code using the existing helpers instead of open coding the same functionality. --- tests/qemumonitortestutils.c | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index ff90c5f..a3c5431 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -133,17 +133,11 @@ qemuMonitorTestProcessCommandJSON(qemuMonitorTestPtr test, " { \"desc\": \"Unexpected command\", " " \"class\": \"UnexpectedCommand\" } }"); } else { - ret = qemuMonitorTestAddReponse(test, - test->items[0]->response); + ret = qemuMonitorTestAddReponse(test, test->items[0]->response); qemuMonitorTestItemFree(test->items[0]); - if (test->nitems == 1) { - VIR_FREE(test->items); - test->nitems = 0; - } else { - memmove(test->items, - test->items + 1, - sizeof(test->items[0]) * (test->nitems - 1)); - VIR_SHRINK_N(test->items, test->nitems, 1); + if (VIR_DELETE_ELEMENT(test->items, 0, test->nitems) < 0) { + ret = -1; + goto cleanup; } } @@ -175,17 +169,11 @@ qemuMonitorTestProcessCommandText(qemuMonitorTestPtr test, ret = qemuMonitorTestAddReponse(test, "unexpected command"); } else { - ret = qemuMonitorTestAddReponse(test, - test->items[0]->response); + ret = qemuMonitorTestAddReponse(test, test->items[0]->response); qemuMonitorTestItemFree(test->items[0]); - if (test->nitems == 1) { - VIR_FREE(test->items); - test->nitems = 0; - } else { - memmove(test->items, - test->items + 1, - sizeof(test->items[0]) * (test->nitems - 1)); - VIR_SHRINK_N(test->items, test->nitems, 1); + if (VIR_DELETE_ELEMENT(test->items, 0, test->nitems) < 0) { + ret = -1; + goto cleanup; } } @@ -421,12 +409,10 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, goto error; virMutexLock(&test->lock); - if (VIR_EXPAND_N(test->items, test->nitems, 1) < 0) { + if (VIR_APPEND_ELEMENT(test->items, test->nitems, item) < 0) { virMutexUnlock(&test->lock); goto error; } - test->items[test->nitems - 1] = item; - virMutexUnlock(&test->lock); return 0; -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Simplify the code using the existing helpers instead of open coding the same functionality. --- tests/qemumonitortestutils.c | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tests/qemumonitortestutils.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index a3c5431..34ca1ae 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -94,12 +94,8 @@ qemuMonitorTestAddReponse(qemuMonitorTestPtr test, } want -= 2; - memcpy(test->outgoing + test->outgoingLength, - response, - want); - memcpy(test->outgoing + test->outgoingLength + want, - "\r\n", - 2); + memcpy(test->outgoing + test->outgoingLength, response, want); + memcpy(test->outgoing + test->outgoingLength + want, "\r\n", 2); test->outgoingLength += want + 2; return 0; } @@ -484,10 +480,7 @@ qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) if (!(test->vm = virDomainObjNew(xmlopt))) goto error; - if (virNetSocketNewListenUNIX(path, - 0700, - getuid(), - getgid(), + if (virNetSocketNewListenUNIX(path, 0700, getuid(), getgid(), &test->server) < 0) goto error; -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
--- tests/qemumonitortestutils.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The qemumonitorjsontest crashed when one of the initialization steps done before starting the worker thread failed. This patch fixes this by trying to pthread_join() the thread only after it was created. --- tests/qemumonitortestutils.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 34ca1ae..5ca569f 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -368,7 +368,8 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) virObjectUnref(test->vm); - virThreadJoin(&test->thread); + if (test->running) + virThreadJoin(&test->thread); if (timer != -1) virEventRemoveTimeout(timer); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
The qemumonitorjsontest crashed when one of the initialization steps done before starting the worker thread failed. This patch fixes this by trying to pthread_join() the thread only after it was created. --- tests/qemumonitortestutils.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK.
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 34ca1ae..5ca569f 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -368,7 +368,8 @@ qemuMonitorTestFree(qemuMonitorTestPtr test)
virObjectUnref(test->vm);
- virThreadJoin(&test->thread); + if (test->running) + virThreadJoin(&test->thread);
if (timer != -1) virEventRemoveTimeout(timer);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The instrumentation for the monitor test can be hacked for qemu agent testing. Split out the monitor specific stuff to allow using the code in guest agent tests in the future. --- tests/qemumonitortestutils.c | 96 ++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 30 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 5ca569f..1785293 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -440,16 +440,11 @@ static qemuMonitorCallbacks qemuCallbacks = { }; -#define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}" -/* We skip the normal handshake reply of "{\"execute\":\"qmp_capabilities\"}" */ - -#define QEMU_TEXT_GREETING "QEMU 1.0,1 monitor - type 'help' for more information" - -qemuMonitorTestPtr -qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) +static qemuMonitorTestPtr +qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt, + virDomainChrSourceDefPtr src) { qemuMonitorTestPtr test = NULL; - virDomainChrSourceDef src; char *path = NULL; char *tmpdir_template = NULL; @@ -477,7 +472,6 @@ qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) if (virAsprintf(&path, "%s/qemumonitorjsontest.sock", test->tmpdir) < 0) goto error; - test->json = json; if (!(test->vm = virDomainObjNew(xmlopt))) goto error; @@ -485,29 +479,36 @@ qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) &test->server) < 0) goto error; - memset(&src, 0, sizeof(src)); - src.type = VIR_DOMAIN_CHR_TYPE_UNIX; - src.data.nix.path = (char *)path; - src.data.nix.listen = false; + memset(src, 0, sizeof(*src)); + src->type = VIR_DOMAIN_CHR_TYPE_UNIX; + src->data.nix.path = (char *)path; + src->data.nix.listen = false; if (virNetSocketListen(test->server, 1) < 0) goto error; - if (!(test->mon = qemuMonitorOpen(test->vm, - &src, - json, - &qemuCallbacks))) - goto error; - virObjectLock(test->mon); +cleanup: + return test; + +error: + VIR_FREE(tmpdir_template); + qemuMonitorTestFree(test); + test = NULL; + goto cleanup; + +} + + +static int +qemuMonitorCommonTestInit(qemuMonitorTestPtr test) +{ + if (!test) + return -1; if (virNetSocketAccept(test->server, &test->client) < 0) goto error; - if (!test->client) - goto error; - if (qemuMonitorTestAddReponse(test, json ? - QEMU_JSON_GREETING : - QEMU_TEXT_GREETING) < 0) + if (!test->client) goto error; if (virNetSocketAddIOCallback(test->client, @@ -528,17 +529,52 @@ qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) test->running = true; virMutexUnlock(&test->lock); -cleanup: - VIR_FREE(path); - return test; + return 0; error: - VIR_FREE(tmpdir_template); qemuMonitorTestFree(test); - test = NULL; - goto cleanup; + return -1; } +#define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}" +/* We skip the normal handshake reply of "{\"execute\":\"qmp_capabilities\"}" */ + +#define QEMU_TEXT_GREETING "QEMU 1.0,1 monitor - type 'help' for more information" + +qemuMonitorTestPtr +qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) +{ + qemuMonitorTestPtr test = NULL; + virDomainChrSourceDef src; + + if (!(test = qemuMonitorCommonTestNew(xmlopt, &src))) + goto error; + + test->json = json; + if (!(test->mon = qemuMonitorOpen(test->vm, + &src, + json, + &qemuCallbacks))) + goto error; + + virObjectLock(test->mon); + + if (qemuMonitorTestAddReponse(test, json ? + QEMU_JSON_GREETING : + QEMU_TEXT_GREETING) < 0) + goto error; + + if (qemuMonitorCommonTestInit(test) < 0) + goto error; + + virDomainChrSourceDefClear(&src); + + return test; + +error: + qemuMonitorTestFree(test); + return NULL; +} qemuMonitorPtr qemuMonitorTestGetMonitor(qemuMonitorTestPtr test) -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
The instrumentation for the monitor test can be hacked for qemu agent testing. Split out the monitor specific stuff to allow using the code in guest agent tests in the future. --- tests/qemumonitortestutils.c | 96 ++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 30 deletions(-)
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 5ca569f..1785293 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -440,16 +440,11 @@ static qemuMonitorCallbacks qemuCallbacks = { };
-#define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}"
Ah, as long as you are refactoring here, you don't have to tweak the earlier patch in the series where I suggested breaking the long line.
+#define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}"
But here, you SHOULD break up the long line :) ACK with that fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/30/13 16:00, Eric Blake wrote:
On 07/30/2013 07:05 AM, Peter Krempa wrote:
The instrumentation for the monitor test can be hacked for qemu agent testing. Split out the monitor specific stuff to allow using the code in guest agent tests in the future. --- tests/qemumonitortestutils.c | 96 ++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 30 deletions(-)
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 5ca569f..1785293 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -440,16 +440,11 @@ static qemuMonitorCallbacks qemuCallbacks = { };
-#define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}"
Ah, as long as you are refactoring here, you don't have to tweak the earlier patch in the series where I suggested breaking the long line.
+#define QEMU_JSON_GREETING "{\"QMP\": {\"version\": {\"qemu\": {\"micro\": 1, \"minor\": 0, \"major\": 1}, \"package\": \" (qemu-kvm-1.0.1)\"}, \"capabilities\": []}}"
I changed the line to: +#define QEMU_JSON_GREETING "{\"QMP\":"\ + " {\"version\":"\ + " {\"qemu\":"\ + " {\"micro\": 1,"\ + " \"minor\": 0,"\ + " \"major\": 1"\ + " },"\ + " \"package\": \"(qemu-kvm-1.0.1)"\ + " \"},"\ + " \"capabilities\": []"\ + " }"\ + "}" /* We skip the normal handshake reply of "{\"execute\":\"qmp_capabilities\"}" */ #define QEMU_TEXT_GREETING "QEMU 1.0,1 monitor - type 'help' for more information" and verified that it still builds and tests run successfully. Peter
Refactor the test helpers to allow adding callbacks to verify the monitor responses instead of simple command name checking and clean up various parts to prepare for adding guest agent tests. --- tests/qemumonitortestutils.c | 220 ++++++++++++++++++++++++------------------- 1 file changed, 121 insertions(+), 99 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 1785293..941dfea 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -38,10 +38,14 @@ typedef struct _qemuMonitorTestItem qemuMonitorTestItem; typedef qemuMonitorTestItem *qemuMonitorTestItemPtr; +typedef int (*qemuMonitorTestResponseCallback)(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *message); struct _qemuMonitorTestItem { - char *command_name; - char *response; + qemuMonitorTestResponseCallback cb; + void *opaque; + virFreeCallback freecb; }; struct _qemuMonitorTest { @@ -74,7 +78,17 @@ struct _qemuMonitorTest { }; -static void qemuMonitorTestItemFree(qemuMonitorTestItemPtr item); +static void +qemuMonitorTestItemFree(qemuMonitorTestItemPtr item) +{ + if (!item) + return; + + if (item->freecb) + (item->freecb)(item->opaque); + + VIR_FREE(item); +} /* @@ -101,95 +115,40 @@ qemuMonitorTestAddReponse(qemuMonitorTestPtr test, } -/* - * Processes a single line, looking for a matching expected - * item to reply with, else replies with an error - */ static int -qemuMonitorTestProcessCommandJSON(qemuMonitorTestPtr test, - const char *cmdstr) +qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test) { - virJSONValuePtr val; - const char *cmdname; - int ret = -1; - - if (!(val = virJSONValueFromString(cmdstr))) - return -1; - - if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Missing command name in %s", cmdstr); - goto cleanup; - } - - if (test->nitems == 0 || - STRNEQ(test->items[0]->command_name, cmdname)) { - ret = qemuMonitorTestAddReponse(test, - "{ \"error\": " - " { \"desc\": \"Unexpected command\", " - " \"class\": \"UnexpectedCommand\" } }"); + if (test->json) { + return qemuMonitorTestAddReponse(test, + "{ \"error\": " + " { \"desc\": \"Unexpected command\", " + " \"class\": \"UnexpectedCommand\" } }"); } else { - ret = qemuMonitorTestAddReponse(test, test->items[0]->response); - qemuMonitorTestItemFree(test->items[0]); - if (VIR_DELETE_ELEMENT(test->items, 0, test->nitems) < 0) { - ret = -1; - goto cleanup; - } + return qemuMonitorTestAddReponse(test, "unexpected command"); } - -cleanup: - virJSONValueFree(val); - return ret; } static int -qemuMonitorTestProcessCommandText(qemuMonitorTestPtr test, - const char *cmdstr) +qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, + const char *cmdstr) { - char *tmp; - char *cmdname; - int ret = -1; + int ret; - if (VIR_STRDUP(cmdname, cmdstr) < 0) - return -1; - if (!(tmp = strchr(cmdname, ' '))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Cannot find command name in '%s'", cmdstr); - goto cleanup; - } - *tmp = '\0'; - - if (test->nitems == 0 || - STRNEQ(test->items[0]->command_name, cmdname)) { - ret = qemuMonitorTestAddReponse(test, - "unexpected command"); + if (test->nitems == 0) { + return qemuMonitorTestAddUnexpectedErrorResponse(test); } else { - ret = qemuMonitorTestAddReponse(test, test->items[0]->response); - qemuMonitorTestItemFree(test->items[0]); - if (VIR_DELETE_ELEMENT(test->items, 0, test->nitems) < 0) { - ret = -1; - goto cleanup; - } + qemuMonitorTestItemPtr item = test->items[0]; + ret = (item->cb)(test, item, cmdstr); + qemuMonitorTestItemFree(item); + if (VIR_DELETE_ELEMENT(test->items, 0, test->nitems) < 0) + return -1; } -cleanup: - VIR_FREE(cmdname); return ret; } -static int -qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, - const char *cmdstr) -{ - if (test->json) - return qemuMonitorTestProcessCommandJSON(test ,cmdstr); - else - return qemuMonitorTestProcessCommandText(test ,cmdstr); -} - - /* * Handles read/write of monitor data on the monitor server side */ @@ -317,19 +276,6 @@ qemuMonitorTestWorker(void *opaque) static void -qemuMonitorTestItemFree(qemuMonitorTestItemPtr item) -{ - if (!item) - return; - - VIR_FREE(item->command_name); - VIR_FREE(item->response); - - VIR_FREE(item); -} - - -static void qemuMonitorTestFreeTimer(int timer ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { @@ -391,19 +337,20 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) } -int -qemuMonitorTestAddItem(qemuMonitorTestPtr test, - const char *command_name, - const char *response) +static int +qemuMonitorTestAddHandler(qemuMonitorTestPtr test, + qemuMonitorTestResponseCallback cb, + void *opaque, + virFreeCallback freecb) { qemuMonitorTestItemPtr item; if (VIR_ALLOC(item) < 0) goto error; - if (VIR_STRDUP(item->command_name, command_name) < 0 || - VIR_STRDUP(item->response, response) < 0) - goto error; + item->cb = cb; + item->freecb = freecb; + item->opaque = opaque; virMutexLock(&test->lock); if (VIR_APPEND_ELEMENT(test->items, test->nitems, item) < 0) { @@ -415,11 +362,86 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, return 0; error: - qemuMonitorTestItemFree(item); + if (freecb) + (freecb)(opaque); + VIR_FREE(item); return -1; } +struct qemuMonitorTestDefaultHandlerData { + const char *command_name; + const char *response; +}; + + +static int +qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *cmdstr) +{ + struct qemuMonitorTestDefaultHandlerData *data = item->opaque; + virJSONValuePtr val = NULL; + char *cmdcopy = NULL; + const char *cmdname; + char *tmp; + int ret = -1; + + if (test->json) { + if (!(val = virJSONValueFromString(cmdstr))) + return -1; + + if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Missing command name in %s", cmdstr); + goto cleanup; + } + } else { + if (VIR_STRDUP(cmdcopy, cmdstr) < 0) + return -1; + + cmdname = cmdcopy; + + if (!(tmp = strchr(cmdcopy, ' '))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Cannot find command name in '%s'", cmdstr); + goto cleanup; + } + *tmp = '\0'; + } + + if (STRNEQ(data->command_name, cmdname)) + ret = qemuMonitorTestAddUnexpectedErrorResponse(test); + else + ret = qemuMonitorTestAddReponse(test, data->response); + +cleanup: + VIR_FREE(cmdcopy); + virJSONValueFree(val); + return ret; +} + + +int +qemuMonitorTestAddItem(qemuMonitorTestPtr test, + const char *command_name, + const char *response) +{ + struct qemuMonitorTestDefaultHandlerData *data; + + if (VIR_ALLOC(data) < 0) + return -1; + + data->command_name = command_name; + data->response = response; + + return qemuMonitorTestAddHandler(test, + qemuMonitorTestProcessCommandDefault, + data, + free); +} + + static void qemuMonitorTestEOFNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm ATTRIBUTE_UNUSED) @@ -434,7 +456,7 @@ qemuMonitorTestErrorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } -static qemuMonitorCallbacks qemuCallbacks = { +static qemuMonitorCallbacks qemuMonitorTestCallbacks = { .eofNotify = qemuMonitorTestEOFNotify, .errorNotify = qemuMonitorTestErrorNotify, }; @@ -512,7 +534,7 @@ qemuMonitorCommonTestInit(qemuMonitorTestPtr test) goto error; if (virNetSocketAddIOCallback(test->client, - VIR_EVENT_HANDLE_WRITABLE, + test->outgoingLength > 0 ? VIR_EVENT_HANDLE_WRITABLE : VIR_EVENT_HANDLE_READABLE, qemuMonitorTestIO, test, NULL) < 0) @@ -554,7 +576,7 @@ qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) if (!(test->mon = qemuMonitorOpen(test->vm, &src, json, - &qemuCallbacks))) + &qemuMonitorTestCallbacks))) goto error; virObjectLock(test->mon); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Refactor the test helpers to allow adding callbacks to verify the monitor responses instead of simple command name checking and clean up various parts to prepare for adding guest agent tests. --- tests/qemumonitortestutils.c | 220 ++++++++++++++++++++++++------------------- 1 file changed, 121 insertions(+), 99 deletions(-)
@@ -512,7 +534,7 @@ qemuMonitorCommonTestInit(qemuMonitorTestPtr test) goto error;
if (virNetSocketAddIOCallback(test->client, - VIR_EVENT_HANDLE_WRITABLE, + test->outgoingLength > 0 ? VIR_EVENT_HANDLE_WRITABLE : VIR_EVENT_HANDLE_READABLE,
Long line. Quite a bit of code, but looks like a reasonable refactor. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The normal monitor uses windows line endings, where the agent monitor uses only newlines. Change this to tolerate both approaches and allow to use the utilities for guest agent tests. --- tests/qemumonitortestutils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 941dfea..6dc430e 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -207,7 +207,7 @@ qemuMonitorTestIO(virNetSocketPtr sock, * if so, handle that command */ t1 = test->incoming; - while ((t2 = strstr(t1, "\r\n"))) { + while ((t2 = strstr(t1, "\n"))) { *t2 = '\0'; if (qemuMonitorTestProcessCommand(test, t1) < 0) { @@ -215,7 +215,7 @@ qemuMonitorTestIO(virNetSocketPtr sock, goto cleanup; } - t1 = t2 + 2; + t1 = t2 + 1; } used = t1 - test->incoming; memmove(test->incoming, t1, test->incomingLength - used); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
The normal monitor uses windows line endings, where the agent monitor uses only newlines. Change this to tolerate both approaches and allow to use the utilities for guest agent tests.
"windows line endings" are also the proscribed line endings for use in telnet; it's not an accident that the qemu monitor was designed to be usable via a telnet connection, and I'm actually a bit surprised that the qemu agent didn't follow suit. Might be worth reporting this as an upstream issue to the qemu list.
--- tests/qemumonitortestutils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add helper functions to open guest agent connections and a handler for replying to the "guest-sync" command. --- tests/qemumonitortestutils.c | 133 ++++++++++++++++++++++++++++++++++++++++++- tests/qemumonitortestutils.h | 6 ++ 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 6dc430e..96cb054 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -28,6 +28,7 @@ #include "virthread.h" #include "qemu/qemu_monitor.h" +#include "qemu/qemu_agent.h" #include "rpc/virnetsocket.h" #include "viralloc.h" #include "virlog.h" @@ -68,6 +69,7 @@ struct _qemuMonitorTest { virNetSocketPtr client; qemuMonitorPtr mon; + qemuAgentPtr agent; char *tmpdir; @@ -118,7 +120,7 @@ qemuMonitorTestAddReponse(qemuMonitorTestPtr test, static int qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test) { - if (test->json) { + if (test->agent || test->json) { return qemuMonitorTestAddReponse(test, "{ \"error\": " " { \"desc\": \"Unexpected command\", " @@ -312,6 +314,11 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) qemuMonitorClose(test->mon); } + if (test->agent) { + virObjectUnlock(test->agent); + qemuAgentClose(test->agent); + } + virObjectUnref(test->vm); if (test->running) @@ -387,7 +394,7 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, char *tmp; int ret = -1; - if (test->json) { + if (test->agent || test->json) { if (!(val = virJSONValueFromString(cmdstr))) return -1; @@ -442,6 +449,72 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, } +static int +qemuMonitorTestProcessGuestAgentSync(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item ATTRIBUTE_UNUSED, + const char *cmdstr) +{ + virJSONValuePtr val = NULL; + virJSONValuePtr args; + unsigned long long id; + const char *cmdname; + char *retmsg = NULL; + int ret = -1; + + if (!(val = virJSONValueFromString(cmdstr))) + return -1; + + if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Missing guest-sync command name"); + goto cleanup; + } + + if (STRNEQ(cmdname, "guest-sync")) { + ret = qemuMonitorTestAddUnexpectedErrorResponse(test); + goto cleanup; + } + + if (!(args = virJSONValueObjectGet(val, "arguments"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Missing arguments for guest-sync"); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(args, "id", &id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Missing id for guest sync"); + goto cleanup; + } + + if (virAsprintf(&retmsg, "{\"return\":%llu}", id) < 0) + goto cleanup; + + + ret = qemuMonitorTestAddReponse(test, retmsg); + +cleanup: + virJSONValueFree(val); + VIR_FREE(retmsg); + return ret; +} + + +int +qemuMonitorTestAddAgentSyncResponse(qemuMonitorTestPtr test) +{ + if (!test->agent) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "This test is not an agent test"); + return -1; + } + + return qemuMonitorTestAddHandler(test, + qemuMonitorTestProcessGuestAgentSync, + NULL, NULL); +} + + static void qemuMonitorTestEOFNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm ATTRIBUTE_UNUSED) @@ -462,6 +535,24 @@ static qemuMonitorCallbacks qemuMonitorTestCallbacks = { }; +static void +qemuMonitorTestAgentEOFNotify(qemuAgentPtr agent ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) +{ +} + +static void +qemuMonitorTestAgentErrorNotify(qemuAgentPtr agent ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) +{ +} + +static qemuAgentCallbacks qemuMonitorTestAgentCallbacks = { + .eofNotify = qemuMonitorTestAgentEOFNotify, + .errorNotify = qemuMonitorTestAgentErrorNotify, +}; + + static qemuMonitorTestPtr qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt, virDomainChrSourceDefPtr src) @@ -594,12 +685,50 @@ qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt) return test; error: + virDomainChrSourceDefClear(&src); + qemuMonitorTestFree(test); + return NULL; +} + +qemuMonitorTestPtr +qemuMonitorTestNewAgent(virDomainXMLOptionPtr xmlopt) +{ + qemuMonitorTestPtr test = NULL; + virDomainChrSourceDef src; + + if (!(test = qemuMonitorCommonTestNew(xmlopt, &src))) + goto error; + + if (!(test->agent = qemuAgentOpen(test->vm, + &src, + &qemuMonitorTestAgentCallbacks))) + goto error; + + virObjectLock(test->agent); + + if (qemuMonitorCommonTestInit(test) < 0) + goto error; + + virDomainChrSourceDefClear(&src); + + return test; + +error: + virDomainChrSourceDefClear(&src); qemuMonitorTestFree(test); return NULL; } + qemuMonitorPtr qemuMonitorTestGetMonitor(qemuMonitorTestPtr test) { return test->mon; } + + +qemuAgentPtr +qemuMonitorTestGetAgent(qemuMonitorTestPtr test) +{ + return test->agent; +} diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index 9df118f..4307feb 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -22,6 +22,7 @@ # include "domain_conf.h" # include "qemu/qemu_monitor.h" +# include "qemu/qemu_agent.h" typedef struct _qemuMonitorTest qemuMonitorTest; typedef qemuMonitorTest *qemuMonitorTestPtr; @@ -31,12 +32,17 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, const char *command_name, const char *response); +int qemuMonitorTestAddAgentSyncResponse(qemuMonitorTestPtr test); + qemuMonitorTestPtr qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt); +qemuMonitorTestPtr qemuMonitorTestNewAgent(virDomainXMLOptionPtr xmlopt); + void qemuMonitorTestFree(qemuMonitorTestPtr test); qemuMonitorPtr qemuMonitorTestGetMonitor(qemuMonitorTestPtr test); +qemuAgentPtr qemuMonitorTestGetAgent(qemuMonitorTestPtr test); #endif /* __VIR_QEMU_MONITOR_TEST_UTILS_H__ */ -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Add helper functions to open guest agent connections and a handler for replying to the "guest-sync" command. --- tests/qemumonitortestutils.c | 133 ++++++++++++++++++++++++++++++++++++++++++- tests/qemumonitortestutils.h | 6 ++ 2 files changed, 137 insertions(+), 2 deletions(-)
@@ -462,6 +535,24 @@ static qemuMonitorCallbacks qemuMonitorTestCallbacks = { };
+static void +qemuMonitorTestAgentEOFNotify(qemuAgentPtr agent ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) +{ +} + +static void +qemuMonitorTestAgentErrorNotify(qemuAgentPtr agent ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) +{ +}
Since these are the same impl, is it worth simplifying slightly to have: static void qemuMonitorTestAgentNofify(...) { }
+ +static qemuAgentCallbacks qemuMonitorTestAgentCallbacks = { + .eofNotify = qemuMonitorTestAgentEOFNotify, + .errorNotify = qemuMonitorTestAgentErrorNotify,
and have both callback pointers initialized to the same function pointer? But that's a minor space optimization.
qemuMonitorTestFree(test); return NULL; }
+ qemuMonitorPtr
Was this newline missed in your earlier patch that sanitized the whitespace between functions? ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the JSON error messages to report errors back to the caller in addition to erroring out. The error reported from the event loop from the mock function of the monitor was later overwritten by the call to the monitor/agent interaction API. This will also allow testing of error reporting. --- tests/qemumonitortestutils.c | 56 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 96cb054..f421b9e 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -103,6 +103,8 @@ qemuMonitorTestAddReponse(qemuMonitorTestPtr test, size_t want = strlen(response) + 2; size_t have = test->outgoingCapacity - test->outgoingLength; + VIR_DEBUG("Adding response to monitor command: '%s", response); + if (have < want) { size_t need = want - have; if (VIR_EXPAND_N(test->outgoing, test->outgoingCapacity, need) < 0) @@ -132,11 +134,48 @@ qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test) static int +qemuMonitorReportError(qemuMonitorTestPtr test, const char *errmsg, ...) +{ + va_list msgargs; + char *msg = NULL; + char *jsonmsg = NULL; + int ret = -1; + + va_start(msgargs, errmsg); + + if (virVasprintf(&msg, errmsg, msgargs) < 0) + goto cleanup; + + if (test->agent || test->json) { + if (virAsprintf(&jsonmsg, "{ \"error\": " + " { \"desc\": \"%s\", " + " \"class\": \"UnexpectedCommand\" } }", + msg) < 0) + goto cleanup; + } else { + if (virAsprintf(&jsonmsg, "error: '%s'", msg) < 0) + goto cleanup; + } + + ret = qemuMonitorTestAddReponse(test, jsonmsg); + +cleanup: + va_end(msgargs); + VIR_FREE(msg); + VIR_FREE(jsonmsg); + return ret; +} + + + +static int qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, const char *cmdstr) { int ret; + VIR_DEBUG("Processing string from monitor handler: '%s", cmdstr); + if (test->nitems == 0) { return qemuMonitorTestAddUnexpectedErrorResponse(test); } else { @@ -399,8 +438,7 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Missing command name in %s", cmdstr); + ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); goto cleanup; } } else { @@ -410,8 +448,9 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, cmdname = cmdcopy; if (!(tmp = strchr(cmdcopy, ' '))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Cannot find command name in '%s'", cmdstr); + ret = qemuMonitorReportError(test, + "Cannot find command name in '%s'", + cmdstr); goto cleanup; } *tmp = '\0'; @@ -465,8 +504,7 @@ qemuMonitorTestProcessGuestAgentSync(qemuMonitorTestPtr test, return -1; if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Missing guest-sync command name"); + ret = qemuMonitorReportError(test, "Missing guest-sync command name"); goto cleanup; } @@ -476,14 +514,12 @@ qemuMonitorTestProcessGuestAgentSync(qemuMonitorTestPtr test, } if (!(args = virJSONValueObjectGet(val, "arguments"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Missing arguments for guest-sync"); + ret = qemuMonitorReportError(test, "Missing arguments for guest-sync"); goto cleanup; } if (virJSONValueObjectGetNumberUlong(args, "id", &id)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Missing id for guest sync"); + ret = qemuMonitorReportError(test, "Missing id for guest sync"); goto cleanup; } -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Use the JSON error messages to report errors back to the caller in addition to erroring out. The error reported from the event loop from the mock function of the monitor was later overwritten by the call to the monitor/agent interaction API. This will also allow testing of error reporting. --- tests/qemumonitortestutils.c | 56 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 10 deletions(-)
+} + + + +static int qemuMonitorTestProcessCommand(qemuMonitorTestPtr test,
Extra blank line. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Use the JSON error messages to report errors back to the caller in addition to erroring out. The error reported from the event loop from the mock function of the monitor was later overwritten by the call to the monitor/agent interaction API. This will also allow testing of error reporting. --- tests/qemumonitortestutils.c | 56 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 10 deletions(-)
My version of gcc complains: qemumonitortestutils.c: In function 'qemuMonitorReportError': qemumonitortestutils.c:140:5: error: function might be possible candidate for 'gnu_printf' format attribute [-Werror=suggest-attribute=format] if (virVasprintf(&msg, errmsg, msgargs) < 0) ^
@@ -132,11 +134,48 @@ qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test)
static int +qemuMonitorReportError(qemuMonitorTestPtr test, const char *errmsg, ...)
You'll need to add ATTRIBUTE_FMT_PRINTF(2, 3) to shut up the warning. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds helpers that allow to check for argument values in commands sent to the monitor. --- tests/qemumonitortestutils.c | 174 ++++++++++++++++++++++++++++++++++++++++--- tests/qemumonitortestutils.h | 5 ++ 2 files changed, 170 insertions(+), 9 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index f421b9e..f383915 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -415,18 +415,47 @@ error: } -struct qemuMonitorTestDefaultHandlerData { - const char *command_name; - const char *response; +typedef struct _qemuMonitorTestCommandArgs qemuMonitorTestCommandArgs; +typedef qemuMonitorTestCommandArgs *qemuMonitorTestCommandArgsPtr; +struct _qemuMonitorTestCommandArgs { + char *argname; + char *argval; }; +struct qemuMonitorTestHandlerData { + char *command_name; + char *response; + size_t nargs; + qemuMonitorTestCommandArgsPtr args; +}; + +static void +qemuMonitorTestHandlerDataFree(void *opaque) +{ + struct qemuMonitorTestHandlerData *data = opaque; + size_t i; + + if (!data) + return; + + for (i = 0; i < data->nargs; i++) { + VIR_FREE(data->args[i].argname); + VIR_FREE(data->args[i].argval); + } + + VIR_FREE(data->command_name); + VIR_FREE(data->response); + VIR_FREE(data->args); + VIR_FREE(data); +} + static int qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, qemuMonitorTestItemPtr item, const char *cmdstr) { - struct qemuMonitorTestDefaultHandlerData *data = item->opaque; + struct qemuMonitorTestHandlerData *data = item->opaque; virJSONValuePtr val = NULL; char *cmdcopy = NULL; const char *cmdname; @@ -473,18 +502,20 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, const char *command_name, const char *response) { - struct qemuMonitorTestDefaultHandlerData *data; + struct qemuMonitorTestHandlerData *data; if (VIR_ALLOC(data) < 0) return -1; - data->command_name = command_name; - data->response = response; + if (VIR_STRDUP(data->command_name, command_name) < 0 || + VIR_STRDUP(data->response, response) < 0) { + qemuMonitorTestHandlerDataFree(data); + return -1; + } return qemuMonitorTestAddHandler(test, qemuMonitorTestProcessCommandDefault, - data, - free); + data, qemuMonitorTestHandlerDataFree); } @@ -551,6 +582,131 @@ qemuMonitorTestAddAgentSyncResponse(qemuMonitorTestPtr test) } +static int +qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *cmdstr) +{ + struct qemuMonitorTestHandlerData *data = item->opaque; + virJSONValuePtr val = NULL; + virJSONValuePtr args; + virJSONValuePtr argobj; + char *argstr = NULL; + const char *cmdname; + size_t i; + int ret = -1; + + if (!(val = virJSONValueFromString(cmdstr))) + return -1; + + if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { + ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + goto cleanup; + } + + if (STRNEQ(data->command_name, cmdname)) { + ret = qemuMonitorTestAddUnexpectedErrorResponse(test); + goto cleanup; + } + + if (!(args = virJSONValueObjectGet(val, "arguments"))) { + ret = qemuMonitorReportError(test, + "Missing arguments section for command '%s'", + data->command_name); + goto cleanup; + } + + /* validate the args */ + for (i = 0; i < data->nargs; i++) { + qemuMonitorTestCommandArgsPtr arg = &data->args[i]; + if (!(argobj = virJSONValueObjectGet(args, arg->argname))) { + ret = qemuMonitorReportError(test, + "Missing argument '%s' for command '%s'", + arg->argname, data->command_name); + goto cleanup; + } + + /* convert the argument to string */ + if (!(argstr = virJSONValueToString(argobj, false))) + goto cleanup; + + /* verify that the argument value is expected */ + if (STRNEQ(argstr, arg->argval)) { + ret = qemuMonitorReportError(test, + "Invalid value of argument '%s' " + "of command '%s': " + "expected '%s' got '%s'", + arg->argname, data->command_name, + arg->argval, argstr); + goto cleanup; + } + + VIR_FREE(argstr); + } + + /* arguments checked out, return the response */ + ret = qemuMonitorTestAddReponse(test, data->response); + +cleanup: + VIR_FREE(argstr); + virJSONValueFree(val); + return ret; +} + + +/* this allows to add a responder that is able to check + * a (shallow) structure of arguments for a command */ +int +qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, + const char *cmdname, + const char *response, + ...) +{ + struct qemuMonitorTestHandlerData *data; + const char *argname; + const char *argval; + va_list args; + + va_start(args, response); + + if (VIR_ALLOC(data) < 0) + return -1; + + if (VIR_STRDUP(data->command_name, cmdname) < 0 || + VIR_STRDUP(data->response, response) < 0) + goto error; + + while ((argname = va_arg(args, char *))) { + size_t i; + if (!(argval = va_arg(args, char *))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Missing argument value for argument '%s'", + argname); + goto error; + } + + i = data->nargs; + if (VIR_EXPAND_N(data->args, data->nargs, 1)) + goto error; + + if (VIR_STRDUP(data->args[i].argname, argname) < 0 || + VIR_STRDUP(data->args[i].argval, argval) < 0) + goto error; + } + + va_end(args); + + return qemuMonitorTestAddHandler(test, + qemuMonitorTestProcessCommandWithArgs, + data, qemuMonitorTestHandlerDataFree); + +error: + va_end(args); + qemuMonitorTestHandlerDataFree(data); + return -1; +} + + static void qemuMonitorTestEOFNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm ATTRIBUTE_UNUSED) diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index 4307feb..5ec5707 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -34,6 +34,11 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, int qemuMonitorTestAddAgentSyncResponse(qemuMonitorTestPtr test); +int qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, + const char *cmdname, + const char *response, + ...); + qemuMonitorTestPtr qemuMonitorTestNew(bool json, virDomainXMLOptionPtr xmlopt); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
This patch adds helpers that allow to check for argument values in commands sent to the monitor. --- tests/qemumonitortestutils.c | 174 ++++++++++++++++++++++++++++++++++++++++--- tests/qemumonitortestutils.h | 5 ++ 2 files changed, 170 insertions(+), 9 deletions(-)
+ "Missing arguments section for command '%s'", + data->command_name); + goto cleanup; + } + + /* validate the args */ + for (i = 0; i < data->nargs; i++) {
Indentation of the comment is off.
+++ b/tests/qemumonitortestutils.h @@ -34,6 +34,11 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test,
int qemuMonitorTestAddAgentSyncResponse(qemuMonitorTestPtr test);
+int qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, + const char *cmdname, + const char *response, + ...);
Might be worth adding an ATTRIBUTE_SENTINEL on the prototype of this function, so that gcc ensures that a caller ends the list with a NULL. ACK with those fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/30/2013 09:05 AM, Peter Krempa wrote: <...snip...>
+/* this allows to add a responder that is able to check + * a (shallow) structure of arguments for a command */ +int +qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, + const char *cmdname, + const char *response, + ...) +{ + struct qemuMonitorTestHandlerData *data; + const char *argname; + const char *argval; + va_list args; + + va_start(args, response); +
Coverity complains: 668 (1) Event va_init: Initializing va_list "args". Also see events: [missing_va_end] 669 va_start(args, response); 670 (2) Event cond_true: Condition "virAlloc(&data, 32UL /* sizeof (*data) */, true /* 1 */, VIR_FROM_NONE, "qemumonitortestutils.c", <anonymous>, 671) < 0", taking true branch 671 if (VIR_ALLOC(data) < 0) (3) Event missing_va_end: va_end was not called for "args". Also see events: [va_init] 672 return -1; 673 John
+ if (VIR_ALLOC(data) < 0) + return -1; + + if (VIR_STRDUP(data->command_name, cmdname) < 0 || + VIR_STRDUP(data->response, response) < 0) + goto error; + + while ((argname = va_arg(args, char *))) { + size_t i; + if (!(argval = va_arg(args, char *))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Missing argument value for argument '%s'", + argname); + goto error; + } + + i = data->nargs; + if (VIR_EXPAND_N(data->args, data->nargs, 1)) + goto error; + + if (VIR_STRDUP(data->args[i].argname, argname) < 0 || + VIR_STRDUP(data->args[i].argval, argval) < 0) + goto error; + } + + va_end(args); + + return qemuMonitorTestAddHandler(test, + qemuMonitorTestProcessCommandWithArgs, + data, qemuMonitorTestHandlerDataFree); + +error: + va_end(args); + qemuMonitorTestHandlerDataFree(data); + return -1; +} + +
Add a basic test framework with two simple tests to test guest agent interaction. --- .gitignore | 1 + tests/Makefile.am | 11 +++- tests/qemuagenttest.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 tests/qemuagenttest.c diff --git a/.gitignore b/.gitignore index 4c79de3..738c6ba 100644 --- a/.gitignore +++ b/.gitignore @@ -165,6 +165,7 @@ /tests/object-locking-files.txt /tests/object-locking.cm[ix] /tests/openvzutilstest +/tests/qemuagenttest /tests/qemuargv2xmltest /tests/qemuhelptest /tests/qemuhotplugtest diff --git a/tests/Makefile.am b/tests/Makefile.am index 5ea806e..9c578fa 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -161,7 +161,8 @@ endif if WITH_QEMU test_programs += qemuxml2argvtest qemuxml2xmltest qemuxmlnstest \ qemuargv2xmltest qemuhelptest domainsnapshotxml2xmltest \ - qemumonitortest qemumonitorjsontest qemuhotplugtest + qemumonitortest qemumonitorjsontest qemuhotplugtest \ + qemuagenttest endif if WITH_LXC @@ -418,6 +419,13 @@ qemumonitorjsontest_SOURCES = \ $(NULL) qemumonitorjsontest_LDADD = $(qemu_LDADDS) libqemumonitortestutils.la +qemuagenttest_SOURCES = \ + qemuagenttest.c \ + testutils.c testutils.h \ + testutilsqemu.c testutilsqemu.h \ + $(NULL) +qemuagenttest_LDADD = $(qemu_LDADDS) libqemumonitortestutils.la + qemuhotplugtest_SOURCES = \ qemuhotplugtest.c \ testutils.c testutils.h \ @@ -434,6 +442,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ qemuxmlnstest.c qemuhelptest.c domainsnapshotxml2xmltest.c \ qemumonitortest.c testutilsqemu.c testutilsqemu.h \ qemumonitorjsontest.c qemuhotplugtest.c \ + qemuagenttest.c \ $(QEMUMONITORTESTUTILS_SOURCES) endif diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c new file mode 100644 index 0000000..7f22ff0 --- /dev/null +++ b/tests/qemuagenttest.c @@ -0,0 +1,163 @@ +/* + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include "testutils.h" +#include "testutilsqemu.h" +#include "qemumonitortestutils.h" +#include "qemu/qemu_conf.h" +#include "qemu/qemu_agent.h" +#include "virthread.h" +#include "virerror.h" +#include "virstring.h" + + +#define VIR_FROM_THIS VIR_FROM_NONE + +static int +testQemuAgentFSFreeze(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-fsfreeze-freeze", + "{ \"return\" : 5 }") < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-fsfreeze-freeze", + "{ \"return\" : 7 }") < 0) + goto cleanup; + + if ((ret = qemuAgentFSFreeze(qemuMonitorTestGetAgent(test))) < 0) + goto cleanup; + + if (ret != 5) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 5 frozen filesystems, got %d", ret); + goto cleanup; + } + + if ((ret = qemuAgentFSFreeze(qemuMonitorTestGetAgent(test))) < 0) + goto cleanup; + + if (ret != 7) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 7 frozen filesystems, got %d", ret); + goto cleanup; + } + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + +static int +testQemuAgentFSThaw(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-fsfreeze-thaw", + "{ \"return\" : 5 }") < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-fsfreeze-thaw", + "{ \"return\" : 7 }") < 0) + goto cleanup; + + if ((ret = qemuAgentFSThaw(qemuMonitorTestGetAgent(test))) < 0) + goto cleanup; + + if (ret != 5) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 5 thawed filesystems, got %d", ret); + goto cleanup; + } + + if ((ret = qemuAgentFSThaw(qemuMonitorTestGetAgent(test))) < 0) + goto cleanup; + + if (ret != 7) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 7 thawed filesystems, got %d", ret); + goto cleanup; + } + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + virDomainXMLOptionPtr xmlopt; + +#if !WITH_YAJL + fputs("libvirt not compiled with yajl, skipping this test\n", stderr); + return EXIT_AM_SKIP; +#endif + + if (virThreadInitialize() < 0 || + !(xmlopt = virQEMUDriverCreateXMLConf(NULL))) + return EXIT_FAILURE; + + virEventRegisterDefaultImpl(); + +#define DO_TEST(name) \ + if (virtTestRun(# name, 1, testQemuAgent ## name, xmlopt) < 0) \ + ret = -1 + + DO_TEST(FSFreeze); + DO_TEST(FSThaw); + + virObjectUnref(xmlopt); + + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Add a basic test framework with two simple tests to test guest agent interaction. --- .gitignore | 1 + tests/Makefile.am | 11 +++- tests/qemuagenttest.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 tests/qemuagenttest.c
ACK. (Doesn't quite match reality, in that executing back-to-back freeze with no thaw between is never what you'd do to a real agent - but this test is about testing the libvirt code paths, not real back-and-forth agent sequences) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tests/qemuagenttest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 7f22ff0..a3b8834 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -132,6 +132,36 @@ cleanup: static int +testQemuAgentFSTrim(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItemParams(test, "guest-fstrim", + "{ \"return\" : {} }", + "minimum", "1337", + NULL) < 0) + goto cleanup; + + if (qemuAgentFSTrim(qemuMonitorTestGetAgent(test), 1337) < 0) + goto cleanup; + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -154,6 +184,7 @@ mymain(void) DO_TEST(FSFreeze); DO_TEST(FSThaw); + DO_TEST(FSTrim); virObjectUnref(xmlopt); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
--- tests/qemuagenttest.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tests/qemuagenttest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index a3b8834..d0b450e 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -162,6 +162,52 @@ cleanup: static int +testQemuAgentSuspend(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + int ret = -1; + size_t i; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-suspend-ram", + "{ \"return\" : {} }") < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-suspend-disk", + "{ \"return\" : {} }") < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-suspend-hybrid", + "{ \"return\" : {} }") < 0) + goto cleanup; + + /* try the commands - fail if ordering changes */ + for (i = 0; i < VIR_NODE_SUSPEND_TARGET_LAST; i++) { + if (qemuAgentSuspend(qemuMonitorTestGetAgent(test), i) < 0) + goto cleanup; + } + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -185,6 +231,7 @@ mymain(void) DO_TEST(FSFreeze); DO_TEST(FSThaw); DO_TEST(FSTrim); + DO_TEST(Suspend); virObjectUnref(xmlopt); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
--- tests/qemuagenttest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch exports a few utility functions and adds testing of shutdown commands of the guest agent. --- tests/qemuagenttest.c | 119 +++++++++++++++++++++++++++++++++++++++++++ tests/qemumonitortestutils.c | 21 ++++---- tests/qemumonitortestutils.h | 28 ++++++++-- 3 files changed, 153 insertions(+), 15 deletions(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index d0b450e..e314bb0 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -207,6 +207,124 @@ cleanup: } +struct qemuAgentShutdownTestData { + const char *mode; + qemuAgentEvent event; +}; + + +static int +qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *cmdstr) +{ + struct qemuAgentShutdownTestData *data; + virJSONValuePtr val = NULL; + virJSONValuePtr args; + const char *cmdname; + const char *mode; + int ret = -1; + + data = qemuMonitorTestItemGetPrivateData(item); + + if (!(val = virJSONValueFromString(cmdstr))) + return -1; + + if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { + ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + goto cleanup; + } + + if (STRNEQ(cmdname, "guest-shutdown")) { + ret = qemuMonitorTestAddUnexpectedErrorResponse(test); + goto cleanup; + } + + if (!(args = virJSONValueObjectGet(val, "arguments"))) { + ret = qemuMonitorReportError(test, + "Missing arguments section"); + goto cleanup; + } + + if (!(mode = virJSONValueObjectGetString(args, "mode"))) { + ret = qemuMonitorReportError(test, "Missing shutdown mode"); + goto cleanup; + } + + /* now don't reply but return a qemu agent event */ + qemuAgentNotifyEvent(qemuMonitorTestGetAgent(test), + data->event); + + ret = 0; + +cleanup: + virJSONValueFree(val); + return ret; + +} + + +static int +testQemuAgentShutdown(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + struct qemuAgentShutdownTestData priv; + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + priv.event = QEMU_AGENT_EVENT_SHUTDOWN; + priv.mode = "shutdown"; + + if (qemuMonitorTestAddHandler(test, qemuAgentShutdownTestMonitorHandler, + &priv, NULL) < 0) + goto cleanup; + + if (qemuAgentShutdown(qemuMonitorTestGetAgent(test), + QEMU_AGENT_SHUTDOWN_HALT) < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + priv.event = QEMU_AGENT_EVENT_SHUTDOWN; + priv.mode = "powerdown"; + + if (qemuMonitorTestAddHandler(test, qemuAgentShutdownTestMonitorHandler, + &priv, NULL) < 0) + goto cleanup; + + if (qemuAgentShutdown(qemuMonitorTestGetAgent(test), + QEMU_AGENT_SHUTDOWN_POWERDOWN) < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + priv.event = QEMU_AGENT_EVENT_RESET; + priv.mode = "reboot"; + + if (qemuMonitorTestAddHandler(test, qemuAgentShutdownTestMonitorHandler, + &priv, NULL) < 0) + goto cleanup; + + if (qemuAgentShutdown(qemuMonitorTestGetAgent(test), + QEMU_AGENT_SHUTDOWN_REBOOT) < 0) + goto cleanup; + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + static int mymain(void) { @@ -232,6 +350,7 @@ mymain(void) DO_TEST(FSThaw); DO_TEST(FSTrim); DO_TEST(Suspend); + DO_TEST(Shutdown); virObjectUnref(xmlopt); diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index f383915..32217a3 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -37,12 +37,6 @@ #define VIR_FROM_THIS VIR_FROM_NONE -typedef struct _qemuMonitorTestItem qemuMonitorTestItem; -typedef qemuMonitorTestItem *qemuMonitorTestItemPtr; -typedef int (*qemuMonitorTestResponseCallback)(qemuMonitorTestPtr test, - qemuMonitorTestItemPtr item, - const char *message); - struct _qemuMonitorTestItem { qemuMonitorTestResponseCallback cb; void *opaque; @@ -96,7 +90,7 @@ qemuMonitorTestItemFree(qemuMonitorTestItemPtr item) /* * Appends data for a reply to the outgoing buffer */ -static int +int qemuMonitorTestAddReponse(qemuMonitorTestPtr test, const char *response) { @@ -119,7 +113,7 @@ qemuMonitorTestAddReponse(qemuMonitorTestPtr test, } -static int +int qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test) { if (test->agent || test->json) { @@ -133,7 +127,7 @@ qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test) } -static int +int qemuMonitorReportError(qemuMonitorTestPtr test, const char *errmsg, ...) { va_list msgargs; @@ -167,7 +161,6 @@ cleanup: } - static int qemuMonitorTestProcessCommand(qemuMonitorTestPtr test, const char *cmdstr) @@ -383,7 +376,7 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) } -static int +int qemuMonitorTestAddHandler(qemuMonitorTestPtr test, qemuMonitorTestResponseCallback cb, void *opaque, @@ -414,6 +407,12 @@ error: return -1; } +void * +qemuMonitorTestItemGetPrivateData(qemuMonitorTestItemPtr item) +{ + return item ? item->opaque : NULL; +} + typedef struct _qemuMonitorTestCommandArgs qemuMonitorTestCommandArgs; typedef qemuMonitorTestCommandArgs *qemuMonitorTestCommandArgsPtr; diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index 5ec5707..845f567 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -27,10 +27,30 @@ typedef struct _qemuMonitorTest qemuMonitorTest; typedef qemuMonitorTest *qemuMonitorTestPtr; -int -qemuMonitorTestAddItem(qemuMonitorTestPtr test, - const char *command_name, - const char *response); +typedef struct _qemuMonitorTestItem qemuMonitorTestItem; +typedef qemuMonitorTestItem *qemuMonitorTestItemPtr; +typedef int (*qemuMonitorTestResponseCallback)(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *message); + +int qemuMonitorTestAddHandler(qemuMonitorTestPtr test, + qemuMonitorTestResponseCallback cb, + void *opaque, + virFreeCallback freecb); + +int qemuMonitorTestAddReponse(qemuMonitorTestPtr test, + const char *response); + +int qemuMonitorTestAddUnexpectedErrorResponse(qemuMonitorTestPtr test); + +int qemuMonitorReportError(qemuMonitorTestPtr test, const char *errmsg, ...); + +void *qemuMonitorTestItemGetPrivateData(qemuMonitorTestItemPtr item); + + +int qemuMonitorTestAddItem(qemuMonitorTestPtr test, + const char *command_name, + const char *response); int qemuMonitorTestAddAgentSyncResponse(qemuMonitorTestPtr test); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
This patch exports a few utility functions and adds testing of shutdown commands of the guest agent. --- tests/qemuagenttest.c | 119 +++++++++++++++++++++++++++++++++++++++++++ tests/qemumonitortestutils.c | 21 ++++---- tests/qemumonitortestutils.h | 28 ++++++++-- 3 files changed, 153 insertions(+), 15 deletions(-)
+++ b/tests/qemumonitortestutils.c @@ -37,12 +37,6 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-typedef struct _qemuMonitorTestItem qemuMonitorTestItem; -typedef qemuMonitorTestItem *qemuMonitorTestItemPtr; -typedef int (*qemuMonitorTestResponseCallback)(qemuMonitorTestPtr test, - qemuMonitorTestItemPtr item, - const char *message); -
Hmm - this series is what added qemuMonitorTestResponseCallback, so this feels like churn. You should probably amend patch 10/20 to declare qemuMonitorTestResponseCallback in the .h in the first place, and move the typedefs at that time.
@@ -167,7 +161,6 @@ cleanup: }
- static int
Definitely some churn going on.
@@ -414,6 +407,12 @@ error: return -1; }
+void * +qemuMonitorTestItemGetPrivateData(qemuMonitorTestItemPtr item)
Missing the typical two blank lines. Even after you hoist hunks to the proper earlier patches, this still feels like two patches rolled into one - I'd split it into the exporting of useful helper functions, then the new agent test that takes advantage of the helpers. At any rate, the test addition itself seems sane, and my complaints are only about presentation of the commit contents. ACK to the end result, and rebasing seems like something you can safely do without me needing to see a v2, as long as you test that each step compiles and passes 'make check'. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/30/2013 09:05 AM, Peter Krempa wrote:
This patch exports a few utility functions and adds testing of shutdown commands of the guest agent. ---
<...snip...>
+ +static int +qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *cmdstr) +{ + struct qemuAgentShutdownTestData *data; + virJSONValuePtr val = NULL; + virJSONValuePtr args; + const char *cmdname; + const char *mode; + int ret = -1; + + data = qemuMonitorTestItemGetPrivateData(item); + + if (!(val = virJSONValueFromString(cmdstr))) + return -1; + + if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { + ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + goto cleanup; + } + + if (STRNEQ(cmdname, "guest-shutdown")) { + ret = qemuMonitorTestAddUnexpectedErrorResponse(test); + goto cleanup; + } + + if (!(args = virJSONValueObjectGet(val, "arguments"))) { + ret = qemuMonitorReportError(test, + "Missing arguments section"); + goto cleanup; + } +
Coverity complains that 'mode' isn't used - while a silly complaint is there anything that could be done to just compare "mode" against an "expected" (or perhaps unexpected value)? 247 } 248 (1) Event returned_pointer: Pointer "mode" returned by "virJSONValueObjectGetString(args, "mode")" is never used. 249 if (!(mode = virJSONValueObjectGetString(args, "mode"))) { 250 ret = qemuMonitorReportError(test, "Missing shutdown mode");
+ if (!(mode = virJSONValueObjectGetString(args, "mode"))) { + ret = qemuMonitorReportError(test, "Missing shutdown mode"); + goto cleanup; + } + + /* now don't reply but return a qemu agent event */ + qemuAgentNotifyEvent(qemuMonitorTestGetAgent(test), + data->event); + + ret = 0; + +cleanup: + virJSONValueFree(val); + return ret; + +}
On 08/01/13 13:39, John Ferlan wrote:
On 07/30/2013 09:05 AM, Peter Krempa wrote:
This patch exports a few utility functions and adds testing of shutdown commands of the guest agent. ---
<...snip...>
+ +static int +qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *cmdstr) +{ + struct qemuAgentShutdownTestData *data; + virJSONValuePtr val = NULL; + virJSONValuePtr args; + const char *cmdname; + const char *mode; + int ret = -1; + + data = qemuMonitorTestItemGetPrivateData(item); + + if (!(val = virJSONValueFromString(cmdstr))) + return -1; + + if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { + ret = qemuMonitorReportError(test, "Missing command name in %s", cmdstr); + goto cleanup; + } + + if (STRNEQ(cmdname, "guest-shutdown")) { + ret = qemuMonitorTestAddUnexpectedErrorResponse(test); + goto cleanup; + } + + if (!(args = virJSONValueObjectGet(val, "arguments"))) { + ret = qemuMonitorReportError(test, + "Missing arguments section"); + goto cleanup; + } +
Coverity complains that 'mode' isn't used - while a silly complaint is there anything that could be done to just compare "mode" against an "expected" (or perhaps unexpected value)?
247 } 248
(1) Event returned_pointer: Pointer "mode" returned by "virJSONValueObjectGetString(args, "mode")" is never used.
249 if (!(mode = virJSONValueObjectGetString(args, "mode"))) { 250 ret = qemuMonitorReportError(test, "Missing shutdown mode");
Oh right, I was intending to do that check but forgot to write the condition. I'll post a fix in a while.
+ if (!(mode = virJSONValueObjectGetString(args, "mode"))) { + ret = qemuMonitorReportError(test, "Missing shutdown mode"); + goto cleanup; + } + + /* now don't reply but return a qemu agent event */ + qemuAgentNotifyEvent(qemuMonitorTestGetAgent(test), + data->event); + + ret = 0; + +cleanup: + virJSONValueFree(val); + return ret; + +}
Peter
Test arbitrary qemu commands and timeouting of the guest agent synchronisation. --- tests/qemuagenttest.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index e314bb0..81c24f2 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -326,6 +326,105 @@ cleanup: static int +qemuAgentTimeoutTestMonitorHandler(qemuMonitorTestPtr test ATTRIBUTE_UNUSED, + qemuMonitorTestItemPtr item ATTRIBUTE_UNUSED, + const char *cmdstr ATTRIBUTE_UNUSED) +{ + return 0; +} + +static const char testQemuAgentArbitraryCommandResponse[] = + "{\"return\":\"bla\"}"; + +static int +testQemuAgentArbitraryCommand(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + int ret = -1; + char *reply = NULL; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "ble", + testQemuAgentArbitraryCommandResponse) < 0) + goto cleanup; + + if (qemuAgentArbitraryCommand(qemuMonitorTestGetAgent(test), + "{\"execute\":\"ble\"}", + &reply, + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) + goto cleanup; + + if (STRNEQ(reply, testQemuAgentArbitraryCommandResponse)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "invalid processing of guest agent reply: " + "got '%s' expected '%s'", + reply, testQemuAgentArbitraryCommandResponse); + goto cleanup; + } + + VIR_FREE(reply); + + /* test timeout */ + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddHandler(test, qemuAgentTimeoutTestMonitorHandler, + NULL, NULL) < 0) + goto cleanup; + + if (qemuAgentArbitraryCommand(qemuMonitorTestGetAgent(test), + "{\"execute\":\"ble\"}", + &reply, + 1) != -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "agent command didn't time out"); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(reply); + qemuMonitorTestFree(test); + return ret; +} + + +static int +testQemuAgentTimeout(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddHandler(test, qemuAgentTimeoutTestMonitorHandler, + NULL, NULL) < 0) + goto cleanup; + + if (qemuAgentFSFreeze(qemuMonitorTestGetAgent(test)) != -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "agent command should have failed"); + goto cleanup; + } + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -351,6 +450,8 @@ mymain(void) DO_TEST(FSTrim); DO_TEST(Suspend); DO_TEST(Shutdown); + DO_TEST(ArbitraryCommand); + DO_TEST(Timeout); virObjectUnref(xmlopt); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Test arbitrary qemu commands and timeouting of the guest agent
s/timeouting/timeouts/ (also in the subject)
synchronisation. --- tests/qemuagenttest.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+)
Hmm, I wonder if it's worth adding some sort of escape hatch, maybe if 'VIR_TEST_FAST' is defined to non-empty in the environment, then we skip this test; developers that don't like the long wait can then export that variable as 1, whereas the spec file can ensure it is 0. That could be a followup patch, though, and it might be worth getting more feedback than just mine on whether the new long-running test needs tweaking to allow developers to avoid waiting, while still avoiding bit-rotting of the test at release time. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jul 30, 2013 at 11:27:49AM -0600, Eric Blake wrote:
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Test arbitrary qemu commands and timeouting of the guest agent
s/timeouting/timeouts/ (also in the subject)
synchronisation. --- tests/qemuagenttest.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+)
Hmm, I wonder if it's worth adding some sort of escape hatch, maybe if 'VIR_TEST_FAST' is defined to non-empty in the environment, then we skip this test; developers that don't like the long wait can then export that variable as 1, whereas the spec file can ensure it is 0. That could be a followup patch, though, and it might be worth getting more feedback than just mine on whether the new long-running test needs tweaking to allow developers to avoid waiting, while still avoiding bit-rotting of the test at release time.
IMHO we don't want any of the tests doing multi-second timeouts by default. IOW, rather than VIR_TEST_FAST, we should have a VIR_TEST_ALLOW_SLEEP=1, and ensure that libvirt.spec.in sets that when doing 'make check' and also make sure that autobuild.sh sets it. So all automated builds fully exercise the tests, but day to day usage isn't delayed
ACK.
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/30/2013 12:52 PM, Daniel P. Berrange wrote:
Hmm, I wonder if it's worth adding some sort of escape hatch, maybe if 'VIR_TEST_FAST' is defined to non-empty in the environment, then we skip this test; developers that don't like the long wait can then export that variable as 1, whereas the spec file can ensure it is 0. That could be a followup patch, though, and it might be worth getting more feedback than just mine on whether the new long-running test needs tweaking to allow developers to avoid waiting, while still avoiding bit-rotting of the test at release time.
IMHO we don't want any of the tests doing multi-second timeouts by default. IOW, rather than VIR_TEST_FAST, we should have a VIR_TEST_ALLOW_SLEEP=1, and ensure that libvirt.spec.in sets that when doing 'make check' and also make sure that autobuild.sh sets it. So all automated builds fully exercise the tests, but day to day usage isn't delayed
For that matter, 'make check' for day-to-day usage should be able to skip the gnulib subdirectory - the results in gnulib/tests will only change if you upgrade the gnulib submodule, glibc, or some other core component, which is not what we change on a day-to-day basis when hacking gnulib, but is also something an autobuilder should be running always. I'll see if I can hack something up to speed up 'make check' for normal users on the gnulib front, which we can then extend into skipping Peter's new test. GNU coreutils calls its variable RUN_EXPENSIVE_TESTS, defaulting to no, but set to yes in autobuilders. Sounds like the best type of naming (maybe VIR_TEST_EXPENSIVE, to keep it in the VIR_ namespace). Anyone else want to chime in with a bikeshed color? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jul 30, 2013 at 01:01:56PM -0600, Eric Blake wrote:
On 07/30/2013 12:52 PM, Daniel P. Berrange wrote:
Hmm, I wonder if it's worth adding some sort of escape hatch, maybe if 'VIR_TEST_FAST' is defined to non-empty in the environment, then we skip this test; developers that don't like the long wait can then export that variable as 1, whereas the spec file can ensure it is 0. That could be a followup patch, though, and it might be worth getting more feedback than just mine on whether the new long-running test needs tweaking to allow developers to avoid waiting, while still avoiding bit-rotting of the test at release time.
IMHO we don't want any of the tests doing multi-second timeouts by default. IOW, rather than VIR_TEST_FAST, we should have a VIR_TEST_ALLOW_SLEEP=1, and ensure that libvirt.spec.in sets that when doing 'make check' and also make sure that autobuild.sh sets it. So all automated builds fully exercise the tests, but day to day usage isn't delayed
For that matter, 'make check' for day-to-day usage should be able to skip the gnulib subdirectory - the results in gnulib/tests will only change if you upgrade the gnulib submodule, glibc, or some other core component, which is not what we change on a day-to-day basis when hacking gnulib, but is also something an autobuilder should be running always. I'll see if I can hack something up to speed up 'make check' for normal users on the gnulib front, which we can then extend into skipping Peter's new test.
Good idea to skip gnulib tests.
GNU coreutils calls its variable RUN_EXPENSIVE_TESTS, defaulting to no, but set to yes in autobuilders. Sounds like the best type of naming (maybe VIR_TEST_EXPENSIVE, to keep it in the VIR_ namespace). Anyone else want to chime in with a bikeshed color?
That sounds like a fine name to me. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/31/13 11:28, Daniel P. Berrange wrote:
On Tue, Jul 30, 2013 at 01:01:56PM -0600, Eric Blake wrote:
On 07/30/2013 12:52 PM, Daniel P. Berrange wrote:
Hmm, I wonder if it's worth adding some sort of escape hatch, maybe if 'VIR_TEST_FAST' is defined to non-empty in the environment, then we skip this test; developers that don't like the long wait can then export that variable as 1, whereas the spec file can ensure it is 0. That could be a followup patch, though, and it might be worth getting more feedback than just mine on whether the new long-running test needs tweaking to allow developers to avoid waiting, while still avoiding bit-rotting of the test at release time.
IMHO we don't want any of the tests doing multi-second timeouts by default. IOW, rather than VIR_TEST_FAST, we should have a VIR_TEST_ALLOW_SLEEP=1, and ensure that libvirt.spec.in sets that when doing 'make check' and also make sure that autobuild.sh sets it. So all automated builds fully exercise the tests, but day to day usage isn't delayed
For that matter, 'make check' for day-to-day usage should be able to skip the gnulib subdirectory - the results in gnulib/tests will only change if you upgrade the gnulib submodule, glibc, or some other core component, which is not what we change on a day-to-day basis when hacking gnulib, but is also something an autobuilder should be running always. I'll see if I can hack something up to speed up 'make check' for normal users on the gnulib front, which we can then extend into skipping Peter's new test.
Good idea to skip gnulib tests.
GNU coreutils calls its variable RUN_EXPENSIVE_TESTS, defaulting to no, but set to yes in autobuilders. Sounds like the best type of naming (maybe VIR_TEST_EXPENSIVE, to keep it in the VIR_ namespace). Anyone else want to chime in with a bikeshed color?
That sounds like a fine name to me.
I like it too.
Daniel
I pushed the rest of the series with addressing review comments except for this patch and I will now try to figure out the right way to skip expensive tests. Thanks for your input. Peter
On 07/31/2013 06:59 AM, Peter Krempa wrote:
For that matter, 'make check' for day-to-day usage should be able to skip the gnulib subdirectory - the results in gnulib/tests will only change if you upgrade the gnulib submodule, glibc, or some other core component, which is not what we change on a day-to-day basis when hacking gnulib, but is also something an autobuilder should be running always. I'll see if I can hack something up to speed up 'make check' for normal users on the gnulib front, which we can then extend into skipping Peter's new test.
Good idea to skip gnulib tests.
GNU coreutils calls its variable RUN_EXPENSIVE_TESTS, defaulting to no, but set to yes in autobuilders. Sounds like the best type of naming (maybe VIR_TEST_EXPENSIVE, to keep it in the VIR_ namespace). Anyone else want to chime in with a bikeshed color?
That sounds like a fine name to me.
I like it too.
v1 patch proposed: https://www.redhat.com/archives/libvir-list/2013-July/msg01986.html For the sake of Peter's test, I think that a getenv("VIR_TEST_EXPENSIVE") in the C code will be easier to implement than Makefile hackery, except that my patch didn't hack the makefile to guarantee that VIR_TEST_EXPENSIVE will be in TESTS_ENVIRONMENT if configure requested enabling tests. (Hmm, this alone makes me think I should make the configure option and environment variable name match, rather than having --enable-gnulib-tests vs. VIR_TEST_EXPENSIVE, so maybe I need a v2 after all) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/30/2013 11:27 AM, Eric Blake wrote:
On 07/30/2013 07:05 AM, Peter Krempa wrote:
Test arbitrary qemu commands and timeouting of the guest agent
s/timeouting/timeouts/ (also in the subject)
synchronisation. --- tests/qemuagenttest.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+)
Hmm, I wonder if it's worth adding some sort of escape hatch, maybe if 'VIR_TEST_FAST' is defined to non-empty in the environment, then we skip this test; developers that don't like the long wait can then export that variable as 1, whereas the spec file can ensure it is 0. That could be a followup patch, though, and it might be worth getting more feedback than just mine on whether the new long-running test needs tweaking to allow developers to avoid waiting, while still avoiding bit-rotting of the test at release time.
In testing this patch, I'll point out that I was using new enough automake that gives us parallel testing, and a command like 'make -j3 check' did not see ANY appreciable delay - we spend more than six seconds on other tests, such that the sleep time of this new test was mostly offset by the other tests running in parallel. Still, it's probably worth an option for pruning this and other sleeps from the testsuite when run by a developer. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tests/qemuagenttest.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 81c24f2..33a6d73 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -423,6 +423,124 @@ cleanup: return ret; } +static const char testQemuAgentCPUResponse[] = + "{\"return\": " + " [" + " {\"online\": true," + " \"can-offline\": false," + " \"logical-id\": 0" + " }," + " {\"online\": true," + " \"can-offline\": true," + " \"logical-id\": 1" + " }," + " {\"online\": true," + " \"can-offline\": true," + " \"logical-id\": 2" + " }," + " {\"online\": false," + " \"can-offline\": true," + " \"logical-id\": 3" + " }" + " ]" + "}"; + +static const char testQemuAgentCPUArguments1[] = + "[{\"logical-id\":0,\"online\":true}," + "{\"logical-id\":1,\"online\":false}," + "{\"logical-id\":2,\"online\":true}," + "{\"logical-id\":3,\"online\":false}]"; + +static const char testQemuAgentCPUArguments2[] = + "[{\"logical-id\":0,\"online\":true}," + "{\"logical-id\":1,\"online\":true}," + "{\"logical-id\":2,\"online\":true}," + "{\"logical-id\":3,\"online\":true}]"; + +static int +testQemuAgentCPU(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + qemuAgentCPUInfoPtr cpuinfo = NULL; + int nvcpus; + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-get-vcpus", + testQemuAgentCPUResponse) < 0) + goto cleanup; + + /* get cpus */ + if ((nvcpus = qemuAgentGetVCPUs(qemuMonitorTestGetAgent(test), + &cpuinfo)) < 0) + goto cleanup; + + if (nvcpus != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected '4' cpus, got '%d'", nvcpus); + goto cleanup; + } + + /* try to unplug one */ + if (qemuAgentUpdateCPUInfo(2, cpuinfo, nvcpus) < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus", + "{ \"return\" : 4 }", + "vcpus", testQemuAgentCPUArguments1, + NULL) < 0) + goto cleanup; + + if ((nvcpus = qemuAgentSetVCPUs(qemuMonitorTestGetAgent(test), + cpuinfo, nvcpus)) < 0) + goto cleanup; + + if (nvcpus != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected '4' cpus updated , got '%d'", nvcpus); + goto cleanup; + } + + /* try to hotplug two */ + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus", + "{ \"return\" : 4 }", + "vcpus", testQemuAgentCPUArguments2, + NULL) < 0) + goto cleanup; + + if (qemuAgentUpdateCPUInfo(4, cpuinfo, nvcpus) < 0) + goto cleanup; + + if ((nvcpus = qemuAgentSetVCPUs(qemuMonitorTestGetAgent(test), + cpuinfo, nvcpus)) < 0) + goto cleanup; + + if (nvcpus != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected '4' cpus updated , got '%d'", nvcpus); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(cpuinfo); + qemuMonitorTestFree(test); + return ret; +} + static int mymain(void) @@ -452,6 +570,7 @@ mymain(void) DO_TEST(Shutdown); DO_TEST(ArbitraryCommand); DO_TEST(Timeout); + DO_TEST(CPU); virObjectUnref(xmlopt); -- 1.8.3.2
On 07/30/2013 07:05 AM, Peter Krempa wrote:
--- tests/qemuagenttest.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jul 30, 2013 at 03:05:35PM +0200, Peter Krempa wrote:
As promised earlier I'm sending a unit test for guest agent interaction.
This series contains a few refactors and additions of monitor test utils and then add tests for all agent interaction functions. The refactors done in this series will allow to do a more thorough testing on the json monitor too.
The small drawback of this test suite is a 6 second run time introduced to test timeout of a guest agent command. The test may be removed in case it's too much.
Peter Krempa (20): conf: Export virDomainChrSourceDefClear() qemu_agent: Output newline at the end of the sync JSON message qemu_agent: Move updater function for VCPU hotplug into qemu_agent.c qemu_agent: Remove obvious comments qemumonitortestutils: Use consistent header style and line spacing qemumonitortestutils: Use VIR_DELETE_ELEMENT and VIR_APPEND_ELEMENT qemumonitortestutils: remove multiline function calls qemumonitortestutils: Don't crash on non fully initialized test qemumonitortestutils: Split up creation of the test to allow reuse qemumonitortestutils: Refactor the test helpers to allow reuse qemumonitortestutils: Split lines on \n instead of \r\n qemumonitortestutils: Add instrumentation for guest agent testing qemumonitortestutils: Improve error reporting from mock qemu monitor qemumonitortestutils: Add the ability to check arguments of commands tests: Add qemuagenttest qemuagenttest: Test the filesystem trimming qemuagenttest: Add testing of agent suspend modes qemuagenttest: Introduce testing of shutdown commands qemuagenttest: Test arbitrary qemu commands and timeouting of commands qemuagenttest: Add tests for CPU plug functions and helpers
.gitignore | 1 + src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 73 ++++- src/qemu/qemu_agent.h | 3 + src/qemu/qemu_driver.c | 64 +--- tests/Makefile.am | 11 +- tests/qemuagenttest.c | 580 ++++++++++++++++++++++++++++++++++++ tests/qemumonitortestutils.c | 684 +++++++++++++++++++++++++++++++++---------- tests/qemumonitortestutils.h | 39 ++- 11 files changed, 1231 insertions(+), 229 deletions(-) create mode 100644 tests/qemuagenttest.c
Great job on this test suite - it turned out very nicely. Good that we have coverage of this code, since handling data from untrusted guests is security critical Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Peter Krempa