qtest_qmp_discard_response(...) is shorthand for
qobject_unref(qtest_qmp(...), except it's not actually shorter.
Moreover, the presence of these functions encourage sloppy testing.
Remove them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/ahci-test.c              | 12 ++++++------
 tests/boot-order-test.c        |  4 ++--
 tests/drive_del-test.c         |  4 ++--
 tests/fdc-test.c               |  9 +++++----
 tests/ide-test.c               |  4 ++--
 tests/libqtest.c               | 27 +-------------------------
 tests/libqtest.h               | 35 ----------------------------------
 tests/migration-test.c         |  2 +-
 tests/test-filter-mirror.c     |  3 ++-
 tests/test-filter-redirector.c |  5 +++--
 tests/virtio-blk-test.c        | 19 +++++++++---------
 11 files changed, 34 insertions(+), 90 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 19a7ab1bca..7a2d70fa34 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1596,8 +1596,8 @@ static void test_atapi_tray(void)
     rsp = qmp_receive();
     qobject_unref(rsp);
 
-    qmp_discard_response("{'execute': 'blockdev-remove-medium', "
-                         "'arguments': {'id': 'cd0'}}");
+    qobject_unref(qmp("{'execute': 'blockdev-remove-medium', "
+                      "'arguments': {'id': 'cd0'}}"));
 
     /* Test the tray without a medium */
     ahci_atapi_load(ahci, port);
@@ -1607,14 +1607,14 @@ static void test_atapi_tray(void)
     atapi_wait_tray(true);
 
     /* Re-insert media */
-    qmp_discard_response("{'execute': 'blockdev-add', "
+    qobject_unref(qmp("{'execute': 'blockdev-add', "
                           "'arguments': {'node-name': 'node0', "
                                         "'driver': 'raw', "
                                         "'file': { 'driver': 'file', "
-                                                  "'filename': %s }}}", iso);
-    qmp_discard_response("{'execute': 'blockdev-insert-medium',"
+                      "'filename': %s }}}", iso));
+    qobject_unref(qmp("{'execute': 'blockdev-insert-medium',"
                           "'arguments': { 'id': 'cd0', "
-                                         "'node-name': 'node0' }}");
+                      "'node-name': 'node0' }}"));
 
     /* Again, the event shows up first */
     qmp_send("{'execute': 'blockdev-close-tray',"
diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index e70f5dedba..2ec86c0ee2 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -13,7 +13,7 @@
 #include "qemu/osdep.h"
 #include "libqos/fw_cfg.h"
 #include "libqtest.h"
-
+#include "qapi/qmp/qdict.h"
 #include "hw/nvram/fw_cfg_keys.h"
 
 typedef struct {
@@ -36,7 +36,7 @@ static void test_a_boot_order(const char *machine,
                                 test_args);
     actual = read_boot_order();
     g_assert_cmphex(actual, ==, expected_boot);
-    qmp_discard_response("{ 'execute': 'system_reset' }");
+    qobject_unref(qmp("{ 'execute': 'system_reset' }"));
     /*
      * system_reset only requests reset.  We get a RESET event after
      * the actual reset completes.  Need to wait for that.
diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 852fefc8f3..92489be0ba 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -36,8 +36,8 @@ static void device_del(void)
     QDict *response;
 
     /* Complication: ignore DEVICE_DELETED event */
-    qmp_discard_response("{'execute': 'device_del',"
-                         " 'arguments': { 'id': 'dev0' } }");
+    qobject_unref(qmp("{'execute': 'device_del',"
+                      " 'arguments': { 'id': 'dev0' } }"));
     response = qmp_receive();
     g_assert(response);
     g_assert(qdict_haskey(response, "return"));
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 325712e0f2..5775555e71 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -26,6 +26,7 @@
 
 
 #include "libqtest.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu-common.h"
 
 #define TEST_IMAGE_SIZE 1440 * 1024
@@ -298,9 +299,9 @@ static void test_media_insert(void)
 
     /* Insert media in drive. DSKCHK should not be reset until a step pulse
      * is sent. */
-    qmp_discard_response("{'execute':'blockdev-change-medium', 'arguments':{"
+    qobject_unref(qmp("{'execute':'blockdev-change-medium', 'arguments':{"
                          " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
-                         test_image);
+                      test_image));
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
@@ -329,8 +330,8 @@ static void test_media_change(void)
 
     /* Eject the floppy and check that DSKCHG is set. Reading it out doesn't
      * reset the bit. */
-    qmp_discard_response("{'execute':'eject', 'arguments':{"
-                         " 'id':'floppy0' }}");
+    qobject_unref(qmp("{'execute':'eject', 'arguments':{"
+                      " 'id':'floppy0' }}"));
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 834ee9005b..48bb0e6519 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -29,7 +29,7 @@
 #include "libqos/libqos.h"
 #include "libqos/pci-pc.h"
 #include "libqos/malloc-pc.h"
-
+#include "qapi/qmp/qdict.h"
 #include "qemu-common.h"
 #include "qemu/bswap.h"
 #include "hw/pci/pci_ids.h"
@@ -721,7 +721,7 @@ static void test_retry_flush(const char *machine)
     qmp_eventwait("STOP");
 
     /* Complete the command */
-    qmp_discard_response("{'execute':'cont' }");
+    qobject_unref(qmp("{'execute':'cont' }"));
 
     /* Check registers */
     data = qpci_io_readb(dev, ide_bar, reg_device);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index ed832cd8e6..2f81bc6382 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -254,7 +254,7 @@ QTestState *qtest_init(const char *extra_args)
     /* Read the QMP greeting and then do the handshake */
     greeting = qtest_qmp_receive(s);
     qobject_unref(greeting);
-    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
+    qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }"));
 
     return s;
 }
@@ -587,23 +587,6 @@ void qtest_qmp_send(QTestState *s, const char *fmt, ...)
     va_end(ap);
 }
 
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
-{
-    QDict *response = qtest_qmpv(s, fmt, ap);
-    qobject_unref(response);
-}
-
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
-{
-    va_list ap;
-    QDict *response;
-
-    va_start(ap, fmt);
-    response = qtest_qmpv(s, fmt, ap);
-    va_end(ap);
-    qobject_unref(response);
-}
-
 QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
 {
     QDict *response;
@@ -975,14 +958,6 @@ void qmp_send(const char *fmt, ...)
     va_end(ap);
 }
 
-void qmp_discard_response(const char *fmt, ...)
-{
-    va_list ap;
-
-    va_start(ap, fmt);
-    qtest_qmpv_discard_response(global_qtest, fmt, ap);
-    va_end(ap);
-}
 char *hmp(const char *fmt, ...)
 {
     va_list ap;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 327e402da5..58dea82c76 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -73,18 +73,6 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
  */
 void qtest_quit(QTestState *s);
 
-/**
- * qtest_qmp_discard_response:
- * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu, formatted like
- * qobject_from_jsonf_nofail().
- * Only understands '%((l|ll|I64)?d|[ipsf])', see parse_escape().
- *
- * Sends a QMP message to QEMU and consumes the response.
- */
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
-    GCC_FMT_ATTR(2, 3);
-
 /**
  * qtest_qmp:
  * @s: #QTestState instance to operate on.
@@ -109,19 +97,6 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
 void qtest_qmp_send(QTestState *s, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 
-/**
- * qtest_qmpv_discard_response:
- * @s: #QTestState instance to operate on.
- * @fmt: QMP message to send to QEMU, formatted like
- * qobject_from_jsonf_nofail().
- * Only understands '%((l|ll|I64)?d|[ipsf])', see parse_escape().
- * @ap: QMP message arguments
- *
- * Sends a QMP message to QEMU and consumes the response.
- */
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
-    GCC_FMT_ATTR(2, 0);
-
 /**
  * qtest_qmpv:
  * @s: #QTestState instance to operate on.
@@ -615,16 +590,6 @@ QDict *qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
  */
 void qmp_send(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
-/**
- * qmp_discard_response:
- * @fmt...: QMP message to send to qemu, formatted like
- * qobject_from_jsonf_nofail().
- * Only understands '%((l|ll|I64)?d|[ipsf])', see parse_escape().
- *
- * Sends a QMP message to QEMU and consumes the response.
- */
-void qmp_discard_response(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-
 /**
  * qmp_receive:
  *
diff --git a/tests/migration-test.c b/tests/migration-test.c
index 132c30891d..1bcd32e284 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -491,7 +491,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
             usleep(1000 * 10);
         } while (dest_byte_a == dest_byte_b);
 
-        qtest_qmp_discard_response(to, "{ 'execute' : 'stop'}");
+        qobject_unref(qtest_qmp(to, "{ 'execute' : 'stop'}"));
 
         /* With it stopped, check nothing changes */
         qtest_memread(to, start_address, &dest_byte_c, 1);
diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
index 6c6f710dc6..1ab4759b80 100644
--- a/tests/test-filter-mirror.c
+++ b/tests/test-filter-mirror.c
@@ -10,6 +10,7 @@
 
 #include "qemu/osdep.h"
 #include "libqtest.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/iov.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
@@ -57,7 +58,7 @@ static void test_mirror(void)
     };
 
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp_discard_response("{ 'execute' : 'query-status'}");
+    qobject_unref(qmp("{ 'execute' : 'query-status'}"));
     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + sizeof(send_buf));
     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
     close(send_sock[0]);
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index fbaf19bbd8..5b6c594867 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -52,6 +52,7 @@
 
 #include "qemu/osdep.h"
 #include "libqtest.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/iov.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
@@ -104,7 +105,7 @@ static void test_redirector_tx(void)
     g_assert_cmpint(recv_sock, !=, -1);
 
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp_discard_response("{ 'execute' : 'query-status'}");
+    qobject_unref(qmp("{ 'execute' : 'query-status'}"));
 
     struct iovec iov[] = {
         {
@@ -182,7 +183,7 @@ static void test_redirector_rx(void)
     send_sock = unix_connect(sock_path1, NULL);
     g_assert_cmpint(send_sock, !=, -1);
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp_discard_response("{ 'execute' : 'query-status'}");
+    qobject_unref(qmp("{ 'execute' : 'query-status'}"));
 
     ret = iov_send(send_sock, iov, 2, 0, sizeof(size) + sizeof(send_buf));
     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index d96b4c69e1..5955bf6d57 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -16,6 +16,7 @@
 #include "libqos/virtio-pci.h"
 #include "libqos/virtio-mmio.h"
 #include "libqos/malloc-generic.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/bswap.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
@@ -409,9 +410,9 @@ static void pci_config(void)
 
     qvirtio_set_driver_ok(&dev->vdev);
 
-    qmp_discard_response("{ 'execute': 'block_resize', "
-                         " 'arguments': { 'device': 'drive0', "
-                         " 'size': %d } }", n_size);
+    qobject_unref(qmp("{ 'execute': 'block_resize', "
+                      " 'arguments': { 'device': 'drive0', "
+                      " 'size': %d } }", n_size));
     qvirtio_wait_config_isr(&dev->vdev, QVIRTIO_BLK_TIMEOUT_US);
 
     capacity = qvirtio_config_readq(&dev->vdev, 0);
@@ -459,9 +460,9 @@ static void pci_msix(void)
 
     qvirtio_set_driver_ok(&dev->vdev);
 
-    qmp_discard_response("{ 'execute': 'block_resize', "
-                         " 'arguments': { 'device': 'drive0', "
-                         " 'size': %d } }", n_size);
+    qobject_unref(qmp("{ 'execute': 'block_resize', "
+                      " 'arguments': { 'device': 'drive0', "
+                      " 'size': %d } }", n_size));
 
     qvirtio_wait_config_isr(&dev->vdev, QVIRTIO_BLK_TIMEOUT_US);
 
@@ -725,9 +726,9 @@ static void mmio_basic(void)
 
     test_basic(&dev->vdev, alloc, vq);
 
-    qmp_discard_response("{ 'execute': 'block_resize', "
-                         " 'arguments': { 'device': 'drive0', "
-                         " 'size': %d } }", n_size);
+    qobject_unref(qmp("{ 'execute': 'block_resize', "
+                      " 'arguments': { 'device': 'drive0', "
+                      " 'size': %d } }", n_size));
 
     qvirtio_wait_queue_isr(&dev->vdev, vq, QVIRTIO_BLK_TIMEOUT_US);
 
-- 
2.17.1

On 07/27/2018 05:13 PM, Markus Armbruster wrote:
> qtest_qmp_discard_response(...) is shorthand for
> qobject_unref(qtest_qmp(...), except it's not actually shorter.

But the latter is IMHO harder to read.
And it might be shorter in the compiled binary (one function call vs. two).

> Moreover, the presence of these functions encourage sloppy testing.

Shouldn't we then rather fix the tests to check for valid responses
instead of replacing this function with harder-to-read code?

 Thomas

On 07/27/2018 11:46 AM, Thomas Huth wrote:
> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>> qtest_qmp_discard_response(...) is shorthand for
>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
> 
> But the latter is IMHO harder to read.

Maybe, but then it lends itself well to:

QObject *rsp = qtest_qmp(...);
qobject_unref(rsp);

which is where you do insert tests for valid responses.

> And it might be shorter in the compiled binary (one function call vs. two).

The size of the test binaries is not our biggest concern.

> 
>> Moreover, the presence of these functions encourage sloppy testing.
> 
> Shouldn't we then rather fix the tests to check for valid responses
> instead of replacing this function with harder-to-read code?

I think such fixes are now easier to make, but can be separate followup 
patches. The mechanical conversion is fine to me.

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

Eric Blake <eblake@redhat.com> writes:

> On 07/27/2018 11:46 AM, Thomas Huth wrote:
>> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>>> qtest_qmp_discard_response(...) is shorthand for
>>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>>
>> But the latter is IMHO harder to read.

Doing things sloppily looks a bit uglier now.  That's a feature.

> Maybe, but then it lends itself well to:
>
> QObject *rsp = qtest_qmp(...);
> qobject_unref(rsp);
>
> which is where you do insert tests for valid responses.
>
>> And it might be shorter in the compiled binary (one function call vs. two).

I'd be quite sympathetic to this argument...

> The size of the test binaries is not our biggest concern.

... outside tests/.

>>> Moreover, the presence of these functions encourage sloppy testing.
>>
>> Shouldn't we then rather fix the tests to check for valid responses
>> instead of replacing this function with harder-to-read code?

I'd welcome such patches, but this series is already pretty long.

> I think such fixes are now easier to make, but can be separate
> followup patches. The mechanical conversion is fine to me.

On 07/30/2018 08:32 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 07/27/2018 11:46 AM, Thomas Huth wrote:
>>> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>>>> qtest_qmp_discard_response(...) is shorthand for
>>>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>>>
>>> But the latter is IMHO harder to read.
> 
> Doing things sloppily looks a bit uglier now.  That's a feature.
> 
>> Maybe, but then it lends itself well to:
>>
>> QObject *rsp = qtest_qmp(...);
>> qobject_unref(rsp);
>>
>> which is where you do insert tests for valid responses.
>>
>>> And it might be shorter in the compiled binary (one function call vs. two).
> 
> I'd be quite sympathetic to this argument...
> 
>> The size of the test binaries is not our biggest concern.
> 
> ... outside tests/.
> 
>>>> Moreover, the presence of these functions encourage sloppy testing.
>>>
>>> Shouldn't we then rather fix the tests to check for valid responses
>>> instead of replacing this function with harder-to-read code?
> 
> I'd welcome such patches, but this series is already pretty long.

Then maybe rather drop this patch from this series, and fix the issues
in a separate series instead?

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 07/30/2018 08:32 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 07/27/2018 11:46 AM, Thomas Huth wrote:
>>>> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>>>>> qtest_qmp_discard_response(...) is shorthand for
>>>>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>>>>
>>>> But the latter is IMHO harder to read.
>> 
>> Doing things sloppily looks a bit uglier now.  That's a feature.
>> 
>>> Maybe, but then it lends itself well to:
>>>
>>> QObject *rsp = qtest_qmp(...);
>>> qobject_unref(rsp);
>>>
>>> which is where you do insert tests for valid responses.
>>>
>>>> And it might be shorter in the compiled binary (one function call vs. two).
>> 
>> I'd be quite sympathetic to this argument...
>> 
>>> The size of the test binaries is not our biggest concern.
>> 
>> ... outside tests/.
>> 
>>>>> Moreover, the presence of these functions encourage sloppy testing.
>>>>
>>>> Shouldn't we then rather fix the tests to check for valid responses
>>>> instead of replacing this function with harder-to-read code?
>> 
>> I'd welcome such patches, but this series is already pretty long.
>
> Then maybe rather drop this patch from this series, and fix the issues
> in a separate series instead?

Do you insist?

I fail to see how changing

    qmp_discard_response("{ 'execute': 'system_reset' }");

to

    qobject_unref(qmp("{ 'execute': 'system_reset' }"));

is so awful it would justify demanding I pause my work on libqtest to
first figure out which parts of ignored responses are worth checking,
then code up the checks.

Would you accept

    rsp = qmp("{ 'execute': 'system_reset' }"));
    qobject_unref(rsp);

?

If none of the above is acceptable to you, then I'll push the crap that
needs to go from libqtest into the crap-using tests, like this:

    /* TODO actually test the results and get rid of this */
    #define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__));

On 08/02/2018 06:53 AM, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 07/30/2018 08:32 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 07/27/2018 11:46 AM, Thomas Huth wrote:
>>>>> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>>>>>> qtest_qmp_discard_response(...) is shorthand for
>>>>>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>>>>>
>>>>> But the latter is IMHO harder to read.
>>>
>>> Doing things sloppily looks a bit uglier now.  That's a feature.
>>>
>>>> Maybe, but then it lends itself well to:
>>>>
>>>> QObject *rsp = qtest_qmp(...);
>>>> qobject_unref(rsp);
>>>>
>>>> which is where you do insert tests for valid responses.
>>>>
>>>>> And it might be shorter in the compiled binary (one function call vs. two).
>>>
>>> I'd be quite sympathetic to this argument...
>>>
>>>> The size of the test binaries is not our biggest concern.
>>>
>>> ... outside tests/.
>>>
>>>>>> Moreover, the presence of these functions encourage sloppy testing.
>>>>>
>>>>> Shouldn't we then rather fix the tests to check for valid responses
>>>>> instead of replacing this function with harder-to-read code?
>>>
>>> I'd welcome such patches, but this series is already pretty long.
>>
>> Then maybe rather drop this patch from this series, and fix the issues
>> in a separate series instead?
> 
> Do you insist?

No. But I'd still like to convince you that this patch is unnecessary
right now.

> I fail to see how changing
> 
>     qmp_discard_response("{ 'execute': 'system_reset' }");
> 
> to
> 
>     qobject_unref(qmp("{ 'execute': 'system_reset' }"));
> 
> is so awful it would justify demanding I pause my work on libqtest to
> first figure out which parts of ignored responses are worth checking,
> then code up the checks.

First, you don't have to pause this series just because of this, since
the remaining two patches do not depend on this one.
Then, I still fail to see the real benefit here. You've found something
that needs proper clean up later (by adding checks for valid responses).
So IMHO simply add a big fat warning comment to the description of
qmp_discard_response would be sufficient. Then you can easily grep for
"qmp_discard_response" later to find the spots that need fixing. If you
replace it with that ugly nested construct instead, we still should fix
it later, but it's a little bit harder to grep, and since we need to
change it later again anyway, it just sounds like unnecessary code churn
to me. So do you really need this so badly (for your later work?), or
could you simply skip this patch?

> Would you accept
> 
>     rsp = qmp("{ 'execute': 'system_reset' }"));
>     qobject_unref(rsp);
> 
> ?

While this is easier to read, I think we lose the easy way to grep for
the spots that need fixing later here, so let's better not do this.

> If none of the above is acceptable to you, then I'll push the crap that
> needs to go from libqtest into the crap-using tests, like this:
> 
>     /* TODO actually test the results and get rid of this */
>     #define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__));

Fine for me.

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 08/02/2018 06:53 AM, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 07/30/2018 08:32 AM, Markus Armbruster wrote:
>>>> Eric Blake <eblake@redhat.com> writes:
>>>>
>>>>> On 07/27/2018 11:46 AM, Thomas Huth wrote:
>>>>>> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>>>>>>> qtest_qmp_discard_response(...) is shorthand for
>>>>>>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>>>>>>
>>>>>> But the latter is IMHO harder to read.
>>>>
>>>> Doing things sloppily looks a bit uglier now.  That's a feature.
>>>>
>>>>> Maybe, but then it lends itself well to:
>>>>>
>>>>> QObject *rsp = qtest_qmp(...);
>>>>> qobject_unref(rsp);
>>>>>
>>>>> which is where you do insert tests for valid responses.
>>>>>
>>>>>> And it might be shorter in the compiled binary (one function call vs. two).
>>>>
>>>> I'd be quite sympathetic to this argument...
>>>>
>>>>> The size of the test binaries is not our biggest concern.
>>>>
>>>> ... outside tests/.
>>>>
>>>>>>> Moreover, the presence of these functions encourage sloppy testing.
>>>>>>
>>>>>> Shouldn't we then rather fix the tests to check for valid responses
>>>>>> instead of replacing this function with harder-to-read code?
>>>>
>>>> I'd welcome such patches, but this series is already pretty long.
>>>
>>> Then maybe rather drop this patch from this series, and fix the issues
>>> in a separate series instead?
>> 
>> Do you insist?
>
> No. But I'd still like to convince you that this patch is unnecessary
> right now.
>
>> I fail to see how changing
>> 
>>     qmp_discard_response("{ 'execute': 'system_reset' }");
>> 
>> to
>> 
>>     qobject_unref(qmp("{ 'execute': 'system_reset' }"));
>> 
>> is so awful it would justify demanding I pause my work on libqtest to
>> first figure out which parts of ignored responses are worth checking,
>> then code up the checks.
>
> First, you don't have to pause this series just because of this, since
> the remaining two patches do not depend on this one.

I intend to swap with the previous patch in v3 to reduce churn.

> Then, I still fail to see the real benefit here. You've found something
> that needs proper clean up later (by adding checks for valid responses).
> So IMHO simply add a big fat warning comment to the description of
> qmp_discard_response would be sufficient.

Warnings in function comments are ineffective at counterproliferation.
People copy code without examining the called functions' comments.

>                                           Then you can easily grep for
> "qmp_discard_response" later to find the spots that need fixing. If you
> replace it with that ugly nested construct instead, we still should fix
> it later, but it's a little bit harder to grep, and since we need to
> change it later again anyway, it just sounds like unnecessary code churn
> to me. So do you really need this so badly (for your later work?), or
> could you simply skip this patch?
>
>> Would you accept
>> 
>>     rsp = qmp("{ 'execute': 'system_reset' }"));
>>     qobject_unref(rsp);
>> 
>> ?
>
> While this is easier to read, I think we lose the easy way to grep for
> the spots that need fixing later here, so let's better not do this.
>
>> If none of the above is acceptable to you, then I'll push the crap that
>> needs to go from libqtest into the crap-using tests, like this:
>> 
>>     /* TODO actually test the results and get rid of this */
>>     #define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__));
>
> Fine for me.

Sold.

On 07/27/2018 10:13 AM, Markus Armbruster wrote:
> qtest_qmp_discard_response(...) is shorthand for
> qobject_unref(qtest_qmp(...), except it's not actually shorter.
> Moreover, the presence of these functions encourage sloppy testing.
> Remove them.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>


> +++ b/tests/ahci-test.c
> @@ -1607,14 +1607,14 @@ static void test_atapi_tray(void)
>       atapi_wait_tray(true);
>   
>       /* Re-insert media */
> -    qmp_discard_response("{'execute': 'blockdev-add', "
> +    qobject_unref(qmp("{'execute': 'blockdev-add', "
>                             "'arguments': {'node-name': 'node0', "
>                                           "'driver': 'raw', "
>                                           "'file': { 'driver': 'file', "
> -                                                  "'filename': %s }}}", iso);
> -    qmp_discard_response("{'execute': 'blockdev-insert-medium',"
> +                      "'filename': %s }}}", iso));

Why did you fix indentation for some, but not all, of the lines here?

> +    qobject_unref(qmp("{'execute': 'blockdev-insert-medium',"
>                             "'arguments': { 'id': 'cd0', "
> -                                         "'node-name': 'node0' }}");
> +                      "'node-name': 'node0' }}"));

Again, indentation looks odd.

> +++ b/tests/fdc-test.c
> @@ -26,6 +26,7 @@
>   
>   
>   #include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
>   #include "qemu-common.h"
>   
>   #define TEST_IMAGE_SIZE 1440 * 1024
> @@ -298,9 +299,9 @@ static void test_media_insert(void)
>   
>       /* Insert media in drive. DSKCHK should not be reset until a step pulse
>        * is sent. */
> -    qmp_discard_response("{'execute':'blockdev-change-medium', 'arguments':{"
> +    qobject_unref(qmp("{'execute':'blockdev-change-medium', 'arguments':{"
>                            " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
> -                         test_image);
> +                      test_image));

Another place where indentation looks odd.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Eric Blake <eblake@redhat.com> writes:

> On 07/27/2018 10:13 AM, Markus Armbruster wrote:
>> qtest_qmp_discard_response(...) is shorthand for
>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>> Moreover, the presence of these functions encourage sloppy testing.
>> Remove them.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>
>> +++ b/tests/ahci-test.c
>> @@ -1607,14 +1607,14 @@ static void test_atapi_tray(void)
>>       atapi_wait_tray(true);
>>         /* Re-insert media */
>> -    qmp_discard_response("{'execute': 'blockdev-add', "
>> +    qobject_unref(qmp("{'execute': 'blockdev-add', "
>>                             "'arguments': {'node-name': 'node0', "
>>                                           "'driver': 'raw', "
>>                                           "'file': { 'driver': 'file', "
>> -                                                  "'filename': %s }}}", iso);
>> -    qmp_discard_response("{'execute': 'blockdev-insert-medium',"
>> +                      "'filename': %s }}}", iso));
>
> Why did you fix indentation for some, but not all, of the lines here?

Editing accident, fixing.

>> +    qobject_unref(qmp("{'execute': 'blockdev-insert-medium',"
>>                             "'arguments': { 'id': 'cd0', "
>> -                                         "'node-name': 'node0' }}");
>> +                      "'node-name': 'node0' }}"));
>
> Again, indentation looks odd.

Likewise.

>> +++ b/tests/fdc-test.c
>> @@ -26,6 +26,7 @@
>>       #include "libqtest.h"
>> +#include "qapi/qmp/qdict.h"
>>   #include "qemu-common.h"
>>     #define TEST_IMAGE_SIZE 1440 * 1024
>> @@ -298,9 +299,9 @@ static void test_media_insert(void)
>>         /* Insert media in drive. DSKCHK should not be reset until a
>> step pulse
>>        * is sent. */
>> -    qmp_discard_response("{'execute':'blockdev-change-medium', 'arguments':{"
>> +    qobject_unref(qmp("{'execute':'blockdev-change-medium', 'arguments':{"
>>                            " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
>> -                         test_image);
>> +                      test_image));
>
> Another place where indentation looks odd.

Likewise.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!