[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 14/22] libqtest: Separate qmp_discard_respons
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 14/22] libqtest: Separate qmp_discard_response() from command |
Date: |
Wed, 9 Aug 2017 10:32:50 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
On 08/09/2017 10:15 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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
[Qemu-devel] [PATCH v4 14/22] libqtest: Separate qmp_discard_response() from command, Eric Blake, 2017/08/03
[Qemu-devel] [PATCH v4 18/22] tests/libqos/usb: Clean up string interpolation into QMP input, Eric Blake, 2017/08/03
[Qemu-devel] [PATCH v4 19/22] libqtest: Add qmp_args_dict() helper, Eric Blake, 2017/08/03
[Qemu-devel] [PATCH v4 21/22] libqtest: Drop now-unused qmp(), Eric Blake, 2017/08/03
[Qemu-devel] [PATCH v4 16/22] libqtest: Add qmp_cmd() helper, Eric Blake, 2017/08/03