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

[Thomas Huth]

On 08/02/2018 06:53 AM, Markus Armbruster wrote:
> 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?

No. But I'd still like to convince you that this patch is unnecessary
right now.

> 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.

First, you don't have to pause this series just because of this, since
the remaining two patches do not depend on this one.
Then, I still fail to see the real benefit here. You've found something
that needs proper clean up later (by adding checks for valid responses).
So IMHO simply add a big fat warning comment to the description of
qmp_discard_response would be sufficient. Then you can easily grep for
"qmp_discard_response" later to find the spots that need fixing. If you
replace it with that ugly nested construct instead, we still should fix
it later, but it's a little bit harder to grep, and since we need to
change it later again anyway, it just sounds like unnecessary code churn
to me. So do you really need this so badly (for your later work?), or
could you simply skip this patch?

> Would you accept
> 
>     rsp = qmp("{ 'execute': 'system_reset' }"));
>     qobject_unref(rsp);
> 
> ?

While this is easier to read, I think we lose the easy way to grep for
the spots that need fixing later here, so let's better not do this.

> 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__));

Fine for me.

 Thomas