RE: [PATCH v2 1/6] tests/qtest: replace qmp_discard_response with qtest_qmp_assert_success

[Zhang, Chen]

> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com>
> Sent: Saturday, April 22, 2023 1:14 AM
> To: qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; Paolo Bonzini <pbonzini@redhat.com>;
> Thomas Huth <thuth@redhat.com>; John Snow <jsnow@redhat.com>; Li
> Zhijian <lizhijian@fujitsu.com>; Juan Quintela <quintela@redhat.com>;
> Stefan Hajnoczi <stefanha@redhat.com>; Zhang, Chen
> <chen.zhang@intel.com>; Laurent Vivier <lvivier@redhat.com>; Daniel P.
> Berrangé <berrange@redhat.com>
> Subject: [PATCH v2 1/6] tests/qtest: replace qmp_discard_response with
> qtest_qmp_assert_success
> 
> The qmp_discard_response method simply ignores the result of the QMP
> command, merely unref'ing the object. This is a bad idea for tests as it 
> leaves
> no trace if the QMP command unexpectedly failed. The
> qtest_qmp_assert_success method will validate that the QMP command
> returned without error, and if errors occur, it will print a message on the
> console aiding debugging.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> ---
>  tests/qtest/ahci-test.c              | 31 ++++++++++++++--------------
>  tests/qtest/boot-order-test.c        |  5 +----
>  tests/qtest/fdc-test.c               | 15 +++++++-------
>  tests/qtest/ide-test.c               |  5 +----
>  tests/qtest/migration-test.c         |  5 +----
>  tests/qtest/test-filter-mirror.c     |  5 +----
>  tests/qtest/test-filter-redirector.c |  7 ++-----
>  tests/qtest/virtio-blk-test.c        | 24 ++++++++++-----------
>  8 files changed, 40 insertions(+), 57 deletions(-)
> 
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c index
> 1967cd5898..abab761c26 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -36,9 +36,6 @@
>  #include "hw/pci/pci_ids.h"
>  #include "hw/pci/pci_regs.h"
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qmp_discard_response(s, ...) qobject_unref(qtest_qmp(s, __VA_ARGS__))
> -
>  /* Test images sizes in MB */
>  #define TEST_IMAGE_SIZE_MB_LARGE (200 * 1024)  #define
> TEST_IMAGE_SIZE_MB_SMALL 64 @@ -1595,9 +1592,9 @@ static void
> test_atapi_tray(void)
>      rsp = qtest_qmp_receive(ahci->parent->qts);
>      qobject_unref(rsp);
> 
> -    qmp_discard_response(ahci->parent->qts,
> -                         "{'execute': 'blockdev-remove-medium', "
> -                         "'arguments': {'id': 'cd0'}}");
> +    qtest_qmp_assert_success(ahci->parent->qts,
> +                             "{'execute': 'blockdev-remove-medium', "
> +                             "'arguments': {'id': 'cd0'}}");
> 
>      /* Test the tray without a medium */
>      ahci_atapi_load(ahci, port);
> @@ -1607,16 +1604,18 @@ static void test_atapi_tray(void)
>      atapi_wait_tray(ahci, true);
> 
>      /* Re-insert media */
> -    qmp_discard_response(ahci->parent->qts,
> -                         "{'execute': 'blockdev-add', "
> -                         "'arguments': {'node-name': 'node0', "
> -                                        "'driver': 'raw', "
> -                                        "'file': { 'driver': 'file', "
> -                                                  "'filename': %s }}}", iso);
> -    qmp_discard_response(ahci->parent->qts,
> -                         "{'execute': 'blockdev-insert-medium',"
> -                         "'arguments': { 'id': 'cd0', "
> -                                         "'node-name': 'node0' }}");
> +    qtest_qmp_assert_success(
> +        ahci->parent->qts,
> +        "{'execute': 'blockdev-add', "
> +        "'arguments': {'node-name': 'node0', "
> +                      "'driver': 'raw', "
> +                      "'file': { 'driver': 'file', "
> +                                "'filename': %s }}}", iso);
> +    qtest_qmp_assert_success(
> +        ahci->parent->qts,
> +        "{'execute': 'blockdev-insert-medium',"
> +        "'arguments': { 'id': 'cd0', "
> +                       "'node-name': 'node0' }}");
> 
>      /* Again, the event shows up first */
>      qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-close-tray', "
> diff --git a/tests/qtest/boot-order-test.c b/tests/qtest/boot-order-test.c
> index 0680d79d6d..8f2b6ef05a 100644
> --- a/tests/qtest/boot-order-test.c
> +++ b/tests/qtest/boot-order-test.c
> @@ -16,9 +16,6 @@
>  #include "qapi/qmp/qdict.h"
>  #include "standard-headers/linux/qemu_fw_cfg.h"
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qmp_discard_response(qs, ...) qobject_unref(qtest_qmp(qs, __VA_ARGS__))
> -
>  typedef struct {
>      const char *args;
>      uint64_t expected_boot;
> @@ -43,7 +40,7 @@ static void test_a_boot_order(const char *machine,
>                        machine ?: "", test_args);
>      actual = read_boot_order(qts);
>      g_assert_cmphex(actual, ==, expected_boot);
> -    qmp_discard_response(qts, "{ 'execute': 'system_reset' }");
> +    qtest_qmp_assert_success(qts, "{ '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/qtest/fdc-test.c b/tests/qtest/fdc-test.c index
> 1f9b99ad6d..5e8fbda9df 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -28,9 +28,6 @@
>  #include "libqtest-single.h"
>  #include "qapi/qmp/qdict.h"
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__))
> -
>  #define DRIVE_FLOPPY_BLANK \
>      "-drive if=floppy,file=null-co://,file.read-
> zeroes=on,format=raw,size=1440k"
> 
> @@ -304,9 +301,10 @@ 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':{"
> -                         " 'id':'floppy0', 'filename': %s, 'format': 'raw' 
> }}",
> -                         test_image);
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{'execute':'blockdev-change-medium', 
> 'arguments':{"
> +                             " 'id':'floppy0', 'filename': %s, 'format': 
> 'raw' }}",
> +                             test_image);
> 
>      dir = inb(FLOPPY_BASE + reg_dir);
>      assert_bit_set(dir, DSKCHG);
> @@ -335,8 +333,9 @@ 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' }}");
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{'execute':'eject', 'arguments':{"
> +                             " 'id':'floppy0' }}");
> 
>      dir = inb(FLOPPY_BASE + reg_dir);
>      assert_bit_set(dir, DSKCHG);
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c index
> dcb050bf9b..d6b4f6e36a 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -34,9 +34,6 @@
>  #include "hw/pci/pci_ids.h"
>  #include "hw/pci/pci_regs.h"
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qmp_discard_response(q, ...) qobject_unref(qtest_qmp(q, __VA_ARGS__))
> -
>  #define TEST_IMAGE_SIZE 64 * 1024 * 1024
> 
>  #define IDE_PCI_DEV     1
> @@ -766,7 +763,7 @@ static void test_pci_retry_flush(void)
>      qtest_qmp_eventwait(qts, "STOP");
> 
>      /* Complete the command */
> -    qmp_discard_response(qts, "{'execute':'cont' }");
> +    qtest_qmp_assert_success(qts, "{'execute':'cont' }");
> 
>      /* Check registers */
>      data = qpci_io_readb(dev, ide_bar, reg_device); diff --git
> a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index
> 3b615b0da9..ac2e8ecac6 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -40,9 +40,6 @@
>  #include "linux/kvm.h"
>  #endif
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qtest_qmp_discard_response(...) qobject_unref(qtest_qmp(__VA_ARGS__))
> -
>  unsigned start_address;
>  unsigned end_address;
>  static bool uffd_feature_thread_id;
> @@ -731,7 +728,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'}");
> +        qtest_qmp_assert_success(to, "{ 'execute' : 'stop'}");
> 
>          /* With it stopped, check nothing changes */
>          qtest_memread(to, start_address, &dest_byte_c, 1); diff --git
> a/tests/qtest/test-filter-mirror.c b/tests/qtest/test-filter-mirror.c
> index 248fc88699..adeada3eb8 100644
> --- a/tests/qtest/test-filter-mirror.c
> +++ b/tests/qtest/test-filter-mirror.c
> @@ -16,9 +16,6 @@
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qmp_discard_response(qs, ...) qobject_unref(qtest_qmp(qs, __VA_ARGS__))
> -
>  static void test_mirror(void)
>  {
>      int send_sock[2], recv_sock[2];
> @@ -52,7 +49,7 @@ static void test_mirror(void)
>      };
> 
>      /* send a qmp command to guarantee that 'connected' is setting to true.
> */
> -    qmp_discard_response(qts, "{ 'execute' : 'query-status'}");
> +    qtest_qmp_assert_success(qts, "{ '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/qtest/test-filter-redirector.c b/tests/qtest/test-filter-
> redirector.c
> index 24ca9280f8..e72e3b7873 100644
> --- a/tests/qtest/test-filter-redirector.c
> +++ b/tests/qtest/test-filter-redirector.c
> @@ -58,9 +58,6 @@
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qmp_discard_response(qs, ...) qobject_unref(qtest_qmp(qs, __VA_ARGS__))
> -
>  static void test_redirector_tx(void)
>  {
>      int backend_sock[2], recv_sock;
> @@ -98,7 +95,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(qts, "{ 'execute' : 'query-status'}");
> +    qtest_qmp_assert_success(qts, "{ 'execute' : 'query-status'}");
> 
>      struct iovec iov[] = {
>          {
> @@ -176,7 +173,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(qts, "{ 'execute' : 'query-status'}");
> +    qtest_qmp_assert_success(qts, "{ '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/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c index
> 19c01f808b..98c906ebb4 100644
> --- a/tests/qtest/virtio-blk-test.c
> +++ b/tests/qtest/virtio-blk-test.c
> @@ -17,9 +17,6 @@
>  #include "libqos/qgraph.h"
>  #include "libqos/virtio-blk.h"
> 
> -/* TODO actually test the results and get rid of this */ -#define
> qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__))
> -
>  #define TEST_IMAGE_SIZE         (64 * 1024 * 1024)
>  #define QVIRTIO_BLK_TIMEOUT_US  (30 * 1000 * 1000)
>  #define PCI_SLOT_HP             0x06
> @@ -453,9 +450,10 @@ static void config(void *obj, void *data,
> QGuestAllocator *t_alloc)
> 
>      qvirtio_set_driver_ok(dev);
> 
> -    qmp_discard_response("{ 'execute': 'block_resize', "
> -                         " 'arguments': { 'device': 'drive0', "
> -                         " 'size': %d } }", n_size);
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{ 'execute': 'block_resize', "
> +                             " 'arguments': { 'device': 'drive0', "
> +                             " 'size': %d } }", n_size);
>      qvirtio_wait_config_isr(dev, QVIRTIO_BLK_TIMEOUT_US);
> 
>      capacity = qvirtio_config_readq(dev, 0); @@ -502,9 +500,10 @@ static
> void msix(void *obj, void *u_data, QGuestAllocator *t_alloc)
> 
>      qvirtio_set_driver_ok(dev);
> 
> -    qmp_discard_response("{ 'execute': 'block_resize', "
> -                         " 'arguments': { 'device': 'drive0', "
> -                         " 'size': %d } }", n_size);
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{ 'execute': 'block_resize', "
> +                             " 'arguments': { 'device': 'drive0', "
> +                             " 'size': %d } }", n_size);
> 
>      qvirtio_wait_config_isr(dev, QVIRTIO_BLK_TIMEOUT_US);
> 
> @@ -758,9 +757,10 @@ static void resize(void *obj, void *data,
> QGuestAllocator *t_alloc)
> 
>      vq = test_basic(dev, t_alloc);
> 
> -    qmp_discard_response("{ 'execute': 'block_resize', "
> -                         " 'arguments': { 'device': 'drive0', "
> -                         " 'size': %d } }", n_size);
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{ 'execute': 'block_resize', "
> +                             " 'arguments': { 'device': 'drive0', "
> +                             " 'size': %d } }", n_size);
> 
>      qvirtio_wait_queue_isr(qts, dev, vq, QVIRTIO_BLK_TIMEOUT_US);
> 
> --
> 2.40.0