[Qemu-devel] [PATCH v2 0/3] Add a tester for HMP commands

[Qemu-devel] [PATCH v2 0/3] Add a tester for HMP commands

[Thomas Huth]

We currently do not test HMP commands automatically yet, so if they
break, we do not notice this until somebody runs into the problem
(like the "info qtree" problem that we recently had on qemu-system-ppc64).
So let's add a simple tester that runs some HMP commands to check if they
can crash or abort QEMU.

Note: Three boards (isapc, puv3 and tricore_testboard) are currently
still blacklisted in the third patch due to crashes/aborts in HMP
commands there. I've added the problems to our BiteSizeTasks wiki page,
so I hope they will get fixed by GSoC students or somebody else soon.
Once the problems are fixed, the blacklisting of these boards can be
removed in the tester, too.

v2:
- Blacklist the boards "xenpv" and "xenfv" since they can not be used
  without Xen (i.e. in plain TCG mode).

Thomas Huth (3):
  libqtest: Ignore QMP events when parsing the response for HMP commands
  libqtest: Add a generic function to run a callback function for every
    machine
  tests: Add a tester for HMP commands

 tests/Makefile.include |   2 +
 tests/libqtest.c       |  36 +++++++++++
 tests/libqtest.h       |   8 +++
 tests/pc-cpu-test.c    |  95 +++++++++++------------------
 tests/qom-test.c       |  36 ++---------
 tests/test-hmp.c       | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 249 insertions(+), 89 deletions(-)
 create mode 100644 tests/test-hmp.c

-- 
1.8.3.1

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

[Thomas Huth]

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);
-- 
1.8.3.1

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

[John Snow]

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?

--js

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

[Thomas Huth]

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

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

[John Snow]

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.

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

[Dr. David Alan Gilbert]

* John Snow (jsnow@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 <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.
> 
> 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 / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

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>

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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> 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>

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 / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> 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>
>
> 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!

[Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine

[Thomas Huth]

Some tests need to run single tests for every available machine of the
current QEMU binary. To avoid code duplication, let's extract this
code that deals with 'query-machines' into a separate function.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/libqtest.c    | 30 +++++++++++++++++
 tests/libqtest.h    |  8 +++++
 tests/pc-cpu-test.c | 95 ++++++++++++++++++++---------------------------------
 tests/qom-test.c    | 36 ++++----------------
 4 files changed, 80 insertions(+), 89 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index c9b2d76..d8b8066 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -938,3 +938,33 @@ bool qtest_big_endian(QTestState *s)
 {
     return s->big_endian;
 }
+
+void qtest_cb_for_every_machine(void (*cb)(const char *machine))
+{
+    QDict *response, *minfo;
+    QList *list;
+    const QListEntry *p;
+    QObject *qobj;
+    QString *qstr;
+    const char *mname;
+
+    qtest_start("-machine none");
+    response = qmp("{ 'execute': 'query-machines' }");
+    g_assert(response);
+    list = qdict_get_qlist(response, "return");
+    g_assert(list);
+
+    for (p = qlist_first(list); p; p = qlist_next(p)) {
+        minfo = qobject_to_qdict(qlist_entry_obj(p));
+        g_assert(minfo);
+        qobj = qdict_get(minfo, "name");
+        g_assert(qobj);
+        qstr = qobject_to_qstring(qobj);
+        g_assert(qstr);
+        mname = qstring_get_str(qstr);
+        cb(mname);
+    }
+
+    qtest_end();
+    QDECREF(response);
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 2c9962d..43ffadd 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -917,4 +917,12 @@ void qmp_fd_send(int fd, const char *fmt, ...);
 QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
 QDict *qmp_fd(int fd, const char *fmt, ...);
 
+/**
+ * qtest_cb_for_every_machine:
+ * @cb: Pointer to the callback function
+ *
+ *  Call a callback function for every name of all available machines.
+ */
+void qtest_cb_for_every_machine(void (*cb)(const char *machine));
+
 #endif
diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
index c3a2633..c4211a4 100644
--- a/tests/pc-cpu-test.c
+++ b/tests/pc-cpu-test.c
@@ -79,69 +79,46 @@ static void test_data_free(gpointer data)
     g_free(pc);
 }
 
-static void add_pc_test_cases(void)
+static void add_pc_test_case(const char *mname)
 {
-    QDict *response, *minfo;
-    QList *list;
-    const QListEntry *p;
-    QObject *qobj;
-    QString *qstr;
-    const char *mname;
     char *path;
     PCTestData *data;
 
-    qtest_start("-machine none");
-    response = qmp("{ 'execute': 'query-machines' }");
-    g_assert(response);
-    list = qdict_get_qlist(response, "return");
-    g_assert(list);
-
-    for (p = qlist_first(list); p; p = qlist_next(p)) {
-        minfo = qobject_to_qdict(qlist_entry_obj(p));
-        g_assert(minfo);
-        qobj = qdict_get(minfo, "name");
-        g_assert(qobj);
-        qstr = qobject_to_qstring(qobj);
-        g_assert(qstr);
-        mname = qstring_get_str(qstr);
-        if (!g_str_has_prefix(mname, "pc-")) {
-            continue;
-        }
-        data = g_malloc(sizeof(PCTestData));
-        data->machine = g_strdup(mname);
-        data->cpu_model = "Haswell"; /* 1.3+ theoretically */
-        data->sockets = 1;
-        data->cores = 3;
-        data->threads = 2;
-        data->maxcpus = data->sockets * data->cores * data->threads * 2;
-        if (g_str_has_suffix(mname, "-1.4") ||
-            (strcmp(mname, "pc-1.3") == 0) ||
-            (strcmp(mname, "pc-1.2") == 0) ||
-            (strcmp(mname, "pc-1.1") == 0) ||
-            (strcmp(mname, "pc-1.0") == 0) ||
-            (strcmp(mname, "pc-0.15") == 0) ||
-            (strcmp(mname, "pc-0.14") == 0) ||
-            (strcmp(mname, "pc-0.13") == 0) ||
-            (strcmp(mname, "pc-0.12") == 0) ||
-            (strcmp(mname, "pc-0.11") == 0) ||
-            (strcmp(mname, "pc-0.10") == 0)) {
-            path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
-                                   mname, data->sockets, data->cores,
-                                   data->threads, data->maxcpus);
-            qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
-                                     test_data_free);
-            g_free(path);
-        } else {
-            path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
-                                   mname, data->sockets, data->cores,
-                                   data->threads, data->maxcpus);
-            qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
-                                     test_data_free);
-            g_free(path);
-        }
+    if (!g_str_has_prefix(mname, "pc-")) {
+        return;
+    }
+    data = g_malloc(sizeof(PCTestData));
+    data->machine = g_strdup(mname);
+    data->cpu_model = "Haswell"; /* 1.3+ theoretically */
+    data->sockets = 1;
+    data->cores = 3;
+    data->threads = 2;
+    data->maxcpus = data->sockets * data->cores * data->threads * 2;
+    if (g_str_has_suffix(mname, "-1.4") ||
+        (strcmp(mname, "pc-1.3") == 0) ||
+        (strcmp(mname, "pc-1.2") == 0) ||
+        (strcmp(mname, "pc-1.1") == 0) ||
+        (strcmp(mname, "pc-1.0") == 0) ||
+        (strcmp(mname, "pc-0.15") == 0) ||
+        (strcmp(mname, "pc-0.14") == 0) ||
+        (strcmp(mname, "pc-0.13") == 0) ||
+        (strcmp(mname, "pc-0.12") == 0) ||
+        (strcmp(mname, "pc-0.11") == 0) ||
+        (strcmp(mname, "pc-0.10") == 0)) {
+        path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
+                               mname, data->sockets, data->cores,
+                               data->threads, data->maxcpus);
+        qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
+                                 test_data_free);
+        g_free(path);
+    } else {
+        path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
+                               mname, data->sockets, data->cores,
+                               data->threads, data->maxcpus);
+        qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
+                                 test_data_free);
+        g_free(path);
     }
-    QDECREF(response);
-    qtest_end();
 }
 
 int main(int argc, char **argv)
@@ -151,7 +128,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-        add_pc_test_cases();
+        qtest_cb_for_every_machine(add_pc_test_case);
     }
 
     return g_test_run();
diff --git a/tests/qom-test.c b/tests/qom-test.c
index d48f890..ab0595d 100644
--- a/tests/qom-test.c
+++ b/tests/qom-test.c
@@ -107,46 +107,22 @@ static void test_machine(gconstpointer data)
     g_free((void *)machine);
 }
 
-static void add_machine_test_cases(void)
+static void add_machine_test_case(const char *mname)
 {
     const char *arch = qtest_get_arch();
-    QDict *response, *minfo;
-    QList *list;
-    const QListEntry *p;
-    QObject *qobj;
-    QString *qstr;
-    const char *mname;
 
-    qtest_start("-machine none");
-    response = qmp("{ 'execute': 'query-machines' }");
-    g_assert(response);
-    list = qdict_get_qlist(response, "return");
-    g_assert(list);
-
-    for (p = qlist_first(list); p; p = qlist_next(p)) {
-        minfo = qobject_to_qdict(qlist_entry_obj(p));
-        g_assert(minfo);
-        qobj = qdict_get(minfo, "name");
-        g_assert(qobj);
-        qstr = qobject_to_qstring(qobj);
-        g_assert(qstr);
-        mname = qstring_get_str(qstr);
-        if (!is_blacklisted(arch, mname)) {
-            char *path = g_strdup_printf("qom/%s", mname);
-            qtest_add_data_func(path, g_strdup(mname), test_machine);
-            g_free(path);
-        }
+    if (!is_blacklisted(arch, mname)) {
+        char *path = g_strdup_printf("qom/%s", mname);
+        qtest_add_data_func(path, g_strdup(mname), test_machine);
+        g_free(path);
     }
-
-    qtest_end();
-    QDECREF(response);
 }
 
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
-    add_machine_test_cases();
+    qtest_cb_for_every_machine(add_machine_test_case);
 
     return g_test_run();
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine

[Philippe Mathieu-Daudé]

On 03/30/2017 04:50 AM, Thomas Huth wrote:
> Some tests need to run single tests for every available machine of the
> current QEMU binary. To avoid code duplication, let's extract this
> code that deals with 'query-machines' into a separate function.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/libqtest.c    | 30 +++++++++++++++++
>  tests/libqtest.h    |  8 +++++
>  tests/pc-cpu-test.c | 95 ++++++++++++++++++++---------------------------------
>  tests/qom-test.c    | 36 ++++----------------
>  4 files changed, 80 insertions(+), 89 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index c9b2d76..d8b8066 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -938,3 +938,33 @@ bool qtest_big_endian(QTestState *s)
>  {
>      return s->big_endian;
>  }
> +
> +void qtest_cb_for_every_machine(void (*cb)(const char *machine))
> +{
> +    QDict *response, *minfo;
> +    QList *list;
> +    const QListEntry *p;
> +    QObject *qobj;
> +    QString *qstr;
> +    const char *mname;
> +
> +    qtest_start("-machine none");
> +    response = qmp("{ 'execute': 'query-machines' }");
> +    g_assert(response);
> +    list = qdict_get_qlist(response, "return");
> +    g_assert(list);
> +
> +    for (p = qlist_first(list); p; p = qlist_next(p)) {
> +        minfo = qobject_to_qdict(qlist_entry_obj(p));
> +        g_assert(minfo);
> +        qobj = qdict_get(minfo, "name");
> +        g_assert(qobj);
> +        qstr = qobject_to_qstring(qobj);
> +        g_assert(qstr);
> +        mname = qstring_get_str(qstr);
> +        cb(mname);
> +    }
> +
> +    qtest_end();
> +    QDECREF(response);
> +}
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 2c9962d..43ffadd 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -917,4 +917,12 @@ void qmp_fd_send(int fd, const char *fmt, ...);
>  QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
>  QDict *qmp_fd(int fd, const char *fmt, ...);
>
> +/**
> + * qtest_cb_for_every_machine:
> + * @cb: Pointer to the callback function
> + *
> + *  Call a callback function for every name of all available machines.
> + */
> +void qtest_cb_for_every_machine(void (*cb)(const char *machine));
> +
>  #endif
> diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
> index c3a2633..c4211a4 100644
> --- a/tests/pc-cpu-test.c
> +++ b/tests/pc-cpu-test.c
> @@ -79,69 +79,46 @@ static void test_data_free(gpointer data)
>      g_free(pc);
>  }
>
> -static void add_pc_test_cases(void)
> +static void add_pc_test_case(const char *mname)
>  {
> -    QDict *response, *minfo;
> -    QList *list;
> -    const QListEntry *p;
> -    QObject *qobj;
> -    QString *qstr;
> -    const char *mname;
>      char *path;
>      PCTestData *data;
>
> -    qtest_start("-machine none");
> -    response = qmp("{ 'execute': 'query-machines' }");
> -    g_assert(response);
> -    list = qdict_get_qlist(response, "return");
> -    g_assert(list);
> -
> -    for (p = qlist_first(list); p; p = qlist_next(p)) {
> -        minfo = qobject_to_qdict(qlist_entry_obj(p));
> -        g_assert(minfo);
> -        qobj = qdict_get(minfo, "name");
> -        g_assert(qobj);
> -        qstr = qobject_to_qstring(qobj);
> -        g_assert(qstr);
> -        mname = qstring_get_str(qstr);
> -        if (!g_str_has_prefix(mname, "pc-")) {
> -            continue;
> -        }
> -        data = g_malloc(sizeof(PCTestData));
> -        data->machine = g_strdup(mname);
> -        data->cpu_model = "Haswell"; /* 1.3+ theoretically */
> -        data->sockets = 1;
> -        data->cores = 3;
> -        data->threads = 2;
> -        data->maxcpus = data->sockets * data->cores * data->threads * 2;
> -        if (g_str_has_suffix(mname, "-1.4") ||
> -            (strcmp(mname, "pc-1.3") == 0) ||
> -            (strcmp(mname, "pc-1.2") == 0) ||
> -            (strcmp(mname, "pc-1.1") == 0) ||
> -            (strcmp(mname, "pc-1.0") == 0) ||
> -            (strcmp(mname, "pc-0.15") == 0) ||
> -            (strcmp(mname, "pc-0.14") == 0) ||
> -            (strcmp(mname, "pc-0.13") == 0) ||
> -            (strcmp(mname, "pc-0.12") == 0) ||
> -            (strcmp(mname, "pc-0.11") == 0) ||
> -            (strcmp(mname, "pc-0.10") == 0)) {
> -            path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
> -                                   mname, data->sockets, data->cores,
> -                                   data->threads, data->maxcpus);
> -            qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
> -                                     test_data_free);
> -            g_free(path);
> -        } else {
> -            path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
> -                                   mname, data->sockets, data->cores,
> -                                   data->threads, data->maxcpus);
> -            qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
> -                                     test_data_free);
> -            g_free(path);
> -        }
> +    if (!g_str_has_prefix(mname, "pc-")) {
> +        return;
> +    }
> +    data = g_malloc(sizeof(PCTestData));
> +    data->machine = g_strdup(mname);
> +    data->cpu_model = "Haswell"; /* 1.3+ theoretically */
> +    data->sockets = 1;
> +    data->cores = 3;
> +    data->threads = 2;
> +    data->maxcpus = data->sockets * data->cores * data->threads * 2;
> +    if (g_str_has_suffix(mname, "-1.4") ||
> +        (strcmp(mname, "pc-1.3") == 0) ||
> +        (strcmp(mname, "pc-1.2") == 0) ||
> +        (strcmp(mname, "pc-1.1") == 0) ||
> +        (strcmp(mname, "pc-1.0") == 0) ||
> +        (strcmp(mname, "pc-0.15") == 0) ||
> +        (strcmp(mname, "pc-0.14") == 0) ||
> +        (strcmp(mname, "pc-0.13") == 0) ||
> +        (strcmp(mname, "pc-0.12") == 0) ||
> +        (strcmp(mname, "pc-0.11") == 0) ||
> +        (strcmp(mname, "pc-0.10") == 0)) {
> +        path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
> +                               mname, data->sockets, data->cores,
> +                               data->threads, data->maxcpus);
> +        qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
> +                                 test_data_free);
> +        g_free(path);
> +    } else {
> +        path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
> +                               mname, data->sockets, data->cores,
> +                               data->threads, data->maxcpus);
> +        qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
> +                                 test_data_free);
> +        g_free(path);
>      }
> -    QDECREF(response);
> -    qtest_end();
>  }
>
>  int main(int argc, char **argv)
> @@ -151,7 +128,7 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
>
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -        add_pc_test_cases();
> +        qtest_cb_for_every_machine(add_pc_test_case);
>      }
>
>      return g_test_run();
> diff --git a/tests/qom-test.c b/tests/qom-test.c
> index d48f890..ab0595d 100644
> --- a/tests/qom-test.c
> +++ b/tests/qom-test.c
> @@ -107,46 +107,22 @@ static void test_machine(gconstpointer data)
>      g_free((void *)machine);
>  }
>
> -static void add_machine_test_cases(void)
> +static void add_machine_test_case(const char *mname)
>  {
>      const char *arch = qtest_get_arch();
> -    QDict *response, *minfo;
> -    QList *list;
> -    const QListEntry *p;
> -    QObject *qobj;
> -    QString *qstr;
> -    const char *mname;
>
> -    qtest_start("-machine none");
> -    response = qmp("{ 'execute': 'query-machines' }");
> -    g_assert(response);
> -    list = qdict_get_qlist(response, "return");
> -    g_assert(list);
> -
> -    for (p = qlist_first(list); p; p = qlist_next(p)) {
> -        minfo = qobject_to_qdict(qlist_entry_obj(p));
> -        g_assert(minfo);
> -        qobj = qdict_get(minfo, "name");
> -        g_assert(qobj);
> -        qstr = qobject_to_qstring(qobj);
> -        g_assert(qstr);
> -        mname = qstring_get_str(qstr);
> -        if (!is_blacklisted(arch, mname)) {
> -            char *path = g_strdup_printf("qom/%s", mname);
> -            qtest_add_data_func(path, g_strdup(mname), test_machine);
> -            g_free(path);
> -        }
> +    if (!is_blacklisted(arch, mname)) {
> +        char *path = g_strdup_printf("qom/%s", mname);
> +        qtest_add_data_func(path, g_strdup(mname), test_machine);
> +        g_free(path);
>      }
> -
> -    qtest_end();
> -    QDECREF(response);
>  }
>
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
>
> -    add_machine_test_cases();
> +    qtest_cb_for_every_machine(add_machine_test_case);
>
>      return g_test_run();
>  }
>

[Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands

[Thomas Huth]

HMP commands do not get any automatic testing yet, so on certain
QEMU machines, some HMP commands were causing crashes in the past.
Thus we should test HMP commands in our test suite, too, to avoid
that such problems creep in again in the future.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include |   2 +
 tests/test-hmp.c       | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 tests/test-hmp.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index f3de81f..0e754ac 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -331,6 +331,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
+check-qtest-generic-y += tests/test-hmp$(EXESUF)
 
 qapi-schema += alternate-any.json
 qapi-schema += alternate-array.json
@@ -720,6 +721,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
 tests/display-vga-test$(EXESUF): tests/display-vga-test.o
 tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
 tests/qom-test$(EXESUF): tests/qom-test.o
+tests/test-hmp$(EXESUF): tests/test-hmp.o
 tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
 tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
 tests/nvme-test$(EXESUF): tests/nvme-test.o
diff --git a/tests/test-hmp.c b/tests/test-hmp.c
new file mode 100644
index 0000000..339ed41
--- /dev/null
+++ b/tests/test-hmp.c
@@ -0,0 +1,161 @@
+/*
+ * Test HMP commands.
+ *
+ * Copyright (c) 2017 Red Hat Inc.
+ *
+ * Author:
+ *    Thomas Huth <thuth@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
+ * or later. See the COPYING file in the top-level directory.
+ *
+ * This test calls some HMP commands for all machines that the current
+ * QEMU binary provides, to check whether they terminate successfully
+ * (i.e. do not crash QEMU).
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+static int verbose;
+
+static const char *hmp_cmds[] = {
+    "boot_set ndc",
+    "chardev-add null,id=testchardev1",
+    "chardev-remove testchardev1",
+    "commit all",
+    "cpu-add 1",
+    "cpu 0",
+    "device_add ?",
+    "device_add usb-mouse,id=mouse1",
+    "mouse_button 7",
+    "mouse_move 10 10",
+    "mouse_button 0",
+    "device_del mouse1",
+    "dump-guest-memory /dev/null 0 4096",
+    "gdbserver",
+    "host_net_add user id=net0",
+    "hostfwd_add tcp::43210-:43210",
+    "hostfwd_remove tcp::43210-:43210",
+    "host_net_remove 0 net0",
+    "i /w 0",
+    "log all",
+    "log none",
+    "memsave 0 4096 \"/dev/null\"",
+    "migrate_set_cache_size 1",
+    "migrate_set_downtime 1",
+    "migrate_set_speed 1",
+    "netdev_add user,id=net1",
+    "set_link net1 off",
+    "set_link net1 on",
+    "netdev_del net1",
+    "nmi",
+    "o /w 0 0x1234",
+    "object_add memory-backend-ram,id=mem1,size=256M",
+    "object_del mem1",
+    "pmemsave 0 4096 \"/dev/null\"",
+    "p $pc + 8",
+    "qom-list /",
+    "qom-set /machine initrd test",
+    "screendump /dev/null",
+    "sendkey x",
+    "singlestep on",
+    "wavcapture /dev/null",
+    "stopcapture 0",
+    "sum 0 512",
+    "x /8i 0x100",
+    "xp /16x 0",
+    NULL
+};
+
+/* Run through the list of pre-defined commands */
+static void test_commands(void)
+{
+    char *response;
+    int i;
+
+    for (i = 0; hmp_cmds[i] != NULL; i++) {
+        if (verbose) {
+            fprintf(stderr, "\t%s\n", hmp_cmds[i]);
+        }
+        response = hmp(hmp_cmds[i]);
+        g_free(response);
+    }
+
+}
+
+/* Run through all info commands and call them blindly (without arguments) */
+static void test_info_commands(void)
+{
+    char *resp, *info, *info_buf, *endp;
+
+    info_buf = info = hmp("help info");
+
+    while (*info) {
+        /* Extract the info command, ignore parameters and description */
+        g_assert(strncmp(info, "info ", 5) == 0);
+        endp = strchr(&info[5], ' ');
+        g_assert(endp != NULL);
+        *endp = '\0';
+        /* Now run the info command */
+        if (verbose) {
+            fprintf(stderr, "\t%s\n", info);
+        }
+        resp = hmp(info);
+        g_free(resp);
+        /* And move forward to the next line */
+        info = strchr(endp + 1, '\n');
+        if (!info) {
+            break;
+        }
+        info += 1;
+    }
+
+    g_free(info_buf);
+}
+
+static void test_machine(gconstpointer data)
+{
+    const char *machine = data;
+    char *args;
+
+    args = g_strdup_printf("-S -M %s", machine);
+    qtest_start(args);
+
+    test_info_commands();
+    test_commands();
+
+    qtest_end();
+    g_free(args);
+    g_free((void *)data);
+}
+
+static void add_machine_test_case(const char *mname)
+{
+    char *path = g_strdup_printf("hmp/%s", mname);
+
+    /* Ignore blacklisted machines that have known problems */
+    if (!strcmp("isapc", mname) == 0 ||  !strcmp("puv3", mname)
+        || !strcmp("tricore_testboard", mname)
+        || !strcmp("xenfv", mname) == 0 || !strcmp("xenpv", mname)) {
+        return;
+    }
+
+    qtest_add_data_func(path, g_strdup(mname), test_machine);
+    g_free(path);
+}
+
+int main(int argc, char **argv)
+{
+    char *v_env = getenv("V");
+
+    if (v_env && *v_env >= '2') {
+        verbose = true;
+    }
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_cb_for_every_machine(add_machine_test_case);
+
+    return g_test_run();
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> HMP commands do not get any automatic testing yet, so on certain
> QEMU machines, some HMP commands were causing crashes in the past.
> Thus we should test HMP commands in our test suite, too, to avoid
> that such problems creep in again in the future.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |   2 +
>  tests/test-hmp.c       | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 163 insertions(+)
>  create mode 100644 tests/test-hmp.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index f3de81f..0e754ac 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -331,6 +331,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>  check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
> +check-qtest-generic-y += tests/test-hmp$(EXESUF)
>  
>  qapi-schema += alternate-any.json
>  qapi-schema += alternate-array.json
> @@ -720,6 +721,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
>  tests/display-vga-test$(EXESUF): tests/display-vga-test.o
>  tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
>  tests/qom-test$(EXESUF): tests/qom-test.o
> +tests/test-hmp$(EXESUF): tests/test-hmp.o
>  tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
>  tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
>  tests/nvme-test$(EXESUF): tests/nvme-test.o
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> new file mode 100644
> index 0000000..339ed41
> --- /dev/null
> +++ b/tests/test-hmp.c
> @@ -0,0 +1,161 @@
> +/*
> + * Test HMP commands.
> + *
> + * Copyright (c) 2017 Red Hat Inc.
> + *
> + * Author:
> + *    Thomas Huth <thuth@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2
> + * or later. See the COPYING file in the top-level directory.
> + *
> + * This test calls some HMP commands for all machines that the current
> + * QEMU binary provides, to check whether they terminate successfully
> + * (i.e. do not crash QEMU).
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +
> +static int verbose;
> +
> +static const char *hmp_cmds[] = {
> +    "boot_set ndc",
> +    "chardev-add null,id=testchardev1",
> +    "chardev-remove testchardev1",
> +    "commit all",
> +    "cpu-add 1",
> +    "cpu 0",
> +    "device_add ?",
> +    "device_add usb-mouse,id=mouse1",
> +    "mouse_button 7",
> +    "mouse_move 10 10",
> +    "mouse_button 0",
> +    "device_del mouse1",
> +    "dump-guest-memory /dev/null 0 4096",
> +    "gdbserver",
> +    "host_net_add user id=net0",
> +    "hostfwd_add tcp::43210-:43210",
> +    "hostfwd_remove tcp::43210-:43210",
> +    "host_net_remove 0 net0",
> +    "i /w 0",
> +    "log all",
> +    "log none",
> +    "memsave 0 4096 \"/dev/null\"",
> +    "migrate_set_cache_size 1",
> +    "migrate_set_downtime 1",
> +    "migrate_set_speed 1",
> +    "netdev_add user,id=net1",
> +    "set_link net1 off",
> +    "set_link net1 on",
> +    "netdev_del net1",
> +    "nmi",
> +    "o /w 0 0x1234",
> +    "object_add memory-backend-ram,id=mem1,size=256M",
> +    "object_del mem1",
> +    "pmemsave 0 4096 \"/dev/null\"",
> +    "p $pc + 8",
> +    "qom-list /",
> +    "qom-set /machine initrd test",
> +    "screendump /dev/null",
> +    "sendkey x",
> +    "singlestep on",
> +    "wavcapture /dev/null",
> +    "stopcapture 0",
> +    "sum 0 512",
> +    "x /8i 0x100",
> +    "xp /16x 0",
> +    NULL
> +};
> +
> +/* Run through the list of pre-defined commands */
> +static void test_commands(void)
> +{
> +    char *response;
> +    int i;
> +
> +    for (i = 0; hmp_cmds[i] != NULL; i++) {
> +        if (verbose) {
> +            fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> +        }
> +        response = hmp(hmp_cmds[i]);
> +        g_free(response);
> +    }
> +
> +}
> +
> +/* Run through all info commands and call them blindly (without arguments) */
> +static void test_info_commands(void)
> +{
> +    char *resp, *info, *info_buf, *endp;
> +
> +    info_buf = info = hmp("help info");
> +
> +    while (*info) {
> +        /* Extract the info command, ignore parameters and description */
> +        g_assert(strncmp(info, "info ", 5) == 0);
> +        endp = strchr(&info[5], ' ');
> +        g_assert(endp != NULL);
> +        *endp = '\0';
> +        /* Now run the info command */
> +        if (verbose) {
> +            fprintf(stderr, "\t%s\n", info);
> +        }
> +        resp = hmp(info);
> +        g_free(resp);
> +        /* And move forward to the next line */
> +        info = strchr(endp + 1, '\n');
> +        if (!info) {
> +            break;
> +        }
> +        info += 1;
> +    }
> +
> +    g_free(info_buf);
> +}
> +
> +static void test_machine(gconstpointer data)
> +{
> +    const char *machine = data;
> +    char *args;
> +
> +    args = g_strdup_printf("-S -M %s", machine);
> +    qtest_start(args);
> +
> +    test_info_commands();
> +    test_commands();
> +
> +    qtest_end();
> +    g_free(args);
> +    g_free((void *)data);
> +}
> +
> +static void add_machine_test_case(const char *mname)
> +{
> +    char *path = g_strdup_printf("hmp/%s", mname);
> +
> +    /* Ignore blacklisted machines that have known problems */
> +    if (!strcmp("isapc", mname) == 0 ||  !strcmp("puv3", mname)
> +        || !strcmp("tricore_testboard", mname)
> +        || !strcmp("xenfv", mname) == 0 || !strcmp("xenpv", mname)) {

You've got a leak of 'path' there.
However, it's only a test and it's small, so we can fix it up later.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and series queued.

Dave

> +        return;
> +    }
> +
> +    qtest_add_data_func(path, g_strdup(mname), test_machine);
> +    g_free(path);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    char *v_env = getenv("V");
> +
> +    if (v_env && *v_env >= '2') {
> +        verbose = true;
> +    }
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_cb_for_every_machine(add_machine_test_case);
> +
> +    return g_test_run();
> +}
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

On 03/30/2017 02:50 AM, Thomas Huth wrote:
> HMP commands do not get any automatic testing yet, so on certain
> QEMU machines, some HMP commands were causing crashes in the past.
> Thus we should test HMP commands in our test suite, too, to avoid
> that such problems creep in again in the future.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |   2 +
>  tests/test-hmp.c       | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 163 insertions(+)
>  create mode 100644 tests/test-hmp.c

We should also update tests/.gitignore to make sure tests/test-hmp
doesn't get accidentally committed.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

On 03/30/2017 02:50 AM, Thomas Huth wrote:
> HMP commands do not get any automatic testing yet, so on certain
> QEMU machines, some HMP commands were causing crashes in the past.
> Thus we should test HMP commands in our test suite, too, to avoid
> that such problems creep in again in the future.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

> +static const char *hmp_cmds[] = {
> +    "boot_set ndc",

> +/* Run through the list of pre-defined commands */
> +static void test_commands(void)
> +{
> +    char *response;
> +    int i;
> +
> +    for (i = 0; hmp_cmds[i] != NULL; i++) {
> +        if (verbose) {
> +            fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> +        }
> +        response = hmp(hmp_cmds[i]);

I failed to notice this sooner, but hmp() is passing its first arg as a
format string through a printf family.  If hmp_cmds[i] ever gets
modified to include something with a %, it will misbehave.  Better is to
use hmp("%s", variable).

I've patched it locally as part of rebasing my work on avoiding dynamic
JSON format strings, if no one beats me to a fix (my series also adds
the gcc format attribute tag, so that the compiler catches any further
mismatches in hmp() format vs. arguments).

> +    while (*info) {
> +        /* Extract the info command, ignore parameters and description */
> +        g_assert(strncmp(info, "info ", 5) == 0);
> +        endp = strchr(&info[5], ' ');
> +        g_assert(endp != NULL);
> +        *endp = '\0';
> +        /* Now run the info command */
> +        if (verbose) {
> +            fprintf(stderr, "\t%s\n", info);
> +        }
> +        resp = hmp(info);

Another instance.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 03/30/2017 02:50 AM, Thomas Huth wrote:
> > HMP commands do not get any automatic testing yet, so on certain
> > QEMU machines, some HMP commands were causing crashes in the past.
> > Thus we should test HMP commands in our test suite, too, to avoid
> > that such problems creep in again in the future.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> 
> > +static const char *hmp_cmds[] = {
> > +    "boot_set ndc",
> 
> > +/* Run through the list of pre-defined commands */
> > +static void test_commands(void)
> > +{
> > +    char *response;
> > +    int i;
> > +
> > +    for (i = 0; hmp_cmds[i] != NULL; i++) {
> > +        if (verbose) {
> > +            fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> > +        }
> > +        response = hmp(hmp_cmds[i]);
> 
> I failed to notice this sooner, but hmp() is passing its first arg as a
> format string through a printf family.  If hmp_cmds[i] ever gets
> modified to include something with a %, it will misbehave.  Better is to
> use hmp("%s", variable).
> 
> I've patched it locally as part of rebasing my work on avoiding dynamic
> JSON format strings, if no one beats me to a fix (my series also adds
> the gcc format attribute tag, so that the compiler catches any further
> mismatches in hmp() format vs. arguments).

Ah yes, good spot.  Please include that fix.

Dave

> > +    while (*info) {
> > +        /* Extract the info command, ignore parameters and description */
> > +        g_assert(strncmp(info, "info ", 5) == 0);
> > +        endp = strchr(&info[5], ' ');
> > +        g_assert(endp != NULL);
> > +        *endp = '\0';
> > +        /* Now run the info command */
> > +        if (verbose) {
> > +            fprintf(stderr, "\t%s\n", info);
> > +        }
> > +        resp = hmp(info);
> 
> Another instance.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK