[PATCH 0/3] qtest: migration: Add validation tests for 'channels' argument in migrate QAPIs

[PATCH 0/3] qtest: migration: Add validation tests for 'channels' argument in migrate QAPIs

[Het Gala]

With recent migrate QAPI changes enabling direct use of the 'channels'
argument, avoiding redundant string parsing of the URI is achieved.

To ensure backward compatibility, both 'uri' and 'channels' arguments are
now optional in migration QMP commands. However, they are mutually exclusive,
requiring at least one for a successful migration connection.

Hence, validating 'uri' and 'channels' becomes crucial to prevent their
simultaneous presence in migrate QAPIs.

Patch Summary:
1. Introduce migrate_qmp() and migrate_qmp_fail() with 'channels' arguments
   and a conversion function from MigrationChannelList to QList.
2. Add a new field in the MigrateCommon struct to support the 'channels'
   argument during migration.
3. Include negative validation tests to disallow both arguments in migration
   QAPIs.

Het Gala (3):
  qtest: migration: Enhance qtest migration functions to support
    'channels' argument
  qtest: migration: Introduce 'connect_channels' in MigrateCommon struct
  qtest: migration: Add negative validation test for 'uri' and
    'channels' both set

 tests/qtest/dbus-vmstate-test.c |   2 +-
 tests/qtest/migration-helpers.c |  93 ++++++++++++++++++++++--
 tests/qtest/migration-helpers.h |  11 +--
 tests/qtest/migration-test.c    | 123 +++++++++++++++++++++++++++-----
 4 files changed, 202 insertions(+), 27 deletions(-)

-- 
2.22.3

[PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument

[Het Gala]

Introduce support for adding a 'channels' argument to migrate_qmp_fail
and migrate_qmp functions within the migration qtest framework, enabling
enhanced control over migration scenarios.

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/dbus-vmstate-test.c |  2 +-
 tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
 tests/qtest/migration-helpers.h | 11 ++--
 tests/qtest/migration-test.c    | 33 ++++++------
 4 files changed, 112 insertions(+), 27 deletions(-)

diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
index 6c990864e3..0ca572e29b 100644
--- a/tests/qtest/dbus-vmstate-test.c
+++ b/tests/qtest/dbus-vmstate-test.c
@@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
 
     thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
 
-    migrate_qmp(src_qemu, uri, "{}");
+    migrate_qmp(src_qemu, uri, NULL, "{}");
     test->src_qemu = src_qemu;
     if (test->migrate_fail) {
         wait_for_migration_fail(src_qemu, true);
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..d153677887 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qemu/ctype.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qlist.h"
 
 #include "migration-helpers.h"
 
@@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
     return false;
 }
 
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+static char *socketAddressType_to_str(SocketAddressType type)
+{
+    switch (type) {
+    case SOCKET_ADDRESS_TYPE_INET:
+        return g_strdup("inet");
+    case SOCKET_ADDRESS_TYPE_UNIX:
+        return g_strdup("unix");
+    case SOCKET_ADDRESS_TYPE_FD:
+        return g_strdup("fd");
+    case SOCKET_ADDRESS_TYPE_VSOCK:
+        return g_strdup("vsock");
+    default:
+        return g_strdup("unknown address type");
+    }
+}
+
+static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
+{
+    MigrationChannel *channel = NULL;
+    MigrationAddress *addr = NULL;
+    SocketAddress *saddr = NULL;
+    g_autofree const char *addr_type = NULL;
+    QList *channelList = qlist_new();
+    QDict *channelDict = qdict_new();
+    QDict *addrDict = qdict_new();
+
+    channel = channels->value;
+    if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
+        fprintf(stderr, "%s: Channel or its type is NULL\n",
+                __func__);
+    }
+    g_assert(channel);
+    if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
+        qdict_put_str(channelDict, "channel-type", g_strdup("main"));
+    }
+
+    addr = channel->addr;
+    if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
+        fprintf(stderr, "%s: addr or its transport is NULL\n",
+                __func__);
+    }
+    g_assert(addr);
+    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        qdict_put_str(addrDict, "transport", g_strdup("socket"));
+    }
+
+    saddr = &addr->u.socket;
+    if (!saddr) {
+        fprintf(stderr, "%s: saddr is NULL\n", __func__);
+    }
+    g_assert(saddr);
+    addr_type = socketAddressType_to_str(saddr->type);
+    qdict_put_str(addrDict, "type", addr_type);
+    qdict_put_str(addrDict, "port", saddr->u.inet.port);
+    qdict_put_str(addrDict, "host", saddr->u.inet.host);
+
+    qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
+    qlist_append_obj(channelList, QOBJECT(channelDict));
+
+    return channelList;
+}
+
+void migrate_qmp_fail(QTestState *who, const char *uri,
+                      MigrationChannelList *channels, const char *fmt, ...)
 {
     va_list ap;
     QDict *args, *err;
@@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
     args = qdict_from_vjsonf_nofail(fmt, ap);
     va_end(ap);
 
-    g_assert(!qdict_haskey(args, "uri"));
-    qdict_put_str(args, "uri", uri);
+    if (uri) {
+        g_assert(!qdict_haskey(args, "uri"));
+        qdict_put_str(args, "uri", uri);
+    }
+
+    if (channels) {
+        g_assert(!qdict_haskey(args, "channels"));
+        QList *channelList = MigrationChannelList_to_QList(channels);
+        qdict_put_obj(args, "channels", QOBJECT(channelList));
+    }
 
     err = qtest_qmp_assert_failure_ref(
         who, "{ 'execute': 'migrate', 'arguments': %p}", args);
@@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
  * Arguments are built from @fmt... (formatted like
  * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
  */
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp(QTestState *who, const char *uri,
+                 MigrationChannelList *channels, const char *fmt, ...)
 {
     va_list ap;
     QDict *args;
@@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
     args = qdict_from_vjsonf_nofail(fmt, ap);
     va_end(ap);
 
-    g_assert(!qdict_haskey(args, "uri"));
-    qdict_put_str(args, "uri", uri);
+    if (uri) {
+        g_assert(!qdict_haskey(args, "uri"));
+        qdict_put_str(args, "uri", uri);
+    }
+
+    if (channels) {
+        g_assert(!qdict_haskey(args, "channels"));
+        QList *channelList = MigrationChannelList_to_QList(channels);
+        qdict_put_obj(args, "channels", QOBJECT(channelList));
+    }
 
     qtest_qmp_assert_success(who,
                              "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..52243bd2df 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -14,6 +14,7 @@
 #define MIGRATION_HELPERS_H
 
 #include "libqtest.h"
+#include "migration/migration.h"
 
 typedef struct QTestMigrationState {
     bool stop_seen;
@@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
 bool migrate_watch_for_events(QTestState *who, const char *name,
                               QDict *event, void *opaque);
 
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp(QTestState *who, const char *uri,
+                 MigrationChannelList *channels, const char *fmt, ...);
 
 G_GNUC_PRINTF(3, 4)
 void migrate_incoming_qmp(QTestState *who, const char *uri,
                           const char *fmt, ...);
 
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+                      MigrationChannelList *channels, const char *fmt, ...);
 
 void migrate_set_capability(QTestState *who, const char *capability,
                             bool value);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 8a5bb1752e..e7f2719dcf 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -18,6 +18,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/range.h"
+#include "migration/migration.h"
 #include "qemu/sockets.h"
 #include "chardev/char.h"
 #include "qapi/qapi-visit-sockets.h"
@@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     wait_for_suspend(from, &src_state);
 
     g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to)
     g_assert_cmpint(ret, ==, 1);
 
     migrate_recover(to, "fd:fd-mig");
-    migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
+    migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}");
 
     /*
      * Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args)
      * Try to rebuild the migration channel using the resume flag and
      * the newly created channel
      */
-    migrate_qmp(from, uri, "{'resume': true}");
+    migrate_qmp(from, uri, NULL, "{'resume': true}");
 
     /* Restore the postcopy bandwidth to unlimited */
     migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1670,7 @@ static void test_baddest(void)
     if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
         return;
     }
-    migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
+    migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}");
     wait_for_migration_fail(from, false);
     test_migrate_end(from, to, false);
 }
@@ -1708,7 +1709,7 @@ static void test_analyze_script(void)
     uri = g_strdup_printf("exec:cat > %s", file);
 
     migrate_ensure_converge(from);
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
     wait_for_migration_complete(from);
 
     pid = fork();
@@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args)
     }
 
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, connect_uri, "{}");
+        migrate_qmp_fail(from, connect_uri, NULL, "{}");
         goto finish;
     }
 
-    migrate_qmp(from, connect_uri, "{}");
+    migrate_qmp(from, connect_uri, NULL, "{}");
 
     if (args->result != MIG_TEST_SUCCEED) {
         bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1869,11 +1870,11 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
     }
 
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, connect_uri, "{}");
+        migrate_qmp_fail(from, connect_uri, NULL, "{}");
         goto finish;
     }
 
-    migrate_qmp(from, connect_uri, "{}");
+    migrate_qmp(from, connect_uri, NULL, "{}");
     wait_for_migration_complete(from);
 
     /*
@@ -2029,7 +2030,7 @@ static void test_ignore_shared(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     if (should_fail) {
         qtest_set_expected_status(to, EXIT_FAILURE);
@@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     /* Wait for throttling begins */
     percentage = 0;
@@ -2869,7 +2870,7 @@ static void test_multifd_tcp_cancel(void)
 
     uri = migrate_get_socket_address(to, "socket-address");
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void)
 
     migrate_ensure_non_converge(from);
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to2);
 
@@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void)
     migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
 
     /* Start migrate */
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
@@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void)
     }
 
     /* Start migrate */
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
-- 
2.22.3

Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument

[Peter Xu]

On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
> Introduce support for adding a 'channels' argument to migrate_qmp_fail
> and migrate_qmp functions within the migration qtest framework, enabling
> enhanced control over migration scenarios.
> 
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/dbus-vmstate-test.c |  2 +-
>  tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
>  tests/qtest/migration-helpers.h | 11 ++--
>  tests/qtest/migration-test.c    | 33 ++++++------
>  4 files changed, 112 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
> index 6c990864e3..0ca572e29b 100644
> --- a/tests/qtest/dbus-vmstate-test.c
> +++ b/tests/qtest/dbus-vmstate-test.c
> @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
>  
>      thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
>  
> -    migrate_qmp(src_qemu, uri, "{}");
> +    migrate_qmp(src_qemu, uri, NULL, "{}");
>      test->src_qemu = src_qemu;
>      if (test->migrate_fail) {
>          wait_for_migration_fail(src_qemu, true);
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index e451dbdbed..d153677887 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/ctype.h"
>  #include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qlist.h"
>  
>  #include "migration-helpers.h"
>  
> @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
>      return false;
>  }
>  
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> +static char *socketAddressType_to_str(SocketAddressType type)
> +{
> +    switch (type) {
> +    case SOCKET_ADDRESS_TYPE_INET:
> +        return g_strdup("inet");
> +    case SOCKET_ADDRESS_TYPE_UNIX:
> +        return g_strdup("unix");
> +    case SOCKET_ADDRESS_TYPE_FD:
> +        return g_strdup("fd");
> +    case SOCKET_ADDRESS_TYPE_VSOCK:
> +        return g_strdup("vsock");
> +    default:
> +        return g_strdup("unknown address type");
> +    }
> +}

Use SocketAddressType_lookup?

> +
> +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
> +{
> +    MigrationChannel *channel = NULL;
> +    MigrationAddress *addr = NULL;
> +    SocketAddress *saddr = NULL;
> +    g_autofree const char *addr_type = NULL;
> +    QList *channelList = qlist_new();
> +    QDict *channelDict = qdict_new();
> +    QDict *addrDict = qdict_new();
> +
> +    channel = channels->value;
> +    if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
> +        fprintf(stderr, "%s: Channel or its type is NULL\n",
> +                __func__);
> +    }
> +    g_assert(channel);
> +    if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
> +        qdict_put_str(channelDict, "channel-type", g_strdup("main"));
> +    }
> +
> +    addr = channel->addr;
> +    if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
> +        fprintf(stderr, "%s: addr or its transport is NULL\n",
> +                __func__);
> +    }
> +    g_assert(addr);
> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> +        qdict_put_str(addrDict, "transport", g_strdup("socket"));
> +    }
> +
> +    saddr = &addr->u.socket;
> +    if (!saddr) {
> +        fprintf(stderr, "%s: saddr is NULL\n", __func__);
> +    }
> +    g_assert(saddr);
> +    addr_type = socketAddressType_to_str(saddr->type);
> +    qdict_put_str(addrDict, "type", addr_type);
> +    qdict_put_str(addrDict, "port", saddr->u.inet.port);
> +    qdict_put_str(addrDict, "host", saddr->u.inet.host);
> +
> +    qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
> +    qlist_append_obj(channelList, QOBJECT(channelDict));
> +
> +    return channelList;
> +}
> +
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> +                      MigrationChannelList *channels, const char *fmt, ...)
>  {
>      va_list ap;
>      QDict *args, *err;
> @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>      args = qdict_from_vjsonf_nofail(fmt, ap);
>      va_end(ap);
>  
> -    g_assert(!qdict_haskey(args, "uri"));
> -    qdict_put_str(args, "uri", uri);
> +    if (uri) {
> +        g_assert(!qdict_haskey(args, "uri"));

IMHO we don't need to assert here?

Rather than doing this, we can also have tests covering when both are set,
or when none are set, to make sure we fail properly in those wrong cases.

> +        qdict_put_str(args, "uri", uri);
> +    }
> +
> +    if (channels) {
> +        g_assert(!qdict_haskey(args, "channels"));
> +        QList *channelList = MigrationChannelList_to_QList(channels);
> +        qdict_put_obj(args, "channels", QOBJECT(channelList));
> +    }
>  
>      err = qtest_qmp_assert_failure_ref(
>          who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>   * Arguments are built from @fmt... (formatted like
>   * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
>   */
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> +void migrate_qmp(QTestState *who, const char *uri,
> +                 MigrationChannelList *channels, const char *fmt, ...)
>  {
>      va_list ap;
>      QDict *args;
> @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
>      args = qdict_from_vjsonf_nofail(fmt, ap);
>      va_end(ap);
>  
> -    g_assert(!qdict_haskey(args, "uri"));
> -    qdict_put_str(args, "uri", uri);
> +    if (uri) {
> +        g_assert(!qdict_haskey(args, "uri"));
> +        qdict_put_str(args, "uri", uri);
> +    }
> +
> +    if (channels) {
> +        g_assert(!qdict_haskey(args, "channels"));
> +        QList *channelList = MigrationChannelList_to_QList(channels);
> +        qdict_put_obj(args, "channels", QOBJECT(channelList));
> +    }

Duplicated chunks; sign of adding some helper?

>  
>      qtest_qmp_assert_success(who,
>                               "{ 'execute': 'migrate', 'arguments': %p}", args);
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 3bf7ded1b9..52243bd2df 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -14,6 +14,7 @@
>  #define MIGRATION_HELPERS_H
>  
>  #include "libqtest.h"
> +#include "migration/migration.h"
>  
>  typedef struct QTestMigrationState {
>      bool stop_seen;
> @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
>  bool migrate_watch_for_events(QTestState *who, const char *name,
>                                QDict *event, void *opaque);
>  
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp(QTestState *who, const char *uri,
> +                 MigrationChannelList *channels, const char *fmt, ...);
>  
>  G_GNUC_PRINTF(3, 4)
>  void migrate_incoming_qmp(QTestState *who, const char *uri,
>                            const char *fmt, ...);
>  
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> +                      MigrationChannelList *channels, const char *fmt, ...);
>  
>  void migrate_set_capability(QTestState *who, const char *capability,
>                              bool value);
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 8a5bb1752e..e7f2719dcf 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -18,6 +18,7 @@
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/range.h"
> +#include "migration/migration.h"
>  #include "qemu/sockets.h"
>  #include "chardev/char.h"
>  #include "qapi/qapi-visit-sockets.h"
> @@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>      wait_for_suspend(from, &src_state);
>  
>      g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      migrate_wait_for_dirty_mem(from, to);
>  
> @@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to)
>      g_assert_cmpint(ret, ==, 1);
>  
>      migrate_recover(to, "fd:fd-mig");
> -    migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
> +    migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}");
>  
>      /*
>       * Make sure both QEMU instances will go into RECOVER stage, then test
> @@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args)
>       * Try to rebuild the migration channel using the resume flag and
>       * the newly created channel
>       */
> -    migrate_qmp(from, uri, "{'resume': true}");
> +    migrate_qmp(from, uri, NULL, "{'resume': true}");
>  
>      /* Restore the postcopy bandwidth to unlimited */
>      migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
> @@ -1669,7 +1670,7 @@ static void test_baddest(void)
>      if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
>          return;
>      }
> -    migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
> +    migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}");
>      wait_for_migration_fail(from, false);
>      test_migrate_end(from, to, false);
>  }
> @@ -1708,7 +1709,7 @@ static void test_analyze_script(void)
>      uri = g_strdup_printf("exec:cat > %s", file);
>  
>      migrate_ensure_converge(from);
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>      wait_for_migration_complete(from);
>  
>      pid = fork();
> @@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args)
>      }
>  
>      if (args->result == MIG_TEST_QMP_ERROR) {
> -        migrate_qmp_fail(from, connect_uri, "{}");
> +        migrate_qmp_fail(from, connect_uri, NULL, "{}");
>          goto finish;
>      }
>  
> -    migrate_qmp(from, connect_uri, "{}");
> +    migrate_qmp(from, connect_uri, NULL, "{}");
>  
>      if (args->result != MIG_TEST_SUCCEED) {
>          bool allow_active = args->result == MIG_TEST_FAIL;
> @@ -1869,11 +1870,11 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
>      }
>  
>      if (args->result == MIG_TEST_QMP_ERROR) {
> -        migrate_qmp_fail(from, connect_uri, "{}");
> +        migrate_qmp_fail(from, connect_uri, NULL, "{}");
>          goto finish;
>      }
>  
> -    migrate_qmp(from, connect_uri, "{}");
> +    migrate_qmp(from, connect_uri, NULL, "{}");
>      wait_for_migration_complete(from);
>  
>      /*
> @@ -2029,7 +2030,7 @@ static void test_ignore_shared(void)
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>  
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      migrate_wait_for_dirty_mem(from, to);
>  
> @@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>  
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      if (should_fail) {
>          qtest_set_expected_status(to, EXIT_FAILURE);
> @@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void)
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>  
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      /* Wait for throttling begins */
>      percentage = 0;
> @@ -2869,7 +2870,7 @@ static void test_multifd_tcp_cancel(void)
>  
>      uri = migrate_get_socket_address(to, "socket-address");
>  
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      migrate_wait_for_dirty_mem(from, to);
>  
> @@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void)
>  
>      migrate_ensure_non_converge(from);
>  
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      migrate_wait_for_dirty_mem(from, to2);
>  
> @@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void)
>      migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
>  
>      /* Start migrate */
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      /* Wait for dirty limit throttle begin */
>      throttle_us_per_full = 0;
> @@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void)
>      }
>  
>      /* Start migrate */
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>  
>      /* Wait for dirty limit throttle begin */
>      throttle_us_per_full = 0;
> -- 
> 2.22.3
> 

-- 
Peter Xu

Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument

[Het Gala]

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



From: Peter Xu <peterx@redhat.com>
Date: Tuesday, 20 February 2024 at 11:33 AM
To: Het Gala <het.gala@nutanix.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, armbru@redhat.com <armbru@redhat.com>, berrange@redhat.com <berrange@redhat.com>, farosas@suse.de <farosas@suse.de>
Subject: Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
> Introduce support for adding a 'channels' argument to migrate_qmp_fail
> and migrate_qmp functions within the migration qtest framework, enabling
> enhanced control over migration scenarios.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/dbus-vmstate-test.c |  2 +-
>  tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
>  tests/qtest/migration-helpers.h | 11 ++--
>  tests/qtest/migration-test.c    | 33 ++++++------
>  4 files changed, 112 insertions(+), 27 deletions(-)
>
> diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
> index 6c990864e3..0ca572e29b 100644
> --- a/tests/qtest/dbus-vmstate-test.c
> +++ b/tests/qtest/dbus-vmstate-test.c
> @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
>
>      thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
>
> -    migrate_qmp(src_qemu, uri, "{}");
> +    migrate_qmp(src_qemu, uri, NULL, "{}");
>      test->src_qemu = src_qemu;
>      if (test->migrate_fail) {
>          wait_for_migration_fail(src_qemu, true);
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index e451dbdbed..d153677887 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/ctype.h"
>  #include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qlist.h"
>
>  #include "migration-helpers.h"
>
> @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
>      return false;
>  }
>
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> +static char *socketAddressType_to_str(SocketAddressType type)
> +{
> +    switch (type) {
> +    case SOCKET_ADDRESS_TYPE_INET:
> +        return g_strdup("inet");
> +    case SOCKET_ADDRESS_TYPE_UNIX:
> +        return g_strdup("unix");
> +    case SOCKET_ADDRESS_TYPE_FD:
> +        return g_strdup("fd");
> +    case SOCKET_ADDRESS_TYPE_VSOCK:
> +        return g_strdup("vsock");
> +    default:
> +        return g_strdup("unknown address type");
> +    }
> +}

Use SocketAddressType_lookup?
Ack, I guess combination of using qapi_enum_parse() and qapi_enum_lookup() would help me eliminate the need for creating this function itself. Let me do the changes in the next patch.  Thanks!

> +
> +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
> +{
> +    MigrationChannel *channel = NULL;
> +    MigrationAddress *addr = NULL;
> +    SocketAddress *saddr = NULL;
> +    g_autofree const char *addr_type = NULL;
> +    QList *channelList = qlist_new();
> +    QDict *channelDict = qdict_new();
> +    QDict *addrDict = qdict_new();
> +
> +    channel = channels->value;
> +    if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
> +        fprintf(stderr, "%s: Channel or its type is NULL\n",
> +                __func__);
> +    }
> +    g_assert(channel);
> +    if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
> +        qdict_put_str(channelDict, "channel-type", g_strdup("main"));
> +    }
> +
> +    addr = channel->addr;
> +    if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
> +        fprintf(stderr, "%s: addr or its transport is NULL\n",
> +                __func__);
> +    }
> +    g_assert(addr);
> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> +        qdict_put_str(addrDict, "transport", g_strdup("socket"));
> +    }
> +
> +    saddr = &addr->u.socket;
> +    if (!saddr) {
> +        fprintf(stderr, "%s: saddr is NULL\n", __func__);
> +    }
> +    g_assert(saddr);
> +    addr_type = socketAddressType_to_str(saddr->type);
> +    qdict_put_str(addrDict, "type", addr_type);
> +    qdict_put_str(addrDict, "port", saddr->u.inet.port);
> +    qdict_put_str(addrDict, "host", saddr->u.inet.host);
> +
> +    qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
> +    qlist_append_obj(channelList, QOBJECT(channelDict));
> +
> +    return channelList;
> +}
> +
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> +                      MigrationChannelList *channels, const char *fmt, ...)
>  {
>      va_list ap;
>      QDict *args, *err;
> @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>      args = qdict_from_vjsonf_nofail(fmt, ap);
>      va_end(ap);
>
> -    g_assert(!qdict_haskey(args, "uri"));
> -    qdict_put_str(args, "uri", uri);
> +    if (uri) {
> +        g_assert(!qdict_haskey(args, "uri"));

IMHO we don't need to assert here?

Rather than doing this, we can also have tests covering when both are set,
or when none are set, to make sure we fail properly in those wrong cases.
(Neglecting your comments here based on Patch 3/3 comment).

> +        qdict_put_str(args, "uri", uri);
> +    }
> +
> +    if (channels) {
> +        g_assert(!qdict_haskey(args, "channels"));
> +        QList *channelList = MigrationChannelList_to_QList(channels);
> +        qdict_put_obj(args, "channels", QOBJECT(channelList));
> +    }
>
>      err = qtest_qmp_assert_failure_ref(
>          who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>   * Arguments are built from @fmt... (formatted like
>   * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
>   */
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> +void migrate_qmp(QTestState *who, const char *uri,
> +                 MigrationChannelList *channels, const char *fmt, ...)
>  {
>      va_list ap;
>      QDict *args;
> @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
>      args = qdict_from_vjsonf_nofail(fmt, ap);
>      va_end(ap);
>
> -    g_assert(!qdict_haskey(args, "uri"));
> -    qdict_put_str(args, "uri", uri);
> +    if (uri) {
> +        g_assert(!qdict_haskey(args, "uri"));
> +        qdict_put_str(args, "uri", uri);
> +    }
> +
> +    if (channels) {
> +        g_assert(!qdict_haskey(args, "channels"));
> +        QList *channelList = MigrationChannelList_to_QList(channels);
> +        qdict_put_obj(args, "channels", QOBJECT(channelList));
> +    }

Duplicated chunks; sign of adding some helper?
I didn’t think of adding a helper function here as it was just 2 lines of code, already calling MigrationChannelList_to_QList() to avoid duplication. Even then if you have something in your mind to create some helper function, please suggest :)

>
>      qtest_qmp_assert_success(who,
>                               "{ 'execute': 'migrate', 'arguments': %p}", args);
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 3bf7ded1b9..52243bd2df 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -14,6 +14,7 @@
>  #define MIGRATION_HELPERS_H
>
>  #include "libqtest.h"
> +#include "migration/migration.h"
>
>  typedef struct QTestMigrationState {
>      bool stop_seen;
> @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
>  bool migrate_watch_for_events(QTestState *who, const char *name,
>                                QDict *event, void *opaque);
>
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp(QTestState *who, const char *uri,
> +                 MigrationChannelList *channels, const char *fmt, ...);
>
>  G_GNUC_PRINTF(3, 4)
>  void migrate_incoming_qmp(QTestState *who, const char *uri,
>                            const char *fmt, ...);
Just thinking, should also add ‘channels’ argument here I guess, would be helpful in future to add some tests where only ‘channels’ parameter wants to be added to the function ?

>
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> +                      MigrationChannelList *channels, const char *fmt, ...);
>
>  void migrate_set_capability(QTestState *who, const char *capability,
>                              bool value);
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 8a5bb1752e..e7f2719dcf 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -18,6 +18,7 @@
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/range.h"
> +#include "migration/migration.h"
>  #include "qemu/sockets.h"
>  #include "chardev/char.h"
>  #include "qapi/qapi-visit-sockets.h"
> @@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>      wait_for_suspend(from, &src_state);
>
>      g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      migrate_wait_for_dirty_mem(from, to);
>
> @@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to)
>      g_assert_cmpint(ret, ==, 1);
>
>      migrate_recover(to, "fd:fd-mig");
> -    migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
> +    migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}");
>
>      /*
>       * Make sure both QEMU instances will go into RECOVER stage, then test
> @@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args)
>       * Try to rebuild the migration channel using the resume flag and
>       * the newly created channel
>       */
> -    migrate_qmp(from, uri, "{'resume': true}");
> +    migrate_qmp(from, uri, NULL, "{'resume': true}");
>
>      /* Restore the postcopy bandwidth to unlimited */
>      migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
> @@ -1669,7 +1670,7 @@ static void test_baddest(void)
>      if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
>          return;
>      }
> -    migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
> +    migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}");
>      wait_for_migration_fail(from, false);
>      test_migrate_end(from, to, false);
>  }
> @@ -1708,7 +1709,7 @@ static void test_analyze_script(void)
>      uri = g_strdup_printf("exec:cat > %s", file);
>
>      migrate_ensure_converge(from);
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>      wait_for_migration_complete(from);
>
>      pid = fork();
> @@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args)
>      }
>
>      if (args->result == MIG_TEST_QMP_ERROR) {
> -        migrate_qmp_fail(from, connect_uri, "{}");
> +        migrate_qmp_fail(from, connect_uri, NULL, "{}");
>          goto finish;
>      }
>
> -    migrate_qmp(from, connect_uri, "{}");
> +    migrate_qmp(from, connect_uri, NULL, "{}");
>
>      if (args->result != MIG_TEST_SUCCEED) {
>          bool allow_active = args->result == MIG_TEST_FAIL;
> @@ -1869,11 +1870,11 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
>      }
>
>      if (args->result == MIG_TEST_QMP_ERROR) {
> -        migrate_qmp_fail(from, connect_uri, "{}");
> +        migrate_qmp_fail(from, connect_uri, NULL, "{}");
>          goto finish;
>      }
>
> -    migrate_qmp(from, connect_uri, "{}");
> +    migrate_qmp(from, connect_uri, NULL, "{}");
>      wait_for_migration_complete(from);
>
>      /*
> @@ -2029,7 +2030,7 @@ static void test_ignore_shared(void)
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      migrate_wait_for_dirty_mem(from, to);
>
> @@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      if (should_fail) {
>          qtest_set_expected_status(to, EXIT_FAILURE);
> @@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void)
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      /* Wait for throttling begins */
>      percentage = 0;
> @@ -2869,7 +2870,7 @@ static void test_multifd_tcp_cancel(void)
>
>      uri = migrate_get_socket_address(to, "socket-address");
>
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      migrate_wait_for_dirty_mem(from, to);
>
> @@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void)
>
>      migrate_ensure_non_converge(from);
>
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      migrate_wait_for_dirty_mem(from, to2);
>
> @@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void)
>      migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
>
>      /* Start migrate */
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      /* Wait for dirty limit throttle begin */
>      throttle_us_per_full = 0;
> @@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void)
>      }
>
>      /* Start migrate */
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>
>      /* Wait for dirty limit throttle begin */
>      throttle_us_per_full = 0;
> --
> 2.22.3
>

--
Peter Xu
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 26259 bytes --]

Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument

[Peter Xu]

On Tue, Feb 20, 2024 at 06:14:46PM +0000, Het Gala wrote:
> 
> 
> From: Peter Xu <peterx@redhat.com>
> Date: Tuesday, 20 February 2024 at 11:33 AM
> To: Het Gala <het.gala@nutanix.com>
> Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, armbru@redhat.com <armbru@redhat.com>, berrange@redhat.com <berrange@redhat.com>, farosas@suse.de <farosas@suse.de>
> Subject: Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
> On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
> > Introduce support for adding a 'channels' argument to migrate_qmp_fail
> > and migrate_qmp functions within the migration qtest framework, enabling
> > enhanced control over migration scenarios.
> >
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >  tests/qtest/dbus-vmstate-test.c |  2 +-
> >  tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
> >  tests/qtest/migration-helpers.h | 11 ++--
> >  tests/qtest/migration-test.c    | 33 ++++++------
> >  4 files changed, 112 insertions(+), 27 deletions(-)
> >
> > diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
> > index 6c990864e3..0ca572e29b 100644
> > --- a/tests/qtest/dbus-vmstate-test.c
> > +++ b/tests/qtest/dbus-vmstate-test.c
> > @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
> >
> >      thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
> >
> > -    migrate_qmp(src_qemu, uri, "{}");
> > +    migrate_qmp(src_qemu, uri, NULL, "{}");
> >      test->src_qemu = src_qemu;
> >      if (test->migrate_fail) {
> >          wait_for_migration_fail(src_qemu, true);
> > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> > index e451dbdbed..d153677887 100644
> > --- a/tests/qtest/migration-helpers.c
> > +++ b/tests/qtest/migration-helpers.c
> > @@ -13,6 +13,7 @@
> >  #include "qemu/osdep.h"
> >  #include "qemu/ctype.h"
> >  #include "qapi/qmp/qjson.h"
> > +#include "qapi/qmp/qlist.h"
> >
> >  #include "migration-helpers.h"
> >
> > @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
> >      return false;
> >  }
> >
> > -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> > +static char *socketAddressType_to_str(SocketAddressType type)
> > +{
> > +    switch (type) {
> > +    case SOCKET_ADDRESS_TYPE_INET:
> > +        return g_strdup("inet");
> > +    case SOCKET_ADDRESS_TYPE_UNIX:
> > +        return g_strdup("unix");
> > +    case SOCKET_ADDRESS_TYPE_FD:
> > +        return g_strdup("fd");
> > +    case SOCKET_ADDRESS_TYPE_VSOCK:
> > +        return g_strdup("vsock");
> > +    default:
> > +        return g_strdup("unknown address type");
> > +    }
> > +}
> 
> Use SocketAddressType_lookup?
> Ack, I guess combination of using qapi_enum_parse() and qapi_enum_lookup() would help me eliminate the need for creating this function itself. Let me do the changes in the next patch.  Thanks!

Ah, what I wanted to say was actually SocketAddressType_str.

> 
> > +
> > +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
> > +{
> > +    MigrationChannel *channel = NULL;
> > +    MigrationAddress *addr = NULL;
> > +    SocketAddress *saddr = NULL;
> > +    g_autofree const char *addr_type = NULL;
> > +    QList *channelList = qlist_new();
> > +    QDict *channelDict = qdict_new();
> > +    QDict *addrDict = qdict_new();
> > +
> > +    channel = channels->value;
> > +    if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
> > +        fprintf(stderr, "%s: Channel or its type is NULL\n",
> > +                __func__);
> > +    }
> > +    g_assert(channel);
> > +    if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
> > +        qdict_put_str(channelDict, "channel-type", g_strdup("main"));
> > +    }
> > +
> > +    addr = channel->addr;
> > +    if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
> > +        fprintf(stderr, "%s: addr or its transport is NULL\n",
> > +                __func__);
> > +    }
> > +    g_assert(addr);
> > +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> > +        qdict_put_str(addrDict, "transport", g_strdup("socket"));
> > +    }
> > +
> > +    saddr = &addr->u.socket;
> > +    if (!saddr) {
> > +        fprintf(stderr, "%s: saddr is NULL\n", __func__);
> > +    }
> > +    g_assert(saddr);
> > +    addr_type = socketAddressType_to_str(saddr->type);
> > +    qdict_put_str(addrDict, "type", addr_type);
> > +    qdict_put_str(addrDict, "port", saddr->u.inet.port);
> > +    qdict_put_str(addrDict, "host", saddr->u.inet.host);
> > +
> > +    qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
> > +    qlist_append_obj(channelList, QOBJECT(channelDict));
> > +
> > +    return channelList;
> > +}
> > +
> > +void migrate_qmp_fail(QTestState *who, const char *uri,
> > +                      MigrationChannelList *channels, const char *fmt, ...)
> >  {
> >      va_list ap;
> >      QDict *args, *err;
> > @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> >      args = qdict_from_vjsonf_nofail(fmt, ap);
> >      va_end(ap);
> >
> > -    g_assert(!qdict_haskey(args, "uri"));
> > -    qdict_put_str(args, "uri", uri);
> > +    if (uri) {
> > +        g_assert(!qdict_haskey(args, "uri"));
> 
> IMHO we don't need to assert here?
> 
> Rather than doing this, we can also have tests covering when both are set,
> or when none are set, to make sure we fail properly in those wrong cases.
> (Neglecting your comments here based on Patch 3/3 comment).
> 
> > +        qdict_put_str(args, "uri", uri);
> > +    }
> > +
> > +    if (channels) {
> > +        g_assert(!qdict_haskey(args, "channels"));
> > +        QList *channelList = MigrationChannelList_to_QList(channels);
> > +        qdict_put_obj(args, "channels", QOBJECT(channelList));
> > +    }
> >
> >      err = qtest_qmp_assert_failure_ref(
> >          who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> > @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> >   * Arguments are built from @fmt... (formatted like
> >   * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
> >   */
> > -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> > +void migrate_qmp(QTestState *who, const char *uri,
> > +                 MigrationChannelList *channels, const char *fmt, ...)
> >  {
> >      va_list ap;
> >      QDict *args;
> > @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> >      args = qdict_from_vjsonf_nofail(fmt, ap);
> >      va_end(ap);
> >
> > -    g_assert(!qdict_haskey(args, "uri"));
> > -    qdict_put_str(args, "uri", uri);
> > +    if (uri) {
> > +        g_assert(!qdict_haskey(args, "uri"));
> > +        qdict_put_str(args, "uri", uri);
> > +    }
> > +
> > +    if (channels) {
> > +        g_assert(!qdict_haskey(args, "channels"));
> > +        QList *channelList = MigrationChannelList_to_QList(channels);
> > +        qdict_put_obj(args, "channels", QOBJECT(channelList));
> > +    }
> 
> Duplicated chunks; sign of adding some helper?
> I didn’t think of adding a helper function here as it was just 2 lines of code, already calling MigrationChannelList_to_QList() to avoid duplication. Even then if you have something in your mind to create some helper function, please suggest :)

migrate_qmp_attach_ports(QDict *args, const char *uri,
                         MigrationChannelList *channels)
{
    if (uri) {
        g_assert(!qdict_haskey(args, "uri"));
        qdict_put_str(args, "uri", uri);
    }

    if (channels) {
        g_assert(!qdict_haskey(args, "channels"));
        QList *channelList = MigrationChannelList_to_QList(channels);
        qdict_put_obj(args, "channels", QOBJECT(channelList));
    }
}

If you plan to work on migrate_incoming_qmp(), it'll also be reusable
there.

> 
> >
> >      qtest_qmp_assert_success(who,
> >                               "{ 'execute': 'migrate', 'arguments': %p}", args);
> > diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> > index 3bf7ded1b9..52243bd2df 100644
> > --- a/tests/qtest/migration-helpers.h
> > +++ b/tests/qtest/migration-helpers.h
> > @@ -14,6 +14,7 @@
> >  #define MIGRATION_HELPERS_H
> >
> >  #include "libqtest.h"
> > +#include "migration/migration.h"
> >
> >  typedef struct QTestMigrationState {
> >      bool stop_seen;
> > @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
> >  bool migrate_watch_for_events(QTestState *who, const char *name,
> >                                QDict *event, void *opaque);
> >
> > -G_GNUC_PRINTF(3, 4)
> > -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
> > +G_GNUC_PRINTF(4, 5)
> > +void migrate_qmp(QTestState *who, const char *uri,
> > +                 MigrationChannelList *channels, const char *fmt, ...);
> >
> >  G_GNUC_PRINTF(3, 4)
> >  void migrate_incoming_qmp(QTestState *who, const char *uri,
> >                            const char *fmt, ...);
> Just thinking, should also add ‘channels’ argument here I guess, would be helpful in future to add some tests where only ‘channels’ parameter wants to be added to the function ?

Sounds good.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument

[Het Gala]

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


On 21/02/24 7:54 am, Peter Xu wrote:
> On Tue, Feb 20, 2024 at 06:14:46PM +0000, Het Gala wrote:
>>
>> From: Peter Xu<peterx@redhat.com>
>> Date: Tuesday, 20 February 2024 at 11:33 AM
>> To: Het Gala<het.gala@nutanix.com>
>> Cc:qemu-devel@nongnu.org  <qemu-devel@nongnu.org>,armbru@redhat.com  <armbru@redhat.com>,berrange@redhat.com  <berrange@redhat.com>,farosas@suse.de  <farosas@suse.de>
>> Subject: Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
>> On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
>>> Introduce support for adding a 'channels' argument to migrate_qmp_fail
>>> and migrate_qmp functions within the migration qtest framework, enabling
>>> enhanced control over migration scenarios.
>>>
>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>> ---
>>>   tests/qtest/dbus-vmstate-test.c |  2 +-
>>>   tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
>>>   tests/qtest/migration-helpers.h | 11 ++--
>>>   tests/qtest/migration-test.c    | 33 ++++++------
>>>   4 files changed, 112 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
>>> index 6c990864e3..0ca572e29b 100644
>>> --- a/tests/qtest/dbus-vmstate-test.c
>>> +++ b/tests/qtest/dbus-vmstate-test.c
>>> @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
>>>
>>>       thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
>>>
>>> -    migrate_qmp(src_qemu, uri, "{}");
>>> +    migrate_qmp(src_qemu, uri, NULL, "{}");
>>>       test->src_qemu = src_qemu;
>>>       if (test->migrate_fail) {
>>>           wait_for_migration_fail(src_qemu, true);
>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>> index e451dbdbed..d153677887 100644
>>> --- a/tests/qtest/migration-helpers.c
>>> +++ b/tests/qtest/migration-helpers.c
>>> @@ -13,6 +13,7 @@
>>>   #include "qemu/osdep.h"
>>>   #include "qemu/ctype.h"
>>>   #include "qapi/qmp/qjson.h"
>>> +#include "qapi/qmp/qlist.h"
>>>
>>>   #include "migration-helpers.h"
>>>
>>> @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
>>>       return false;
>>>   }
>>>
>>> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>>> +static char *socketAddressType_to_str(SocketAddressType type)
>>> +{
>>> +    switch (type) {
>>> +    case SOCKET_ADDRESS_TYPE_INET:
>>> +        return g_strdup("inet");
>>> +    case SOCKET_ADDRESS_TYPE_UNIX:
>>> +        return g_strdup("unix");
>>> +    case SOCKET_ADDRESS_TYPE_FD:
>>> +        return g_strdup("fd");
>>> +    case SOCKET_ADDRESS_TYPE_VSOCK:
>>> +        return g_strdup("vsock");
>>> +    default:
>>> +        return g_strdup("unknown address type");
>>> +    }
>>> +}
>> Use SocketAddressType_lookup?
>> Ack, I guess combination of using qapi_enum_parse() and qapi_enum_lookup() would help me eliminate the need for creating this function itself. Let me do the changes in the next patch.  Thanks!
> Ah, what I wanted to say was actually SocketAddressType_str.
Oh okay, got it !
>>> +
>>> +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
>>> +{
>>> +    MigrationChannel *channel = NULL;
>>> +    MigrationAddress *addr = NULL;
>>> +    SocketAddress *saddr = NULL;
>>> +    g_autofree const char *addr_type = NULL;
>>> +    QList *channelList = qlist_new();
>>> +    QDict *channelDict = qdict_new();
>>> +    QDict *addrDict = qdict_new();
>>> +
>>> +    channel = channels->value;
>>> +    if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
>>> +        fprintf(stderr, "%s: Channel or its type is NULL\n",
>>> +                __func__);
>>> +    }
>>> +    g_assert(channel);
>>> +    if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
>>> +        qdict_put_str(channelDict, "channel-type", g_strdup("main"));
>>> +    }
>>> +
>>> +    addr = channel->addr;
>>> +    if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
>>> +        fprintf(stderr, "%s: addr or its transport is NULL\n",
>>> +                __func__);
>>> +    }
>>> +    g_assert(addr);
>>> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>>> +        qdict_put_str(addrDict, "transport", g_strdup("socket"));
>>> +    }
>>> +
>>> +    saddr = &addr->u.socket;
>>> +    if (!saddr) {
>>> +        fprintf(stderr, "%s: saddr is NULL\n", __func__);
>>> +    }
>>> +    g_assert(saddr);
>>> +    addr_type = socketAddressType_to_str(saddr->type);
>>> +    qdict_put_str(addrDict, "type", addr_type);
>>> +    qdict_put_str(addrDict, "port", saddr->u.inet.port);
>>> +    qdict_put_str(addrDict, "host", saddr->u.inet.host);
>>> +
>>> +    qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
>>> +    qlist_append_obj(channelList, QOBJECT(channelDict));
>>> +
>>> +    return channelList;
>>> +}
>>> +
>>> +void migrate_qmp_fail(QTestState *who, const char *uri,
>>> +                      MigrationChannelList *channels, const char *fmt, ...)
>>>   {
>>>       va_list ap;
>>>       QDict *args, *err;
>>> @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>>>       args = qdict_from_vjsonf_nofail(fmt, ap);
>>>       va_end(ap);
>>>
>>> -    g_assert(!qdict_haskey(args, "uri"));
>>> -    qdict_put_str(args, "uri", uri);
>>> +    if (uri) {
>>> +        g_assert(!qdict_haskey(args, "uri"));
>> IMHO we don't need to assert here?
>>
>> Rather than doing this, we can also have tests covering when both are set,
>> or when none are set, to make sure we fail properly in those wrong cases.
>> (Neglecting your comments here based on Patch 3/3 comment).
>>
>>> +        qdict_put_str(args, "uri", uri);
>>> +    }
>>> +
>>> +    if (channels) {
>>> +        g_assert(!qdict_haskey(args, "channels"));
>>> +        QList *channelList = MigrationChannelList_to_QList(channels);
>>> +        qdict_put_obj(args, "channels", QOBJECT(channelList));
>>> +    }
>>>
>>>       err = qtest_qmp_assert_failure_ref(
>>>           who, "{ 'execute': 'migrate', 'arguments': %p}", args);
>>> @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>>>    * Arguments are built from @fmt... (formatted like
>>>    * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
>>>    */
>>> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
>>> +void migrate_qmp(QTestState *who, const char *uri,
>>> +                 MigrationChannelList *channels, const char *fmt, ...)
>>>   {
>>>       va_list ap;
>>>       QDict *args;
>>> @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
>>>       args = qdict_from_vjsonf_nofail(fmt, ap);
>>>       va_end(ap);
>>>
>>> -    g_assert(!qdict_haskey(args, "uri"));
>>> -    qdict_put_str(args, "uri", uri);
>>> +    if (uri) {
>>> +        g_assert(!qdict_haskey(args, "uri"));
>>> +        qdict_put_str(args, "uri", uri);
>>> +    }
>>> +
>>> +    if (channels) {
>>> +        g_assert(!qdict_haskey(args, "channels"));
>>> +        QList *channelList = MigrationChannelList_to_QList(channels);
>>> +        qdict_put_obj(args, "channels", QOBJECT(channelList));
>>> +    }
>> Duplicated chunks; sign of adding some helper?
>> I didn’t think of adding a helper function here as it was just 2 lines of code, already calling MigrationChannelList_to_QList() to avoid duplication. Even then if you have something in your mind to create some helper function, please suggest :)
> migrate_qmp_attach_ports(QDict *args, const char *uri,
>                           MigrationChannelList *channels)
> {
>      if (uri) {
>          g_assert(!qdict_haskey(args, "uri"));
>          qdict_put_str(args, "uri", uri);
>      }
>
>      if (channels) {
>          g_assert(!qdict_haskey(args, "channels"));
>          QList *channelList = MigrationChannelList_to_QList(channels);
>          qdict_put_obj(args, "channels", QOBJECT(channelList));
>      }
> }
>
> If you plan to work on migrate_incoming_qmp(), it'll also be reusable
> there.
Ack, thanks!
>>>       qtest_qmp_assert_success(who,
>>>                                "{ 'execute': 'migrate', 'arguments': %p}", args);
>>> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
>>> index 3bf7ded1b9..52243bd2df 100644
>>> --- a/tests/qtest/migration-helpers.h
>>> +++ b/tests/qtest/migration-helpers.h
>>> @@ -14,6 +14,7 @@
>>>   #define MIGRATION_HELPERS_H
>>>
>>>   #include "libqtest.h"
>>> +#include "migration/migration.h"
>>>
>>>   typedef struct QTestMigrationState {
>>>       bool stop_seen;
>>> @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
>>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>>>                                 QDict *event, void *opaque);
>>>
>>> -G_GNUC_PRINTF(3, 4)
>>> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
>>> +G_GNUC_PRINTF(4, 5)
>>> +void migrate_qmp(QTestState *who, const char *uri,
>>> +                 MigrationChannelList *channels, const char *fmt, ...);
>>>
>>>   G_GNUC_PRINTF(3, 4)
>>>   void migrate_incoming_qmp(QTestState *who, const char *uri,
>>>                             const char *fmt, ...);
>> Just thinking, should also add ‘channels’ argument here I guess, would be helpful in future to add some tests where only ‘channels’ parameter wants to be added to the function ?
> Sounds good.
>
> Thanks,

Regards,

Het Gala

[-- Attachment #2: Type: text/html, Size: 11819 bytes --]

[PATCH 2/3] qtest: migration: Introduce 'connect_channels' in MigrateCommon struct

[Het Gala]

migration QAPIs can now work with either 'channels' or 'uri' as their
argument.

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-test.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e7f2719dcf..0bc69b1943 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -704,6 +704,13 @@ typedef struct {
      */
     const char *connect_uri;
 
+    /*
+     * Optional: list of migration stream channels, each connected
+     * to a dst QEMU. It can be used instead of URI to carry out
+     * the same task as listen_uri or connect_uri.
+     */
+    MigrationChannelList *connect_channels;
+
     /* Optional: callback to run at start to set migration parameters */
     TestMigrateStartHook start_hook;
     /* Optional: callback to run at finish to cleanup */
-- 
2.22.3

Re: [PATCH 2/3] qtest: migration: Introduce 'connect_channels' in MigrateCommon struct

[Peter Xu]

On Fri, Feb 16, 2024 at 09:06:23AM +0000, Het Gala wrote:
> migration QAPIs can now work with either 'channels' or 'uri' as their
> argument.
> 
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-test.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index e7f2719dcf..0bc69b1943 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -704,6 +704,13 @@ typedef struct {
>       */
>      const char *connect_uri;
>  
> +    /*
> +     * Optional: list of migration stream channels, each connected
> +     * to a dst QEMU. It can be used instead of URI to carry out
> +     * the same task as listen_uri or connect_uri.
> +     */
> +    MigrationChannelList *connect_channels;
> +
>      /* Optional: callback to run at start to set migration parameters */
>      TestMigrateStartHook start_hook;
>      /* Optional: callback to run at finish to cleanup */

Please squash this patch into the follow up patch that uses it.  Thanks,

-- 
Peter Xu

Re: [PATCH 2/3] qtest: migration: Introduce 'connect_channels' in MigrateCommon struct

[Het Gala]

On 20/02/24 11:34 am, Peter Xu wrote:
> On Fri, Feb 16, 2024 at 09:06:23AM +0000, Het Gala wrote:
>> migration QAPIs can now work with either 'channels' or 'uri' as their
>> argument.
>>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   tests/qtest/migration-test.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index e7f2719dcf..0bc69b1943 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -704,6 +704,13 @@ typedef struct {
>>        */
>>       const char *connect_uri;
>>   
>> +    /*
>> +     * Optional: list of migration stream channels, each connected
>> +     * to a dst QEMU. It can be used instead of URI to carry out
>> +     * the same task as listen_uri or connect_uri.
>> +     */
>> +    MigrationChannelList *connect_channels;
>> +
>>       /* Optional: callback to run at start to set migration parameters */
>>       TestMigrateStartHook start_hook;
>>       /* Optional: callback to run at finish to cleanup */
> Please squash this patch into the follow up patch that uses it.  Thanks,

Yes sure.

I am also planning to convert this field into a bool (just say whether 
connect_channels would be present or not). It would prove useful for 
positive cases actually where only channel is being used, because if I 
convert them before hand itself port is 0 but kernel converts port 0 and 
gives a random port number for migration. And positive tests fail there.

Will be more clear when I post the v2 patchset. Let me know, if it does 
not sound right then.

Regards,

Het Gala

[PATCH 3/3] qtest: migration: Add negative validation test for 'uri' and 'channels' both set

[Het Gala]

Ideally QAPI 'migrate' and 'migrate-incoming' does not allow 'uri' and
'channels' both arguments to be present in the arguments list as they
are mutually exhaustive.

Add a negative test case to validate the same. Even before the migration
connection is established, qmp command will error out with
MIG_TEST_QMP_ERROR.

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-test.c | 83 ++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 0bc69b1943..9b9395307f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -26,6 +26,7 @@
 #include "qapi/qobject-output-visitor.h"
 #include "crypto/tlscredspsk.h"
 #include "qapi/qmp/qlist.h"
+#include "qemu/cutils.h"
 
 #include "migration-helpers.h"
 #include "tests/migration/migration-test.h"
@@ -2516,6 +2517,86 @@ static void test_validate_uuid_dst_not_set(void)
     do_test_validate_uuid(&args, false);
 }
 
+static MigrationChannelList *uri_to_channels(const char *uri)
+{
+    MigrationChannelList *channels = g_new0(MigrationChannelList, 1);
+    MigrationChannel *val = g_new0(MigrationChannel, 1);
+    MigrationAddress *addr = g_new0(MigrationAddress, 1);
+    char **saddr;
+
+    addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
+
+    saddr = g_strsplit((char *)uri, ":", 3);
+    if (!saddr[0] || saddr[0] != g_strdup("tcp")) {
+        fprintf(stderr, "%s: Invalid URI: %s\n", __func__, uri);
+    }
+    addr->u.socket.type = SOCKET_ADDRESS_TYPE_INET;
+    addr->u.socket.u.inet.host = g_strdup(saddr[1]);
+    addr->u.socket.u.inet.port = g_strdup(saddr[2]);
+    g_strfreev(saddr);
+
+    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
+    val->addr = addr;
+    channels->value = val;
+
+    return channels;
+}
+
+static void do_test_validate_uri_channel(MigrateCommon *args, bool should_fail)
+{
+    QTestState *from, *to;
+
+    if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
+        return;
+    }
+
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    /*
+     * 'uri' and 'channels' validation is checked even before the migration
+     * starts.
+     */
+    if (args->result == MIG_TEST_QMP_ERROR) {
+        migrate_qmp_fail(from, 
+                args->connect_uri ? args->connect_uri : NULL,
+                args->connect_channels ? args->connect_channels : NULL, "{}");
+        goto finish;
+    }
+
+    migrate_qmp(from,
+                args->connect_uri ? args->connect_uri : NULL,
+                args->connect_channels ? args->connect_channels : NULL, "{}");
+
+    if (should_fail) {
+        qtest_set_expected_status(to, EXIT_FAILURE);
+        wait_for_migration_fail(from, false);
+    } else {
+        wait_for_migration_complete(from);
+    }
+
+finish:
+    test_migrate_end(from, to, args->result == MIG_TEST_SUCCEED);
+}
+
+static void
+test_validate_uri_channels_both_set(void)
+{
+    g_autofree char *uri = g_strdup("tcp:127.0.0.1:0");
+
+    MigrateCommon args = {
+        .start = {
+            .hide_stderr = true,
+        },
+        .connect_uri = uri,
+        .connect_channels = uri_to_channels(uri),
+        .listen_uri = "defer",
+        .result = MIG_TEST_QMP_ERROR,
+    };
+
+    do_test_validate_uri_channel(&args, true);
+}
+
 /*
  * The way auto_converge works, we need to do too many passes to
  * run this test.  Auto_converge logic is only run once every
@@ -3544,6 +3625,8 @@ int main(int argc, char **argv)
                        test_validate_uuid_src_not_set);
     migration_test_add("/migration/validate_uuid_dst_not_set",
                        test_validate_uuid_dst_not_set);
+    migration_test_add("/migration/validate_uri/channels/both_set",
+                       test_validate_uri_channels_both_set);
     /*
      * See explanation why this test is slow on function definition
      */
-- 
2.22.3

Re: [PATCH 3/3] qtest: migration: Add negative validation test for 'uri' and 'channels' both set

[Peter Xu]

On Fri, Feb 16, 2024 at 09:06:24AM +0000, Het Gala wrote:
> Ideally QAPI 'migrate' and 'migrate-incoming' does not allow 'uri' and
> 'channels' both arguments to be present in the arguments list as they
> are mutually exhaustive.
> 
> Add a negative test case to validate the same. Even before the migration
> connection is established, qmp command will error out with
> MIG_TEST_QMP_ERROR.
> 
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-test.c | 83 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 0bc69b1943..9b9395307f 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -26,6 +26,7 @@
>  #include "qapi/qobject-output-visitor.h"
>  #include "crypto/tlscredspsk.h"
>  #include "qapi/qmp/qlist.h"
> +#include "qemu/cutils.h"
>  
>  #include "migration-helpers.h"
>  #include "tests/migration/migration-test.h"
> @@ -2516,6 +2517,86 @@ static void test_validate_uuid_dst_not_set(void)
>      do_test_validate_uuid(&args, false);
>  }
>  
> +static MigrationChannelList *uri_to_channels(const char *uri)
> +{
> +    MigrationChannelList *channels = g_new0(MigrationChannelList, 1);
> +    MigrationChannel *val = g_new0(MigrationChannel, 1);
> +    MigrationAddress *addr = g_new0(MigrationAddress, 1);
> +    char **saddr;
> +
> +    addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
> +
> +    saddr = g_strsplit((char *)uri, ":", 3);
> +    if (!saddr[0] || saddr[0] != g_strdup("tcp")) {
> +        fprintf(stderr, "%s: Invalid URI: %s\n", __func__, uri);

Can use g_assert() in such a test; stderr can be easily ignored and
forgotten when it hits.

I'd think parsing it from URI is a slight overkill, as we can pass in
rubbish in the "channels" parameter, but it's still okay.

> +    }
> +    addr->u.socket.type = SOCKET_ADDRESS_TYPE_INET;
> +    addr->u.socket.u.inet.host = g_strdup(saddr[1]);
> +    addr->u.socket.u.inet.port = g_strdup(saddr[2]);
> +    g_strfreev(saddr);
> +
> +    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
> +    val->addr = addr;
> +    channels->value = val;
> +
> +    return channels;
> +}
> +
> +static void do_test_validate_uri_channel(MigrateCommon *args, bool should_fail)
> +{
> +    QTestState *from, *to;
> +
> +    if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
> +        return;
> +    }
> +
> +    /* Wait for the first serial output from the source */
> +    wait_for_serial("src_serial");
> +
> +    /*
> +     * 'uri' and 'channels' validation is checked even before the migration
> +     * starts.
> +     */
> +    if (args->result == MIG_TEST_QMP_ERROR) {
> +        migrate_qmp_fail(from, 
> +                args->connect_uri ? args->connect_uri : NULL,
> +                args->connect_channels ? args->connect_channels : NULL, "{}");
> +        goto finish;
> +    }

IIUC below are dead code.  We can drop them.

> +
> +    migrate_qmp(from,
> +                args->connect_uri ? args->connect_uri : NULL,
> +                args->connect_channels ? args->connect_channels : NULL, "{}");
> +
> +    if (should_fail) {
> +        qtest_set_expected_status(to, EXIT_FAILURE);
> +        wait_for_migration_fail(from, false);
> +    } else {
> +        wait_for_migration_complete(from);
> +    }
> +
> +finish:
> +    test_migrate_end(from, to, args->result == MIG_TEST_SUCCEED);
> +}
> +
> +static void
> +test_validate_uri_channels_both_set(void)
> +{
> +    g_autofree char *uri = g_strdup("tcp:127.0.0.1:0");
> +
> +    MigrateCommon args = {
> +        .start = {
> +            .hide_stderr = true,
> +        },
> +        .connect_uri = uri,
> +        .connect_channels = uri_to_channels(uri),
> +        .listen_uri = "defer",
> +        .result = MIG_TEST_QMP_ERROR,
> +    };

Instead of using MigrateCommon/MIG_TEST_QMP_ERROR, IMHO you can unwrap the
whole do_test_validate_uri_channel() here, invoke migrate_qmp_fail()
directly with the wrong parameter set.

See, for excample, test_baddest().

PS: please scratch my previous comment on patch 1 over the assertion; I
just read that all wrongly.. sorry.

> +
> +    do_test_validate_uri_channel(&args, true);
> +}
> +
>  /*
>   * The way auto_converge works, we need to do too many passes to
>   * run this test.  Auto_converge logic is only run once every
> @@ -3544,6 +3625,8 @@ int main(int argc, char **argv)
>                         test_validate_uuid_src_not_set);
>      migration_test_add("/migration/validate_uuid_dst_not_set",
>                         test_validate_uuid_dst_not_set);
> +    migration_test_add("/migration/validate_uri/channels/both_set",
> +                       test_validate_uri_channels_both_set);
>      /*
>       * See explanation why this test is slow on function definition
>       */
> -- 
> 2.22.3
> 

-- 
Peter Xu

Re: [PATCH 3/3] qtest: migration: Add negative validation test for 'uri' and 'channels' both set

[Het Gala]

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


On 20/02/24 11:57 am, Peter Xu wrote:
> On Fri, Feb 16, 2024 at 09:06:24AM +0000, Het Gala wrote:
>> Ideally QAPI 'migrate' and 'migrate-incoming' does not allow 'uri' and
>> 'channels' both arguments to be present in the arguments list as they
>> are mutually exhaustive.
>>
>> Add a negative test case to validate the same. Even before the migration
>> connection is established, qmp command will error out with
>> MIG_TEST_QMP_ERROR.
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> ---
>>   tests/qtest/migration-test.c | 83 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 83 insertions(+)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 0bc69b1943..9b9395307f 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -26,6 +26,7 @@
>>   #include "qapi/qobject-output-visitor.h"
>>   #include "crypto/tlscredspsk.h"
>>   #include "qapi/qmp/qlist.h"
>> +#include "qemu/cutils.h"
>>   
>>   #include "migration-helpers.h"
>>   #include "tests/migration/migration-test.h"
>> @@ -2516,6 +2517,86 @@ static void test_validate_uuid_dst_not_set(void)
>>       do_test_validate_uuid(&args, false);
>>   }
>>   
>> +static MigrationChannelList *uri_to_channels(const char *uri)
>> +{
>> +    MigrationChannelList *channels = g_new0(MigrationChannelList, 1);
>> +    MigrationChannel *val = g_new0(MigrationChannel, 1);
>> +    MigrationAddress *addr = g_new0(MigrationAddress, 1);
>> +    char **saddr;
>> +
>> +    addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
>> +
>> +    saddr = g_strsplit((char *)uri, ":", 3);
>> +    if (!saddr[0] || saddr[0] != g_strdup("tcp")) {
>> +        fprintf(stderr, "%s: Invalid URI: %s\n", __func__, uri);
> Can use g_assert() in such a test; stderr can be easily ignored and
> forgotten when it hits.
Ack. Will change that.
> I'd think parsing it from URI is a slight overkill, as we can pass in
> rubbish in the "channels" parameter, but it's still okay.
That's why I didn't make the dummy function of socket_parse(), which 
would have been a overkill, and just a split function was added. Let me 
have it for the v2 patchset. If it still feels like a overkill, will 
just have a dummy value for host and port, and type.
>> +    }
>> +    addr->u.socket.type = SOCKET_ADDRESS_TYPE_INET;
>> +    addr->u.socket.u.inet.host = g_strdup(saddr[1]);
>> +    addr->u.socket.u.inet.port = g_strdup(saddr[2]);
>> +    g_strfreev(saddr);
>> +
>> +    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
>> +    val->addr = addr;
>> +    channels->value = val;
>> +
>> +    return channels;
>> +}
>> +
>> +static void do_test_validate_uri_channel(MigrateCommon *args, bool should_fail)
>> +{
>> +    QTestState *from, *to;
>> +
>> +    if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
>> +        return;
>> +    }
>> +
>> +    /* Wait for the first serial output from the source */
>> +    wait_for_serial("src_serial");
>> +
>> +    /*
>> +     * 'uri' and 'channels' validation is checked even before the migration
>> +     * starts.
>> +     */
>> +    if (args->result == MIG_TEST_QMP_ERROR) {
>> +        migrate_qmp_fail(from,
>> +                args->connect_uri ? args->connect_uri : NULL,
>> +                args->connect_channels ? args->connect_channels : NULL, "{}");
>> +        goto finish;
>> +    }
> IIUC below are dead code.  We can drop them.
I have kept it intentional, inorder to add positive test cases.In 
positive test cases, migration would be performed successfully. Will add 
a patch for positive test case too in v2 patchset series.
>> +
>> +    migrate_qmp(from,
>> +                args->connect_uri ? args->connect_uri : NULL,
>> +                args->connect_channels ? args->connect_channels : NULL, "{}");
>> +
>> +    if (should_fail) {
>> +        qtest_set_expected_status(to, EXIT_FAILURE);
>> +        wait_for_migration_fail(from, false);
>> +    } else {
>> +        wait_for_migration_complete(from);
>> +    }
>> +
>> +finish:
>> +    test_migrate_end(from, to, args->result == MIG_TEST_SUCCEED);
>> +}
>> +
>> +static void
>> +test_validate_uri_channels_both_set(void)
>> +{
>> +    g_autofree char *uri = g_strdup("tcp:127.0.0.1:0");
>> +
>> +    MigrateCommon args = {
>> +        .start = {
>> +            .hide_stderr = true,
>> +        },
>> +        .connect_uri = uri,
>> +        .connect_channels = uri_to_channels(uri),
>> +        .listen_uri = "defer",
>> +        .result = MIG_TEST_QMP_ERROR,
>> +    };
> Instead of using MigrateCommon/MIG_TEST_QMP_ERROR, IMHO you can unwrap the
> whole do_test_validate_uri_channel() here, invoke migrate_qmp_fail()
> directly with the wrong parameter set.
Again, kept this for separating negative test cases from positive ones. 
If it still does not make sense in 2nd patchset, can unwrap into a 
single function itself.
> See, for excample, test_baddest().
>
> PS: please scratch my previous comment on patch 1 over the assertion; I
> just read that all wrongly.. sorry.
Ack, no worries.
>> +
>> +    do_test_validate_uri_channel(&args, true);
>> +}
>> +
>>   /*
>>    * The way auto_converge works, we need to do too many passes to
>>    * run this test.  Auto_converge logic is only run once every
>> @@ -3544,6 +3625,8 @@ int main(int argc, char **argv)
>>                          test_validate_uuid_src_not_set);
>>       migration_test_add("/migration/validate_uuid_dst_not_set",
>>                          test_validate_uuid_dst_not_set);
>> +    migration_test_add("/migration/validate_uri/channels/both_set",
>> +                       test_validate_uri_channels_both_set);
>>       /*
>>        * See explanation why this test is slow on function definition
>>        */
>> -- 
>> 2.22.3
>>
Regards,

Het Gala

[-- Attachment #2: Type: text/html, Size: 7511 bytes --]