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

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

[Het Gala]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3762 bytes --]

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-6:
---------
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 7-8:
---------
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.


v6->v7 Changelog:
----------------
1. Rebased the series withthe latest master
2. Added "tests/qtest/migration" prefix in all patches.

v5->v6 Changelog:
----------------
1. Pass args->connect_channels variable instead of NULL while calling
   migrate_qmp for .../multifd/channels/plain/none test. Passing NULL
   beats the whole purpose of the test. 

v4->v5 Changelog:
----------------
1. Remove redundant imports from migration-test.c after moving helper
   functions to migration-helpers.c
2. call migrate_get_socket_address(to) and internally let qdict_get() call
   “socket-address” parameter to make more sense to the reader.
3. qdict needs to be freed, other small fixups.

v3->v4 Changelog:
----------------
1. introduced migrate_get_connect_uri and migrate_get_connect_qdict to
   both used migrate_get_socket_address to get dest uri in socket-
   address, and then use SokcketAddress_to_qdict to convert it into qdict.
2. Misc code changes.

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 (8):
  tests/qtest/migration: Add 'to' object into migrate_qmp()
  tests/qtest/migration: Replace connect_uri and move
    migrate_get_socket_address inside migrate_qmp
  tests/qtest/migration: Replace migrate_get_connect_uri inplace of
    migrate_get_socket_address
  tests/qtest/migration: Add channels parameter in migrate_qmp_fail
  tests/qtest/migration: Add migrate_set_ports into migrate_qmp to
    update migration port value
  tests/qtest/migration: Add channels parameter in migrate_qmp
  tests/qtest/migration: Add multifd_tcp_plain test using list of
    channels instead of uri
  tests/qtest/migration: Add negative tests to validate migration QAPIs

 tests/qtest/migration-helpers.c | 158 +++++++++++++++++++++++++++-
 tests/qtest/migration-helpers.h |  10 +-
 tests/qtest/migration-test.c    | 180 +++++++++++++++++---------------
 3 files changed, 257 insertions(+), 91 deletions(-)

-- 
2.22.3

[PATCH v7 1/8] tests/qtest/migration: 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>
Reviewed-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 71895abb7f..dc1fc002f5 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);
 
@@ -2605,7 +2605,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);
@@ -2708,7 +2708,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;
@@ -3077,7 +3077,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);
 
@@ -3109,7 +3109,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);
 
@@ -3442,7 +3442,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;
@@ -3483,7 +3483,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

[PATCH v7 2/8] tests/qtest/migration: 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>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 54 ++++++++++++++++++++-
 tests/qtest/migration-test.c    | 83 ++++-----------------------------
 2 files changed, 63 insertions(+), 74 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b6206a04fb..3e8c19c4de 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,9 @@
 #include "qemu/osdep.h"
 #include "qemu/ctype.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
 
 #include "migration-helpers.h"
 
@@ -24,6 +27,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 +121,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 dc1fc002f5..7f6a14b19a 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -13,16 +13,12 @@
 #include "qemu/osdep.h"
 
 #include "libqtest.h"
-#include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/range.h"
 #include "qemu/sockets.h"
 #include "chardev/char.h"
-#include "qapi/qapi-visit-sockets.h"
-#include "qapi/qobject-input-visitor.h"
-#include "qapi/qobject-output-visitor.h"
 #include "crypto/tlscredspsk.h"
 #include "qapi/qmp/qlist.h"
 
@@ -369,50 +365,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 +1301,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 +1684,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 +1716,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 +1787,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 +1812,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) {
@@ -3054,7 +2997,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;
@@ -3075,9 +3017,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);
 
@@ -3102,14 +3042,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);
 
@@ -3442,7 +3379,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;
@@ -3483,7 +3420,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

[PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Het Gala]

Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.

migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.

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

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3e8c19c4de..8806dc841e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
     }
 }
 
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
 {
     QDict *rsp;
-    char *result;
     SocketAddressList *addrs;
+    SocketAddress *addr;
     Visitor *iv = NULL;
     QObject *object;
 
     rsp = migrate_query(who);
-    object = qdict_get(rsp, parameter);
+    object = qdict_get(rsp, "socket-address");
 
     iv = qobject_input_visitor_new(object);
     visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
+    addr = addrs->value;
     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;
+    return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who)
+{
+    SocketAddress *addrs;
+    char *connect_uri;
+
+    addrs = migrate_get_socket_address(who);
+    connect_uri = SocketAddress_to_str(addrs);
+
+    qapi_free_SocketAddress(addrs);
+    return connect_uri;
 }
 
 bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
 
     g_assert(!qdict_haskey(args, "uri"));
     if (!uri) {
-        connect_uri = migrate_get_socket_address(to, "socket-address");
+        connect_uri = migrate_get_connect_uri(to);
     }
     qdict_put_str(args, "uri", uri ? uri : connect_uri);
 
-- 
2.22.3

Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Fabiano Rosas]

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

> Refactor migrate_get_socket_address to internally utilize 'socket-address'
> parameter, reducing redundancy in the function definition.
>
> migrate_get_socket_address implicitly converts SocketAddress into str.
> Move migrate_get_socket_address inside migrate_get_connect_uri which
> should return the uri string instead.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 3e8c19c4de..8806dc841e 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>      }
>  }
>  
> -static char *
> -migrate_get_socket_address(QTestState *who, const char *parameter)
> +static SocketAddress *migrate_get_socket_address(QTestState *who)
>  {
>      QDict *rsp;
> -    char *result;
>      SocketAddressList *addrs;
> +    SocketAddress *addr;
>      Visitor *iv = NULL;
>      QObject *object;
>  
>      rsp = migrate_query(who);
> -    object = qdict_get(rsp, parameter);
> +    object = qdict_get(rsp, "socket-address");

Just a heads up, none of what I'm about to say applies to current
master.

This can return NULL if there is no socket-address, such as with a file
migration. Then the visitor code below just barfs. It would be nice if
we touched this up eventually.

I only noticed this because I was fiddling with the file migration API
and this series helped me a lot to test my changes. So thanks for that,
Het.

Another point is: we really need to encourage people to write tests
using the new channels API. I added the FileMigrationArgs with the
'offset' as a required parameter, not even knowing optional parameters
were a thing. So it's obviously not enough to write support for the new
API if no tests ever touch it.

>  
>      iv = qobject_input_visitor_new(object);
>      visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
> +    addr = addrs->value;
>      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;
> +    return addr;
> +}
> +
> +static char *
> +migrate_get_connect_uri(QTestState *who)
> +{
> +    SocketAddress *addrs;
> +    char *connect_uri;
> +
> +    addrs = migrate_get_socket_address(who);
> +    connect_uri = SocketAddress_to_str(addrs);
> +
> +    qapi_free_SocketAddress(addrs);
> +    return connect_uri;
>  }
>  
>  bool migrate_watch_for_events(QTestState *who, const char *name,
> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>  
>      g_assert(!qdict_haskey(args, "uri"));
>      if (!uri) {
> -        connect_uri = migrate_get_socket_address(to, "socket-address");
> +        connect_uri = migrate_get_connect_uri(to);
>      }
>      qdict_put_str(args, "uri", uri ? uri : connect_uri);

Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Het Gala]

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


On 15/03/24 6:28 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Refactor migrate_get_socket_address to internally utilize 'socket-address'
>> parameter, reducing redundancy in the function definition.
>>
>> migrate_get_socket_address implicitly converts SocketAddress into str.
>> Move migrate_get_socket_address inside migrate_get_connect_uri which
>> should return the uri string instead.
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>> Reviewed-by: Fabiano Rosas<farosas@suse.de>
>> ---
>>   tests/qtest/migration-helpers.c | 29 +++++++++++++++++++----------
>>   1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index 3e8c19c4de..8806dc841e 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>>       }
>>   }
>>   
>> -static char *
>> -migrate_get_socket_address(QTestState *who, const char *parameter)
>> +static SocketAddress *migrate_get_socket_address(QTestState *who)
>>   {
>>       QDict *rsp;
>> -    char *result;
>>       SocketAddressList *addrs;
>> +    SocketAddress *addr;
>>       Visitor *iv = NULL;
>>       QObject *object;
>>   
>>       rsp = migrate_query(who);
>> -    object = qdict_get(rsp, parameter);
>> +    object = qdict_get(rsp, "socket-address");
> Just a heads up, none of what I'm about to say applies to current
> master.
>
> This can return NULL if there is no socket-address, such as with a file
> migration. Then the visitor code below just barfs. It would be nice if
> we touched this up eventually.

Yes. I agree this is not full proof solution and covers for all the cases.
It would only for 'socket-address'. Could you elaborate on what other than
socket-address the QObject can have ?

> I only noticed this because I was fiddling with the file migration API
> and this series helped me a lot to test my changes. So thanks for that,
> Het.
>
> Another point is: we really need to encourage people to write tests
> using the new channels API. I added the FileMigrationArgs with the
> 'offset' as a required parameter, not even knowing optional parameters
> were a thing. So it's obviously not enough to write support for the new
> API if no tests ever touch it.
Yes, definitely we need more tests with the new channels API to test other
than just tcp connection. I could give a try for vsock and unix with the
new QAPI syntax, and add some tests.

I also wanted to bring in attention that, this solution I what i feel is 
also
not complete. If we are using new channel syntax for migrate_qmp, then the
same syntax should also be used for migrate_qmp_incoming. But we haven't
touch that, and it still prints the old syntax. We might need a lot changes
in design maybe to incorporate that too in new tests totally with the new
syntax.

Another thing that you also noted down while discussing on the patches that
we should have a standard pattern on how to define the migration tests. Even
that would be helpful for the users, on how to add new tests, where to add
new tests in the file, and which test is needed to run if a specific change
needs to be tested.

>>   
>>       iv = qobject_input_visitor_new(object);
>>       visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
>> +    addr = addrs->value;
>>       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;
>> +    return addr;
>> +}
>> +
>> +static char *
>> +migrate_get_connect_uri(QTestState *who)
>> +{
>> +    SocketAddress *addrs;
>> +    char *connect_uri;
>> +
>> +    addrs = migrate_get_socket_address(who);
>> +    connect_uri = SocketAddress_to_str(addrs);
>> +
>> +    qapi_free_SocketAddress(addrs);
>> +    return connect_uri;
>>   }
>>   
>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>   
>>       g_assert(!qdict_haskey(args, "uri"));
>>       if (!uri) {
>> -        connect_uri = migrate_get_socket_address(to, "socket-address");
>> +        connect_uri = migrate_get_connect_uri(to);
>>       }
>>       qdict_put_str(args, "uri", uri ? uri : connect_uri);

Regards,
Het Gala

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

Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Fabiano Rosas]

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

> On 15/03/24 6:28 pm, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> Refactor migrate_get_socket_address to internally utilize 'socket-address'
>>> parameter, reducing redundancy in the function definition.
>>>
>>> migrate_get_socket_address implicitly converts SocketAddress into str.
>>> Move migrate_get_socket_address inside migrate_get_connect_uri which
>>> should return the uri string instead.
>>>
>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>> Reviewed-by: Fabiano Rosas<farosas@suse.de>
>>> ---
>>>   tests/qtest/migration-helpers.c | 29 +++++++++++++++++++----------
>>>   1 file changed, 19 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>> index 3e8c19c4de..8806dc841e 100644
>>> --- a/tests/qtest/migration-helpers.c
>>> +++ b/tests/qtest/migration-helpers.c
>>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>>>       }
>>>   }
>>>   
>>> -static char *
>>> -migrate_get_socket_address(QTestState *who, const char *parameter)
>>> +static SocketAddress *migrate_get_socket_address(QTestState *who)
>>>   {
>>>       QDict *rsp;
>>> -    char *result;
>>>       SocketAddressList *addrs;
>>> +    SocketAddress *addr;
>>>       Visitor *iv = NULL;
>>>       QObject *object;
>>>   
>>>       rsp = migrate_query(who);
>>> -    object = qdict_get(rsp, parameter);
>>> +    object = qdict_get(rsp, "socket-address");
>> Just a heads up, none of what I'm about to say applies to current
>> master.
>>
>> This can return NULL if there is no socket-address, such as with a file
>> migration. Then the visitor code below just barfs. It would be nice if
>> we touched this up eventually.
>
> Yes. I agree this is not full proof solution and covers for all the cases.
> It would only for 'socket-address'. Could you elaborate on what other than
> socket-address the QObject can have ?

I can just not have the socket-address, AFAICS. We'd just need to not
crash if that's the case.

>
>> I only noticed this because I was fiddling with the file migration API
>> and this series helped me a lot to test my changes. So thanks for that,
>> Het.
>>
>> Another point is: we really need to encourage people to write tests
>> using the new channels API. I added the FileMigrationArgs with the
>> 'offset' as a required parameter, not even knowing optional parameters
>> were a thing. So it's obviously not enough to write support for the new
>> API if no tests ever touch it.
> Yes, definitely we need more tests with the new channels API to test other
> than just tcp connection. I could give a try for vsock and unix with the
> new QAPI syntax, and add some tests.
>
> I also wanted to bring in attention that, this solution I what i feel is 
> also
> not complete. If we are using new channel syntax for migrate_qmp, then the
> same syntax should also be used for migrate_qmp_incoming. But we haven't
> touch that, and it still prints the old syntax. We might need a lot changes
> in design maybe to incorporate that too in new tests totally with the new
> syntax.

Adding migrate_qmp_incoming support should be relatively simple. You had
patches for that in another version, no?

>
> Another thing that you also noted down while discussing on the patches that
> we should have a standard pattern on how to define the migration tests. Even
> that would be helpful for the users, on how to add new tests, where to add
> new tests in the file, and which test is needed to run if a specific change
> needs to be tested.
>
>>>   
>>>       iv = qobject_input_visitor_new(object);
>>>       visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
>>> +    addr = addrs->value;
>>>       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;
>>> +    return addr;
>>> +}
>>> +
>>> +static char *
>>> +migrate_get_connect_uri(QTestState *who)
>>> +{
>>> +    SocketAddress *addrs;
>>> +    char *connect_uri;
>>> +
>>> +    addrs = migrate_get_socket_address(who);
>>> +    connect_uri = SocketAddress_to_str(addrs);
>>> +
>>> +    qapi_free_SocketAddress(addrs);
>>> +    return connect_uri;
>>>   }
>>>   
>>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>   
>>>       g_assert(!qdict_haskey(args, "uri"));
>>>       if (!uri) {
>>> -        connect_uri = migrate_get_socket_address(to, "socket-address");
>>> +        connect_uri = migrate_get_connect_uri(to);
>>>       }
>>>       qdict_put_str(args, "uri", uri ? uri : connect_uri);
>
> Regards,
> Het Gala

Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Het Gala]

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


On 18/03/24 7:46 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 15/03/24 6:28 pm, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>   writes:
>>>
>>>> Refactor migrate_get_socket_address to internally utilize 'socket-address'
>>>> parameter, reducing redundancy in the function definition.
>>>>
>>>> migrate_get_socket_address implicitly converts SocketAddress into str.
>>>> Move migrate_get_socket_address inside migrate_get_connect_uri which
>>>> should return the uri string instead.
>>>>
>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>>> Reviewed-by: Fabiano Rosas<farosas@suse.de>
>>>> ---
>>>>    tests/qtest/migration-helpers.c | 29 +++++++++++++++++++----------
>>>>    1 file changed, 19 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>>> index 3e8c19c4de..8806dc841e 100644
>>>> --- a/tests/qtest/migration-helpers.c
>>>> +++ b/tests/qtest/migration-helpers.c
>>>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>>>>        }
>>>>    }
>>>>    
>>>> -static char *
>>>> -migrate_get_socket_address(QTestState *who, const char *parameter)
>>>> +static SocketAddress *migrate_get_socket_address(QTestState *who)
>>>>    {
>>>>        QDict *rsp;
>>>> -    char *result;
>>>>        SocketAddressList *addrs;
>>>> +    SocketAddress *addr;
>>>>        Visitor *iv = NULL;
>>>>        QObject *object;
>>>>    
>>>>        rsp = migrate_query(who);
>>>> -    object = qdict_get(rsp, parameter);
>>>> +    object = qdict_get(rsp, "socket-address");
>>> Just a heads up, none of what I'm about to say applies to current
>>> master.
>>>
>>> This can return NULL if there is no socket-address, such as with a file
>>> migration. Then the visitor code below just barfs. It would be nice if
>>> we touched this up eventually.
>> Yes. I agree this is not full proof solution and covers for all the cases.
>> It would only for 'socket-address'. Could you elaborate on what other than
>> socket-address the QObject can have ?
> I can just not have the socket-address, AFAICS. We'd just need to not
> crash if that's the case.

value: {
     "status": "setup",
     "socket-address": [
         {
             "port": "46213",
             "ipv4": true,
             "host": "127.0.0.1",
             "type": "inet"
         }
     ]
}

Okay, I understood your ask here. This is what gets printed from the QDict.
Let me raise a patch to return with a message if the QDict does not have key
with 'socket-address'. This would prevent the crash later on.

I wanted to know what other than "socket-address" key can he QDict give us
because, if that's the case, for other than socket migration, then we can
make this function more generic rather than having it as 
'migrate_get_socket_address'
>>> I only noticed this because I was fiddling with the file migration API
>>> and this series helped me a lot to test my changes. So thanks for that,
>>> Het.
>>>
>>> Another point is: we really need to encourage people to write tests
>>> using the new channels API. I added the FileMigrationArgs with the
>>> 'offset' as a required parameter, not even knowing optional parameters
>>> were a thing. So it's obviously not enough to write support for the new
>>> API if no tests ever touch it.
>> Yes, definitely we need more tests with the new channels API to test other
>> than just tcp connection. I could give a try for vsock and unix with the
>> new QAPI syntax, and add some tests.
>>
>> I also wanted to bring in attention that, this solution I what i feel is
>> also
>> not complete. If we are using new channel syntax for migrate_qmp, then the
>> same syntax should also be used for migrate_qmp_incoming. But we haven't
>> touch that, and it still prints the old syntax. We might need a lot changes
>> in design maybe to incorporate that too in new tests totally with the new
>> syntax.
> Adding migrate_qmp_incoming support should be relatively simple. You had
> patches for that in another version, no?
No Fabiano, what I meant was, in migration-test.c, change in 
migrate_incoming_qmp
would need to change the callback function and ultimately change all the 
callback
handlers ? In that sense, it would require a big change ?
Inside the migrate_incoming_qmp function, adding implementation for 
channels is
same as other migrate_* function.
>> Another thing that you also noted down while discussing on the patches that
>> we should have a standard pattern on how to define the migration tests. Even
>> that would be helpful for the users, on how to add new tests, where to add
>> new tests in the file, and which test is needed to run if a specific change
>> needs to be tested.
>>
>>>>    
>>>>        iv = qobject_input_visitor_new(object);
>>>>        visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
>>>> +    addr = addrs->value;
>>>>        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;
>>>> +    return addr;
>>>> +}
>>>> +
>>>> +static char *
>>>> +migrate_get_connect_uri(QTestState *who)
>>>> +{
>>>> +    SocketAddress *addrs;
>>>> +    char *connect_uri;
>>>> +
>>>> +    addrs = migrate_get_socket_address(who);
>>>> +    connect_uri = SocketAddress_to_str(addrs);
>>>> +
>>>> +    qapi_free_SocketAddress(addrs);
>>>> +    return connect_uri;
>>>>    }
>>>>    
>>>>    bool migrate_watch_for_events(QTestState *who, const char *name,
>>>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>>    
>>>>        g_assert(!qdict_haskey(args, "uri"));
>>>>        if (!uri) {
>>>> -        connect_uri = migrate_get_socket_address(to, "socket-address");
>>>> +        connect_uri = migrate_get_connect_uri(to);
>>>>        }
>>>>        qdict_put_str(args, "uri", uri ? uri : connect_uri);
>> Regards,
>> Het Gala
Regards,
Het Gala

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

Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Fabiano Rosas]

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

> On 18/03/24 7:46 pm, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> On 15/03/24 6:28 pm, Fabiano Rosas wrote:
>>>> Het Gala<het.gala@nutanix.com>   writes:
>>>>
>>>>> Refactor migrate_get_socket_address to internally utilize 'socket-address'
>>>>> parameter, reducing redundancy in the function definition.
>>>>>
>>>>> migrate_get_socket_address implicitly converts SocketAddress into str.
>>>>> Move migrate_get_socket_address inside migrate_get_connect_uri which
>>>>> should return the uri string instead.
>>>>>
>>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>>>> Reviewed-by: Fabiano Rosas<farosas@suse.de>
>>>>> ---
>>>>>    tests/qtest/migration-helpers.c | 29 +++++++++++++++++++----------
>>>>>    1 file changed, 19 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>>>> index 3e8c19c4de..8806dc841e 100644
>>>>> --- a/tests/qtest/migration-helpers.c
>>>>> +++ b/tests/qtest/migration-helpers.c
>>>>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>>>>>        }
>>>>>    }
>>>>>    
>>>>> -static char *
>>>>> -migrate_get_socket_address(QTestState *who, const char *parameter)
>>>>> +static SocketAddress *migrate_get_socket_address(QTestState *who)
>>>>>    {
>>>>>        QDict *rsp;
>>>>> -    char *result;
>>>>>        SocketAddressList *addrs;
>>>>> +    SocketAddress *addr;
>>>>>        Visitor *iv = NULL;
>>>>>        QObject *object;
>>>>>    
>>>>>        rsp = migrate_query(who);
>>>>> -    object = qdict_get(rsp, parameter);
>>>>> +    object = qdict_get(rsp, "socket-address");
>>>> Just a heads up, none of what I'm about to say applies to current
>>>> master.
>>>>
>>>> This can return NULL if there is no socket-address, such as with a file
>>>> migration. Then the visitor code below just barfs. It would be nice if
>>>> we touched this up eventually.
>>> Yes. I agree this is not full proof solution and covers for all the cases.
>>> It would only for 'socket-address'. Could you elaborate on what other than
>>> socket-address the QObject can have ?
>> I can just not have the socket-address, AFAICS. We'd just need to not
>> crash if that's the case.
>
> value: {
>      "status": "setup",
>      "socket-address": [
>          {
>              "port": "46213",
>              "ipv4": true,
>              "host": "127.0.0.1",
>              "type": "inet"
>          }
>      ]
> }
>
> Okay, I understood your ask here. This is what gets printed from the QDict.
> Let me raise a patch to return with a message if the QDict does not have key
> with 'socket-address'. This would prevent the crash later on.
>
> I wanted to know what other than "socket-address" key can he QDict give us
> because, if that's the case, for other than socket migration, then we can
> make this function more generic rather than having it as 
> 'migrate_get_socket_address'

For now, there's nothing else. Let's just ignore when socket-address is
missing in the reply so we don't break future tests that use a
non-socket type.

>>>> I only noticed this because I was fiddling with the file migration API
>>>> and this series helped me a lot to test my changes. So thanks for that,
>>>> Het.
>>>>
>>>> Another point is: we really need to encourage people to write tests
>>>> using the new channels API. I added the FileMigrationArgs with the
>>>> 'offset' as a required parameter, not even knowing optional parameters
>>>> were a thing. So it's obviously not enough to write support for the new
>>>> API if no tests ever touch it.
>>> Yes, definitely we need more tests with the new channels API to test other
>>> than just tcp connection. I could give a try for vsock and unix with the
>>> new QAPI syntax, and add some tests.
>>>
>>> I also wanted to bring in attention that, this solution I what i feel is
>>> also
>>> not complete. If we are using new channel syntax for migrate_qmp, then the
>>> same syntax should also be used for migrate_qmp_incoming. But we haven't
>>> touch that, and it still prints the old syntax. We might need a lot changes
>>> in design maybe to incorporate that too in new tests totally with the new
>>> syntax.
>> Adding migrate_qmp_incoming support should be relatively simple. You had
>> patches for that in another version, no?
> No Fabiano, what I meant was, in migration-test.c, change in 
> migrate_incoming_qmp
> would need to change the callback function and ultimately change all the 
> callback
> handlers ? In that sense, it would require a big change ?
> Inside the migrate_incoming_qmp function, adding implementation for 
> channels is
> same as other migrate_* function.

You could add the parameter to migrate_incoming_qmp and use NULL when
calling. The callbacks don't need to be changed. When we add more tests
then we'd alter the callbacks accordingly.

I might convert the file tests soon, you can leave that part to me if
you want.

>>> Another thing that you also noted down while discussing on the patches that
>>> we should have a standard pattern on how to define the migration tests. Even
>>> that would be helpful for the users, on how to add new tests, where to add
>>> new tests in the file, and which test is needed to run if a specific change
>>> needs to be tested.
>>>
>>>>>    
>>>>>        iv = qobject_input_visitor_new(object);
>>>>>        visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
>>>>> +    addr = addrs->value;
>>>>>        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;
>>>>> +    return addr;
>>>>> +}
>>>>> +
>>>>> +static char *
>>>>> +migrate_get_connect_uri(QTestState *who)
>>>>> +{
>>>>> +    SocketAddress *addrs;
>>>>> +    char *connect_uri;
>>>>> +
>>>>> +    addrs = migrate_get_socket_address(who);
>>>>> +    connect_uri = SocketAddress_to_str(addrs);
>>>>> +
>>>>> +    qapi_free_SocketAddress(addrs);
>>>>> +    return connect_uri;
>>>>>    }
>>>>>    
>>>>>    bool migrate_watch_for_events(QTestState *who, const char *name,
>>>>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>>>    
>>>>>        g_assert(!qdict_haskey(args, "uri"));
>>>>>        if (!uri) {
>>>>> -        connect_uri = migrate_get_socket_address(to, "socket-address");
>>>>> +        connect_uri = migrate_get_connect_uri(to);
>>>>>        }
>>>>>        qdict_put_str(args, "uri", uri ? uri : connect_uri);
>>> Regards,
>>> Het Gala
> Regards,
> Het Gala

Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Het Gala]

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


On 20/03/24 12:33 am, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 18/03/24 7:46 pm, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>   writes:
>>>
>>>> On 15/03/24 6:28 pm, Fabiano Rosas wrote:
>>>>> Het Gala<het.gala@nutanix.com>    writes:
>>>>>
>>>>>> Refactor migrate_get_socket_address to internally utilize 'socket-address'
>>>>>> parameter, reducing redundancy in the function definition.
>>>>>>
>>>>>> migrate_get_socket_address implicitly converts SocketAddress into str.
>>>>>> Move migrate_get_socket_address inside migrate_get_connect_uri which
>>>>>> should return the uri string instead.
>>>>>>
>>>>>> -static char *
>>>>>> -migrate_get_socket_address(QTestState *who, const char *parameter)
>>>>>> +static SocketAddress *migrate_get_socket_address(QTestState *who)
>>>>>>     {
>>>>>>         QDict *rsp;
>>>>>> -    char *result;
>>>>>>         SocketAddressList *addrs;
>>>>>> +    SocketAddress *addr;
>>>>>>         Visitor *iv = NULL;
>>>>>>         QObject *object;
>>>>>>     
>>>>>>         rsp = migrate_query(who);
>>>>>> -    object = qdict_get(rsp, parameter);
>>>>>> +    object = qdict_get(rsp, "socket-address");
>>>>> Just a heads up, none of what I'm about to say applies to current
>>>>> master.
>>>>>
>>>>> This can return NULL if there is no socket-address, such as with a file
>>>>> migration. Then the visitor code below just barfs. It would be nice if
>>>>> we touched this up eventually.
>>>> Yes. I agree this is not full proof solution and covers for all the cases.
>>>> It would only for 'socket-address'. Could you elaborate on what other than
>>>> socket-address the QObject can have ?
>>> I can just not have the socket-address, AFAICS. We'd just need to not
>>> crash if that's the case.
>> value: {
>>       "status": "setup",
>>       "socket-address": [
>>           {
>>               "port": "46213",
>>               "ipv4": true,
>>               "host": "127.0.0.1",
>>               "type": "inet"
>>           }
>>       ]
>> }
>>
>> Okay, I understood your ask here. This is what gets printed from the QDict.
>> Let me raise a patch to return with a message if the QDict does not have key
>> with 'socket-address'. This would prevent the crash later on.
>>
>> I wanted to know what other than "socket-address" key can he QDict give us
>> because, if that's the case, for other than socket migration, then we can
>> make this function more generic rather than having it as
>> 'migrate_get_socket_address'
> For now, there's nothing else. Let's just ignore when socket-address is
> missing in the reply so we don't break future tests that use a
> non-socket type.
Okay, Done. Can find the build here: 
https://gitlab.com/galahet/Qemu/-/pipelines/1219841944
>>>>> I only noticed this because I was fiddling with the file migration API
>>>>> and this series helped me a lot to test my changes. So thanks for that,
>>>>> Het.
>>>>>
>>>>> Another point is: we really need to encourage people to write tests
>>>>> using the new channels API. I added the FileMigrationArgs with the
>>>>> 'offset' as a required parameter, not even knowing optional parameters
>>>>> were a thing. So it's obviously not enough to write support for the new
>>>>> API if no tests ever touch it.
>>>> Yes, definitely we need more tests with the new channels API to test other
>>>> than just tcp connection. I could give a try for vsock and unix with the
>>>> new QAPI syntax, and add some tests.
>>>>
>>>> I also wanted to bring in attention that, this solution I what i feel is
>>>> also
>>>> not complete. If we are using new channel syntax for migrate_qmp, then the
>>>> same syntax should also be used for migrate_qmp_incoming. But we haven't
>>>> touch that, and it still prints the old syntax. We might need a lot changes
>>>> in design maybe to incorporate that too in new tests totally with the new
>>>> syntax.
>>> Adding migrate_qmp_incoming support should be relatively simple. You had
>>> patches for that in another version, no?
>> No Fabiano, what I meant was, in migration-test.c, change in
>> migrate_incoming_qmp
>> would need to change the callback function and ultimately change all the
>> callback
>> handlers ? In that sense, it would require a big change ?
>> Inside the migrate_incoming_qmp function, adding implementation for
>> channels is
>> same as other migrate_* function.
> You could add the parameter to migrate_incoming_qmp and use NULL when
> calling. The callbacks don't need to be changed. When we add more tests
> then we'd alter the callbacks accordingly.
>
> I might convert the file tests soon, you can leave that part to me if
> you want.
Okay, sure. Will leave those changes to you.
>>>> Another thing that you also noted down while discussing on the patches that
>>>> we should have a standard pattern on how to define the migration tests. Even
>>>> that would be helpful for the users, on how to add new tests, where to add
>>>> new tests in the file, and which test is needed to run if a specific change
>>>> needs to be tested.
>>>>
>>>>>>     
>>>>>>         iv = qobject_input_visitor_new(object);
>>>>>>         visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
>>>>>> +    addr = addrs->value;
>>>>>>         visit_free(iv);
>>>>>>     
>>>>>> -    /* we are only using a single address */
>>>>>>
>>>>>>
Regards,
Het Gala

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

[PATCH v7 4/8] tests/qtest/migration: 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>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 13 +++++++++++--
 tests/qtest/migration-helpers.h |  5 +++--
 tests/qtest/migration-test.c    |  4 ++--
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 8806dc841e..f215f44467 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -100,7 +100,8 @@ 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;
@@ -110,7 +111,15 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
     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) {
+        QObject *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 7f6a14b19a..cda07f8f97 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1717,7 +1717,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;
     }
 
@@ -1812,7 +1812,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

[PATCH v7 5/8] tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value

[Het Gala]

migrate_get_connect_qdict gets qdict with the dst QEMU parameters.

migrate_set_ports() from list of channels reads each QDict for port,
and fills the port with correct value in case it was 0 in the test.

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

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index f215f44467..ed8d812e9d 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -16,6 +16,8 @@
 #include "qapi/qapi-visit-sockets.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+#include "qemu/cutils.h"
 
 #include "migration-helpers.h"
 
@@ -48,6 +50,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
     }
 }
 
+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;
+}
+
 static SocketAddress *migrate_get_socket_address(QTestState *who)
 {
     QDict *rsp;
@@ -81,6 +114,46 @@ migrate_get_connect_uri(QTestState *who)
     return connect_uri;
 }
 
+static QDict *
+migrate_get_connect_qdict(QTestState *who)
+{
+    SocketAddress *addrs;
+    QDict *connect_qdict;
+
+    addrs = migrate_get_socket_address(who);
+    connect_qdict = SocketAddress_to_qdict(addrs);
+
+    qapi_free_SocketAddress(addrs);
+    return connect_qdict;
+}
+
+static void migrate_set_ports(QTestState *to, QList *channel_list)
+{
+    QDict *addr;
+    QListEntry *entry;
+    g_autofree const char *addr_port = NULL;
+
+    if (channel_list == NULL) {
+        return;
+    }
+
+    addr = migrate_get_connect_qdict(to);
+
+    QLIST_FOREACH_ENTRY(channel_list, entry) {
+        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+        QDict *addrdict = qdict_get_qdict(channel, "addr");
+
+        if (qdict_haskey(addrdict, "port") &&
+            qdict_haskey(addr, "port") &&
+            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+                addr_port = qdict_get_str(addr, "port");
+                qdict_put_str(addrdict, "port", addr_port);
+        }
+    }
+
+    qobject_unref(addr);
+}
+
 bool migrate_watch_for_events(QTestState *who, const char *name,
                               QDict *event, void *opaque)
 {
@@ -139,6 +212,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
 {
     va_list ap;
     QDict *args;
+    QList *channel_list = NULL;
     g_autofree char *connect_uri = NULL;
 
     va_start(ap, fmt);
@@ -149,6 +223,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
     if (!uri) {
         connect_uri = migrate_get_connect_uri(to);
     }
+    migrate_set_ports(to, channel_list);
     qdict_put_str(args, "uri", uri ? uri : connect_uri);
 
     qtest_qmp_assert_success(who,
-- 
2.22.3

[PATCH v7 6/8] tests/qtest/migration: 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>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 22 +++++++++++++---------
 tests/qtest/migration-helpers.h |  4 ++--
 tests/qtest/migration-test.c    | 28 ++++++++++++++--------------
 3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index ed8d812e9d..b2a90469fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -133,10 +133,6 @@ static void migrate_set_ports(QTestState *to, QList *channel_list)
     QListEntry *entry;
     g_autofree const char *addr_port = NULL;
 
-    if (channel_list == NULL) {
-        return;
-    }
-
     addr = migrate_get_connect_qdict(to);
 
     QLIST_FOREACH_ENTRY(channel_list, entry) {
@@ -208,11 +204,10 @@ 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;
-    QList *channel_list = NULL;
     g_autofree char *connect_uri = NULL;
 
     va_start(ap, fmt);
@@ -220,11 +215,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_connect_uri(to);
+        qdict_put_str(args, "uri", connect_uri);
+    }
+
+    g_assert(!qdict_haskey(args, "channels"));
+    if (channels) {
+        QObject *channels_obj = qobject_from_json(channels, &error_abort);
+        QList *channel_list = qobject_to(QList, channels_obj);
+        migrate_set_ports(to, channel_list);
+        qdict_put_obj(args, "channels", channels_obj);
     }
-    migrate_set_ports(to, channel_list);
-    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 cda07f8f97..62e52a29cc 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1301,7 +1301,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);
 
@@ -1451,7 +1451,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
@@ -1539,7 +1539,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);
@@ -1620,7 +1620,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);
 }
@@ -1659,7 +1659,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();
@@ -1721,7 +1721,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;
@@ -1816,7 +1816,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);
 
     /*
@@ -1972,7 +1972,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);
 
@@ -2548,7 +2548,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);
@@ -2651,7 +2651,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;
@@ -3017,7 +3017,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);
 
@@ -3046,7 +3046,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);
 
@@ -3379,7 +3379,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;
@@ -3420,7 +3420,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

[PATCH v7 7/8] tests/qtest/migration: 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>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 62e52a29cc..7ea13c8e5b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -655,6 +655,13 @@ typedef struct {
      */
     const char *connect_uri;
 
+    /*
+     * Optional: JSON-formatted list of src QEMU URIs. If a port is
+     * defined as '0' in any QDict key a value of '0' will be
+     * automatically converted to the correct destination port.
+     */
+    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 */
@@ -2758,7 +2765,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",
@@ -2803,6 +2810,21 @@ static void test_multifd_tcp_no_zero_page(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 = {
@@ -3712,8 +3734,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/zero-page/legacy",
                        test_multifd_tcp_zero_page_legacy);
     migration_test_add("/migration/multifd/tcp/plain/zero-page/none",
-- 
2.22.3

[PATCH v7 8/8] tests/qtest/migration: 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>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 55 +++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7ea13c8e5b..bd9f4b9dbb 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1724,7 +1724,7 @@ static void test_precopy_common(MigrateCommon *args)
     }
 
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
+        migrate_qmp_fail(from, args->connect_uri, args->connect_uri, "{}");
         goto finish;
     }
 
@@ -2608,6 +2608,55 @@ 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;
+
+    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
@@ -3722,6 +3771,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 v7 0/8] tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs

[Het Gala]

Can find the passed build here: 
https://gitlab.com/galahet/Qemu/-/pipelines/1210931136

On 13/03/24 1:56 am, Het Gala wrote:
> 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.
[...]
>
> Het Gala (8):
>    tests/qtest/migration: Add 'to' object into migrate_qmp()
>    tests/qtest/migration: Replace connect_uri and move
>      migrate_get_socket_address inside migrate_qmp
>    tests/qtest/migration: Replace migrate_get_connect_uri inplace of
>      migrate_get_socket_address
>    tests/qtest/migration: Add channels parameter in migrate_qmp_fail
>    tests/qtest/migration: Add migrate_set_ports into migrate_qmp to
>      update migration port value
>    tests/qtest/migration: Add channels parameter in migrate_qmp
>    tests/qtest/migration: Add multifd_tcp_plain test using list of
>      channels instead of uri
>    tests/qtest/migration: Add negative tests to validate migration QAPIs
>
>   tests/qtest/migration-helpers.c | 158 +++++++++++++++++++++++++++-
>   tests/qtest/migration-helpers.h |  10 +-
>   tests/qtest/migration-test.c    | 180 +++++++++++++++++---------------
>   3 files changed, 257 insertions(+), 91 deletions(-)
>
Regards,
Het Gala

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

[Peter Xu]

On Tue, Mar 12, 2024 at 08:26:26PM +0000, Het Gala wrote:
> 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.

queued for 9.1, thanks.

-- 
Peter Xu