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

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

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

I intend to swap with the previous patch in v3 to reduce churn.

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

Warnings in function comments are ineffective at counterproliferation.
People copy code without examining the called functions' comments.

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

Sold.