[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands |
Date: |
Mon, 24 Apr 2017 12:31:48 +0100 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
* Markus Armbruster (address@hidden) wrote:
> John Snow <address@hidden> 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 <address@hidden>
> >>>> ---
> >>>> 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 <address@hidden>
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
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK