diff mbox series

[04/20] libqtest: Clean up how we read the QMP greeting

Message ID 20180712111221.20326-5-armbru@redhat.com
State New
Headers show
Series tests: Compile-time format string checking for libqtest.h | expand

Commit Message

Markus Armbruster July 12, 2018, 11:12 a.m. UTC
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(-)

Comments

Thomas Huth July 12, 2018, 4:26 p.m. UTC | #1
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>
Markus Armbruster July 16, 2018, 6:16 a.m. UTC | #2
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!
diff mbox series

Patch

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;