Re: [Qemu-devel] [PATCH v2 21/23] libqtest: Remove qtest_qmp_discard_response() & friends

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> On 07/30/2018 08:32 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> On 07/27/2018 11:46 AM, Thomas Huth wrote:
>>>> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>>>>> qtest_qmp_discard_response(...) is shorthand for
>>>>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>>>>
>>>> But the latter is IMHO harder to read.
>> 
>> Doing things sloppily looks a bit uglier now.  That's a feature.
>> 
>>> Maybe, but then it lends itself well to:
>>>
>>> QObject *rsp = qtest_qmp(...);
>>> qobject_unref(rsp);
>>>
>>> which is where you do insert tests for valid responses.
>>>
>>>> And it might be shorter in the compiled binary (one function call vs. two).
>> 
>> I'd be quite sympathetic to this argument...
>> 
>>> The size of the test binaries is not our biggest concern.
>> 
>> ... outside tests/.
>> 
>>>>> Moreover, the presence of these functions encourage sloppy testing.
>>>>
>>>> Shouldn't we then rather fix the tests to check for valid responses
>>>> instead of replacing this function with harder-to-read code?
>> 
>> I'd welcome such patches, but this series is already pretty long.
>
> Then maybe rather drop this patch from this series, and fix the issues
> in a separate series instead?

Do you insist?

I fail to see how changing

    qmp_discard_response("{ 'execute': 'system_reset' }");

to

    qobject_unref(qmp("{ 'execute': 'system_reset' }"));

is so awful it would justify demanding I pause my work on libqtest to
first figure out which parts of ignored responses are worth checking,
then code up the checks.

Would you accept

    rsp = qmp("{ 'execute': 'system_reset' }"));
    qobject_unref(rsp);

?

If none of the above is acceptable to you, then I'll push the crap that
needs to go from libqtest into the crap-using tests, like this:

    /* TODO actually test the results and get rid of this */
    #define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__));