Re: [Qemu-devel] [PATCH v4 14/22] libqtest: Separate qmp_discard_response() from command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3842 bytes --]

On 08/09/2017 10:15 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]