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

[PATCH v3 0/7] 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
exhaustive, 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 test to
enforce only use of 'channel' argument as the initial entry point for
migration QAPIs.

Patch Summary:
-------------
Patch 1-2:
---------
Introduce 'to' object inside migrate_qmp() so and move the calls to
migrate_get_socket_address() inside migrate_qmp. Also, replace connect_uri
with args->connect_uri everywhere.

Patch 3-5:
---------
Add channels argument to allow both migration QAPI arguments independently
into migrate_qmp and migrate_qmp_fail. migrate_qmp requires the port value to
be changed from 0 to port value coming from migrate_get_socket_address. Add
migrate_set_ports to address this change of port value.

Patch 6-7:
---------
Add 2 negative tests to validate mutually exhaustive behaviour of migration
QAPIs. Add a positive multifd_tcp_plain qtest with only channels as the
initial entry point for migration QAPIs.

v2->v3 Changelog:
-----------------
1. 'channels' introduction is not required now for migrate_qmp_incoming
2. Refactor the code into 7 different patches
3. 'channels' introduction is not required now for migrate_qmp_incoming
4. Remove custom function for converting string to MigrationChannelList
5. move calls for migrate_get_socket_address inside migrate_qmp so that
   migrate_set_ports can replace the QAPI's port with correct value.

Het Gala (7):
  Add 'to' object into migrate_qmp()
  Replace connect_uri and move migrate_get_socket_address inside
    migrate_qmp
  Add channels parameter in migrate_qmp_fail
  Add migrate_set_ports into migrate_qmp to change migration port number
  Add channels parameter in migrate_qmp
  Add multifd_tcp_plain test using list of channels instead of uri
  Add negative tests to validate migration QAPIs

 tests/qtest/migration-helpers.c | 109 +++++++++++++++++++-
 tests/qtest/migration-helpers.h |  10 +-
 tests/qtest/migration-test.c    | 176 ++++++++++++++++++--------------
 3 files changed, 208 insertions(+), 87 deletions(-)

-- 
2.22.3

[PATCH v3 1/7] Add 'to' object into migrate_qmp()

[Het Gala]

Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() inside migrate_qmp() to get
the port value. This is not applied to other migrate_qmp*
because they don't need the port.

Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c |  3 ++-
 tests/qtest/migration-helpers.h |  5 +++--
 tests/qtest/migration-test.c    | 28 ++++++++++++++--------------
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..b6206a04fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -68,7 +68,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, QTestState *to, const char *uri,
+                 const char *fmt, ...)
 {
     va_list ap;
     QDict *args;
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..e16a34c796 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,8 +25,9 @@ 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, QTestState *to, const char *uri,
+                 const char *fmt, ...);
 
 G_GNUC_PRINTF(3, 4)
 void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 83512bce85..f645ab26f2 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1350,7 +1350,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, to, uri, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -1500,7 +1500,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, to, "fd:fd-mig", "{'resume': true}");
 
     /*
      * Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1588,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, to, uri, "{'resume': true}");
 
     /* Restore the postcopy bandwidth to unlimited */
     migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1669,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, to, "tcp:127.0.0.1:0", "{}");
     wait_for_migration_fail(from, false);
     test_migrate_end(from, to, false);
 }
@@ -1708,7 +1708,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, to, uri, "{}");
     wait_for_migration_complete(from);
 
     pid = fork();
@@ -1777,7 +1777,7 @@ static void test_precopy_common(MigrateCommon *args)
         goto finish;
     }
 
-    migrate_qmp(from, connect_uri, "{}");
+    migrate_qmp(from, to, connect_uri, "{}");
 
     if (args->result != MIG_TEST_SUCCEED) {
         bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1873,7 +1873,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
         goto finish;
     }
 
-    migrate_qmp(from, connect_uri, "{}");
+    migrate_qmp(from, to, connect_uri, "{}");
     wait_for_migration_complete(from);
 
     /*
@@ -2029,7 +2029,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, to, uri, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2494,7 +2494,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, to, uri, "{}");
 
     if (should_fail) {
         qtest_set_expected_status(to, EXIT_FAILURE);
@@ -2597,7 +2597,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, to, uri, "{}");
 
     /* Wait for throttling begins */
     percentage = 0;
@@ -2908,7 +2908,7 @@ static void test_multifd_tcp_cancel(void)
 
     uri = migrate_get_socket_address(to, "socket-address");
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, to, uri, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2940,7 +2940,7 @@ static void test_multifd_tcp_cancel(void)
 
     migrate_ensure_non_converge(from);
 
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, to2, uri, "{}");
 
     migrate_wait_for_dirty_mem(from, to2);
 
@@ -3273,7 +3273,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, to, uri, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
@@ -3314,7 +3314,7 @@ static void test_migrate_dirty_limit(void)
     }
 
     /* Start migrate */
-    migrate_qmp(from, uri, "{}");
+    migrate_qmp(from, to, uri, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
-- 
2.22.3

Re: [PATCH v3 1/7] Add 'to' object into migrate_qmp()

[Fabiano Rosas]

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

> Add the 'to' object into migrate_qmp(), so we can use
> migrate_get_socket_address() inside migrate_qmp() to get
> the port value. This is not applied to other migrate_qmp*
> because they don't need the port.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v3 2/7] Replace connect_uri and move migrate_get_socket_address inside migrate_qmp

[Het Gala]

Move the calls to migrate_get_socket_address() into migrate_qmp().
Get rid of connect_uri and replace it with args->connect_uri only
because 'to' object will help to generate connect_uri with the
correct port number.

Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 55 ++++++++++++++++++++++-
 tests/qtest/migration-test.c    | 79 +++++----------------------------
 2 files changed, 64 insertions(+), 70 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b6206a04fb..9af3c7d4d5 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,10 @@
 #include "qemu/osdep.h"
 #include "qemu/ctype.h"
 #include "qapi/qmp/qjson.h"
+#include "qemu/sockets.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
 
 #include "migration-helpers.h"
 
@@ -24,6 +28,51 @@
  */
 #define MIGRATION_STATUS_WAIT_TIMEOUT 120
 
+static char *SocketAddress_to_str(SocketAddress *addr)
+{
+    switch (addr->type) {
+    case SOCKET_ADDRESS_TYPE_INET:
+        return g_strdup_printf("tcp:%s:%s",
+                               addr->u.inet.host,
+                               addr->u.inet.port);
+    case SOCKET_ADDRESS_TYPE_UNIX:
+        return g_strdup_printf("unix:%s",
+                               addr->u.q_unix.path);
+    case SOCKET_ADDRESS_TYPE_FD:
+        return g_strdup_printf("fd:%s", addr->u.fd.str);
+    case SOCKET_ADDRESS_TYPE_VSOCK:
+        return g_strdup_printf("tcp:%s:%s",
+                               addr->u.vsock.cid,
+                               addr->u.vsock.port);
+    default:
+        return g_strdup("unknown address type");
+    }
+}
+
+static char *
+migrate_get_socket_address(QTestState *who, const char *parameter)
+{
+    QDict *rsp;
+    char *result;
+    SocketAddressList *addrs;
+    Visitor *iv = NULL;
+    QObject *object;
+
+    rsp = migrate_query(who);
+    object = qdict_get(rsp, parameter);
+
+    iv = qobject_input_visitor_new(object);
+    visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
+    visit_free(iv);
+
+    /* we are only using a single address */
+    result = SocketAddress_to_str(addrs->value);
+
+    qapi_free_SocketAddressList(addrs);
+    qobject_unref(rsp);
+    return result;
+}
+
 bool migrate_watch_for_events(QTestState *who, const char *name,
                               QDict *event, void *opaque)
 {
@@ -73,13 +122,17 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
 {
     va_list ap;
     QDict *args;
+    g_autofree char *connect_uri = NULL;
 
     va_start(ap, 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) {
+        connect_uri = migrate_get_socket_address(to, "socket-address");
+    }
+    qdict_put_str(args, "uri", uri ? uri : connect_uri);
 
     qtest_qmp_assert_success(who,
                              "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f645ab26f2..20b1dd031a 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -369,50 +369,6 @@ static void cleanup(const char *filename)
     unlink(path);
 }
 
-static char *SocketAddress_to_str(SocketAddress *addr)
-{
-    switch (addr->type) {
-    case SOCKET_ADDRESS_TYPE_INET:
-        return g_strdup_printf("tcp:%s:%s",
-                               addr->u.inet.host,
-                               addr->u.inet.port);
-    case SOCKET_ADDRESS_TYPE_UNIX:
-        return g_strdup_printf("unix:%s",
-                               addr->u.q_unix.path);
-    case SOCKET_ADDRESS_TYPE_FD:
-        return g_strdup_printf("fd:%s", addr->u.fd.str);
-    case SOCKET_ADDRESS_TYPE_VSOCK:
-        return g_strdup_printf("tcp:%s:%s",
-                               addr->u.vsock.cid,
-                               addr->u.vsock.port);
-    default:
-        return g_strdup("unknown address type");
-    }
-}
-
-static char *migrate_get_socket_address(QTestState *who, const char *parameter)
-{
-    QDict *rsp;
-    char *result;
-    SocketAddressList *addrs;
-    Visitor *iv = NULL;
-    QObject *object;
-
-    rsp = migrate_query(who);
-    object = qdict_get(rsp, parameter);
-
-    iv = qobject_input_visitor_new(object);
-    visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
-    visit_free(iv);
-
-    /* we are only using a single address */
-    result = SocketAddress_to_str(addrs->value);
-
-    qapi_free_SocketAddressList(addrs);
-    qobject_unref(rsp);
-    return result;
-}
-
 static long long migrate_get_parameter_int(QTestState *who,
                                            const char *parameter)
 {
@@ -1349,8 +1305,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     wait_for_serial("src_serial");
     wait_for_suspend(from, &src_state);
 
-    g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
-    migrate_qmp(from, to, uri, "{}");
+    migrate_qmp(from, to, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -1733,7 +1688,6 @@ static void test_precopy_common(MigrateCommon *args)
 {
     QTestState *from, *to;
     void *data_hook = NULL;
-    g_autofree char *connect_uri = NULL;
 
     if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
         return;
@@ -1766,18 +1720,12 @@ static void test_precopy_common(MigrateCommon *args)
         }
     }
 
-    if (!args->connect_uri) {
-        connect_uri = migrate_get_socket_address(to, "socket-address");
-    } else {
-        connect_uri = g_strdup(args->connect_uri);
-    }
-
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, connect_uri, "{}");
+        migrate_qmp_fail(from, args->connect_uri, "{}");
         goto finish;
     }
 
-    migrate_qmp(from, to, connect_uri, "{}");
+    migrate_qmp(from, to, args->connect_uri, "{}");
 
     if (args->result != MIG_TEST_SUCCEED) {
         bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1843,7 +1791,6 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
 {
     QTestState *from, *to;
     void *data_hook = NULL;
-    g_autofree char *connect_uri = g_strdup(args->connect_uri);
 
     if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
         return;
@@ -1869,18 +1816,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, args->connect_uri, "{}");
         goto finish;
     }
 
-    migrate_qmp(from, to, connect_uri, "{}");
+    migrate_qmp(from, to, args->connect_uri, "{}");
     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, args->connect_uri, "{}");
     wait_for_migration_complete(to);
 
     if (stop_src) {
@@ -2885,7 +2832,6 @@ static void test_multifd_tcp_cancel(void)
         .hide_stderr = true,
     };
     QTestState *from, *to, *to2;
-    g_autofree char *uri = NULL;
 
     if (test_migrate_start(&from, &to, "defer", &args)) {
         return;
@@ -2906,9 +2852,7 @@ static void test_multifd_tcp_cancel(void)
     /* 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, to, uri, "{}");
+    migrate_qmp(from, to, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2933,14 +2877,11 @@ static void test_multifd_tcp_cancel(void)
     /* Start incoming migration from the 1st socket */
     migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}");
 
-    g_free(uri);
-    uri = migrate_get_socket_address(to2, "socket-address");
-
     wait_for_migration_status(from, "cancelled", NULL);
 
     migrate_ensure_non_converge(from);
 
-    migrate_qmp(from, to2, uri, "{}");
+    migrate_qmp(from, to2, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to2);
 
@@ -3273,7 +3214,7 @@ static void test_migrate_dirty_limit(void)
     migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
 
     /* Start migrate */
-    migrate_qmp(from, to, uri, "{}");
+    migrate_qmp(from, to, args.connect_uri, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
@@ -3314,7 +3255,7 @@ static void test_migrate_dirty_limit(void)
     }
 
     /* Start migrate */
-    migrate_qmp(from, to, uri, "{}");
+    migrate_qmp(from, to, args.connect_uri, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
-- 
2.22.3

Re: [PATCH v3 2/7] Replace connect_uri and move migrate_get_socket_address inside migrate_qmp

[Fabiano Rosas]

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

> Move the calls to migrate_get_socket_address() into migrate_qmp().
> Get rid of connect_uri and replace it with args->connect_uri only
> because 'to' object will help to generate connect_uri with the
> correct port number.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v3 3/7] Add channels parameter in migrate_qmp_fail

[Het Gala]

Alter migrate_qmp_fail() to allow both uri and channels
independently. For channels, convert string to a Dict.
No dealing with migrate_get_socket_address() here because
we will fail before starting the migration anyway.

Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 15 +++++++++++++--
 tests/qtest/migration-helpers.h |  5 +++--
 tests/qtest/migration-test.c    |  4 ++--
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 9af3c7d4d5..478c1f259b 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -92,17 +92,28 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
     return false;
 }
 
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+                      const char *channels, const char *fmt, ...)
 {
     va_list ap;
     QDict *args, *err;
+    Error *error_abort = NULL;
+    QObject *channels_obj = NULL;
 
     va_start(ap, 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) {
+        qdict_put_str(args, "uri", uri);
+    }
+
+    g_assert(!qdict_haskey(args, "channels"));
+    if (channels) {
+        channels_obj = qobject_from_json(channels, &error_abort);
+        qdict_put_obj(args, "channels", channels_obj);
+    }
 
     err = qtest_qmp_assert_failure_ref(
         who, "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e16a34c796..4e664148a5 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -33,8 +33,9 @@ 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,
+                      const char *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 20b1dd031a..19bb93cb7d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1721,7 +1721,7 @@ static void test_precopy_common(MigrateCommon *args)
     }
 
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, args->connect_uri, "{}");
+        migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
         goto finish;
     }
 
@@ -1816,7 +1816,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
     }
 
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, args->connect_uri, "{}");
+        migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
         goto finish;
     }
 
-- 
2.22.3

Re: [PATCH v3 3/7] Add channels parameter in migrate_qmp_fail

[Fabiano Rosas]

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

> Alter migrate_qmp_fail() to allow both uri and channels
> independently. For channels, convert string to a Dict.
> No dealing with migrate_get_socket_address() here because
> we will fail before starting the migration anyway.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.c | 15 +++++++++++++--
>  tests/qtest/migration-helpers.h |  5 +++--
>  tests/qtest/migration-test.c    |  4 ++--
>  3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 9af3c7d4d5..478c1f259b 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -92,17 +92,28 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
>      return false;
>  }
>  
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> +                      const char *channels, const char *fmt, ...)
>  {
>      va_list ap;
>      QDict *args, *err;
> +    Error *error_abort = NULL;

The error_abort needs to be the one defined in error.c. Just remove this
line.

> +    QObject *channels_obj = NULL;
>  
>      va_start(ap, 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) {
> +        qdict_put_str(args, "uri", uri);
> +    }
> +
> +    g_assert(!qdict_haskey(args, "channels"));
> +    if (channels) {
> +        channels_obj = qobject_from_json(channels, &error_abort);
> +        qdict_put_obj(args, "channels", channels_obj);
> +    }
>  
>      err = qtest_qmp_assert_failure_ref(
>          who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index e16a34c796..4e664148a5 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -33,8 +33,9 @@ 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,
> +                      const char *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 20b1dd031a..19bb93cb7d 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1721,7 +1721,7 @@ static void test_precopy_common(MigrateCommon *args)
>      }
>  
>      if (args->result == MIG_TEST_QMP_ERROR) {
> -        migrate_qmp_fail(from, args->connect_uri, "{}");
> +        migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
>          goto finish;
>      }
>  
> @@ -1816,7 +1816,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
>      }
>  
>      if (args->result == MIG_TEST_QMP_ERROR) {
> -        migrate_qmp_fail(from, args->connect_uri, "{}");
> +        migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
>          goto finish;
>      }

Re: [PATCH v3 3/7] Add channels parameter in migrate_qmp_fail

[Het Gala]

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


On 06/03/24 8:10 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Alter migrate_qmp_fail() to allow both uri and channels
>> independently. For channels, convert string to a Dict.
>> No dealing with migrate_get_socket_address() here because
>> we will fail before starting the migration anyway.
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>> ---
>>   tests/qtest/migration-helpers.c | 15 +++++++++++++--
>>   tests/qtest/migration-helpers.h |  5 +++--
>>   tests/qtest/migration-test.c    |  4 ++--
>>   3 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index 9af3c7d4d5..478c1f259b 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -92,17 +92,28 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
>>       return false;
>>   }
>>   
>> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>> +void migrate_qmp_fail(QTestState *who, const char *uri,
>> +                      const char *channels, const char *fmt, ...)
>>   {
>>       va_list ap;
>>       QDict *args, *err;
>> +    Error *error_abort = NULL;
> The error_abort needs to be the one defined in error.c. Just remove this
> line.
Ack.


Regards

Het Gala

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

[PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Het Gala]

Add a migrate_set_ports() function that from each QDict, 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 more than one channel.

Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 478c1f259b..df4978bf17 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,6 +17,8 @@
 #include "qapi/qapi-visit-sockets.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+
 
 #include "migration-helpers.h"
 
@@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
     return result;
 }
 
+static void migrate_set_ports(QTestState *to, QList *channelList)
+{
+    g_autofree char *addr = NULL;
+    g_autofree char *addr_port = NULL;
+    QListEntry *entry;
+
+    addr = migrate_get_socket_address(to, "socket-address");
+    addr_port = g_strsplit(addr, ":", 3)[2];
+
+    QLIST_FOREACH_ENTRY(channelList, entry) {
+        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+        QObject *addr_obj = qdict_get(channel, "addr");
+
+        if (qobject_type(addr_obj) == QTYPE_QDICT) {
+            QDict *addrdict = qobject_to(QDict, addr_obj);
+            if (qdict_haskey(addrdict, "port") &&
+            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+                qdict_put_str(addrdict, "port", addr_port);
+            }
+        }
+    }
+}
+
 bool migrate_watch_for_events(QTestState *who, const char *name,
                               QDict *event, void *opaque)
 {
@@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
     if (!uri) {
         connect_uri = migrate_get_socket_address(to, "socket-address");
     }
+    migrate_set_ports(to, NULL);
     qdict_put_str(args, "uri", uri ? uri : connect_uri);
 
     qtest_qmp_assert_success(who,
-- 
2.22.3

Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Fabiano Rosas]

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

> Add a migrate_set_ports() function that from each QDict, 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 more than one channel.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 478c1f259b..df4978bf17 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -17,6 +17,8 @@
>  #include "qapi/qapi-visit-sockets.h"
>  #include "qapi/qobject-input-visitor.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qlist.h"
> +

Extra line here. This is unwanted because it sometimes trips git into
thinking there's a conflict here when another patch changes the
surrounding lines.

>  
>  #include "migration-helpers.h"
>  
> @@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
>      return result;
>  }
>  
> +static void migrate_set_ports(QTestState *to, QList *channelList)
> +{
> +    g_autofree char *addr = NULL;
> +    g_autofree char *addr_port = NULL;
> +    QListEntry *entry;
> +
> +    addr = migrate_get_socket_address(to, "socket-address");
> +    addr_port = g_strsplit(addr, ":", 3)[2];

Will this always to the right thing when the src/dst use different types
of channels? If there is some kind of mismatch (say one side uses vsock
and the other inet), it's better that this function doesn't touch the
channels dict instead of putting garbage in the port field.

Also what happens if the dst is using unix: or fd:?

> +
> +    QLIST_FOREACH_ENTRY(channelList, entry) {
> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
> +        QObject *addr_obj = qdict_get(channel, "addr");
> +
> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
> +            QDict *addrdict = qobject_to(QDict, addr_obj);

You might not need these two lines if at the start you use:

QDict *addr = qdict_get_dict(channel, "addr");

> +            if (qdict_haskey(addrdict, "port") &&
> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
> +                qdict_put_str(addrdict, "port", addr_port);
> +            }
> +        }
> +    }
> +}
> +
>  bool migrate_watch_for_events(QTestState *who, const char *name,
>                                QDict *event, void *opaque)
>  {
> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>      if (!uri) {
>          connect_uri = migrate_get_socket_address(to, "socket-address");
>      }
> +    migrate_set_ports(to, NULL);

migrate_set_ports is not prepared to take NULL. This breaks the tests in
this commit. All individual commits should work, otherwise it will break
bisecting.

>      qdict_put_str(args, "uri", uri ? uri : connect_uri);
>  
>      qtest_qmp_assert_success(who,

Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Het Gala]

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


On 06/03/24 8:06 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Add a migrate_set_ports() function that from each QDict, 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 more than one channel.
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>> ---
>>   tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index 478c1f259b..df4978bf17 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -17,6 +17,8 @@
>>   #include "qapi/qapi-visit-sockets.h"
>>   #include "qapi/qobject-input-visitor.h"
>>   #include "qapi/error.h"
>> +#include "qapi/qmp/qlist.h"
>> +
> Extra line here. This is unwanted because it sometimes trips git into
> thinking there's a conflict here when another patch changes the
> surrounding lines.
Ack, that makes sense
>>   
>>   #include "migration-helpers.h"
>>   
>> @@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
>>       return result;
>>   }
>>   
>> +static void migrate_set_ports(QTestState *to, QList *channelList)
>> +{
>> +    g_autofree char *addr = NULL;
>> +    g_autofree char *addr_port = NULL;
>> +    QListEntry *entry;
>> +
>> +    addr = migrate_get_socket_address(to, "socket-address");
>> +    addr_port = g_strsplit(addr, ":", 3)[2];
> Will this always do the right thing when the src/dst use different types
> of channels? If there is some kind of mismatch (say one side uses vsock
> and the other inet), it's better that this function doesn't touch the
> channels dict instead of putting garbage in the port field.

Yes you are right. This will fail if there is a mismatch in type of 
channels.

Better idea would be to check if 'port' key is present in both, i.e. in 
'addr'
as well as 'addrdict' and only then change the port ?

> Also what happens if the dst is using unix: or fd:?
yes in that case - how should the migration behaviour be ? src and dst 
should be of the same type right
>> +
>> +    QLIST_FOREACH_ENTRY(channelList, entry) {
>> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>> +        QObject *addr_obj = qdict_get(channel, "addr");
>> +
>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
> You might not need these two lines if at the start you use:
>
> QDict *addr = qdict_get_dict(channel, "addr");

If you are commenting regarding this two lines:

+        if (qobject_type(addr_obj) == QTYPE_QDICT) {
+            QDict *addrdict = qobject_to(QDict, addr_obj);

then, I am not sure, because addrdict and addr is different right? Also addrdict can also
be a QList, like in the case of exec migration, it can be a list instead of dict
ex:
# -> { "execute": "migrate",
#      "arguments": {
#          "channels": [ { "channel-type": "main",
#                          "addr": { "transport": "exec",
#                                    "args": [ "/bin/nc", "-p", "6000",
#                                              "/some/sock" ] } } ] } }

>
>> +            if (qdict_haskey(addrdict, "port") &&
>> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>> +                qdict_put_str(addrdict, "port", addr_port);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>>                                 QDict *event, void *opaque)
>>   {
>> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>       if (!uri) {
>>           connect_uri = migrate_get_socket_address(to, "socket-address");
>>       }
>> +    migrate_set_ports(to, NULL);
> migrate_set_ports is not prepared to take NULL. This breaks the tests in
> this commit. All individual commits should work, otherwise it will break
> bisecting.
Okay, so in that case, is it better to merge this with the next patch ? 
because if I just
define the migrate_set_ports function and not use it anywhere, it gives 
a warning/error
(depending upon what compiler is used)
>>       qdict_put_str(args, "uri", uri ? uri : connect_uri);
>>   
>>       qtest_qmp_assert_success(who,

Regards,

Het Gala

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

Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Fabiano Rosas]

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

> On 06/03/24 8:06 pm, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> Add a migrate_set_ports() function that from each QDict, 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 more than one channel.
>>>
>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>> ---
>>>   tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>>>   1 file changed, 26 insertions(+)
>>>
>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>> index 478c1f259b..df4978bf17 100644
>>> --- a/tests/qtest/migration-helpers.c
>>> +++ b/tests/qtest/migration-helpers.c
>>> @@ -17,6 +17,8 @@
>>>   #include "qapi/qapi-visit-sockets.h"
>>>   #include "qapi/qobject-input-visitor.h"
>>>   #include "qapi/error.h"
>>> +#include "qapi/qmp/qlist.h"
>>> +
>> Extra line here. This is unwanted because it sometimes trips git into
>> thinking there's a conflict here when another patch changes the
>> surrounding lines.
> Ack, that makes sense
>>>   
>>>   #include "migration-helpers.h"
>>>   
>>> @@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
>>>       return result;
>>>   }
>>>   
>>> +static void migrate_set_ports(QTestState *to, QList *channelList)
>>> +{
>>> +    g_autofree char *addr = NULL;
>>> +    g_autofree char *addr_port = NULL;
>>> +    QListEntry *entry;
>>> +
>>> +    addr = migrate_get_socket_address(to, "socket-address");
>>> +    addr_port = g_strsplit(addr, ":", 3)[2];
>> Will this always do the right thing when the src/dst use different types
>> of channels? If there is some kind of mismatch (say one side uses vsock
>> and the other inet), it's better that this function doesn't touch the
>> channels dict instead of putting garbage in the port field.
>
> Yes you are right. This will fail if there is a mismatch in type of 
> channels.
>
> Better idea would be to check if 'port' key is present in both, i.e. in 
> 'addr'
> as well as 'addrdict' and only then change the port ?
>

Yep, either parse the type from string or add a version of
migrate_get_socket_address that returns a dict. Then check if type
matches and port exists.

>> Also what happens if the dst is using unix: or fd:?
> yes in that case - how should the migration behaviour be ? src and dst 
> should be of the same type right

Remember this is test code. If the test was written incorrectly either
by developer mistake or on purpose to test some condition, then it's not
this function's reponsibility to fix that.

Replace the port only if the transport type allows for a port, there is
a port in both addr and addrdict and port is 0. Anything else, leave the
channelList untouched and let QEMU deal with the bad input.

>>> +
>>> +    QLIST_FOREACH_ENTRY(channelList, entry) {
>>> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>>> +        QObject *addr_obj = qdict_get(channel, "addr");
>>> +
>>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>> You might not need these two lines if at the start you use:
>>
>> QDict *addr = qdict_get_dict(channel, "addr");
>
> If you are commenting regarding this two lines:
>
> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>
> then, I am not sure, because addrdict and addr is different right? Also addrdict can also
> be a QList, like in the case of exec migration, it can be a list instead of dict
> ex:
> # -> { "execute": "migrate",
> #      "arguments": {
> #          "channels": [ { "channel-type": "main",
> #                          "addr": { "transport": "exec",
> #                                    "args": [ "/bin/nc", "-p", "6000",
> #                                              "/some/sock" ] } } ] } }

"addr" is always a dict, no? Even in the example you just gave.

>
>>
>>> +            if (qdict_haskey(addrdict, "port") &&
>>> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>>> +                qdict_put_str(addrdict, "port", addr_port);
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>> +
>>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>>>                                 QDict *event, void *opaque)
>>>   {
>>> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>       if (!uri) {
>>>           connect_uri = migrate_get_socket_address(to, "socket-address");
>>>       }
>>> +    migrate_set_ports(to, NULL);
>> migrate_set_ports is not prepared to take NULL. This breaks the tests in
>> this commit. All individual commits should work, otherwise it will break
>> bisecting.
> Okay, so in that case, is it better to merge this with the next patch ? 
> because if I just
> define the migrate_set_ports function and not use it anywhere, it gives 
> a warning/error
> (depending upon what compiler is used)

You can return early at migrate_set_ports if channelList is NULL.

Also, I just noticed: s/channelList/channel_list/

>>>       qdict_put_str(args, "uri", uri ? uri : connect_uri);
>>>   
>>>       qtest_qmp_assert_success(who,
>
> Regards,
>
> Het Gala

Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Het Gala]

On 06/03/24 9:31 pm, Fabiano Rosas wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> On 06/03/24 8:06 pm, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>  writes:
>>>
>>>> Add a migrate_set_ports() function that from each QDict, 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 more than one channel.
>>>>
>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>>> ---
>>>>    tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>>> index 478c1f259b..df4978bf17 100644
>>>> --- a/tests/qtest/migration-helpers.c
>>>> +++ b/tests/qtest/migration-helpers.c
>>>> @@ -17,6 +17,8 @@
>>>>    #include "qapi/qapi-visit-sockets.h"
>>>>    #include "qapi/qobject-input-visitor.h"
>>>>    #include "qapi/error.h"
>>>> +#include "qapi/qmp/qlist.h"
>>>> +
>>> Extra line here. This is unwanted because it sometimes trips git into
>>> thinking there's a conflict here when another patch changes the
>>> surrounding lines.
>> Ack, that makes sense
>>>>    
>>>>    #include "migration-helpers.h"
>>>>    
>>>> @@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
>>>>        return result;
>>>>    }
>>>>    
>>>> +static void migrate_set_ports(QTestState *to, QList *channelList)
>>>> +{
>>>> +    g_autofree char *addr = NULL;
>>>> +    g_autofree char *addr_port = NULL;
>>>> +    QListEntry *entry;
>>>> +
>>>> +    addr = migrate_get_socket_address(to, "socket-address");
>>>> +    addr_port = g_strsplit(addr, ":", 3)[2];
>>> Will this always do the right thing when the src/dst use different types
>>> of channels? If there is some kind of mismatch (say one side uses vsock
>>> and the other inet), it's better that this function doesn't touch the
>>> channels dict instead of putting garbage in the port field.
>> Yes you are right. This will fail if there is a mismatch in type of
>> channels.
>>
>> Better idea would be to check if 'port' key is present in both, i.e. in
>> 'addr'
>> as well as 'addrdict' and only then change the port ?
>>
> Yep, either parse the type from string or add a version of
> migrate_get_socket_address that returns a dict. Then check if type
> matches and port exists.
>
>>> Also what happens if the dst is using unix: or fd:?
>> yes in that case - how should the migration behaviour be ? src and dst
>> should be of the same type right
> Remember this is test code. If the test was written incorrectly either
> by developer mistake or on purpose to test some condition, then it's not
> this function's reponsibility to fix that.
>
> Replace the port only if the transport type allows for a port, there is
> a port in both addr and addrdict and port is 0. Anything else, leave the
> channelList untouched and let QEMU deal with the bad input.
>
>>>> +
>>>> +    QLIST_FOREACH_ENTRY(channelList, entry) {
>>>> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>>>> +        QObject *addr_obj = qdict_get(channel, "addr");
>>>> +
>>>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>>>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>>> You might not need these two lines if at the start you use:
>>>
>>> QDict *addr = qdict_get_dict(channel, "addr");
>> If you are commenting regarding this two lines:
>>
>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>>
>> then, I am not sure, because addrdict and addr is different right? Also addrdict can also
>> be a QList, like in the case of exec migration, it can be a list instead of dict
>> ex:
>> # -> { "execute": "migrate",
>> #      "arguments": {
>> #          "channels": [ { "channel-type": "main",
>> #                          "addr": { "transport": "exec",
>> #                                    "args": [ "/bin/nc", "-p", "6000",
>> #                                              "/some/sock" ] } } ] } }
> "addr" is always a dict, no? Even in the example you just gave.

Sorry, my apologies. I had args <-> addrdict in back of my mind.
You are correct. Will make the changes in next patchset.

>>>> +            if (qdict_haskey(addrdict, "port") &&
>>>> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>>>> +                qdict_put_str(addrdict, "port", addr_port);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>>    bool migrate_watch_for_events(QTestState *who, const char *name,
>>>>                                  QDict *event, void *opaque)
>>>>    {
>>>> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>>        if (!uri) {
>>>>            connect_uri = migrate_get_socket_address(to, "socket-address");
>>>>        }
>>>> +    migrate_set_ports(to, NULL);
>>> migrate_set_ports is not prepared to take NULL. This breaks the tests in
>>> this commit. All individual commits should work, otherwise it will break
>>> bisecting.
>> Okay, so in that case, is it better to merge this with the next patch ?
>> because if I just
>> define the migrate_set_ports function and not use it anywhere, it gives
>> a warning/error
>> (depending upon what compiler is used)
> You can return early at migrate_set_ports if channelList is NULL.
>
> Also, I just noticed: s/channelList/channel_list/
Ack. Thanks


Regards,

Het Gala

Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Het Gala]

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


On 06/03/24 9:31 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 06/03/24 8:06 pm, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>   writes:
>>>
>>>> Add a migrate_set_ports() function that from each QDict, 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 more than one channel.
>>>>
>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>>> ---
>>>>    tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>>> index 478c1f259b..df4978bf17 100644
>>>> --- a/tests/qtest/migration-helpers.c
>>>> +++ b/tests/qtest/migration-helpers.c
>>>> @@ -17,6 +17,8 @@
>>>>    #include "qapi/qapi-visit-sockets.h"
>>>>    #include "qapi/qobject-input-visitor.h"
>>>>    #include "qapi/error.h"
>>>> +#include "qapi/qmp/qlist.h"
>>>> +
>>> Extra line here. This is unwanted because it sometimes trips git into
>>> thinking there's a conflict here when another patch changes the
>>> surrounding lines.
>> Ack, that makes sense
>>>>    
>>>>    #include "migration-helpers.h"
>>>>    
>>>> @@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
>>>>        return result;
>>>>    }
>>>>    
>>>> +static void migrate_set_ports(QTestState *to, QList *channelList)
>>>> +{
>>>> +    g_autofree char *addr = NULL;
>>>> +    g_autofree char *addr_port = NULL;
>>>> +    QListEntry *entry;
>>>> +
>>>> +    addr = migrate_get_socket_address(to, "socket-address");
>>>> +    addr_port = g_strsplit(addr, ":", 3)[2];
>>> Will this always do the right thing when the src/dst use different types
>>> of channels? If there is some kind of mismatch (say one side uses vsock
>>> and the other inet), it's better that this function doesn't touch the
>>> channels dict instead of putting garbage in the port field.
>> Yes you are right. This will fail if there is a mismatch in type of
>> channels.
>>
>> Better idea would be to check if 'port' key is present in both, i.e. in
>> 'addr'
>> as well as 'addrdict' and only then change the port ?
>>
> Yep, either parse the type from string or add a version of
> migrate_get_socket_address that returns a dict. Then check if type
> matches and port exists.

one silly question here, why are we not having tests for exec and rdma 
specifically ?

Another suggestion required: Parsing uri to qdict is easy to implement 
but (little)
messy codewise, and the other hand migrate_get_qdict looks clean, but 
under the hood we would convert it to socketaddress and then call 
SocketAddress_to_qdict. Which one we can prefer more here ?

Regards,

Het Gala

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

Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Fabiano Rosas]

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

> On 06/03/24 9:31 pm, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> On 06/03/24 8:06 pm, Fabiano Rosas wrote:
>>>> Het Gala<het.gala@nutanix.com>   writes:
>>>>
>>>>> Add a migrate_set_ports() function that from each QDict, 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 more than one channel.
>>>>>
>>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>>>> ---
>>>>>    tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>>>>>    1 file changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>>>> index 478c1f259b..df4978bf17 100644
>>>>> --- a/tests/qtest/migration-helpers.c
>>>>> +++ b/tests/qtest/migration-helpers.c
>>>>> @@ -17,6 +17,8 @@
>>>>>    #include "qapi/qapi-visit-sockets.h"
>>>>>    #include "qapi/qobject-input-visitor.h"
>>>>>    #include "qapi/error.h"
>>>>> +#include "qapi/qmp/qlist.h"
>>>>> +
>>>> Extra line here. This is unwanted because it sometimes trips git into
>>>> thinking there's a conflict here when another patch changes the
>>>> surrounding lines.
>>> Ack, that makes sense
>>>>>    
>>>>>    #include "migration-helpers.h"
>>>>>    
>>>>> @@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
>>>>>        return result;
>>>>>    }
>>>>>    
>>>>> +static void migrate_set_ports(QTestState *to, QList *channelList)
>>>>> +{
>>>>> +    g_autofree char *addr = NULL;
>>>>> +    g_autofree char *addr_port = NULL;
>>>>> +    QListEntry *entry;
>>>>> +
>>>>> +    addr = migrate_get_socket_address(to, "socket-address");
>>>>> +    addr_port = g_strsplit(addr, ":", 3)[2];
>>>> Will this always do the right thing when the src/dst use different types
>>>> of channels? If there is some kind of mismatch (say one side uses vsock
>>>> and the other inet), it's better that this function doesn't touch the
>>>> channels dict instead of putting garbage in the port field.
>>> Yes you are right. This will fail if there is a mismatch in type of
>>> channels.
>>>
>>> Better idea would be to check if 'port' key is present in both, i.e. in
>>> 'addr'
>>> as well as 'addrdict' and only then change the port ?
>>>
>> Yep, either parse the type from string or add a version of
>> migrate_get_socket_address that returns a dict. Then check if type
>> matches and port exists.
>
> one silly question here, why are we not having tests for exec and rdma 
> specifically ?

exec because we intend to deprecate it, so no one is paying too much
attention to it.

rdma because no one wants to write them.

>
> Another suggestion required: Parsing uri to qdict is easy to implement 
> but (little)
> messy codewise, and the other hand migrate_get_qdict looks clean, but 
> under the hood we would convert it to socketaddress and then call 
> SocketAddress_to_qdict. Which one we can prefer more here ?

The latter. It's easier to work with.

static QDict *SocketAddress_to_qdict(SocketAddress *addr)
{
    QDict *dict = qdict_new();

    switch (addr->type) {
    case SOCKET_ADDRESS_TYPE_INET:
        qdict_put_str(dict, "type", "inet");
        qdict_put_str(dict, "host", addr->u.inet.host);
        qdict_put_str(dict, "port", addr->u.inet.port);

        break;
    case SOCKET_ADDRESS_TYPE_UNIX:
        qdict_put_str(dict, "type", "unix");
        qdict_put_str(dict, "path", addr->u.q_unix.path);

        break;
    case SOCKET_ADDRESS_TYPE_FD:
        qdict_put_str(dict, "type", "fd");
        qdict_put_str(dict, "str", addr->u.fd.str);

        break;
    case SOCKET_ADDRESS_TYPE_VSOCK:
        qdict_put_str(dict, "type", "vsock");
        qdict_put_str(dict, "cid", addr->u.vsock.cid);
        qdict_put_str(dict, "port", addr->u.vsock.port);

        break;
    default:
        g_assert_not_reached();
        break;
    }

    return dict;
}

>
> Regards,
>
> Het Gala

[PATCH v3 5/7] Add channels parameter in migrate_qmp

[Het Gala]

Alter migrate_qmp() to allow use of channels parameter, but only
fill the uri with correct port number if there are no channels.
Here we don't want to allow the wrong cases of having both or
none (ex: migrate_qmp_fail).

Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 20 +++++++++++++++-----
 tests/qtest/migration-helpers.h |  4 ++--
 tests/qtest/migration-test.c    | 28 ++++++++++++++--------------
 3 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index df4978bf17..0b891351a5 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -19,7 +19,6 @@
 #include "qapi/error.h"
 #include "qapi/qmp/qlist.h"
 
-
 #include "migration-helpers.h"
 
 /*
@@ -154,10 +153,12 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
  * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
  */
 void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
-                 const char *fmt, ...)
+                 const char *channels, const char *fmt, ...)
 {
     va_list ap;
     QDict *args;
+    Error *error_abort = NULL;
+    QObject *channels_obj = NULL;
     g_autofree char *connect_uri = NULL;
 
     va_start(ap, fmt);
@@ -165,11 +166,20 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
     va_end(ap);
 
     g_assert(!qdict_haskey(args, "uri"));
-    if (!uri) {
+    if (uri) {
+        qdict_put_str(args, "uri", uri);
+    } else if (!channels) {
         connect_uri = migrate_get_socket_address(to, "socket-address");
+        qdict_put_str(args, "uri", connect_uri);
+    }
+
+    g_assert(!qdict_haskey(args, "channels"));
+    if (channels) {
+        channels_obj = qobject_from_json(channels, &error_abort);
+        QList *channelList = qobject_to(QList, channels_obj);
+        migrate_set_ports(to, channelList);
+        qdict_put_obj(args, "channels", channels_obj);
     }
-    migrate_set_ports(to, NULL);
-    qdict_put_str(args, "uri", uri ? uri : connect_uri);
 
     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 4e664148a5..1339835698 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,9 +25,9 @@ typedef struct QTestMigrationState {
 bool migrate_watch_for_events(QTestState *who, const char *name,
                               QDict *event, void *opaque);
 
-G_GNUC_PRINTF(4, 5)
+G_GNUC_PRINTF(5, 6)
 void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
-                 const char *fmt, ...);
+                 const char *channels, const char *fmt, ...);
 
 G_GNUC_PRINTF(3, 4)
 void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 19bb93cb7d..f94fe713b2 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1305,7 +1305,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     wait_for_serial("src_serial");
     wait_for_suspend(from, &src_state);
 
-    migrate_qmp(from, to, NULL, "{}");
+    migrate_qmp(from, to, NULL, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -1455,7 +1455,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to)
     g_assert_cmpint(ret, ==, 1);
 
     migrate_recover(to, "fd:fd-mig");
-    migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
+    migrate_qmp(from, to, "fd:fd-mig", NULL, "{'resume': true}");
 
     /*
      * Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1543,7 +1543,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, to, uri, "{'resume': true}");
+    migrate_qmp(from, to, uri, NULL, "{'resume': true}");
 
     /* Restore the postcopy bandwidth to unlimited */
     migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1624,7 +1624,7 @@ static void test_baddest(void)
     if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
         return;
     }
-    migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
+    migrate_qmp(from, to, "tcp:127.0.0.1:0", NULL, "{}");
     wait_for_migration_fail(from, false);
     test_migrate_end(from, to, false);
 }
@@ -1663,7 +1663,7 @@ static void test_analyze_script(void)
     uri = g_strdup_printf("exec:cat > %s", file);
 
     migrate_ensure_converge(from);
-    migrate_qmp(from, to, uri, "{}");
+    migrate_qmp(from, to, uri, NULL, "{}");
     wait_for_migration_complete(from);
 
     pid = fork();
@@ -1725,7 +1725,7 @@ static void test_precopy_common(MigrateCommon *args)
         goto finish;
     }
 
-    migrate_qmp(from, to, args->connect_uri, "{}");
+    migrate_qmp(from, to, args->connect_uri, NULL, "{}");
 
     if (args->result != MIG_TEST_SUCCEED) {
         bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1820,7 +1820,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
         goto finish;
     }
 
-    migrate_qmp(from, to, args->connect_uri, "{}");
+    migrate_qmp(from, to, args->connect_uri, NULL, "{}");
     wait_for_migration_complete(from);
 
     /*
@@ -1976,7 +1976,7 @@ static void test_ignore_shared(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    migrate_qmp(from, to, uri, "{}");
+    migrate_qmp(from, to, uri, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2441,7 +2441,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, to, uri, "{}");
+    migrate_qmp(from, to, uri, NULL, "{}");
 
     if (should_fail) {
         qtest_set_expected_status(to, EXIT_FAILURE);
@@ -2544,7 +2544,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, to, uri, "{}");
+    migrate_qmp(from, to, uri, NULL, "{}");
 
     /* Wait for throttling begins */
     percentage = 0;
@@ -2852,7 +2852,7 @@ static void test_multifd_tcp_cancel(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    migrate_qmp(from, to, NULL, "{}");
+    migrate_qmp(from, to, NULL, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -2881,7 +2881,7 @@ static void test_multifd_tcp_cancel(void)
 
     migrate_ensure_non_converge(from);
 
-    migrate_qmp(from, to2, NULL, "{}");
+    migrate_qmp(from, to2, NULL, NULL, "{}");
 
     migrate_wait_for_dirty_mem(from, to2);
 
@@ -3214,7 +3214,7 @@ static void test_migrate_dirty_limit(void)
     migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
 
     /* Start migrate */
-    migrate_qmp(from, to, args.connect_uri, "{}");
+    migrate_qmp(from, to, args.connect_uri, NULL, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
@@ -3255,7 +3255,7 @@ static void test_migrate_dirty_limit(void)
     }
 
     /* Start migrate */
-    migrate_qmp(from, to, args.connect_uri, "{}");
+    migrate_qmp(from, to, args.connect_uri, NULL, "{}");
 
     /* Wait for dirty limit throttle begin */
     throttle_us_per_full = 0;
-- 
2.22.3

Re: [PATCH v3 5/7] Add channels parameter in migrate_qmp

[Fabiano Rosas]

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

> Alter migrate_qmp() to allow use of channels parameter, but only
> fill the uri with correct port number if there are no channels.
> Here we don't want to allow the wrong cases of having both or
> none (ex: migrate_qmp_fail).
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.c | 20 +++++++++++++++-----
>  tests/qtest/migration-helpers.h |  4 ++--
>  tests/qtest/migration-test.c    | 28 ++++++++++++++--------------
>  3 files changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index df4978bf17..0b891351a5 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -19,7 +19,6 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qlist.h"
>  
> -
>  #include "migration-helpers.h"
>  
>  /*
> @@ -154,10 +153,12 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
>   * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
>   */
>  void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
> -                 const char *fmt, ...)
> +                 const char *channels, const char *fmt, ...)
>  {
>      va_list ap;
>      QDict *args;
> +    Error *error_abort = NULL;

Remove this.

> +    QObject *channels_obj = NULL;
>      g_autofree char *connect_uri = NULL;
>  
>      va_start(ap, fmt);

Re: [PATCH v3 5/7] Add channels parameter in migrate_qmp

[Het Gala]

On 06/03/24 8:12 pm, Fabiano Rosas wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> Alter migrate_qmp() to allow use of channels parameter, but only
>> fill the uri with correct port number if there are no channels.
>> Here we don't want to allow the wrong cases of having both or
>> none (ex: migrate_qmp_fail).
>>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> Suggested-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/migration-helpers.c | 20 +++++++++++++++-----
>>   tests/qtest/migration-helpers.h |  4 ++--
>>   tests/qtest/migration-test.c    | 28 ++++++++++++++--------------
>>   3 files changed, 31 insertions(+), 21 deletions(-)
>>
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index df4978bf17..0b891351a5 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -19,7 +19,6 @@
>>   #include "qapi/error.h"
>>   #include "qapi/qmp/qlist.h"
>>   
>> -
>>   #include "migration-helpers.h"
>>   
>>   /*
>> @@ -154,10 +153,12 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
>>    * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
>>    */
>>   void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>> -                 const char *fmt, ...)
>> +                 const char *channels, const char *fmt, ...)
>>   {
>>       va_list ap;
>>       QDict *args;
>> +    Error *error_abort = NULL;
> Remove this.
Ack.
>
>> +    QObject *channels_obj = NULL;
>>       g_autofree char *connect_uri = NULL;
>>   
>>       va_start(ap, fmt);

Regards,

Het Gala

[PATCH v3 6/7] Add multifd_tcp_plain test using list of channels instead of uri

[Het Gala]

Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.

Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f94fe713b2..05e5f3ebe5 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -659,6 +659,12 @@ typedef struct {
      */
     const char *connect_uri;
 
+    /*
+     * Optional: the JSON formatted list of URIs for the src
+     * QEMU to connect to
+     */
+    const char *connect_channels;
+
     /* Optional: callback to run at start to set migration parameters */
     TestMigrateStartHook start_hook;
     /* Optional: callback to run at finish to cleanup */
@@ -2623,7 +2629,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_uri_none(void)
 {
     MigrateCommon args = {
         .listen_uri = "defer",
@@ -2638,6 +2644,21 @@ static void test_multifd_tcp_none(void)
     test_precopy_common(&args);
 }
 
+static void test_multifd_tcp_channels_none(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "defer",
+        .start_hook = test_migrate_precopy_tcp_multifd_start,
+        .live = true,
+        .connect_channels = "[ { 'channel-type': 'main',"
+                            "    'addr': { 'transport': 'socket',"
+                            "              'type': 'inet',"
+                            "              'host': '127.0.0.1',"
+                            "              'port': '0' } } ]",
+    };
+    test_precopy_common(&args);
+}
+
 static void test_multifd_tcp_zlib(void)
 {
     MigrateCommon args = {
@@ -3531,8 +3552,10 @@ 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/uri/plain/none",
+                       test_multifd_tcp_uri_none);
+    migration_test_add("/migration/multifd/tcp/channels/plain/none",
+                       test_multifd_tcp_channels_none);
     migration_test_add("/migration/multifd/tcp/plain/cancel",
                        test_multifd_tcp_cancel);
     migration_test_add("/migration/multifd/tcp/plain/zlib",
-- 
2.22.3

Re: [PATCH v3 6/7] Add multifd_tcp_plain test using list of channels instead of uri

[Fabiano Rosas]

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

> Add a positive test to check multifd live migration but this time
> using list of channels (restricted to 1) as the starting point
> instead of simple uri string.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-test.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index f94fe713b2..05e5f3ebe5 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -659,6 +659,12 @@ typedef struct {
>       */
>      const char *connect_uri;
>  
> +    /*
> +     * Optional: the JSON formatted list of URIs for the src
> +     * QEMU to connect to
> +     */

You could add some words here mentioning that a port of '0' will be
automatically converted to the correct port that the destination is
using.

> +    const char *connect_channels;
> +
>      /* Optional: callback to run at start to set migration parameters */
>      TestMigrateStartHook start_hook;
>      /* Optional: callback to run at finish to cleanup */
> @@ -2623,7 +2629,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_uri_none(void)
>  {
>      MigrateCommon args = {
>          .listen_uri = "defer",
> @@ -2638,6 +2644,21 @@ static void test_multifd_tcp_none(void)
>      test_precopy_common(&args);
>  }
>  
> +static void test_multifd_tcp_channels_none(void)
> +{
> +    MigrateCommon args = {
> +        .listen_uri = "defer",
> +        .start_hook = test_migrate_precopy_tcp_multifd_start,
> +        .live = true,
> +        .connect_channels = "[ { 'channel-type': 'main',"
> +                            "    'addr': { 'transport': 'socket',"
> +                            "              'type': 'inet',"
> +                            "              'host': '127.0.0.1',"
> +                            "              'port': '0' } } ]",
> +    };
> +    test_precopy_common(&args);
> +}
> +
>  static void test_multifd_tcp_zlib(void)
>  {
>      MigrateCommon args = {
> @@ -3531,8 +3552,10 @@ 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/uri/plain/none",
> +                       test_multifd_tcp_uri_none);
> +    migration_test_add("/migration/multifd/tcp/channels/plain/none",
> +                       test_multifd_tcp_channels_none);

We should eventually make a pass to standardize/simplify these
strings. We could have a little "grammar" defined on how to construct
them.

/<test-type>/<migration-mode>/<transport>/<invocation>/<compression>/<encryption>

test-type      :: migrate | validate
migration-mode :: multifd | precopy | postcopy
transport      :: tcp | fd | unix | file
invocation     :: uri | channels
compression    :: zlib | zstd | none
encryption     :: tls | plain

Anyway, work for the future.

Re: [PATCH v3 6/7] Add multifd_tcp_plain test using list of channels instead of uri

[Het Gala]

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


On 06/03/24 8:37 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Add a positive test to check multifd live migration but this time
>> using list of channels (restricted to 1) as the starting point
>> instead of simple uri string.
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>> ---
>>   tests/qtest/migration-test.c | 29 ++++++++++++++++++++++++++---
>>   1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index f94fe713b2..05e5f3ebe5 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -659,6 +659,12 @@ typedef struct {
>>        */
>>       const char *connect_uri;
>>   
>> +    /*
>> +     * Optional: the JSON formatted list of URIs for the src
>> +     * QEMU to connect to
>> +     */
> You could add some words here mentioning that a port of '0' will be
> automatically converted to the correct port that the destination is
> using.
Ack, will add these while defining connect_channels.
>> +    const char *connect_channels;
>> +
>>       /* Optional: callback to run at start to set migration parameters */
>>       TestMigrateStartHook start_hook;
>>       /* Optional: callback to run at finish to cleanup */
>> @@ -2623,7 +2629,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_uri_none(void)
>>   {
>>       MigrateCommon args = {
>>           .listen_uri = "defer", @@ -2638,6 +2644,21 @@ static void test_multifd_tcp_none(void) 
>> test_precopy_common(&args); } +static void 
>> test_multifd_tcp_channels_none(void) +{ + MigrateCommon args = { + 
>> .listen_uri = "defer",
>> +        .start_hook = test_migrate_precopy_tcp_multifd_start,
>> +        .live = true,
>> +        .connect_channels = "[ { 'channel-type': 'main',"
>> +                            "    'addr': { 'transport': 'socket',"
>> +                            "              'type': 'inet',"
>> +                            "              'host': '127.0.0.1',"
>> +                            "              'port': '0' } } ]",
>> +    };
>> +    test_precopy_common(&args);
>> +}
>> +
>>   static void test_multifd_tcp_zlib(void)
>>   {
>>       MigrateCommon args = {
>> @@ -3531,8 +3552,10 @@ 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/uri/plain/none",
>> +                       test_multifd_tcp_uri_none);
>> +    migration_test_add("/migration/multifd/tcp/channels/plain/none",
>> +                       test_multifd_tcp_channels_none);
> We should eventually make a pass to standardize/simplify these
> strings. We could have a little "grammar" defined on how to construct
> them.
>
> /<test-type>/<migration-mode>/<transport>/<invocation>/<compression>/<encryption>
>
> test-type      :: migrate | validate
> migration-mode :: multifd | precopy | postcopy
> transport      :: tcp | fd | unix | file
> invocation     :: uri | channels
> compression    :: zlib | zstd | none
> encryption     :: tls | plain
>
> Anyway, work for the future.

Yes, completely agree with you. It makes it much easier for people to 
identify and define every test.

I can take this up as a separate patchset after this one gets merged maybe


Regards,

Het Gala

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

[PATCH v3 7/7] Add negative tests to validate migration QAPIs

[Het Gala]

Migration QAPI arguments - uri and channels are mutually exhaustive.
Add negative validation tests, one with both arguments present and
one with none present.

Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 54 ++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 05e5f3ebe5..1317f87b22 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2500,6 +2500,56 @@ 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;
+
+    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.
+     */
+    migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
+    test_migrate_end(from, to, false);
+}
+
+static void test_validate_uri_channels_both_set(void)
+{
+    MigrateCommon args = {
+        .start = {
+            .hide_stderr = true,
+        },
+        .listen_uri = "defer",
+        .connect_uri = "tcp:127.0.0.1:0",
+        .connect_channels = "[ { 'channel-type': 'main',"
+                            "    'addr': { 'transport': 'socket',"
+                            "              'type': 'inet',"
+                            "              'host': '127.0.0.1',"
+                            "              'port': '0' } } ]",
+    };
+
+    do_test_validate_uri_channel(&args);
+}
+
+static void test_validate_uri_channels_none_set(void)
+{
+    MigrateCommon args = {
+        .start = {
+            .hide_stderr = true,
+        },
+        .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
@@ -3540,6 +3590,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

Re: [PATCH v3 7/7] Add negative tests to validate migration QAPIs

[Fabiano Rosas]

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

> Migration QAPI arguments - uri and channels are mutually exhaustive.
> Add negative validation tests, one with both arguments present and
> one with none present.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v3 0/7] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs

[Het Gala]

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

On 06/03/24 4:19 pm, Het Gala wrote:

Can also find the successful build here: 
https://gitlab.com/galahet/Qemu/-/pipelines/1201488612
> Het Gala (7):
>    Add 'to' object into migrate_qmp()
>    Replace connect_uri and move migrate_get_socket_address inside
>      migrate_qmp
>    Add channels parameter in migrate_qmp_fail
>    Add migrate_set_ports into migrate_qmp to change migration port number
>    Add channels parameter in migrate_qmp
>    Add multifd_tcp_plain test using list of channels instead of uri
>    Add negative tests to validate migration QAPIs
>
>   tests/qtest/migration-helpers.c | 109 +++++++++++++++++++-
>   tests/qtest/migration-helpers.h |  10 +-
>   tests/qtest/migration-test.c    | 176 ++++++++++++++++++--------------
>   3 files changed, 208 insertions(+), 87 deletions(-)
>
Regards,

Het Gala

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