"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> John Snow <js...@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 <th...@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 <arm...@redhat.com>
>
> OK, I've added that comment to qtest_hmp and qtest_hmpv when I've
> just queued it.
> (and removed an extra 'v')
>
> /**
> - * qtest_hmpv:
> + * qtest_hmp:
> * @s: #QTestState instance to operate on.
> * @fmt...: HMP command to send to QEMU
> *
> * Send HMP command to QEMU via QMP's human-monitor-command.
> + * QMP events are discarded.
> *
> * Returns: the command's output. The caller should g_free() it.
> */
> @@ -149,6 +150,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
> * @ap: HMP command arguments
> *
> * Send HMP command to QEMU via QMP's human-monitor-command.
> + * QMP events are discarded.
> *
> * Returns: the command's output. The caller should g_free() it.
> */
>
> Dave
Works for me, thanks!
