[libvirt] [PATCH 0/6] qemu: fix UDP chardev hotplug and make qemumonitorjsontest less useless
Most of the time qemumonitorjson test isn't really testing much. Add support for actually testing chardev hotplug and fix it for the UDP case. Peter Krempa (6): conf: Sanitize formatting of UDP chardev source tests: qemu: Add support for testing aguments on monitor verbatim tests: qemumonitorjson: Don't do multiple tests in one virTestRun tests: qemumonitorjsontest: Do some actual testing in qemuMonitorJSONTestAttachChardev qemu: monitor: Simplify construction of chardev backends qemu: monitor: Properly configure backend for UDP chardevs src/conf/domain_conf.c | 42 ++++---- src/qemu/qemu_monitor_json.c | 51 +++++----- tests/qemumonitorjsontest.c | 222 ++++++++++++++++++++++++++++++++----------- tests/qemumonitortestutils.c | 112 ++++++++++++++++++++++ tests/qemumonitortestutils.h | 6 ++ 5 files changed, 326 insertions(+), 107 deletions(-) -- 2.10.0
Use much simpler logic to determine parts of the code to print. --- src/conf/domain_conf.c | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a06da46..3f34540 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21431,32 +21431,22 @@ virDomainChrSourceDefFormat(virBufferPtr buf, break; case VIR_DOMAIN_CHR_TYPE_UDP: - if (def->data.udp.bindService && - def->data.udp.bindHost) { - virBufferEscapeString(buf, "<source mode='bind' host='%s' ", - def->data.udp.bindHost); - virBufferEscapeString(buf, "service='%s'/>\n", - def->data.udp.bindService); - } else if (def->data.udp.bindHost) { - virBufferEscapeString(buf, "<source mode='bind' host='%s'/>\n", - def->data.udp.bindHost); - } else if (def->data.udp.bindService) { - virBufferEscapeString(buf, "<source mode='bind' service='%s'/>\n", - def->data.udp.bindService); - } - - if (def->data.udp.connectService && - def->data.udp.connectHost) { - virBufferEscapeString(buf, "<source mode='connect' host='%s' ", - def->data.udp.connectHost); - virBufferEscapeString(buf, "service='%s'/>\n", - def->data.udp.connectService); - } else if (def->data.udp.connectHost) { - virBufferEscapeString(buf, "<source mode='connect' host='%s'/>\n", - def->data.udp.connectHost); - } else if (def->data.udp.connectService) { - virBufferEscapeString(buf, "<source mode='connect' service='%s'/>\n", - def->data.udp.connectService); + if (def->data.udp.bindService || def->data.udp.bindHost) { + virBufferAddLit(buf, "<source mode='bind'"); + if (def->data.udp.bindService) + virBufferEscapeString(buf, " host='%s'", def->data.udp.bindHost); + if (def->data.udp.bindService) + virBufferEscapeString(buf, " service='%s'", def->data.udp.bindService); + virBufferAddLit(buf, "/>\n"); + } + + if (def->data.udp.connectService || def->data.udp.connectHost) { + virBufferAddLit(buf, "<source mode='connect'"); + if (def->data.udp.connectService) + virBufferEscapeString(buf, " host='%s'", def->data.udp.connectHost); + if (def->data.udp.connectService) + virBufferEscapeString(buf, " service='%s'", def->data.udp.connectService); + virBufferAddLit(buf, "/>\n"); } break; -- 2.10.0
On 09/27/2016 12:39 PM, Peter Krempa wrote:
Use much simpler logic to determine parts of the code to print. --- src/conf/domain_conf.c | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-)
ACK John
Add code that takes a string and matches it against the data passed as arguments from qemu. This is a simpler version of qemuMonitorTestAddItemParams. --- tests/qemumonitortestutils.c | 112 +++++++++++++++++++++++++++++++++++++++++++ tests/qemumonitortestutils.h | 6 +++ 2 files changed, 118 insertions(+) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index c86a27a..50bf174 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -439,6 +439,7 @@ struct qemuMonitorTestHandlerData { char *response; size_t nargs; qemuMonitorTestCommandArgsPtr args; + char *expectArgs; }; static void @@ -458,6 +459,7 @@ qemuMonitorTestHandlerDataFree(void *opaque) VIR_FREE(data->command_name); VIR_FREE(data->response); VIR_FREE(data->args); + VIR_FREE(data->expectArgs); VIR_FREE(data); } @@ -668,6 +670,7 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, } + /* this allows to add a responder that is able to check * a (shallow) structure of arguments for a command */ int @@ -721,6 +724,115 @@ qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, } +static int +qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *cmdstr) +{ + struct qemuMonitorTestHandlerData *data = item->opaque; + virJSONValuePtr val = NULL; + virJSONValuePtr args; + char *argstr = NULL; + const char *cmdname; + 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; + } + + /* convert the arguments to string */ + if (!(argstr = virJSONValueToString(args, false))) + goto cleanup; + + /* verify that the argument value is expected */ + if (STRNEQ(argstr, data->expectArgs)) { + ret = qemuMonitorReportError(test, + "%s: expected arguments: '%s', got: '%s'", + data->command_name, + data->expectArgs, argstr); + goto cleanup; + } + + + VIR_FREE(argstr); + + /* arguments checked out, return the response */ + ret = qemuMonitorTestAddResponse(test, data->response); + + cleanup: + VIR_FREE(argstr); + virJSONValueFree(val); + return ret; +} + + +/** + * qemuMonitorTestAddItemExpect: + * + * @test: test monitor object + * @cmdname: command name + * @cmdargs: expected arguments of the command + * @apostrophe: convert apostrophes (') in @cmdargs to quotes (") + * @response: simulated response of the command + * + * Simulates a qemu monitor command. Checks that the 'arguments' of the qmp + * command are expected. If @apostrophe is true apostrophes are converted to + * quotes for simplification of writing the strings into code. + */ +int +qemuMonitorTestAddItemExpect(qemuMonitorTestPtr test, + const char *cmdname, + const char *cmdargs, + bool apostrophe, + const char *response) +{ + struct qemuMonitorTestHandlerData *data; + + if (VIR_ALLOC(data) < 0) + goto error; + + if (VIR_STRDUP(data->command_name, cmdname) < 0 || + VIR_STRDUP(data->response, response) < 0 || + VIR_STRDUP(data->expectArgs, cmdargs) < 0) + goto error; + + if (apostrophe) { + char *tmp = data->expectArgs; + + while (*tmp != '\0') { + if (*tmp == '\'') + *tmp = '"'; + + tmp++; + } + } + + return qemuMonitorTestAddHandler(test, + qemuMonitorTestProcessCommandWithArgStr, + data, qemuMonitorTestHandlerDataFree); + + error: + 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 8e2f371..3890cd4 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -60,6 +60,12 @@ int qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, ...) ATTRIBUTE_SENTINEL; +int qemuMonitorTestAddItemExpect(qemuMonitorTestPtr test, + const char *cmdname, + const char *cmdargs, + bool apostrophe, + const char *response); + # define qemuMonitorTestNewSimple(json, xmlopt) \ qemuMonitorTestNew(json, xmlopt, NULL, NULL, NULL) -- 2.10.0
On 09/27/2016 12:39 PM, Peter Krempa wrote:
Add code that takes a string and matches it against the data passed as arguments from qemu. This is a simpler version of qemuMonitorTestAddItemParams. --- tests/qemumonitortestutils.c | 112 +++++++++++++++++++++++++++++++++++++++++++ tests/qemumonitortestutils.h | 6 +++ 2 files changed, 118 insertions(+)
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index c86a27a..50bf174 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -439,6 +439,7 @@ struct qemuMonitorTestHandlerData { char *response; size_t nargs; qemuMonitorTestCommandArgsPtr args; + char *expectArgs; };
static void @@ -458,6 +459,7 @@ qemuMonitorTestHandlerDataFree(void *opaque) VIR_FREE(data->command_name); VIR_FREE(data->response); VIR_FREE(data->args); + VIR_FREE(data->expectArgs); VIR_FREE(data); }
@@ -668,6 +670,7 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr test, }
+
Technically extra line, but I'll assume it's making possible for the 2 lines between functions...
/* this allows to add a responder that is able to check * a (shallow) structure of arguments for a command */ int @@ -721,6 +724,115 @@ qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, }
+static int +qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTestPtr test, + qemuMonitorTestItemPtr item, + const char *cmdstr) +{ + struct qemuMonitorTestHandlerData *data = item->opaque; + virJSONValuePtr val = NULL; + virJSONValuePtr args; + char *argstr = NULL; + const char *cmdname; + 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; + } + + /* convert the arguments to string */ + if (!(argstr = virJSONValueToString(args, false))) + goto cleanup; + + /* verify that the argument value is expected */ + if (STRNEQ(argstr, data->expectArgs)) { + ret = qemuMonitorReportError(test, + "%s: expected arguments: '%s', got: '%s'", + data->command_name, + data->expectArgs, argstr); + goto cleanup; + } + + + VIR_FREE(argstr);
This one's duplicitous since we fall through to cleanup
+ + /* arguments checked out, return the response */ + ret = qemuMonitorTestAddResponse(test, data->response); + + cleanup: + VIR_FREE(argstr); + virJSONValueFree(val); + return ret; +} + + +/** + * qemuMonitorTestAddItemExpect: + * + * @test: test monitor object + * @cmdname: command name + * @cmdargs: expected arguments of the command + * @apostrophe: convert apostrophes (') in @cmdargs to quotes (") + * @response: simulated response of the command + * + * Simulates a qemu monitor command. Checks that the 'arguments' of the qmp + * command are expected. If @apostrophe is true apostrophes are converted to + * quotes for simplification of writing the strings into code. + */ +int +qemuMonitorTestAddItemExpect(qemuMonitorTestPtr test, + const char *cmdname, + const char *cmdargs, + bool apostrophe, + const char *response) +{ + struct qemuMonitorTestHandlerData *data; + + if (VIR_ALLOC(data) < 0) + goto error; + + if (VIR_STRDUP(data->command_name, cmdname) < 0 || + VIR_STRDUP(data->response, response) < 0 || + VIR_STRDUP(data->expectArgs, cmdargs) < 0) + goto error; + + if (apostrophe) { + char *tmp = data->expectArgs; + + while (*tmp != '\0') { + if (*tmp == '\'') + *tmp = '"'; + + tmp++; + }
Would there ever be a time when apostrophe would be false? ACK with at least the duplicitous free resolved. Your call on the above loop. John
+ } + + return qemuMonitorTestAddHandler(test, + qemuMonitorTestProcessCommandWithArgStr, + data, qemuMonitorTestHandlerDataFree); + + error: + 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 8e2f371..3890cd4 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -60,6 +60,12 @@ int qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, ...) ATTRIBUTE_SENTINEL;
+int qemuMonitorTestAddItemExpect(qemuMonitorTestPtr test, + const char *cmdname, + const char *cmdargs, + bool apostrophe, + const char *response); + # define qemuMonitorTestNewSimple(json, xmlopt) \ qemuMonitorTestNew(json, xmlopt, NULL, NULL, NULL)
On Wed, Oct 05, 2016 at 09:37:50 -0400, John Ferlan wrote:
On 09/27/2016 12:39 PM, Peter Krempa wrote:
Add code that takes a string and matches it against the data passed as arguments from qemu. This is a simpler version of qemuMonitorTestAddItemParams. --- tests/qemumonitortestutils.c | 112 +++++++++++++++++++++++++++++++++++++++++++ tests/qemumonitortestutils.h | 6 +++ 2 files changed, 118 insertions(+)
[...]
+/** + * qemuMonitorTestAddItemExpect: + * + * @test: test monitor object + * @cmdname: command name + * @cmdargs: expected arguments of the command + * @apostrophe: convert apostrophes (') in @cmdargs to quotes (") + * @response: simulated response of the command + * + * Simulates a qemu monitor command. Checks that the 'arguments' of the qmp + * command are expected. If @apostrophe is true apostrophes are converted to + * quotes for simplification of writing the strings into code. + */ +int +qemuMonitorTestAddItemExpect(qemuMonitorTestPtr test, + const char *cmdname, + const char *cmdargs, + bool apostrophe, + const char *response) +{ + struct qemuMonitorTestHandlerData *data; + + if (VIR_ALLOC(data) < 0) + goto error; + + if (VIR_STRDUP(data->command_name, cmdname) < 0 || + VIR_STRDUP(data->response, response) < 0 || + VIR_STRDUP(data->expectArgs, cmdargs) < 0) + goto error; + + if (apostrophe) { + char *tmp = data->expectArgs; + + while (*tmp != '\0') { + if (*tmp == '\'') + *tmp = '"'; + + tmp++; + }
Would there ever be a time when apostrophe would be false?
It certainly can be. If you construct a proper JSON string, then basically everything is enclosed in quotes. But those are very hard to do in C code, thus you can cut the corner by using apostrophes. This won't work though if you e.g. want to send a string containing an apostrophe, thus you will need to do it properly and set @apostrophe to false.
The chardev attach test would do all the tests in one virTestRun instance. If one sub-test failed then the test would report failure improperly and the error would be hard to debug since the error pointer was overwritten. --- tests/qemumonitorjsontest.c | 147 ++++++++++++++++++++++++++++++-------------- 1 file changed, 102 insertions(+), 45 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 4e7bb71..23877f8 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -705,87 +705,143 @@ testQemuMonitorJSONGetCommandLineOptionParameters(const void *data) return ret; } + +struct qemuMonitorJSONTestAttachChardevData { + qemuMonitorTestPtr test; + virDomainChrSourceDefPtr chr; + const char *expectPty; + bool fail; +}; + static int -testQemuMonitorJSONAttachChardev(const void *data) +testQemuMonitorJSONAttachChardev(const void *opaque) { - virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; - qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); - virDomainChrSourceDef chr; - int ret = 0; + const struct qemuMonitorJSONTestAttachChardevData *data = opaque; + int rc; - if (!test) + if ((rc = qemuMonitorAttachCharDev(qemuMonitorTestGetMonitor(data->test), + "alias", data->chr)) < 0) + goto cleanup; + + if (data->chr->type == VIR_DOMAIN_CHR_TYPE_PTY) { + if (STRNEQ_NULLABLE(data->expectPty, data->chr->data.file.path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected PTY path: %s got: %s", + NULLSTR(data->expectPty), + NULLSTR(data->chr->data.file.path)); + rc = -1; + } + + VIR_FREE(data->chr->data.file.path); + } + + cleanup: + if ((rc != 0) != data->fail) return -1; + else + return 0; +} -#define DO_CHECK(chrID, reply, fail) \ - if (qemuMonitorTestAddItem(test, "chardev-add", reply) < 0) \ - goto cleanup; \ - if (qemuMonitorAttachCharDev(qemuMonitorTestGetMonitor(test), \ - chrID, &chr) < 0) \ - ret = fail ? ret : -1; \ - else \ - ret = fail ? -1 : ret; \ -#define CHECK(chrID, reply) \ - DO_CHECK(chrID, reply, false) +static int +qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, + const char *label, + virDomainChrSourceDefPtr chr, + const char *reply, + const char *expectPty, + bool fail) -#define CHECK_FAIL(chrID, reply) \ - DO_CHECK(chrID, reply, true) +{ + struct qemuMonitorJSONTestAttachChardevData data; + char *jsonreply = NULL; + char *fulllabel = NULL; + int ret = -1; + + if (!reply) + reply = ""; + + if (virAsprintf(&jsonreply, "{\"return\": {%s}}", reply) < 0) + goto cleanup; + + if (virAsprintf(&fulllabel, "qemuMonitorJSONTestAttachChardev(%s)", label) < 0) + goto cleanup; + + data.chr = chr; + data.fail = fail; + data.expectPty = expectPty; + if (!(data.test = qemuMonitorTestNewSimple(true, xmlopt))) + goto cleanup; + + if (qemuMonitorTestAddItem(data.test, "chardev-add", jsonreply) < 0) + goto cleanup; + + if (virTestRun(fulllabel, &testQemuMonitorJSONAttachChardev, &data) < 0) + goto cleanup; + + ret = 0; + + cleanup: + qemuMonitorTestFree(data.test); + VIR_FREE(jsonreply); + VIR_FREE(fulllabel); + return ret; +} + +static int +qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr xmlopt) +{ + virDomainChrSourceDef chr; + int ret = 0; + +#define CHECK(label, fail) \ + if (qemuMonitorJSONTestAttachOneChardev(xmlopt, label, &chr, NULL, NULL, \ + fail) < 0) \ + ret = -1 chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_NULL }; - CHECK("chr_null", "{\"return\": {}}"); + CHECK("null", false); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_VC }; - CHECK("chr_vc", "{\"return\": {}}"); + CHECK("vc", false); -#define PTY_PATH "/dev/ttyS0" chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - CHECK("chr_pty", "{\"return\": {\"pty\" : \"" PTY_PATH "\"}}"); - if (STRNEQ_NULLABLE(PTY_PATH, chr.data.file.path)) { - VIR_FREE(chr.data.file.path); - virReportError(VIR_ERR_INTERNAL_ERROR, - "expected PTY path: %s got: %s", - PTY_PATH, NULLSTR(chr.data.file.path)); + if (qemuMonitorJSONTestAttachOneChardev(xmlopt, "pty", &chr, + "\"pty\" : \"/dev/pts/0\"", + "/dev/pts/0", false) < 0) ret = -1; - } - VIR_FREE(chr.data.file.path); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - CHECK_FAIL("chr_pty_fail", "{\"return\": {}}"); -#undef PTY_PATH + CHECK("pty missing path", true); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_FILE }; - CHECK("chr_file", "{\"return\": {}}"); + CHECK("file", false); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_DEV }; - CHECK("chr_dev", "{\"return\": {}}"); + CHECK("device", false); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_TCP }; - CHECK("chr_tcp", "{\"return\": {}}"); + CHECK("tcp", false); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UDP }; - CHECK("chr_udp", "{\"return\": {}}"); + CHECK("udp", false); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UNIX }; - CHECK("chr_unix", "{\"return\": {}}"); + CHECK("unix", false); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_SPICEVMC }; - CHECK("chr_spicevmc", "{\"return\": {}}"); + CHECK("spicevmc", false); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PIPE }; - CHECK_FAIL("chr_pipe", "{\"return\": {}}"); + CHECK("pipe", true); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_STDIO }; - CHECK_FAIL("chr_stdio", "{\"return\": {}}"); - + CHECK("stdio", true); #undef CHECK -#undef CHECK_FAIL -#undef DO_CHECK - cleanup: - qemuMonitorTestFree(test); return ret; } + static int testQemuMonitorJSONDetachChardev(const void *data) { @@ -2520,7 +2576,8 @@ mymain(void) DO_TEST(GetCommands); DO_TEST(GetTPMModels); DO_TEST(GetCommandLineOptionParameters); - DO_TEST(AttachChardev); + if (qemuMonitorJSONTestAttachChardev(driver.xmlopt) < 0) + ret = -1; DO_TEST(DetachChardev); DO_TEST(GetListPaths); DO_TEST(GetObjectProperty); -- 2.10.0
On 09/27/2016 12:39 PM, Peter Krempa wrote:
The chardev attach test would do all the tests in one virTestRun instance. If one sub-test failed then the test would report failure improperly and the error would be hard to debug since the error pointer was overwritten. --- tests/qemumonitorjsontest.c | 147 ++++++++++++++++++++++++++++++-------------- 1 file changed, 102 insertions(+), 45 deletions(-)
ACK - seems reasonable (not a pleasant diff to read) John
Until now the test was rather useless since it didn't check the arguments formatted and didn't use properly configured chardev objects. Add the expected arguments and instrument the test to validate them. Modify some test cases to actually add valid data. Note that the UDP test data is currently wrong due to a bug. --- tests/qemumonitorjsontest.c | 93 +++++++++++++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 25 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 23877f8..61344b7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -747,6 +747,7 @@ static int qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, const char *label, virDomainChrSourceDefPtr chr, + const char *expectargs, const char *reply, const char *expectPty, bool fail) @@ -772,7 +773,8 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, if (!(data.test = qemuMonitorTestNewSimple(true, xmlopt))) goto cleanup; - if (qemuMonitorTestAddItem(data.test, "chardev-add", jsonreply) < 0) + if (qemuMonitorTestAddItemExpect(data.test, "chardev-add", + expectargs, true, jsonreply) < 0) goto cleanup; if (virTestRun(fulllabel, &testQemuMonitorJSONAttachChardev, &data) < 0) @@ -793,49 +795,90 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr xmlopt) virDomainChrSourceDef chr; int ret = 0; -#define CHECK(label, fail) \ - if (qemuMonitorJSONTestAttachOneChardev(xmlopt, label, &chr, NULL, NULL, \ - fail) < 0) \ +#define CHECK(label, fail, expectargs) \ + if (qemuMonitorJSONTestAttachOneChardev(xmlopt, label, &chr, expectargs, \ + NULL, NULL, fail) < 0) \ ret = -1 chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_NULL }; - CHECK("null", false); + CHECK("null", false, + "{'id':'alias','backend':{'type':'null','data':{}}}"); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_VC }; - CHECK("vc", false); + CHECK("vc", false, + "{'id':'alias','backend':{'type':'null','data':{}}}"); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; if (qemuMonitorJSONTestAttachOneChardev(xmlopt, "pty", &chr, + "{'id':'alias'," + "'backend':{'type':'pty'," + "'data':{}}}", "\"pty\" : \"/dev/pts/0\"", "/dev/pts/0", false) < 0) ret = -1; chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - CHECK("pty missing path", true); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_FILE }; - CHECK("file", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_DEV }; - CHECK("device", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_TCP }; - CHECK("tcp", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UDP }; - CHECK("udp", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UNIX }; - CHECK("unix", false); + CHECK("pty missing path", true, + "{'id':'alias','backend':{'type':'pty','data':{}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_FILE; + chr.data.file.path = (char *) "/test/path"; + CHECK("file", false, + "{'id':'alias','backend':{'type':'file','data':{'out':'/test/path'}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_DEV; + chr.data.file.path = (char *) "/test/path"; + CHECK("device", false, + "{'id':'alias','backend':{'type':'serial','data':{'device':'/test/path'}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_TCP; + chr.data.tcp.host = (char *) "example.com"; + chr.data.tcp.service = (char *) "1234"; + CHECK("tcp", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}," + "'wait':false," + "'telnet':false," + "'server':false}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_UDP; + chr.data.udp.connectHost = (char *) "example.com"; + chr.data.udp.connectService = (char *) "1234"; + CHECK("udp", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; + chr.data.nix.path = (char *) "/path/to/socket"; + CHECK("unix", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'unix'," + "'data':{'path':'/path/to/socket'}}," + "'wait':false," + "'server':false}}}"); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_SPICEVMC }; - CHECK("spicevmc", false); + CHECK("spicevmc", false, + "{'id':'alias','backend':{'type':'spicevmc','" + "data':{'type':'vdagent'}}}"); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PIPE }; - CHECK("pipe", true); + CHECK("pipe", true, ""); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_STDIO }; - CHECK("stdio", true); + CHECK("stdio", true, ""); #undef CHECK return ret; -- 2.10.0
On 09/27/2016 12:39 PM, Peter Krempa wrote:
Until now the test was rather useless since it didn't check the arguments formatted and didn't use properly configured chardev objects.
Add the expected arguments and instrument the test to validate them. Modify some test cases to actually add valid data.
Note that the UDP test data is currently wrong due to a bug. --- tests/qemumonitorjsontest.c | 93 +++++++++++++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 25 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 23877f8..61344b7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -747,6 +747,7 @@ static int qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, const char *label, virDomainChrSourceDefPtr chr, + const char *expectargs, const char *reply, const char *expectPty, bool fail) @@ -772,7 +773,8 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, if (!(data.test = qemuMonitorTestNewSimple(true, xmlopt))) goto cleanup;
- if (qemuMonitorTestAddItem(data.test, "chardev-add", jsonreply) < 0) + if (qemuMonitorTestAddItemExpect(data.test, "chardev-add", + expectargs, true, jsonreply) < 0)
FWIW: The "knowledge" of whether the expectargs has an apostrophe would seem to lie in the caller. If in fact you still keep the apostrophe check in patch 2, then I would think the caller would be able to supply true/false, but that's a minor thing. Also, something that just occurred to me... 'expectargs' had better not be NULL; otherwise, patch 2 will core at the while (*tmp != '\0') ACK in principle - your call on the apostrophe thing - although perhaps a check in patch 2 should be added for (apostrophe && data->expectArgs) to protect from future mishaps... John
goto cleanup;
if (virTestRun(fulllabel, &testQemuMonitorJSONAttachChardev, &data) < 0) @@ -793,49 +795,90 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr xmlopt) virDomainChrSourceDef chr; int ret = 0;
-#define CHECK(label, fail) \ - if (qemuMonitorJSONTestAttachOneChardev(xmlopt, label, &chr, NULL, NULL, \ - fail) < 0) \ +#define CHECK(label, fail, expectargs) \ + if (qemuMonitorJSONTestAttachOneChardev(xmlopt, label, &chr, expectargs, \ + NULL, NULL, fail) < 0) \ ret = -1
chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_NULL }; - CHECK("null", false); + CHECK("null", false, + "{'id':'alias','backend':{'type':'null','data':{}}}");
chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_VC }; - CHECK("vc", false); + CHECK("vc", false, + "{'id':'alias','backend':{'type':'null','data':{}}}");
chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; if (qemuMonitorJSONTestAttachOneChardev(xmlopt, "pty", &chr, + "{'id':'alias'," + "'backend':{'type':'pty'," + "'data':{}}}", "\"pty\" : \"/dev/pts/0\"", "/dev/pts/0", false) < 0) ret = -1;
chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - CHECK("pty missing path", true); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_FILE }; - CHECK("file", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_DEV }; - CHECK("device", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_TCP }; - CHECK("tcp", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UDP }; - CHECK("udp", false); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UNIX }; - CHECK("unix", false); + CHECK("pty missing path", true, + "{'id':'alias','backend':{'type':'pty','data':{}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_FILE; + chr.data.file.path = (char *) "/test/path"; + CHECK("file", false, + "{'id':'alias','backend':{'type':'file','data':{'out':'/test/path'}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_DEV; + chr.data.file.path = (char *) "/test/path"; + CHECK("device", false, + "{'id':'alias','backend':{'type':'serial','data':{'device':'/test/path'}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_TCP; + chr.data.tcp.host = (char *) "example.com"; + chr.data.tcp.service = (char *) "1234"; + CHECK("tcp", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}," + "'wait':false," + "'telnet':false," + "'server':false}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_UDP; + chr.data.udp.connectHost = (char *) "example.com"; + chr.data.udp.connectService = (char *) "1234"; + CHECK("udp", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}}}}"); + + memset(&chr, 0, sizeof(chr)); + chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; + chr.data.nix.path = (char *) "/path/to/socket"; + CHECK("unix", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'unix'," + "'data':{'path':'/path/to/socket'}}," + "'wait':false," + "'server':false}}}");
chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_SPICEVMC }; - CHECK("spicevmc", false); + CHECK("spicevmc", false, + "{'id':'alias','backend':{'type':'spicevmc','" + "data':{'type':'vdagent'}}}");
chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PIPE }; - CHECK("pipe", true); + CHECK("pipe", true, "");
chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_STDIO }; - CHECK("stdio", true); + CHECK("stdio", true, ""); #undef CHECK
return ret;
On Wed, Oct 05, 2016 at 09:38:16 -0400, John Ferlan wrote:
On 09/27/2016 12:39 PM, Peter Krempa wrote:
Until now the test was rather useless since it didn't check the arguments formatted and didn't use properly configured chardev objects.
Add the expected arguments and instrument the test to validate them. Modify some test cases to actually add valid data.
Note that the UDP test data is currently wrong due to a bug. --- tests/qemumonitorjsontest.c | 93 +++++++++++++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 25 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 23877f8..61344b7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -747,6 +747,7 @@ static int qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, const char *label, virDomainChrSourceDefPtr chr, + const char *expectargs, const char *reply, const char *expectPty, bool fail) @@ -772,7 +773,8 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt, if (!(data.test = qemuMonitorTestNewSimple(true, xmlopt))) goto cleanup;
- if (qemuMonitorTestAddItem(data.test, "chardev-add", jsonreply) < 0) + if (qemuMonitorTestAddItemExpect(data.test, "chardev-add", + expectargs, true, jsonreply) < 0)
FWIW: The "knowledge" of whether the expectargs has an apostrophe would seem to lie in the caller. If in fact you still keep the apostrophe
That acutally means that the string uses apostrophes instead of quotes for easier embedding into C. In this exact case, all the strings tested use apostrophes instead of quotes so this will always be true. I don't see a benefit of adding it as an argument for every single test case in this test.
check in patch 2, then I would think the caller would be able to supply true/false, but that's a minor thing.
This code is always using the shortcut, since I'm lazy to escape every single quote.
Also, something that just occurred to me... 'expectargs' had better not be NULL; otherwise, patch 2 will core at the while (*tmp != '\0')
There are other monitor testing APIs that you can use if you don't care about the arguments. Additionally the worker function that is verifying the data is not able to expect a NULL string as these APIs are made to test the actual arguments. For commands which don't have arguments you'll get an earlier error. The expected string should thus never be NULL, since it would be a programmer mistake. And if it will be by accident, the testsuite will crash which is correct in this regard IMO.
ACK in principle - your call on the apostrophe thing - although perhaps a check in patch 2 should be added for (apostrophe && data->expectArgs) to protect from future mishaps...
John
--- src/qemu/qemu_monitor_json.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e1494df..126927e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5937,22 +5937,17 @@ qemuMonitorJSONBuildInetSocketAddress(const char *host, virJSONValuePtr addr = NULL; virJSONValuePtr data = NULL; - if (!(data = virJSONValueNewObject()) || - !(addr = virJSONValueNewObject())) - goto error; + if (virJSONValueObjectCreate(&data, "s:host", host, + "s:port", port, NULL) < 0) + return NULL; - /* port is really expected as a string here by qemu */ - if (virJSONValueObjectAppendString(data, "host", host) < 0 || - virJSONValueObjectAppendString(data, "port", port) < 0 || - virJSONValueObjectAppendString(addr, "type", "inet") < 0 || - virJSONValueObjectAppend(addr, "data", data) < 0) - goto error; + if (virJSONValueObjectCreate(&addr, "s:type", "inet", + "a:data", data, NULL) < 0) { + virJSONValueFree(data); + return NULL; + } return addr; - error: - virJSONValueFree(data); - virJSONValueFree(addr); - return NULL; } static virJSONValuePtr @@ -5961,20 +5956,16 @@ qemuMonitorJSONBuildUnixSocketAddress(const char *path) virJSONValuePtr addr = NULL; virJSONValuePtr data = NULL; - if (!(data = virJSONValueNewObject()) || - !(addr = virJSONValueNewObject())) - goto error; + if (virJSONValueObjectCreate(&data, "s:path", path, NULL) < 0) + return NULL; - if (virJSONValueObjectAppendString(data, "path", path) < 0 || - virJSONValueObjectAppendString(addr, "type", "unix") < 0 || - virJSONValueObjectAppend(addr, "data", data) < 0) - goto error; + if (virJSONValueObjectCreate(&addr, "s:type", "unix", + "a:data", data, NULL) < 0) { + virJSONValueFree(data); + return NULL; + } return addr; - error: - virJSONValueFree(data); - virJSONValueFree(addr); - return NULL; } int -- 2.10.0
On 09/27/2016 12:39 PM, Peter Krempa wrote:
--- src/qemu/qemu_monitor_json.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-)
ACK, John
Since introduction of chardev hotplug the code was wrong for the UDP case and basically created a TCP socket instead. Use proper objects and type for UDP. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1377602 --- src/qemu/qemu_monitor_json.c | 12 ++++++++++-- tests/qemumonitorjsontest.c | 20 ++++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 126927e..c720376 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6212,12 +6212,20 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, break; case VIR_DOMAIN_CHR_TYPE_UDP: - backend_type = "socket"; + backend_type = "udp"; addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.udp.connectHost, chr->data.udp.connectService); if (!addr || - virJSONValueObjectAppend(data, "addr", addr) < 0) + virJSONValueObjectAppend(data, "remote", addr) < 0) goto error; + + if (chr->data.udp.bindHost) { + addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.udp.bindHost, + chr->data.udp.bindService); + if (!addr || + virJSONValueObjectAppend(data, "local", addr) < 0) + goto error; + } addr = NULL; break; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 61344b7..0574f8c 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -853,10 +853,22 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr xmlopt) chr.data.udp.connectService = (char *) "1234"; CHECK("udp", false, "{'id':'alias'," - "'backend':{'type':'socket'," - "'data':{'addr':{'type':'inet'," - "'data':{'host':'example.com'," - "'port':'1234'}}}}}"); + "'backend':{'type':'udp'," + "'data':{'remote':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}}}}"); + + chr.data.udp.bindHost = (char *) "localhost"; + chr.data.udp.bindService = (char *) "4321"; + CHECK("udp", false, + "{'id':'alias'," + "'backend':{'type':'udp'," + "'data':{'remote':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}," + "'local':{'type':'inet'," + "'data':{'host':'localhost'," + "'port':'4321'}}}}}"); memset(&chr, 0, sizeof(chr)); chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; -- 2.10.0
On 09/27/2016 12:39 PM, Peter Krempa wrote:
Since introduction of chardev hotplug the code was wrong for the UDP case and basically created a TCP socket instead. Use proper objects and type for UDP.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1377602 --- src/qemu/qemu_monitor_json.c | 12 ++++++++++-- tests/qemumonitorjsontest.c | 20 ++++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-)
As I read the bug, the code below, and the qemu_command code - it seems the bug describes issues with both qemu_command and the hotplug code. Although for sure the hotplug code is wrong - there's no adjustment in the qemu_command code, so it doesn't seem this patch addresses all the concerns in the bz. ACK to the change, but I'm not sure you can claim victory over the bz if the qemu_command logic is still flawed... John
On Wed, Oct 05, 2016 at 09:39:06 -0400, John Ferlan wrote:
On 09/27/2016 12:39 PM, Peter Krempa wrote:
Since introduction of chardev hotplug the code was wrong for the UDP case and basically created a TCP socket instead. Use proper objects and type for UDP.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1377602 --- src/qemu/qemu_monitor_json.c | 12 ++++++++++-- tests/qemumonitorjsontest.c | 20 ++++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-)
As I read the bug, the code below, and the qemu_command code - it seems the bug describes issues with both qemu_command and the hotplug code.
The command line portion actually is not wrong. It generates the address pair with type UDP as it should. The reporter of that bugzilla has just improper expectations of UDP socket capabilities. Also it's very unlikely that somebody is actually using the qemu UDP backend. It was broken for rather long time up until last year. Peter
participants (2)
-
John Ferlan -
Peter Krempa