| Message ID | 1500645206-13559-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/21/2017 08:53 AM, Markus Armbruster wrote: > When you build QMP input manually like this > > cmd = g_strdup_printf("{ 'execute': 'migrate'," > "'arguments': { 'uri': '%s' } }", > uri); > rsp = qmp(cmd); > g_free(cmd); > > you're responsible for escaping the interpolated values for JSON. Not > done here, and therefore works only for sufficiently nice @uri. For > instance, if @uri contained a single "'", qobject_from_jsonv() would > fail, qmp_fd_sendv() would misinterpret the failure as empty input and > do nothing, and the test would hang waiting for a response that never > comes. > > Leaving interpolation into JSON to qmp() is more robust: > > rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri); > > It's also more concise. > > Clean up the simple cases where we interpolate exactly a JSON value. > > Bonus: gets rid of non-literal format strings. A step towards > compile-time format string checking without triggering > -Wformat-nonliteral. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > + qmp_fd_send(fixture->fd, > + "\xff{'execute': 'guest-sync-delimited'," > + " 'arguments': {'id': %u } }", Ouch. Per 2/9: * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])'). That will need to be %d now. Or, more likely, we need to update the comments in json-lexer.c and in 2/9 (/me goes to read json-lexer.c...) /* escape */ [IN_ESCAPE_LL] = { ['d'] = JSON_ESCAPE, ['u'] = JSON_ESCAPE, }, ... [IN_ESCAPE] = { ['d'] = JSON_ESCAPE, ['i'] = JSON_ESCAPE, ['p'] = JSON_ESCAPE, ['s'] = JSON_ESCAPE, ['u'] = JSON_ESCAPE, ['f'] = JSON_ESCAPE, ['l'] = IN_ESCAPE_L, ['I'] = IN_ESCAPE_I, Aha, that 'd' should be '[du]' in all of the comments. > @@ -411,10 +408,10 @@ static void test_qga_file_ops(gconstpointer fix) > > enc = g_base64_encode(helloworld, sizeof(helloworld)); > /* write */ > - cmd = g_strdup_printf("{'execute': 'guest-file-write'," > - " 'arguments': { 'handle': %" PRId64 "," > - " 'buf-b64': '%s' } }", id, enc); > - ret = qmp_fd(fixture->fd, cmd); > + ret = qmp_fd(fixture->fd, > + "{'execute': 'guest-file-write'," > + " 'arguments': { 'handle': %" PRId64 ", 'buf-b64': %s } }", Ouch; you are reverting commit 1792d7d0. We tried hard to make json-lexer.c understand lots of different 64-bit format spellings, but we KNOW that we don't support MacOS (see commit 29a6731). We either need to beef up json-lexer.c to understand %qd, or get rid of JSON psuedo-strings, if we expect this to work; otherwise, you should use %lld and long long instead of PRId64 and uint64_t. Overall, I like the patch, but we need to fix the problems for the next round of this series.
Eric Blake <eblake@redhat.com> writes: > On 07/21/2017 08:53 AM, Markus Armbruster wrote: >> When you build QMP input manually like this >> >> cmd = g_strdup_printf("{ 'execute': 'migrate'," >> "'arguments': { 'uri': '%s' } }", >> uri); >> rsp = qmp(cmd); >> g_free(cmd); >> >> you're responsible for escaping the interpolated values for JSON. Not >> done here, and therefore works only for sufficiently nice @uri. For >> instance, if @uri contained a single "'", qobject_from_jsonv() would >> fail, qmp_fd_sendv() would misinterpret the failure as empty input and >> do nothing, and the test would hang waiting for a response that never >> comes. >> >> Leaving interpolation into JSON to qmp() is more robust: >> >> rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri); >> >> It's also more concise. >> >> Clean up the simple cases where we interpolate exactly a JSON value. >> >> Bonus: gets rid of non-literal format strings. A step towards >> compile-time format string checking without triggering >> -Wformat-nonliteral. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- >> + qmp_fd_send(fixture->fd, >> + "\xff{'execute': 'guest-sync-delimited'," >> + " 'arguments': {'id': %u } }", > > Ouch. Per 2/9: > * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])'). > > That will need to be %d now. Or, more likely, we need to update the > comments in json-lexer.c and in 2/9 (/me goes to read json-lexer.c...) > /* escape */ > [IN_ESCAPE_LL] = { > ['d'] = JSON_ESCAPE, > ['u'] = JSON_ESCAPE, > }, > ... > [IN_ESCAPE] = { > ['d'] = JSON_ESCAPE, > ['i'] = JSON_ESCAPE, > ['p'] = JSON_ESCAPE, > ['s'] = JSON_ESCAPE, > ['u'] = JSON_ESCAPE, > ['f'] = JSON_ESCAPE, > ['l'] = IN_ESCAPE_L, > ['I'] = IN_ESCAPE_I, > > Aha, that 'd' should be '[du]' in all of the comments. Yes. We really need %u, or else parse_escape() would call qnum_from_int() instead of qnum_from_uint(). I think the doc fix for json-lexer.c can be squashed in. Keeping it separate could simplify commit message writing, though. Your choice. >> @@ -411,10 +408,10 @@ static void test_qga_file_ops(gconstpointer fix) >> >> enc = g_base64_encode(helloworld, sizeof(helloworld)); >> /* write */ >> - cmd = g_strdup_printf("{'execute': 'guest-file-write'," >> - " 'arguments': { 'handle': %" PRId64 "," >> - " 'buf-b64': '%s' } }", id, enc); >> - ret = qmp_fd(fixture->fd, cmd); >> + ret = qmp_fd(fixture->fd, >> + "{'execute': 'guest-file-write'," >> + " 'arguments': { 'handle': %" PRId64 ", 'buf-b64': %s } }", > > Ouch; you are reverting commit 1792d7d0. We tried hard to make > json-lexer.c understand lots of different 64-bit format spellings, but > we KNOW that we don't support MacOS (see commit 29a6731). We either > need to beef up json-lexer.c to understand %qd, or get rid of JSON > psuedo-strings, if we expect this to work; otherwise, you should use > %lld and long long instead of PRId64 and uint64_t. I forgot that PRId64 expands into non-standard crap on some systems. Options: 1. Outlaw use of PRI macros, limit integer length modifiers for conversion specifiers "d" and "u" to "l" and "ll". When PRI macros creep in, the build breaks on hosts where they expand to anything else (hopefully, our CI will catch that). Interpolating int64_t values become a bit awkward. Required work: fix my patches not to use PRId64, drop support for length modifier "I64" from parse_escape(). 2. Support PRId64 and PRIu64, whatever their actual value may be. a. Support all possible values. This is what we've tried before. Nasty: fails only at run-time on hosts with sufficiently creative values. Required work: add support for length modifier "q", to unbreak MacOS. Optional work: try to turn the run-time failure into a compile- time failure, ideally in configure. b. Support exactly the host's PRId64 and PRIu64 values. Work required: replace support of "I64d" and "I64u" by support of PRId64 and PRIu64. Easy enough in parse_escape(), but not in json-lexer.c. We could perhaps have the lexer accept the shortest string that's followed by a conversion specifier as length modifier, then reject invalid length modifiers in a semantic action. Optional work: try to simplify code that currently works around unavailability of PRId64 and PRIu64. Preferences? I like 2b, but I'm not sure I'll like the code implementing it. One way to find out... > Overall, I like the patch, but we need to fix the problems for the next > round of this series. Yes.
On 07/21/2017 11:48 AM, Markus Armbruster wrote: > I forgot that PRId64 expands into non-standard crap on some systems. > > Options: > > 1. Outlaw use of PRI macros, limit integer length modifiers for > conversion specifiers "d" and "u" to "l" and "ll". When PRI macros > creep in, the build breaks on hosts where they expand to anything > else (hopefully, our CI will catch that). Interpolating int64_t > values become a bit awkward. > > Required work: fix my patches not to use PRId64, drop support for > length modifier "I64" from parse_escape(). Yuck. (But motivation for my earlier series to nuke qobject_from_jsonf()) > > 2. Support PRId64 and PRIu64, whatever their actual value may be. > > a. Support all possible values. This is what we've tried before. > Nasty: fails only at run-time on hosts with sufficiently creative > values. > > Required work: add support for length modifier "q", to unbreak > MacOS. > > Optional work: try to turn the run-time failure into a compile- > time failure, ideally in configure. Accepts garbage (although -Wformat detection will help avoid the worst of it). You can't check string equality in the preprocessor, and you can't do things like "case PRId64[0]: case 'l':" to force compilation errors due to duplicate labels, but we CAN limit ourselves to the preprocessor and grep that output to see what PRId64 expands to. I'll see if I can come up with a configure check. Still, it may be easier to just fail configure and tell people to report the bug on their odd libc, at which point we update json-lexer.c and configure, than it is to try and reuse configure results in json-lexer.c (since the PRId64 string is not a constant width, it gets hard to code an exact value into our lexer state machine, but easier to code every reported value). > > b. Support exactly the host's PRId64 and PRIu64 values. > > Work required: replace support of "I64d" and "I64u" by support of > PRId64 and PRIu64. Easy enough in parse_escape(), but not in > json-lexer.c. We could perhaps have the lexer accept the shortest > string that's followed by a conversion specifier as length > modifier, then reject invalid length modifiers in a semantic > action. Interesting idea. I'm playing with it... > > Optional work: try to simplify code that currently works around > unavailability of PRId64 and PRIu64. > > Preferences? > > I like 2b, but I'm not sure I'll like the code implementing it. One way > to find out... In the lexer, widen the state machine to accept up to three unknown characters between % and d. Hmm, there's also the possibility of int64_t being mapped to %jd, in addition to our known culprits of %ld, %lld, %I64d, and %qd. > >> Overall, I like the patch, but we need to fix the problems for the next >> round of this series. > > Yes. At this point, I think I'll spin the next round. But it's not longer a 2.10 priority, so it may be a few days.
On 07/21/2017 03:08 PM, Eric Blake wrote: >> 2. Support PRId64 and PRIu64, whatever their actual value may be. >> >> a. Support all possible values. This is what we've tried before. >> >> b. Support exactly the host's PRId64 and PRIu64 values. >> Preferences? >> >> I like 2b, but I'm not sure I'll like the code implementing it. One way >> to find out... I ended up implementing 2a in the C code, but 2b in the configure check to make sure our C code gets updated if we ever encounter any more oddballs. Of course, I don't have easy access to Mac OS, so I'm relying on others to test the patch for me; but once we get that patch going to our liking, I can rebase the rest of this series on top of it. https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg07039.html
Eric Blake <eblake@redhat.com> writes: > On 07/21/2017 11:48 AM, Markus Armbruster wrote: >> I forgot that PRId64 expands into non-standard crap on some systems. >> >> Options: >> >> 1. Outlaw use of PRI macros, limit integer length modifiers for >> conversion specifiers "d" and "u" to "l" and "ll". When PRI macros >> creep in, the build breaks on hosts where they expand to anything >> else (hopefully, our CI will catch that). Interpolating int64_t >> values become a bit awkward. >> >> Required work: fix my patches not to use PRId64, drop support for >> length modifier "I64" from parse_escape(). > > Yuck. (But motivation for my earlier series to nuke qobject_from_jsonf()) > >> >> 2. Support PRId64 and PRIu64, whatever their actual value may be. >> >> a. Support all possible values. This is what we've tried before. Clarification: all the values we expect on the hosts we support. Currently "l", "ll", "I64" and "q". >> Nasty: fails only at run-time on hosts with sufficiently creative >> values. >> >> Required work: add support for length modifier "q", to unbreak >> MacOS. We already support the other three. >> Optional work: try to turn the run-time failure into a compile- >> time failure, ideally in configure. > > Accepts garbage (although -Wformat detection will help avoid the worst > of it). You can't check string equality in the preprocessor, and you > can't do things like "case PRId64[0]: case 'l':" to force compilation > errors due to duplicate labels, but we CAN limit ourselves to the > preprocessor and grep that output to see what PRId64 expands to. I'll > see if I can come up with a configure check. Still, it may be easier to > just fail configure and tell people to report the bug on their odd libc, > at which point we update json-lexer.c and configure, than it is to try > and reuse configure results in json-lexer.c (since the PRId64 string is > not a constant width, it gets hard to code an exact value into our lexer > state machine, but easier to code every reported value). Auto-configuring our handwritten lexer to recognize PRId64 and PRIu64 would be awkward. Certainly more awkward than substituting an auto-configured string into the input of flex. It's 2b anyway. >> b. Support exactly the host's PRId64 and PRIu64 values. >> >> Work required: replace support of "I64d" and "I64u" by support of >> PRId64 and PRIu64. Easy enough in parse_escape(), but not in >> json-lexer.c. We could perhaps have the lexer accept the shortest >> string that's followed by a conversion specifier as length >> modifier, then reject invalid length modifiers in a semantic >> action. > > Interesting idea. I'm playing with it... > >> >> Optional work: try to simplify code that currently works around >> unavailability of PRId64 and PRIu64. >> >> Preferences? >> >> I like 2b, but I'm not sure I'll like the code implementing it. One way >> to find out... > > In the lexer, widen the state machine to accept up to three unknown > characters between % and d. Hmm, there's also the possibility of > int64_t being mapped to %jd, in addition to our known culprits of %ld, > %lld, %I64d, and %qd. The lexer could accept any length modifier matching a suitable regular expression, leaving rejection of invalid ones to semantic actions. The only hard restriction on non-standard length modifiers is that the resulting conversion specifications must be syntactically unambiguous, and the ones the standard specifies still mean the same. Example: length modifier "f" to conversion specifier "d" is impossible, because "%fd" must be parsed as conversion specifier "%f" followed by ordinary character "d". Example: similarly, length modifiers starting with a flag character, a digit, '.' or '*' are not possible because such a character must be parsed as flag, minimum field width or precision, respectively. In practice, length modifiers starting with characters used elsewhere in conversion specifications, or containing conversion specifier characters would be too confusing. Example: length modifier "qp" to conversion specifier "d" could technically be done as long as "q" isn't also made a length modifier to "p". But it would be nuts. That leaves us with the regular expression [^-+ #0-9*.diouxXfFeEgGaAcspn%][^diouxXfFeEgGaAcspn%]* I think we can rule out control characters, too. >>> Overall, I like the patch, but we need to fix the problems for the next >>> round of this series. >> >> Yes. > > At this point, I think I'll spin the next round. But it's not longer a > 2.10 priority, so it may be a few days. No need to rush this work into 2.10.
diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c index 6226546..42c5315 100644 --- a/tests/libqos/libqos.c +++ b/tests/libqos/libqos.c @@ -86,20 +86,12 @@ void set_context(QOSState *s) static QDict *qmp_execute(const char *command) { - char *fmt; - QDict *rsp; - - fmt = g_strdup_printf("{ 'execute': '%s' }", command); - rsp = qmp(fmt); - g_free(fmt); - - return rsp; + return qmp("{ 'execute': %s }", command); } void migrate(QOSState *from, QOSState *to, const char *uri) { const char *st; - char *s; QDict *rsp, *sub; bool running; @@ -114,11 +106,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri) QDECREF(rsp); /* Issue the migrate command. */ - s = g_strdup_printf("{ 'execute': 'migrate'," - "'arguments': { 'uri': '%s' } }", - uri); - rsp = qmp(s); - g_free(s); + rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri); g_assert(qdict_haskey(rsp, "return")); QDECREF(rsp); diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c index ded1c54..d40aa9d 100644 --- a/tests/libqos/pci-pc.c +++ b/tests/libqos/pci-pc.c @@ -159,14 +159,9 @@ void qpci_free_pc(QPCIBus *bus) void qpci_unplug_acpi_device_test(const char *id, uint8_t slot) { QDict *response; - char *cmd; - cmd = g_strdup_printf("{'execute': 'device_del'," - " 'arguments': {" - " 'id': '%s'" - "}}", id); - response = qmp(cmd); - g_free(cmd); + response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}", + id); g_assert(response); g_assert(!qdict_haskey(response, "error")); QDECREF(response); diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c index 8142f2a..e2ead87 100644 --- a/tests/postcopy-test.c +++ b/tests/postcopy-test.c @@ -358,7 +358,7 @@ static void test_migrate(void) char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); QTestState *global = global_qtest, *from, *to; unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d; - gchar *cmd, *cmd_src, *cmd_dst; + gchar *cmd_src, *cmd_dst; QDict *rsp; char *bootpath = g_strdup_printf("%s/bootsect", tmpfs); @@ -445,11 +445,7 @@ static void test_migrate(void) /* Wait for the first serial output from the source */ wait_for_serial("src_serial"); - cmd = g_strdup_printf("{ 'execute': 'migrate'," - "'arguments': { 'uri': '%s' } }", - uri); - rsp = qmp(cmd); - g_free(cmd); + rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri); g_assert(qdict_haskey(rsp, "return")); QDECREF(rsp); diff --git a/tests/test-qga.c b/tests/test-qga.c index 06783e7..91a7b6e 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -144,12 +144,11 @@ static void test_qga_sync_delimited(gconstpointer fix) guint32 v, r = g_random_int(); unsigned char c; QDict *ret; - gchar *cmd; - cmd = g_strdup_printf("\xff{'execute': 'guest-sync-delimited'," - " 'arguments': {'id': %u } }", r); - qmp_fd_send(fixture->fd, cmd); - g_free(cmd); + qmp_fd_send(fixture->fd, + "\xff{'execute': 'guest-sync-delimited'," + " 'arguments': {'id': %u } }", + r); /* * Read and ignore garbage until resynchronized. @@ -186,7 +185,6 @@ static void test_qga_sync(gconstpointer fix) const TestFixture *fixture = fix; guint32 v, r = g_random_int(); QDict *ret; - gchar *cmd; /* * TODO guest-sync is inherently limited: we cannot distinguish @@ -199,10 +197,9 @@ static void test_qga_sync(gconstpointer fix) * invalid JSON. Testing of '\xff' handling is done in * guest-sync-delimited instead. */ - cmd = g_strdup_printf("{'execute': 'guest-sync'," - " 'arguments': {'id': %u } }", r); - ret = qmp_fd(fixture->fd, cmd); - g_free(cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-sync', 'arguments': {'id': %u } }", + r); g_assert_nonnull(ret); qmp_assert_no_error(ret); @@ -393,7 +390,7 @@ static void test_qga_file_ops(gconstpointer fix) const TestFixture *fixture = fix; const unsigned char helloworld[] = "Hello World!\n"; const char *b64; - gchar *cmd, *path, *enc; + gchar *path, *enc; unsigned char *dec; QDict *ret, *val; int64_t id, eof; @@ -411,10 +408,10 @@ static void test_qga_file_ops(gconstpointer fix) enc = g_base64_encode(helloworld, sizeof(helloworld)); /* write */ - cmd = g_strdup_printf("{'execute': 'guest-file-write'," - " 'arguments': { 'handle': %" PRId64 "," - " 'buf-b64': '%s' } }", id, enc); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-write'," + " 'arguments': { 'handle': %" PRId64 ", 'buf-b64': %s } }", + id, enc); g_assert_nonnull(ret); qmp_assert_no_error(ret); @@ -424,23 +421,20 @@ static void test_qga_file_ops(gconstpointer fix) g_assert_cmpint(count, ==, sizeof(helloworld)); g_assert_cmpint(eof, ==, 0); QDECREF(ret); - g_free(cmd); /* flush */ - cmd = g_strdup_printf("{'execute': 'guest-file-flush'," - " 'arguments': {'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-flush'," + " 'arguments': {'handle': %" PRId64 "} }", + id); QDECREF(ret); - g_free(cmd); /* close */ - cmd = g_strdup_printf("{'execute': 'guest-file-close'," - " 'arguments': {'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-close'," + " 'arguments': {'handle': %" PRId64 "} }", + id); QDECREF(ret); - g_free(cmd); /* check content */ path = g_build_filename(fixture->test_dir, "foo", NULL); @@ -462,10 +456,10 @@ static void test_qga_file_ops(gconstpointer fix) QDECREF(ret); /* read */ - cmd = g_strdup_printf("{'execute': 'guest-file-read'," - " 'arguments': { 'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-read'," + " 'arguments': { 'handle': %" PRId64 "} }", + id); val = qdict_get_qdict(ret, "return"); count = qdict_get_int(val, "count"); eof = qdict_get_bool(val, "eof"); @@ -475,14 +469,13 @@ static void test_qga_file_ops(gconstpointer fix) g_assert_cmpstr(b64, ==, enc); QDECREF(ret); - g_free(cmd); g_free(enc); /* read eof */ - cmd = g_strdup_printf("{'execute': 'guest-file-read'," - " 'arguments': { 'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-read'," + " 'arguments': { 'handle': %" PRId64 "} }", + id); val = qdict_get_qdict(ret, "return"); count = qdict_get_int(val, "count"); eof = qdict_get_bool(val, "eof"); @@ -491,14 +484,13 @@ static void test_qga_file_ops(gconstpointer fix) g_assert(eof); g_assert_cmpstr(b64, ==, ""); QDECREF(ret); - g_free(cmd); /* seek */ - cmd = g_strdup_printf("{'execute': 'guest-file-seek'," - " 'arguments': { 'handle': %" PRId64 ", " - " 'offset': %d, 'whence': '%s' } }", - id, 6, "set"); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-seek'," + " 'arguments': { 'handle': %" PRId64 ", " + " 'offset': %d, 'whence': %s } }", + id, 6, "set"); qmp_assert_no_error(ret); val = qdict_get_qdict(ret, "return"); count = qdict_get_int(val, "position"); @@ -506,13 +498,12 @@ static void test_qga_file_ops(gconstpointer fix) g_assert_cmpint(count, ==, 6); g_assert(!eof); QDECREF(ret); - g_free(cmd); /* partial read */ - cmd = g_strdup_printf("{'execute': 'guest-file-read'," - " 'arguments': { 'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-read'," + " 'arguments': { 'handle': %" PRId64 "} }", + id); val = qdict_get_qdict(ret, "return"); count = qdict_get_int(val, "count"); eof = qdict_get_bool(val, "eof"); @@ -525,15 +516,13 @@ static void test_qga_file_ops(gconstpointer fix) g_free(dec); QDECREF(ret); - g_free(cmd); /* close */ - cmd = g_strdup_printf("{'execute': 'guest-file-close'," - " 'arguments': {'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-close'," + " 'arguments': {'handle': %" PRId64 "} }", + id); QDECREF(ret); - g_free(cmd); } static void test_qga_file_write_read(gconstpointer fix) @@ -541,7 +530,7 @@ static void test_qga_file_write_read(gconstpointer fix) const TestFixture *fixture = fix; const unsigned char helloworld[] = "Hello World!\n"; const char *b64; - gchar *cmd, *enc; + gchar *enc; QDict *ret, *val; int64_t id, eof; gsize count; @@ -556,10 +545,10 @@ static void test_qga_file_write_read(gconstpointer fix) enc = g_base64_encode(helloworld, sizeof(helloworld)); /* write */ - cmd = g_strdup_printf("{'execute': 'guest-file-write'," - " 'arguments': { 'handle': %" PRId64 "," - " 'buf-b64': '%s' } }", id, enc); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-write'," + " 'arguments': { 'handle': %" PRId64 "," + " 'buf-b64': %s } }", id, enc); g_assert_nonnull(ret); qmp_assert_no_error(ret); @@ -569,13 +558,12 @@ static void test_qga_file_write_read(gconstpointer fix) g_assert_cmpint(count, ==, sizeof(helloworld)); g_assert_cmpint(eof, ==, 0); QDECREF(ret); - g_free(cmd); /* read (check implicit flush) */ - cmd = g_strdup_printf("{'execute': 'guest-file-read'," - " 'arguments': { 'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-read'," + " 'arguments': { 'handle': %" PRId64 "} }", + id); val = qdict_get_qdict(ret, "return"); count = qdict_get_int(val, "count"); eof = qdict_get_bool(val, "eof"); @@ -584,14 +572,13 @@ static void test_qga_file_write_read(gconstpointer fix) g_assert(eof); g_assert_cmpstr(b64, ==, ""); QDECREF(ret); - g_free(cmd); /* seek to 0 */ - cmd = g_strdup_printf("{'execute': 'guest-file-seek'," - " 'arguments': { 'handle': %" PRId64 ", " - " 'offset': %d, 'whence': '%s' } }", - id, 0, "set"); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-seek'," + " 'arguments': { 'handle': %" PRId64 ", " + " 'offset': %d, 'whence': %s } }", + id, 0, "set"); qmp_assert_no_error(ret); val = qdict_get_qdict(ret, "return"); count = qdict_get_int(val, "position"); @@ -599,13 +586,12 @@ static void test_qga_file_write_read(gconstpointer fix) g_assert_cmpint(count, ==, 0); g_assert(!eof); QDECREF(ret); - g_free(cmd); /* read */ - cmd = g_strdup_printf("{'execute': 'guest-file-read'," - " 'arguments': { 'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-read'," + " 'arguments': { 'handle': %" PRId64 "} }", + id); val = qdict_get_qdict(ret, "return"); count = qdict_get_int(val, "count"); eof = qdict_get_bool(val, "eof"); @@ -614,16 +600,14 @@ static void test_qga_file_write_read(gconstpointer fix) g_assert(eof); g_assert_cmpstr(b64, ==, enc); QDECREF(ret); - g_free(cmd); g_free(enc); /* close */ - cmd = g_strdup_printf("{'execute': 'guest-file-close'," - " 'arguments': {'handle': %" PRId64 "} }", - id); - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-file-close'," + " 'arguments': {'handle': %" PRId64 "} }", + id); QDECREF(ret); - g_free(cmd); } static void test_qga_get_time(gconstpointer fix) @@ -647,7 +631,6 @@ static void test_qga_set_time(gconstpointer fix) const TestFixture *fixture = fix; QDict *ret; int64_t current, time; - gchar *cmd; /* get current time */ ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}"); @@ -673,11 +656,10 @@ static void test_qga_set_time(gconstpointer fix) QDECREF(ret); /* set back current time */ - cmd = g_strdup_printf("{'execute': 'guest-set-time'," - " 'arguments': { 'time': %" PRId64 " } }", - current + time * 1000); - ret = qmp_fd(fixture->fd, cmd); - g_free(cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-set-time'," + " 'arguments': { 'time': %" PRId64 " } }", + current + time * 1000); g_assert_nonnull(ret); qmp_assert_no_error(ret); QDECREF(ret); @@ -864,7 +846,6 @@ static void test_qga_guest_exec(gconstpointer fix) int64_t pid, now, exitcode; gsize len; bool exited; - char *cmd; /* exec 'echo foo bar' */ ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {" @@ -879,10 +860,10 @@ static void test_qga_guest_exec(gconstpointer fix) /* wait for completion */ now = g_get_monotonic_time(); - cmd = g_strdup_printf("{'execute': 'guest-exec-status'," - " 'arguments': { 'pid': %" PRId64 " } }", pid); do { - ret = qmp_fd(fixture->fd, cmd); + ret = qmp_fd(fixture->fd, + "{'execute': 'guest-exec-status'," + " 'arguments': { 'pid': %" PRId64 " } }", pid); g_assert_nonnull(ret); val = qdict_get_qdict(ret, "return"); exited = qdict_get_bool(val, "exited"); @@ -892,7 +873,6 @@ static void test_qga_guest_exec(gconstpointer fix) } while (!exited && g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND); g_assert(exited); - g_free(cmd); /* check stdout */ exitcode = qdict_get_int(val, "exitcode"); diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index d4da09f..f2a2b6c 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -662,11 +662,7 @@ static void test_migrate(void) g_assert(qdict_haskey(rsp, "return")); QDECREF(rsp); - cmd = g_strdup_printf("{ 'execute': 'migrate'," - "'arguments': { 'uri': '%s' } }", - uri); - rsp = qmp(cmd); - g_free(cmd); + rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri); g_assert(qdict_haskey(rsp, "return")); QDECREF(rsp);
When you build QMP input manually like this cmd = g_strdup_printf("{ 'execute': 'migrate'," "'arguments': { 'uri': '%s' } }", uri); rsp = qmp(cmd); g_free(cmd); you're responsible for escaping the interpolated values for JSON. Not done here, and therefore works only for sufficiently nice @uri. For instance, if @uri contained a single "'", qobject_from_jsonv() would fail, qmp_fd_sendv() would misinterpret the failure as empty input and do nothing, and the test would hang waiting for a response that never comes. Leaving interpolation into JSON to qmp() is more robust: rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri); It's also more concise. Clean up the simple cases where we interpolate exactly a JSON value. Bonus: gets rid of non-literal format strings. A step towards compile-time format string checking without triggering -Wformat-nonliteral. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/libqos/libqos.c | 16 +---- tests/libqos/pci-pc.c | 9 +-- tests/postcopy-test.c | 8 +-- tests/test-qga.c | 160 +++++++++++++++++++++--------------------------- tests/vhost-user-test.c | 6 +- 5 files changed, 77 insertions(+), 122 deletions(-)