On 08/09/2017 10:15 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> Upcoming patches will be adding new convenience methods for >> constructing QMP commands. But making every variation of sending >> support every variation of response handling becomes unwieldy; >> it's easier to specify that discarding a JSON response is >> unassociated with sending the command, where qmp_async() already >> fits the bill for sending a command without tying up a reference >> to the response. >> >> Doing this renders qtest_qmp[v]_discard_response() unused. >> >> Bonus: gets rid of a non-literal format string, which is a step >> towards compile-time format string checking without triggering >> -Wformat-nonliteral. > > Where? (I'm feeling lazy today) >
Sure: > > +++ b/tests/ide-test.c > @@ -624,7 +624,6 @@ static void test_retry_flush(const char *machine) > QPCIDevice *dev; > QPCIBar bmdma_bar, ide_bar; > uint8_t data; > - const char *s; > > prepare_blkdebug_script(debug_path, "flush_to_disk"); > > @@ -652,8 +651,8 @@ static void test_retry_flush(const char *machine) > qmp_eventwait("STOP"); > > /* Complete the command */ > - s = "{'execute':'cont' }"; > - qmp_discard_response(s); > + qmp_async("{'execute':'cont' }"); > + qmp_discard_response(); Yes, I can update the commit message to focus in on it more precisely. >> +++ b/tests/libqtest.c >> @@ -235,7 +235,8 @@ QTestState *qtest_init(const char *extra_args) >> /* Read the QMP greeting and then do the handshake */ >> greeting = qtest_qmp_receive(s); >> QDECREF(greeting); >> - qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }"); >> + greeting = qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }"); >> + QDECREF(greeting); > > Here, you replace > > qtest_qmp_discard_response(ARGS...); > > effectively by > > QDECREF(qtest_qmp(ARGS...)); > > which is how qtest_qmp_discard_response() does its job before this > patch. Okay. I deleted qtest_qmp_discard_response(). If I had kept it around (but made it static instead), I could have written this one as: /* Read the QMP greeting and then do the handshake */ qtest_qmp_discard_response(s); qtest_qmp_async(s, "{'execute': 'qmp_capabilities'}"); qtest_qmp_discard_response(s); But as the only place that needed to pass 's' on through, I was just as comfortable open-coding the QDECREF after the fact. >> +++ b/tests/ahci-test.c >> @@ -1596,8 +1596,9 @@ static void test_atapi_tray(void) >> rsp = qmp_receive(); >> QDECREF(rsp); Hmm - this code already was manually using QDECREF()... >> >> - qmp_discard_response("{'execute': 'x-blockdev-remove-medium', " >> - "'arguments': {'device': 'drive0'}}"); >> + qmp_async("{'execute': 'x-blockdev-remove-medium', " >> + "'arguments': {'device': 'drive0'}}"); >> + qmp_discard_response(); which means this could have reused rsp = qmp(...); QDECREF(rsp); as well. > > Here, you replace the same pattern (less the QTestState argument) by > > qmp_async(F, ...); > qmp_discard_response(); > > Also okay. But why two ways to do the same things? > > Apropos QTestState argument: do we have a test with more than one state, > or is having two versions of every function just "avoiding global state > is virtuous" self-flagellation? We DO have tests that use more than one state - AND those tests prefer calling the qmp() version (rather than the qtest_qmp() version) with manipulation of the global_qtest as needed. See my cleanup in 15/22. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature