Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
"Dr. David Alan Gilbert" writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> John Snow 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
>> ---
>> 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
>
> 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!
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
* Markus Armbruster (arm...@redhat.com) wrote:
> John Snow 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
> ---
> 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
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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
John Snow 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
---
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
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
* John Snow (js...@redhat.com) wrote:
>
>
> 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
> >>> ---
> >>> 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.
>
> 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.
>
> I guess the event queue only exists in python though, so it's not as
> trivial as I was thinking it would be...
I think it's OK to discard the QMP events - it feels rare to mix and match
in a test; if you care about QMP events you'll probably be basing the
test around QMP rather than HMP.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
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
>>> ---
>>> 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.
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.
I guess the event queue only exists in python though, so it's not as
trivial as I was thinking it would be...
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
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
>> ---
>> 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
Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
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
> ---
> 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?
--js
[Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
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
---
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);
--
1.8.3.1
