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

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

[Het Gala]

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

To ensure backward compatibility, both 'uri' and 'channels' are kept as
optional parameters in migration QMP commands. However, they are mutually
exclusive, requiring at least one for a successful migration connection.
This patchset adds qtests to validate 'uri' and 'channels' arguments'
mututally exhaustive behaviour.

Additionally, all migration qtests fail to employ 'channel' as the primary
method for validating migration QAPIs. This patchset also adds validation
test to enforce only use of 'channel' argument as the initial entry point
for migration QAPIs.

Patch Summary:
-------------
1. Introduce migrate_qmp(), migrate_qmp_fail() and migrate_incoming_qmp()
   with 'channels' arguments and a conversion function from
   MigrationChannelList to QList.
2. Add a new bool filed - connect_channels and add two negative validation
   tests to confirm QAPI arguments' mutually exhaustive behaviour
3. Include negative validation tests to disallow both arguments in migration
   QAPIs.

v1->v2 Changelog:
-----------------
1. Introduce 'channels' to migrate_incoming_qmp() too.
2. Squash definition of connect_channels and negative test cases together
3. Add one more negative test case where both 'uri' and 'channels' are not
   passed simulatneously to migrate QAPI.
4. The common function do_test_validate_uri_channel() is modified, similar
   to test_baddest() - causing thefunction to always fail.
5. Add a new positive qtest to start migration qmp command with 'channels'
   argument in place of 'uri' argument.

Het Gala (3):
  qtest: migration: Enhance qtest migration functions to support
    'channels' argument
  qtest: migration: Add negative validation tests for 'uri' and
    'channels'
  qtest: migration: Start migration with 'channels' argument

 tests/qtest/dbus-vmstate-test.c   |   2 +-
 tests/qtest/migration-helpers.c   |  78 ++++++++++++--
 tests/qtest/migration-helpers.h   |  15 +--
 tests/qtest/migration-test.c      | 164 +++++++++++++++++++++++++-----
 tests/qtest/virtio-net-failover.c |   8 +-
 5 files changed, 223 insertions(+), 44 deletions(-)

-- 
2.22.3

[PATCH v2 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,
migrate_incoming_qmp 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   | 78 +++++++++++++++++++++++++++----
 tests/qtest/migration-helpers.h   | 15 +++---
 tests/qtest/migration-test.c      | 43 ++++++++---------
 tests/qtest/virtio-net-failover.c |  8 ++--
 5 files changed, 105 insertions(+), 41 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..15c3650b55 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,67 @@ 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 QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
+{
+    MigrationChannel *channel = NULL;
+    MigrationAddress *addr = NULL;
+    SocketAddress *saddr = 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);
+    qdict_put_str(addrDict, "type", SocketAddressType_str(saddr->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;
+}
+
+static void 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));
+    }
+}
+
+void migrate_qmp_fail(QTestState *who, const char *uri,
+                      MigrationChannelList *channels, const char *fmt, ...)
 {
     va_list ap;
     QDict *args, *err;
@@ -52,8 +113,7 @@ 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);
+    migrate_qmp_attach_ports(args, uri, channels);
 
     err = qtest_qmp_assert_failure_ref(
         who, "{ 'execute': 'migrate', 'arguments': %p}", args);
@@ -68,7 +128,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 +138,7 @@ 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);
+    migrate_qmp_attach_ports(args, uri, channels);
 
     qtest_qmp_assert_success(who,
                              "{ 'execute': 'migrate', 'arguments': %p}", args);
@@ -95,7 +155,8 @@ void migrate_set_capability(QTestState *who, const char *capability,
                              capability, value);
 }
 
-void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
+void migrate_incoming_qmp(QTestState *to, const char *uri,
+                          MigrationChannelList *channels, const char *fmt, ...)
 {
     va_list ap;
     QDict *args, *rsp, *data;
@@ -104,8 +165,7 @@ void migrate_incoming_qmp(QTestState *to, 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);
+    migrate_qmp_attach_ports(args, uri, channels);
 
     migrate_set_capability(to, "events", true);
 
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..390e386274 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)
+G_GNUC_PRINTF(4, 5)
 void migrate_incoming_qmp(QTestState *who, const char *uri,
-                          const char *fmt, ...);
+                        MigrationChannelList *channels, 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..e5547b8746 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,18 +1870,18 @@ 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);
 
     /*
      * We need to wait for the source to finish before starting the
      * destination.
      */
-    migrate_incoming_qmp(to, connect_uri, "{}");
+    migrate_incoming_qmp(to, connect_uri, NULL, "{}");
     wait_for_migration_complete(to);
 
     if (stop_src) {
@@ -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);
 
@@ -2387,7 +2388,7 @@ static void *test_migrate_fd_start_hook(QTestState *from,
     close(pair[0]);
 
     /* Start incoming migration from the 1st socket */
-    migrate_incoming_qmp(to, "fd:fd-mig", "{}");
+    migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}");
 
     /* Send the 2nd socket to the target */
     qtest_qmp_fds_assert_success(from, &pair[1], 1,
@@ -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;
@@ -2609,7 +2610,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
     migrate_set_capability(to, "multifd", true);
 
     /* Start incoming migration from the 1st socket */
-    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
 
     return NULL;
 }
@@ -2862,14 +2863,14 @@ static void test_multifd_tcp_cancel(void)
     migrate_set_capability(to, "multifd", true);
 
     /* Start incoming migration from the 1st socket */
-    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
 
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
     uri = migrate_get_socket_address(to, "socket-address");
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, uri, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2892,7 +2893,7 @@ static void test_multifd_tcp_cancel(void)
     migrate_set_capability(to2, "multifd", true);
 
     /* Start incoming migration from the 1st socket */
-    migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}");
+    migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", NULL, "{}");
 
     g_free(uri);
     uri = migrate_get_socket_address(to2, "socket-address");
@@ -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;
diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
index 73dfabc272..e263ecd80e 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -772,7 +772,7 @@ static void test_migrate_in(gconstpointer opaque)
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, false, "primary0", MAC_PRIMARY0);
 
-    migrate_incoming_qmp(qts, uri, "{}");
+    migrate_incoming_qmp(qts, uri, NULL, "{}");
 
     resp = get_failover_negociated_event(qts);
     g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");
@@ -894,7 +894,7 @@ static void test_off_migrate_in(gconstpointer opaque)
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, true, "primary0", MAC_PRIMARY0);
 
-    migrate_incoming_qmp(qts, uri, "{}");
+    migrate_incoming_qmp(qts, uri, NULL, "{}");
 
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, true, "primary0", MAC_PRIMARY0);
@@ -1021,7 +1021,7 @@ static void test_guest_off_migrate_in(gconstpointer opaque)
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, false, "primary0", MAC_PRIMARY0);
 
-    migrate_incoming_qmp(qts, uri, "{}");
+    migrate_incoming_qmp(qts, uri, NULL, "{}");
 
     check_one_card(qts, true, "standby0", MAC_STANDBY0);
     check_one_card(qts, false, "primary0", MAC_PRIMARY0);
@@ -1746,7 +1746,7 @@ static void test_multi_in(gconstpointer opaque)
     check_one_card(qts, true, "standby1", MAC_STANDBY1);
     check_one_card(qts, false, "primary1", MAC_PRIMARY1);
 
-    migrate_incoming_qmp(qts, uri, "{}");
+    migrate_incoming_qmp(qts, uri, NULL, "{}");
 
     resp = get_failover_negociated_event(qts);
     g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");
-- 
2.22.3

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

[Het Gala]

CI Run for the whole patchset series: 
https://gitlab.com/galahet/Qemu/-/pipelines/1188155391

On 23/02/24 8:55 pm, Het Gala wrote:
> Introduce support for adding a 'channels' argument to migrate_qmp_fail,
> migrate_incoming_qmp 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   | 78 +++++++++++++++++++++++++++----
>   tests/qtest/migration-helpers.h   | 15 +++---
>   tests/qtest/migration-test.c      | 43 ++++++++---------
>   tests/qtest/virtio-net-failover.c |  8 ++--
>   5 files changed, 105 insertions(+), 41 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..15c3650b55 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,67 @@ 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 QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
> +{
> +    MigrationChannel *channel = NULL;
> +    MigrationAddress *addr = NULL;
> +    SocketAddress *saddr = 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);
> +    qdict_put_str(addrDict, "type", SocketAddressType_str(saddr->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;
> +}
> +
> +static void 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));
> +    }
> +}
> +
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> +                      MigrationChannelList *channels, const char *fmt, ...)
>   {
>       va_list ap;
>       QDict *args, *err;
> @@ -52,8 +113,7 @@ 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);
> +    migrate_qmp_attach_ports(args, uri, channels);
>   
>       err = qtest_qmp_assert_failure_ref(
>           who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> @@ -68,7 +128,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 +138,7 @@ 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);
> +    migrate_qmp_attach_ports(args, uri, channels);
>   
>       qtest_qmp_assert_success(who,
>                                "{ 'execute': 'migrate', 'arguments': %p}", args);
> @@ -95,7 +155,8 @@ void migrate_set_capability(QTestState *who, const char *capability,
>                                capability, value);
>   }
>   
> -void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
> +void migrate_incoming_qmp(QTestState *to, const char *uri,
> +                          MigrationChannelList *channels, const char *fmt, ...)
>   {
>       va_list ap;
>       QDict *args, *rsp, *data;
> @@ -104,8 +165,7 @@ void migrate_incoming_qmp(QTestState *to, 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);
> +    migrate_qmp_attach_ports(args, uri, channels);
>   
>       migrate_set_capability(to, "events", true);
>   
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 3bf7ded1b9..390e386274 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)
> +G_GNUC_PRINTF(4, 5)
>   void migrate_incoming_qmp(QTestState *who, const char *uri,
> -                          const char *fmt, ...);
> +                        MigrationChannelList *channels, 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..e5547b8746 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,18 +1870,18 @@ 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);
>   
>       /*
>        * We need to wait for the source to finish before starting the
>        * destination.
>        */
> -    migrate_incoming_qmp(to, connect_uri, "{}");
> +    migrate_incoming_qmp(to, connect_uri, NULL, "{}");
>       wait_for_migration_complete(to);
>   
>       if (stop_src) {
> @@ -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);
>   
> @@ -2387,7 +2388,7 @@ static void *test_migrate_fd_start_hook(QTestState *from,
>       close(pair[0]);
>   
>       /* Start incoming migration from the 1st socket */
> -    migrate_incoming_qmp(to, "fd:fd-mig", "{}");
> +    migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}");
>   
>       /* Send the 2nd socket to the target */
>       qtest_qmp_fds_assert_success(from, &pair[1], 1,
> @@ -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;
> @@ -2609,7 +2610,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
>       migrate_set_capability(to, "multifd", true);
>   
>       /* Start incoming migration from the 1st socket */
> -    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
> +    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>   
>       return NULL;
>   }
> @@ -2862,14 +2863,14 @@ static void test_multifd_tcp_cancel(void)
>       migrate_set_capability(to, "multifd", true);
>   
>       /* Start incoming migration from the 1st socket */
> -    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
> +    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>   
>       /* Wait for the first serial output from the source */
>       wait_for_serial("src_serial");
>   
>       uri = migrate_get_socket_address(to, "socket-address");
>   
> -    migrate_qmp(from, uri, "{}");
> +    migrate_qmp(from, uri, NULL, "{}");
>   
>       migrate_wait_for_dirty_mem(from, to);
>   
> @@ -2892,7 +2893,7 @@ static void test_multifd_tcp_cancel(void)
>       migrate_set_capability(to2, "multifd", true);
>   
>       /* Start incoming migration from the 1st socket */
> -    migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}");
> +    migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", NULL, "{}");
>   
>       g_free(uri);
>       uri = migrate_get_socket_address(to2, "socket-address");
> @@ -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;
> diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
> index 73dfabc272..e263ecd80e 100644
> --- a/tests/qtest/virtio-net-failover.c
> +++ b/tests/qtest/virtio-net-failover.c
> @@ -772,7 +772,7 @@ static void test_migrate_in(gconstpointer opaque)
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, false, "primary0", MAC_PRIMARY0);
>   
> -    migrate_incoming_qmp(qts, uri, "{}");
> +    migrate_incoming_qmp(qts, uri, NULL, "{}");
>   
>       resp = get_failover_negociated_event(qts);
>       g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");
> @@ -894,7 +894,7 @@ static void test_off_migrate_in(gconstpointer opaque)
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, true, "primary0", MAC_PRIMARY0);
>   
> -    migrate_incoming_qmp(qts, uri, "{}");
> +    migrate_incoming_qmp(qts, uri, NULL, "{}");
>   
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, true, "primary0", MAC_PRIMARY0);
> @@ -1021,7 +1021,7 @@ static void test_guest_off_migrate_in(gconstpointer opaque)
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, false, "primary0", MAC_PRIMARY0);
>   
> -    migrate_incoming_qmp(qts, uri, "{}");
> +    migrate_incoming_qmp(qts, uri, NULL, "{}");
>   
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, false, "primary0", MAC_PRIMARY0);
> @@ -1746,7 +1746,7 @@ static void test_multi_in(gconstpointer opaque)
>       check_one_card(qts, true, "standby1", MAC_STANDBY1);
>       check_one_card(qts, false, "primary1", MAC_PRIMARY1);
>   
> -    migrate_incoming_qmp(qts, uri, "{}");
> +    migrate_incoming_qmp(qts, uri, NULL, "{}");
>   
>       resp = get_failover_negociated_event(qts);
>       g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");

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

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

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

Can't we just pass a channels string like you did in the original series
with migrate_postcopy_prepare?

We'd change migrate_* functions like this:

  void migrate_qmp(QTestState *who, const char *uri, const char *channels,
                   const char *fmt, ...)
  {
  ...
      g_assert(!qdict_haskey(args, "uri"));
      if (uri) {
          qdict_put_str(args, "uri", uri);
      }
  
      g_assert(!qdict_haskey(args, "channels"));
      if (channels) {
          qdict_put_str(args, "channels", channels);
      }
  }

Write the test like this:

  static void test_multifd_tcp_none_channels(void)
  {
      MigrateCommon args = {
          .listen_uri = "defer",
          .start_hook = test_migrate_precopy_tcp_multifd_start,
          .live = true,
          .connect_channels = "'channels': [ { 'channel-type': 'main',"
                              "      'addr': { 'transport': 'socket',"
                              "                'type': 'inet',"
                              "                'host': '127.0.0.1',"
                              "                'port': '0' } } ]",
          .connect_uri = NULL;
                               
      };
      test_precopy_common(&args);
  }

  static void do_test_validate_uri_channel(MigrateCommon *args)
  {
      QTestState *from, *to;
      g_autofree char *connect_uri = NULL;
  
      if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
          return;
      }
  
      wait_for_serial("src_serial");
  
      if (args->result == MIG_TEST_QMP_ERROR) {
          migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
      } else {
          migrate_qmp(from, args->connect_uri, args->connect_channels, "{}");
      }
  
      test_migrate_end(from, to, false);
  }

It's better to require test writers to pass in their own uri and channel
strings. Otherwise any new transport added will require people to modify
these conversion helpers.

Also, using the same string as the user would use in QMP helps with
development in general. One could refer to the tests to see how to
invoke the migration or experiment with the string in the tests during
development.

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

[Het Gala]

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


On 24/02/24 1:42 am, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Introduce support for adding a 'channels' argument to migrate_qmp_fail,
>> migrate_incoming_qmp and migrate_qmp functions within the migration qtest
>> framework, enabling enhanced control over migration scenarios.
> Can't we just pass a channels string like you did in the original series
> with migrate_postcopy_prepare?
>
> We'd change migrate_* functions like this:
>
>    void migrate_qmp(QTestState *who, const char *uri, const char *channels,
>                     const char *fmt, ...)
>    {
>    ...
>        g_assert(!qdict_haskey(args, "uri"));
>        if (uri) {
>            qdict_put_str(args, "uri", uri);
>        }
>    
>        g_assert(!qdict_haskey(args, "channels"));
>        if (channels) {
>            qdict_put_str(args, "channels", channels);
>        }
>    }
>
> Write the test like this:
>
>    static void test_multifd_tcp_none_channels(void)
>    {
>        MigrateCommon args = {
>            .listen_uri = "defer",
>            .start_hook = test_migrate_precopy_tcp_multifd_start,
>            .live = true,
>            .connect_channels = "'channels': [ { 'channel-type': 'main',"
>                                "      'addr': { 'transport': 'socket',"
>                                "                'type': 'inet',"
>                                "                'host': '127.0.0.1',"
>                                "                'port': '0' } } ]",
>            .connect_uri = NULL;
>                                 
>        };
>        test_precopy_common(&args);
>    }

this was the same first approach that I attempted. It won't work because

The final 'migrate' QAPI with channels string would look like

{ "execute": "migrate", "arguments": { "channels": "[ { "channel-type": 
"main", "addr": { "transport": "socket", "type": "inet", "host": 
"10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }

instead of

{ "execute": "migrate", "arguments": { "channels": [ { "channel-type": 
"main", "addr": { "transport": "socket", "type": "inet", "host": 
"10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }

It would complain, that channels should be an *array* and not a string.

So, that's the reason parsing was required in qtest too.

I would be glad to hear if there are any ideas to convert /string -> 
json object -> add it inside qdict along with uri/ ?

>    static void do_test_validate_uri_channel(MigrateCommon *args)
>    {
>        QTestState *from, *to;
>        g_autofree char *connect_uri = NULL;
>    
>        if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
>            return;
>        }
>    
>        wait_for_serial("src_serial");
>    
>        if (args->result == MIG_TEST_QMP_ERROR) {
>            migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
>        } else {
>            migrate_qmp(from, args->connect_uri, args->connect_channels, "{}");
>        }
>    
>        test_migrate_end(from, to, false);
>    }
>
> It's better to require test writers to pass in their own uri and channel
> strings. Otherwise any new transport added will require people to modify
> these conversion helpers.
I agree with your point here. I was thinking to have a general but a 
hacky version of migrate_uri_parse() but that too seemed like a 
overkill. I don't have a better solution to this right now
> Also, using the same string as the user would use in QMP helps with
> development in general. One could refer to the tests to see how to
> invoke the migration or experiment with the string in the tests during
> development.

For examples, I think - enough examples with 'channel' argument are 
provided where 'migrate' QAPI is defined. users can directly copy the 
qmp command from there itself.


Regards,

Het Gala

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

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

[Het Gala]

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


On 24/02/24 6:18 pm, Het Gala wrote:
>
>
> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> Introduce support for adding a 'channels' argument to migrate_qmp_fail,
>>> migrate_incoming_qmp and migrate_qmp functions within the migration qtest
>>> framework, enabling enhanced control over migration scenarios.
>> [...]
>>
>> Write the test like this:
>>
>>    static void test_multifd_tcp_none_channels(void)
>>    {
>>        MigrateCommon args = {
>>            .listen_uri = "defer",
>>            .start_hook = test_migrate_precopy_tcp_multifd_start,
>>            .live = true,
>>            .connect_channels = "'channels': [ { 'channel-type': 'main',"
>>                                "      'addr': { 'transport': 'socket',"
>>                                "                'type': 'inet',"
>>                                "                'host': '127.0.0.1',"
>>                                "                'port': '0' } } ]",
>>            .connect_uri = NULL;
>>                                 
>>        };
>>        test_precopy_common(&args);
>>    }
>
> this was the same first approach that I attempted. It won't work because
>
> The final 'migrate' QAPI with channels string would look like
>
> { "execute": "migrate", "arguments": { "channels": "[ { 
> "channel-type": "main", "addr": { "transport": "socket", "type": 
> "inet", "host": "10.117.29.84", "port": "4000" }, "multifd-channels": 
> 2 } ]" } }
>
Sorry, In your example given above, the output looks like :

{"execute": "migrate", "arguments": {"uri": "tcp:127.0.0.1:0", 
"channels": "'channels': [ { 'channel-type': 'main',      'addr': { 
'transport': 'socket',                'type': 'inet',                
'host': '127.0.0.1',                'port': '0' } } ]"}}

> instead of
>
> { "execute": "migrate", "arguments": { "channels": [ { "channel-type": 
> "main", "addr": { "transport": "socket", "type": "inet", "host": 
> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
>
> It would complain, that channels should be an *array* and not a string.
>
because of /"channels": "'channels': [ { 'channel-type': ....
/
> [...]
>
> Regards,
>
> Het Gala
>
Regards,

Het Gala

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

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

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> Introduce support for adding a 'channels' argument to migrate_qmp_fail,
>>> migrate_incoming_qmp and migrate_qmp functions within the migration qtest
>>> framework, enabling enhanced control over migration scenarios.
>> Can't we just pass a channels string like you did in the original series
>> with migrate_postcopy_prepare?
>>
>> We'd change migrate_* functions like this:
>>
>>    void migrate_qmp(QTestState *who, const char *uri, const char *channels,
>>                     const char *fmt, ...)
>>    {
>>    ...
>>        g_assert(!qdict_haskey(args, "uri"));
>>        if (uri) {
>>            qdict_put_str(args, "uri", uri);
>>        }
>>    
>>        g_assert(!qdict_haskey(args, "channels"));
>>        if (channels) {
>>            qdict_put_str(args, "channels", channels);
>>        }
>>    }
>>
>> Write the test like this:
>>
>>    static void test_multifd_tcp_none_channels(void)
>>    {
>>        MigrateCommon args = {
>>            .listen_uri = "defer",
>>            .start_hook = test_migrate_precopy_tcp_multifd_start,
>>            .live = true,
>>            .connect_channels = "'channels': [ { 'channel-type': 'main',"
>>                                "      'addr': { 'transport': 'socket',"
>>                                "                'type': 'inet',"
>>                                "                'host': '127.0.0.1',"
>>                                "                'port': '0' } } ]",
>>            .connect_uri = NULL;
>>                                 
>>        };
>>        test_precopy_common(&args);
>>    }
>
> this was the same first approach that I attempted. It won't work because
>
> The final 'migrate' QAPI with channels string would look like
>
> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": 
> "main", "addr": { "transport": "socket", "type": "inet", "host": 
> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
>
> instead of
>
> { "execute": "migrate", "arguments": { "channels": [ { "channel-type": 
> "main", "addr": { "transport": "socket", "type": "inet", "host": 
> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
>
> It would complain, that channels should be an *array* and not a string.
>
> So, that's the reason parsing was required in qtest too.
>
> I would be glad to hear if there are any ideas to convert /string -> 
> json object -> add it inside qdict along with uri/ ?
>

Isn't this what the various qobject_from_json do? How does it work with
the existing tests?

    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
                             "  'arguments': { "
                             "      'channels': [ { 'channel-type': 'main',"
                             "      'addr': { 'transport': 'socket',"
                             "                'type': 'inet',"
                             "                'host': '127.0.0.1',"
                             "                'port': '0' } } ] } }");

We can pass this^ string successfully to QMP somehow...

>>    static void do_test_validate_uri_channel(MigrateCommon *args)
>>    {
>>        QTestState *from, *to;
>>        g_autofree char *connect_uri = NULL;
>>    
>>        if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
>>            return;
>>        }
>>    
>>        wait_for_serial("src_serial");
>>    
>>        if (args->result == MIG_TEST_QMP_ERROR) {
>>            migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
>>        } else {
>>            migrate_qmp(from, args->connect_uri, args->connect_channels, "{}");
>>        }
>>    
>>        test_migrate_end(from, to, false);
>>    }
>>
>> It's better to require test writers to pass in their own uri and channel
>> strings. Otherwise any new transport added will require people to modify
>> these conversion helpers.
> I agree with your point here. I was thinking to have a general but a 
> hacky version of migrate_uri_parse() but that too seemed like a 
> overkill. I don't have a better solution to this right now
>> Also, using the same string as the user would use in QMP helps with
>> development in general. One could refer to the tests to see how to
>> invoke the migration or experiment with the string in the tests during
>> development.
>
> For examples, I think - enough examples with 'channel' argument are 
> provided where 'migrate' QAPI is defined. users can directly copy the 
> qmp command from there itself.
>
>
> Regards,
>
> Het Gala

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

[Het Gala]

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


On 26/02/24 6:31 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>> this was the same first approach that I attempted. It won't work because
>>
>> The final 'migrate' QAPI with channels string would look like
>>
>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
>>
>> instead of
>>
>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type":
>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
>>
>> It would complain, that channels should be an *array* and not a string.
>>
>> So, that's the reason parsing was required in qtest too.
>>
>> I would be glad to hear if there are any ideas to convert /string ->
>> json object -> add it inside qdict along with uri/ ?
>>
> Isn't this what the various qobject_from_json do? How does it work with
> the existing tests?
>
>      qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
>                               "  'arguments': { "
>                               "      'channels': [ { 'channel-type': 'main',"
>                               "      'addr': { 'transport': 'socket',"
>                               "                'type': 'inet',"
>                               "                'host': '127.0.0.1',"
>                               "                'port': '0' } } ] } }");
>
> We can pass this^ string successfully to QMP somehow...

I think, here in qtest_qmp_assert_success, we actually can pass the 
whole QMP command, and it just asserts that return key is present in the 
response, though I am not very much familiar with qtest codebase to 
verify how swiftly we can convert string into an actual QObject.

[...]

>>>     static void do_test_validate_uri_channel(MigrateCommon *args)
>>>     {
>>>         QTestState *from, *to;
>>>         g_autofree char *connect_uri = NULL;
>>>     
>>>         if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
>>>             return;
>>>         }
>>>
>>>
>>>
>>> Regards,
>>>
>>> Het Gala

Regards,

Het Gala

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

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

[Het Gala]

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


On 27/02/24 1:04 am, Het Gala wrote:
>
>
> On 26/02/24 6:31 pm, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>>> this was the same first approach that I attempted. It won't work because
>>>
>>> The final 'migrate' QAPI with channels string would look like
>>>
>>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
>>>
>>> instead of
>>>
>>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type":
>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
>>>
>>> It would complain, that channels should be an *array* and not a string.
>>>
>>> So, that's the reason parsing was required in qtest too.
>>>
>>> I would be glad to hear if there are any ideas to convert /string ->
>>> json object -> add it inside qdict along with uri/ ?
>>>
>> Isn't this what the various qobject_from_json do? How does it work with
>> the existing tests?
>>
>>      qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
>>                               "  'arguments': { "
>>                               "      'channels': [ { 'channel-type': 'main',"
>>                               "      'addr': { 'transport': 'socket',"
>>                               "                'type': 'inet',"
>>                               "                'host': '127.0.0.1',"
>>                               "                'port': '0' } } ] } }");
>>
>> We can pass this^ string successfully to QMP somehow...
>
> I think, here in qtest_qmp_assert_success, we actually can pass the 
> whole QMP command, and it just asserts that return key is present in 
> the response, though I am not very much familiar with qtest codebase 
> to verify how swiftly we can convert string into an actual QObject.
>
> [...]
>
I tried with qobject_from_json type of utility functions and the error I 
got was this :

migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: 
qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < 
QTYPE__MAX' failed.

And I suppose this was the case because, there are only limited types of 
QTYPE available

typedefenumQType{
QTYPE_NONE,
QTYPE_QNULL,
QTYPE_QNUM,
QTYPE_QSTRING,
QTYPE_QDICT,
QTYPE_QLIST,
QTYPE_QBOOL,
QTYPE__MAX,
} QType;

And 'channels' is a mixture of QDICT and QLIST and hence it is not able 
to easily convert from string to json.

Thoughts on this ?

>>>>     static void do_test_validate_uri_channel(MigrateCommon *args)
>>>>     {
>>>>         QTestState *from, *to;
>>>>         g_autofree char *connect_uri = NULL;
>>>>     
>>>>         if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
>>>>             return;
>>>>         }
>>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Het Gala

Regards,

Het Gala


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

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

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> On 27/02/24 1:04 am, Het Gala wrote:
>>
>>
>> On 26/02/24 6:31 pm, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>  writes:
>>>
>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>>>> this was the same first approach that I attempted. It won't work because
>>>>
>>>> The final 'migrate' QAPI with channels string would look like
>>>>
>>>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
>>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
>>>>
>>>> instead of
>>>>
>>>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type":
>>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
>>>>
>>>> It would complain, that channels should be an *array* and not a string.
>>>>
>>>> So, that's the reason parsing was required in qtest too.
>>>>
>>>> I would be glad to hear if there are any ideas to convert /string ->
>>>> json object -> add it inside qdict along with uri/ ?
>>>>
>>> Isn't this what the various qobject_from_json do? How does it work with
>>> the existing tests?
>>>
>>>      qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
>>>                               "  'arguments': { "
>>>                               "      'channels': [ { 'channel-type': 'main',"
>>>                               "      'addr': { 'transport': 'socket',"
>>>                               "                'type': 'inet',"
>>>                               "                'host': '127.0.0.1',"
>>>                               "                'port': '0' } } ] } }");
>>>
>>> We can pass this^ string successfully to QMP somehow...
>>
>> I think, here in qtest_qmp_assert_success, we actually can pass the 
>> whole QMP command, and it just asserts that return key is present in 
>> the response, though I am not very much familiar with qtest codebase 
>> to verify how swiftly we can convert string into an actual QObject.
>>
>> [...]
>>
> I tried with qobject_from_json type of utility functions and the error I 
> got was this :
>
> migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: 
> qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < 
> QTYPE__MAX' failed.
>
> And I suppose this was the case because, there are only limited types of 
> QTYPE available
>
> typedefenumQType{
> QTYPE_NONE,
> QTYPE_QNULL,
> QTYPE_QNUM,
> QTYPE_QSTRING,
> QTYPE_QDICT,
> QTYPE_QLIST,
> QTYPE_QBOOL,
> QTYPE__MAX,
> } QType;
>
> And 'channels' is a mixture of QDICT and QLIST and hence it is not able 
> to easily convert from string to json.
>
> Thoughts on this ?

I'm not sure what you tried. This works:

    g_assert(!qdict_haskey(args, "channels"));
    if (channels) {
        channels_obj = qobject_from_json(channels, errp);
        qdict_put_obj(args, "channels", channels_obj);
    }

And in the test:

        .connect_channels = "[ { 'channel-type': 'main',"
                            "    'addr': { 'transport': 'socket',"
                            "              'type': 'inet',"
                            "              'host': '127.0.0.1',"
                            "              'port': '0' } } ]",
        .listen_uri = "tcp:127.0.0.1:0",
        .result = MIG_TEST_QMP_ERROR

However, the real issue is how to inject the port for the source
migration. The example above only works for the tests that are expected
to fail. For a test that should pass, 0 as a port does not work.

I'm thinking it might be better to alter migrate_qmp like this:

  void migrate_qmp(QTestState *from, QTestState *to, const char *channels,
                   const char *fmt, ...)

Invocations would be:

  migrate_qmp(from, to, NULL, "{uri: %s}", connect_uri);
  migrate_qmp(from, to, args->channels, "{}");

In this last case, if the test provided a port, we use it, otherwise we
resolve it from the 'to' instance and put it in the QDict directly.

I'll play with this a bit more tomorrow, let me know what you think.

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

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Het Gala <het.gala@nutanix.com> writes:
>
>> On 27/02/24 1:04 am, Het Gala wrote:
>>>
>>>
>>> On 26/02/24 6:31 pm, Fabiano Rosas wrote:
>>>> Het Gala<het.gala@nutanix.com>  writes:
>>>>
>>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>>>>> this was the same first approach that I attempted. It won't work because
>>>>>
>>>>> The final 'migrate' QAPI with channels string would look like
>>>>>
>>>>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
>>>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
>>>>>
>>>>> instead of
>>>>>
>>>>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type":
>>>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
>>>>>
>>>>> It would complain, that channels should be an *array* and not a string.
>>>>>
>>>>> So, that's the reason parsing was required in qtest too.
>>>>>
>>>>> I would be glad to hear if there are any ideas to convert /string ->
>>>>> json object -> add it inside qdict along with uri/ ?
>>>>>
>>>> Isn't this what the various qobject_from_json do? How does it work with
>>>> the existing tests?
>>>>
>>>>      qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
>>>>                               "  'arguments': { "
>>>>                               "      'channels': [ { 'channel-type': 'main',"
>>>>                               "      'addr': { 'transport': 'socket',"
>>>>                               "                'type': 'inet',"
>>>>                               "                'host': '127.0.0.1',"
>>>>                               "                'port': '0' } } ] } }");
>>>>
>>>> We can pass this^ string successfully to QMP somehow...
>>>
>>> I think, here in qtest_qmp_assert_success, we actually can pass the 
>>> whole QMP command, and it just asserts that return key is present in 
>>> the response, though I am not very much familiar with qtest codebase 
>>> to verify how swiftly we can convert string into an actual QObject.
>>>
>>> [...]
>>>
>> I tried with qobject_from_json type of utility functions and the error I 
>> got was this :
>>
>> migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: 
>> qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < 
>> QTYPE__MAX' failed.
>>
>> And I suppose this was the case because, there are only limited types of 
>> QTYPE available
>>
>> typedefenumQType{
>> QTYPE_NONE,
>> QTYPE_QNULL,
>> QTYPE_QNUM,
>> QTYPE_QSTRING,
>> QTYPE_QDICT,
>> QTYPE_QLIST,
>> QTYPE_QBOOL,
>> QTYPE__MAX,
>> } QType;
>>
>> And 'channels' is a mixture of QDICT and QLIST and hence it is not able 
>> to easily convert from string to json.
>>
>> Thoughts on this ?
>
> I'm not sure what you tried. This works:
>
>     g_assert(!qdict_haskey(args, "channels"));
>     if (channels) {
>         channels_obj = qobject_from_json(channels, errp);
>         qdict_put_obj(args, "channels", channels_obj);
>     }
>
> And in the test:
>
>         .connect_channels = "[ { 'channel-type': 'main',"
>                             "    'addr': { 'transport': 'socket',"
>                             "              'type': 'inet',"
>                             "              'host': '127.0.0.1',"
>                             "              'port': '0' } } ]",
>         .listen_uri = "tcp:127.0.0.1:0",
>         .result = MIG_TEST_QMP_ERROR
>
> However, the real issue is how to inject the port for the source
> migration. The example above only works for the tests that are expected
> to fail. For a test that should pass, 0 as a port does not work.
>
> I'm thinking it might be better to alter migrate_qmp like this:
>
>   void migrate_qmp(QTestState *from, QTestState *to, const char *channels,
>                    const char *fmt, ...)
>
> Invocations would be:
>
>   migrate_qmp(from, to, NULL, "{uri: %s}", connect_uri);
>   migrate_qmp(from, to, args->channels, "{}");
>
> In this last case, if the test provided a port, we use it, otherwise we
> resolve it from the 'to' instance and put it in the QDict directly.
>
> I'll play with this a bit more tomorrow, let me know what you think.

Ok, so here's what I think we should do:

1) Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() to get the port. Leave the other
migrate_qmp* alone because they don't need the port;

2) Move the calls to migrate_get_socket_address() into migrate_qmp() and
get rid of that connect_uri, use args->connect_uri instead everywhere;

3) Add a new function SocketAddress_to_qdict() that does the same as
SocketAddress_to_str(), but fills in a QDict instead;

4) Add args->connect_channels and pass it to migrate_qmp() and
migrate_qmp_fail(). Convert the string to a dict;

    if (channels) {
        channels_obj = qobject_from_json(channels, &error_abort);
        qdict_put_obj(args, "channels", channels_obj);
    }

5) Add a migrate_set_ports() function that uses 3) and fills in the port
in case it was 0 in the test. Handle a list of channels so we can add a
negative test that passes two channels;

  addr = migrate_get_socket_address(to);
  for each channel:
      if channel["port"] && channel["port"] == "0":
          channel["port"] = SocketAddress_to_qdict(addr->value)["port"]

(optionally iterate over addr to be prepared for when we add more
channels)

6) Alter migrate_qmp_fail() to allow both uri and channels
independently. No dealing with migrate_get_socket_address() because we
will fail before starting the migration anyway;

    if (uri) {
        qdict_put_str(args, "uri", uri);
    }

    if (channels) {
        channels_obj = qobject_from_json(channels, &error_abort);
        qdict_put_obj(args, "channels", channels_obj);
    }

7) Alter migrate_qmp() to only fill the uri if there are no
channels. Here we don't want to allow the wrong cases of having both or
none;

    if (uri) {
        qdict_put_str(args, "uri", uri);
    } else if (!channels) {
        qdict_put_str(args, "uri", migrate_get_socket_address(to));
    }

    if (channels) {
        channels_obj = qobject_from_json(channels, &error_abort);
        migrate_set_ports(to, qobject_to(QList, channels_obj));            
        qdict_put_obj(args, "channels", channels_obj);
    }

8) Write the tests to pass the list of channels;

    .connect_channels = "[ { 'channel-type': 'main',"
                        "    'addr': { 'transport': 'socket',"
                        "              'type': 'inet',"
                        "              'host': '127.0.0.1',"
                        "              'port': '0' } } ]",

With this we can test multiple channels, test other types of channel,
add more channel types in the future, etc and not need to change the
test infra.

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

[Het Gala]

On 29/02/24 6:47 am, Fabiano Rosas wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> On 27/02/24 1:04 am, Het Gala wrote:
>>>
>>> On 26/02/24 6:31 pm, Fabiano Rosas wrote:
>>>> Het Gala<het.gala@nutanix.com>  writes:
>>>>
>>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>>>>> this was the same first approach that I attempted. It won't work because
>>>>>
>>>>> The final 'migrate' QAPI with channels string would look like
>>>>>
>>>>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
>>>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
>>>>>
>>>>> instead of
>>>>>
>>>>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type":
>>>>> "main", "addr": { "transport": "socket", "type": "inet", "host":
>>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
>>>>>
>>>>> It would complain, that channels should be an *array* and not a string.
>>>>>
>>>>> So, that's the reason parsing was required in qtest too.
>>>>>
>>>>> I would be glad to hear if there are any ideas to convert /string ->
>>>>> json object -> add it inside qdict along with uri/ ?
>>>>>
>>>> Isn't this what the various qobject_from_json do? How does it work with
>>>> the existing tests?
>>>>
>>>>       qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
>>>>                                "  'arguments': { "
>>>>                                "      'channels': [ { 'channel-type': 'main',"
>>>>                                "      'addr': { 'transport': 'socket',"
>>>>                                "                'type': 'inet',"
>>>>                                "                'host': '127.0.0.1',"
>>>>                                "                'port': '0' } } ] } }");
>>>>
>>>> We can pass this^ string successfully to QMP somehow...
>>> I think, here in qtest_qmp_assert_success, we actually can pass the
>>> whole QMP command, and it just asserts that return key is present in
>>> the response, though I am not very much familiar with qtest codebase
>>> to verify how swiftly we can convert string into an actual QObject.
>>>
>>> [...]
>>>
>> I tried with qobject_from_json type of utility functions and the error I
>> got was this :
>>
>> migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126:
>> qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type <
>> QTYPE__MAX' failed.
>>
>> And I suppose this was the case because, there are only limited types of
>> QTYPE available
>>
>> typedefenumQType{
>> QTYPE_NONE,
>> QTYPE_QNULL,
>> QTYPE_QNUM,
>> QTYPE_QSTRING,
>> QTYPE_QDICT,
>> QTYPE_QLIST,
>> QTYPE_QBOOL,
>> QTYPE__MAX,
>> } QType;
>>
>> And 'channels' is a mixture of QDICT and QLIST and hence it is not able
>> to easily convert from string to json.
>>
>> Thoughts on this ?
> I'm not sure what you tried. This works:
>
>      g_assert(!qdict_haskey(args, "channels"));
>      if (channels) {
>          channels_obj = qobject_from_json(channels, errp);
>          qdict_put_obj(args, "channels", channels_obj);
>      }

Are you sure the above works ?

Sorry I want to get this doubt cleared first, Let's me take a step back 
and just focus on the above part and not focus on port issue for now.

Adding a validation test for uri and channels both used together

migration_test_add("/migration/validate_uri/channels/both_set",
                    test_validate_uri_channels_both_set);
static void test_validate_uri_channels_both_set(void)
{

    QTestState *from, *to;

    MigrateCommon args = {
         .connect_channels = "'channels': [ { 'channel-type': 'main',"
                             "      'addr': { 'transport': 'socket',"
                             "                'type': 'inet',"
                             "                'host': '127.0.0.1',"
                             "                'port': '0' } } ]",
         .listen_uri = "tcp:127.0.0.1:0",
         .result = MIG_TEST_QMP_ERROR,

     };

     if (test_migrate_start(&from, &to, args.listen_uri, &args.start)) {
         return;
     }

     wait_for_serial("src_serial");

     if (args.result == MIG_TEST_QMP_ERROR) {
         migrate_qmp_fail(from, args.connect_uri, args.connect_channels, 
"{}");
     } else {
         migrate_qmp(from, args.connect_uri, args.connect_channels, "{}");
     }

     test_migrate_end(from, to, false);
}

In the migration-helpers.c file, inside migrate_qmp_fail function:

void migrate_qmp_fail(QTestState *who, const char *uri,
                       const char *channels, const char *fmt, ...)
{
     [...]

     Error **errp = NULL;
     QObject *channels_obj = NULL;

     [...]

     g_assert(!qdict_haskey(args, "channels"));
     if (channels) {
         channels_obj = qobject_from_json(channels, errp);
         if (!channels_obj) {
             fprintf(stdout, "Everything is right till here also ?\n");
             goto cleanup;
         }
         qdict_put_obj(args, "channels", channels_obj);
     }

     err = qtest_qmp_assert_failure_ref(
         who, "{ 'execute': 'migrate', 'arguments': %p}", args);

    [...]

cleanup:
     qobject_unref(channels_obj);
}

When I ran the test alone, test passed, but instead of giving output as

{"error": {"class": "GenericError", "desc": "need either 'uri' or 
'channels' argument"}}

It just passed, i.e. channels_obj for me was NULL and hence just ended 
the test. What wrong am I doing here according to you ? Because, IMO if 
qobject_from_json() is not working properly, this approach of passing 
channels as string might not work at all.


Regards,

Het Gala

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

[Het Gala]

On 01/03/24 2:19 pm, Het Gala wrote:
>
> On 29/02/24 6:47 am, Fabiano Rosas wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> On 27/02/24 1:04 am, Het Gala wrote:
>>>>
>>>> On 26/02/24 6:31 pm, Fabiano Rosas wrote:
>>>>> Het Gala<het.gala@nutanix.com>  writes:
>>>>>
>>>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>>>>>> this was the same first approach that I attempted. It won't work 
>>>>>> because
>>>>>>
>>>>>> The final 'migrate' QAPI with channels string would look like
>>>>>>
[...]
>> I'm not sure what you tried. This works:
>>
>>      g_assert(!qdict_haskey(args, "channels"));
>>      if (channels) {
>>          channels_obj = qobject_from_json(channels, errp);
>>          qdict_put_obj(args, "channels", channels_obj);
>>      }
>
> Are you sure the above works ?

Sorry, please ignore the below doubt. I understood what silly mistakes I 
was doing. you were right, qobject_from_json() works fine and converts 
string to json object.

Will follow your latest reply and send out the patch really soon. Thank 
you for unblocking me here quickly :)

> [...]

Regards,

Het Gala

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

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> On 01/03/24 2:19 pm, Het Gala wrote:
>>
>> On 29/02/24 6:47 am, Fabiano Rosas wrote:
>>> Het Gala <het.gala@nutanix.com> writes:
>>>
>>>> On 27/02/24 1:04 am, Het Gala wrote:
>>>>>
>>>>> On 26/02/24 6:31 pm, Fabiano Rosas wrote:
>>>>>> Het Gala<het.gala@nutanix.com>  writes:
>>>>>>
>>>>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>>>>>>> this was the same first approach that I attempted. It won't work 
>>>>>>> because
>>>>>>>
>>>>>>> The final 'migrate' QAPI with channels string would look like
>>>>>>>
> [...]
>>> I'm not sure what you tried. This works:
>>>
>>>      g_assert(!qdict_haskey(args, "channels"));
>>>      if (channels) {
>>>          channels_obj = qobject_from_json(channels, errp);
>>>          qdict_put_obj(args, "channels", channels_obj);
>>>      }
>>
>> Are you sure the above works ?
>
> Sorry, please ignore the below doubt. I understood what silly mistakes I 
> was doing. you were right, qobject_from_json() works fine and converts 
> string to json object.
>
> Will follow your latest reply and send out the patch really soon. Thank 
> you for unblocking me here quickly :)

Just please don't make it all one patch. Each of those steps could be a
separate patch.

[PATCH v2 2/3] qtest: migration: Add negative validation tests for 'uri' and 'channels'

[Het Gala]

Ideally 'migrate' and 'migrate-incoming' QAPIs should  not allow 'uri' and
'channels' both arguments to be present simultaneously, but also one of
them should be present in the QAPI. IOW they are mututally exhaustive
arguments for QAPI

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

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

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e5547b8746..c1d9ee6466 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -370,6 +370,30 @@ static void cleanup(const char *filename)
     unlink(path);
 }
 
+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);
+    g_assert(saddr[0] && strcmp(saddr[0], "tcp") == 0);
+
+    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 char *SocketAddress_to_str(SocketAddress *addr)
 {
     switch (addr->type) {
@@ -704,6 +728,13 @@ typedef struct {
      */
     const char *connect_uri;
 
+    /*
+     * Optional:  bool variable to connect list of migration stream
+     * channels, to a dest QEMU, can be utilized instead of a URI to
+     * accomplish the same task as listen_uri or connect_uri.
+     */
+    bool connect_channels;
+
     /* Optional: callback to run at start to set migration parameters */
     TestMigrateStartHook start_hook;
     /* Optional: callback to run at finish to cleanup */
@@ -2509,6 +2540,62 @@ static void test_validate_uuid_dst_not_set(void)
     do_test_validate_uuid(&args, false);
 }
 
+static void do_test_validate_uri_channel(MigrateCommon *args)
+{
+    QTestState *from, *to;
+    g_autofree char *connect_uri = NULL;
+    g_autofree MigrationChannelList *connect_channels = NULL;
+
+    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");
+
+    if (strcmp(args->listen_uri, "defer") != 0) {
+        connect_uri = migrate_get_socket_address(to, "socket-address");
+    }
+
+    if (args->connect_channels) {
+        connect_channels = uri_to_channels(connect_uri);
+    }
+    /*
+     * 'uri' and 'channels' validation is checked even before the migration
+     * starts.
+     */
+    migrate_qmp_fail(from, connect_uri, connect_channels, "{}");
+    test_migrate_end(from, to, false);
+}
+
+static void
+test_validate_uri_channels_both_set(void)
+{
+    MigrateCommon args = {
+        .start = {
+            .hide_stderr = true,
+        },
+        .connect_channels = true,
+        .listen_uri = "tcp:127.0.0.1:0",
+    };
+
+    do_test_validate_uri_channel(&args);
+}
+
+static void
+test_validate_uri_channels_none_set(void)
+{
+    MigrateCommon args = {
+        .start = {
+            .hide_stderr = true,
+        },
+        .connect_channels = false,
+        .listen_uri = "defer",
+    };
+
+    do_test_validate_uri_channel(&args);
+}
+
 /*
  * The way auto_converge works, we need to do too many passes to
  * run this test.  Auto_converge logic is only run once every
@@ -3537,6 +3624,10 @@ 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);
+    migration_test_add("/migration/validate_uri/channels/none_set",
+                       test_validate_uri_channels_none_set);
     /*
      * See explanation why this test is slow on function definition
      */
-- 
2.22.3

[PATCH v2 3/3] qtest: migration: Start migration with 'channels' argument

[Het Gala]

'uri' and 'channels' arguments are mutually exhaustive in nature for
migration QAPIs. All existing  migration qtests solely use 'uri'
argument and lack 'channels' as the entrypoint for migration QAPIs.

This commit addresses this gap by introducing support for 'channels'
as an alternative entrypoint argument. Additionally, validates the
functionality of MigrationChannelList in the context of establishing
migration connection.

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

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index c1d9ee6466..f044a3018a 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1766,6 +1766,7 @@ static void test_precopy_common(MigrateCommon *args)
     QTestState *from, *to;
     void *data_hook = NULL;
     g_autofree char *connect_uri = NULL;
+    g_autofree MigrationChannelList *channels = NULL;
 
     if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
         return;
@@ -1804,12 +1805,21 @@ static void test_precopy_common(MigrateCommon *args)
         connect_uri = g_strdup(args->connect_uri);
     }
 
+    if (args->connect_channels) {
+        channels = uri_to_channels(connect_uri);
+    }
+
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, connect_uri, NULL, "{}");
+        migrate_qmp_fail(from, connect_uri, channels, "{}");
         goto finish;
     }
 
-    migrate_qmp(from, connect_uri, NULL, "{}");
+    if (args->connect_channels) {
+        migrate_qmp(from, NULL, channels, "{}");
+    } else {
+        migrate_qmp(from, connect_uri, NULL, "{}");
+    }
+
 
     if (args->result != MIG_TEST_SUCCEED) {
         bool allow_active = args->result == MIG_TEST_FAIL;
@@ -2725,7 +2735,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState *from,
 }
 #endif /* CONFIG_ZSTD */
 
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_none_uri(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
@@ -2740,6 +2750,17 @@ static void test_multifd_tcp_none(void)
     test_precopy_common(&args);
 }
 
+static void test_multifd_tcp_none_channels(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "defer",
+        .start_hook = test_migrate_precopy_tcp_multifd_start,
+        .live = true,
+        .connect_channels = true,
+    };
+    test_precopy_common(&args);
+}
+
 static void test_multifd_tcp_zlib(void)
 {
     MigrateCommon args = {
@@ -3640,8 +3661,11 @@ int main(int argc, char **argv)
                                test_migrate_dirty_limit);
         }
     }
-    migration_test_add("/migration/multifd/tcp/plain/none",
-                       test_multifd_tcp_none);
+    migration_test_add("/migration/multifd/tcp/plain/none/uri",
+                       test_multifd_tcp_none_uri);
+    migration_test_add("/migration/multifd/tcp/plain/none/channels",
+                       test_multifd_tcp_none_channels);
+
     migration_test_add("/migration/multifd/tcp/plain/cancel",
                        test_multifd_tcp_cancel);
     migration_test_add("/migration/multifd/tcp/plain/zlib",
-- 
2.22.3