Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On 04/04/2017 03:31 AM, Thomas Huth wrote:
>> On 03.04.2017 21:09, John Snow wrote:
>>>
>>>
>>> On 03/30/2017 03:50 AM, Thomas Huth wrote:
>>>> When running certain HMP commands (like "device_del") via QMP, we
>>>> can sometimes get a QMP event in the response first, so that the
>>>> "g_assert(ret)" statement in qtest_hmp() triggers and the test
>>>> fails. Fix this by ignoring such QMP events while looking for the
>>>> real return value from QMP.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  tests/libqtest.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>>>> index a5c3d2b..c9b2d76 100644
>>>> --- a/tests/libqtest.c
>>>> +++ b/tests/libqtest.c
>>>> @@ -580,6 +580,12 @@ char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
>>>>                       " 'arguments': {'command-line': %s}}",
>>>>                       cmd);
>>>>      ret = g_strdup(qdict_get_try_str(resp, "return"));
>>>> +    while (ret == NULL && qdict_get_try_str(resp, "event")) {
>>>> +        /* Ignore asynchronous QMP events */
>>>> +        QDECREF(resp);
>>>> +        resp = qtest_qmp_receive(s);
>>>> +        ret = g_strdup(qdict_get_try_str(resp, "return"));
>>>> +    }
>>>>      g_assert(ret);
>>>>      QDECREF(resp);
>>>>      g_free(cmd);
>>>>
>>>
>>> You've probably been asked this, but can you just shove the QMP response
>>> you don't want into the event queue for consumption by other calls?
>> 
>> Well, this is the qtest_hmpv() function, so I assume that the caller
>> just wants to execute a HMP command and does not really care about QMP
>> events. If you care about QMP events, you should use the qmp functions
>> instead.
>> 
>>  Thomas
>> 
>
> I don't think it's obvious that using HMP functions should cause the QMP
> stream to become faulty, though.

qtest_hmpv() is a helper function.  It tries to be convenient for the
common case.  In particular, it receives, checks and consumes the QMP
reply.  Before the patch, it screws up when QMP events arrive before the
reply.  The patch fixes it by also consuming the events.  Makes sense.

The non-obviousness should be addressed in the function comment.

> If someone uses an HMP function and then tries to wait on a QMP event to
> confirm that some key condition has occurred (pausing or resuming, for
> instance) it would not be immediately apparent from the user's POV that
> this function just eats replies because it was convenient to do so.

Code that needs to see events can't use this helper function.  It needs
to use more primitive functions instead.

> I guess the event queue only exists in python though, so it's not as
> trivial as I was thinking it would be...

More sophisticated handling of QMP events in libqtest is out of scope
for this patch.  I'm not passing judgement on whether it would be
useful :)

With the function comment updated to mention QMP events get consumed:
Reviewed-by: Markus Armbruster <armbru@redhat.com>