[Qemu-devel] [PATCH 04/20] libqtest: Clean up how we read the QMP greeting

Markus Armbruster posted 20 patches 7 years, 3 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 04/20] libqtest: Clean up how we read the QMP greeting
Posted by Markus Armbruster 7 years, 3 months ago
qtest_init() still uses the qtest_qmp_discard_response(s, "") hack to
receive the greeting, even though we have qtest_qmp_receive() since
commit 66e0c7b187e.  Put it to use.

Bonus: gets rid of an empty format string.  A step towards
compile-time format string checking without triggering
-Wformat-zero-length.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 071d7eb7b1..c2c08a890c 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -249,9 +249,11 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
+    QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
-    qtest_qmp_discard_response(s, "");
+    greeting = qtest_qmp_receive(s);
+    qobject_unref(greeting);
     qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
 
     return s;
-- 
2.17.1


Re: [Qemu-devel] [PATCH 04/20] libqtest: Clean up how we read the QMP greeting
Posted by Thomas Huth 7 years, 3 months ago
On 12.07.2018 13:12, Markus Armbruster wrote:
> qtest_init() still uses the qtest_qmp_discard_response(s, "") hack to
> receive the greeting, even though we have qtest_qmp_receive() since
> commit 66e0c7b187e.  Put it to use.
> 
> Bonus: gets rid of an empty format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-zero-length.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqtest.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 071d7eb7b1..c2c08a890c 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -249,9 +249,11 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
>  QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
> +    QDict *greeting;
>  
>      /* Read the QMP greeting and then do the handshake */
> -    qtest_qmp_discard_response(s, "");
> +    greeting = qtest_qmp_receive(s);
> +    qobject_unref(greeting);
>      qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
>  
>      return s;

I wonder whether we should actually check the greeting for some expected
information? Anyway, that's something for later, and not related to your
patch.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 04/20] libqtest: Clean up how we read the QMP greeting
Posted by Markus Armbruster 7 years, 3 months ago
Thomas Huth <thuth@redhat.com> writes:

> On 12.07.2018 13:12, Markus Armbruster wrote:
>> qtest_init() still uses the qtest_qmp_discard_response(s, "") hack to
>> receive the greeting, even though we have qtest_qmp_receive() since
>> commit 66e0c7b187e.  Put it to use.
>> 
>> Bonus: gets rid of an empty format string.  A step towards
>> compile-time format string checking without triggering
>> -Wformat-zero-length.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/libqtest.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index 071d7eb7b1..c2c08a890c 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -249,9 +249,11 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
>>  QTestState *qtest_init(const char *extra_args)
>>  {
>>      QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
>> +    QDict *greeting;
>>  
>>      /* Read the QMP greeting and then do the handshake */
>> -    qtest_qmp_discard_response(s, "");
>> +    greeting = qtest_qmp_receive(s);
>> +    qobject_unref(greeting);
>>      qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
>>  
>>      return s;
>
> I wonder whether we should actually check the greeting for some expected
> information? Anyway, that's something for later, and not related to your
> patch.

This kind of sloppy testing is quite common.

Checking test results manually in code maximizes flexibility.  It also
maximizes temptation to cut corners, because doing a complete job is so
tedious.  One reason I prefer "normalize and diff against expected
results".  The closest we got to support for that way of testing is
qlit_equal_qobject().

Digression: QOM also maximizes flexibility by doing stuff in code rather
than data.  The two go back to the same stratum of QEMU development.
Hardly coincidence, in my opinion.  We've since extended QOM to support
doing more in data, but there's still plenty of old code doing it in
code, and plenty of new code following that old code's lead.  Recovering
from such damage is hard work.

> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!