Introduce support for adding a 'channels' argument to migrate_qmp_fail
and migrate_qmp functions within the migration qtest framework, enabling
enhanced control over migration scenarios.
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
tests/qtest/dbus-vmstate-test.c | 2 +-
tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
tests/qtest/migration-helpers.h | 11 ++--
tests/qtest/migration-test.c | 33 ++++++------
4 files changed, 112 insertions(+), 27 deletions(-)
diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
index 6c990864e3..0ca572e29b 100644
--- a/tests/qtest/dbus-vmstate-test.c
+++ b/tests/qtest/dbus-vmstate-test.c
@@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
- migrate_qmp(src_qemu, uri, "{}");
+ migrate_qmp(src_qemu, uri, NULL, "{}");
test->src_qemu = src_qemu;
if (test->migrate_fail) {
wait_for_migration_fail(src_qemu, true);
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..d153677887 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "qemu/ctype.h"
#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qlist.h"
#include "migration-helpers.h"
@@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
return false;
}
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+static char *socketAddressType_to_str(SocketAddressType type)
+{
+ switch (type) {
+ case SOCKET_ADDRESS_TYPE_INET:
+ return g_strdup("inet");
+ case SOCKET_ADDRESS_TYPE_UNIX:
+ return g_strdup("unix");
+ case SOCKET_ADDRESS_TYPE_FD:
+ return g_strdup("fd");
+ case SOCKET_ADDRESS_TYPE_VSOCK:
+ return g_strdup("vsock");
+ default:
+ return g_strdup("unknown address type");
+ }
+}
+
+static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
+{
+ MigrationChannel *channel = NULL;
+ MigrationAddress *addr = NULL;
+ SocketAddress *saddr = NULL;
+ g_autofree const char *addr_type = NULL;
+ QList *channelList = qlist_new();
+ QDict *channelDict = qdict_new();
+ QDict *addrDict = qdict_new();
+
+ channel = channels->value;
+ if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
+ fprintf(stderr, "%s: Channel or its type is NULL\n",
+ __func__);
+ }
+ g_assert(channel);
+ if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
+ qdict_put_str(channelDict, "channel-type", g_strdup("main"));
+ }
+
+ addr = channel->addr;
+ if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
+ fprintf(stderr, "%s: addr or its transport is NULL\n",
+ __func__);
+ }
+ g_assert(addr);
+ if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+ qdict_put_str(addrDict, "transport", g_strdup("socket"));
+ }
+
+ saddr = &addr->u.socket;
+ if (!saddr) {
+ fprintf(stderr, "%s: saddr is NULL\n", __func__);
+ }
+ g_assert(saddr);
+ addr_type = socketAddressType_to_str(saddr->type);
+ qdict_put_str(addrDict, "type", addr_type);
+ qdict_put_str(addrDict, "port", saddr->u.inet.port);
+ qdict_put_str(addrDict, "host", saddr->u.inet.host);
+
+ qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
+ qlist_append_obj(channelList, QOBJECT(channelDict));
+
+ return channelList;
+}
+
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ MigrationChannelList *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *err;
@@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
- g_assert(!qdict_haskey(args, "uri"));
- qdict_put_str(args, "uri", uri);
+ if (uri) {
+ g_assert(!qdict_haskey(args, "uri"));
+ qdict_put_str(args, "uri", uri);
+ }
+
+ if (channels) {
+ g_assert(!qdict_haskey(args, "channels"));
+ QList *channelList = MigrationChannelList_to_QList(channels);
+ qdict_put_obj(args, "channels", QOBJECT(channelList));
+ }
err = qtest_qmp_assert_failure_ref(
who, "{ 'execute': 'migrate', 'arguments': %p}", args);
@@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
* Arguments are built from @fmt... (formatted like
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp(QTestState *who, const char *uri,
+ MigrationChannelList *channels, const char *fmt, ...)
{
va_list ap;
QDict *args;
@@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
- g_assert(!qdict_haskey(args, "uri"));
- qdict_put_str(args, "uri", uri);
+ if (uri) {
+ g_assert(!qdict_haskey(args, "uri"));
+ qdict_put_str(args, "uri", uri);
+ }
+
+ if (channels) {
+ g_assert(!qdict_haskey(args, "channels"));
+ QList *channelList = MigrationChannelList_to_QList(channels);
+ qdict_put_obj(args, "channels", QOBJECT(channelList));
+ }
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..52243bd2df 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -14,6 +14,7 @@
#define MIGRATION_HELPERS_H
#include "libqtest.h"
+#include "migration/migration.h"
typedef struct QTestMigrationState {
bool stop_seen;
@@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp(QTestState *who, const char *uri,
+ MigrationChannelList *channels, const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ MigrationChannelList *channels, const char *fmt, ...);
void migrate_set_capability(QTestState *who, const char *capability,
bool value);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 8a5bb1752e..e7f2719dcf 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -18,6 +18,7 @@
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/range.h"
+#include "migration/migration.h"
#include "qemu/sockets.h"
#include "chardev/char.h"
#include "qapi/qapi-visit-sockets.h"
@@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_suspend(from, &src_state);
g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
- migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
+ migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
- migrate_qmp(from, uri, "{'resume': true}");
+ migrate_qmp(from, uri, NULL, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1670,7 @@ static void test_baddest(void)
if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
return;
}
- migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
+ migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1708,7 +1709,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat > %s", file);
migrate_ensure_converge(from);
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
wait_for_migration_complete(from);
pid = fork();
@@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
- migrate_qmp_fail(from, connect_uri, "{}");
+ migrate_qmp_fail(from, connect_uri, NULL, "{}");
goto finish;
}
- migrate_qmp(from, connect_uri, "{}");
+ migrate_qmp(from, connect_uri, NULL, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1869,11 +1870,11 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
}
if (args->result == MIG_TEST_QMP_ERROR) {
- migrate_qmp_fail(from, connect_uri, "{}");
+ migrate_qmp_fail(from, connect_uri, NULL, "{}");
goto finish;
}
- migrate_qmp(from, connect_uri, "{}");
+ migrate_qmp(from, connect_uri, NULL, "{}");
wait_for_migration_complete(from);
/*
@@ -2029,7 +2030,7 @@ static void test_ignore_shared(void)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
if (should_fail) {
qtest_set_expected_status(to, EXIT_FAILURE);
@@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
/* Wait for throttling begins */
percentage = 0;
@@ -2869,7 +2870,7 @@ static void test_multifd_tcp_cancel(void)
uri = migrate_get_socket_address(to, "socket-address");
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void)
migrate_ensure_non_converge(from);
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
migrate_wait_for_dirty_mem(from, to2);
@@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void)
migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
/* Start migrate */
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
/* Wait for dirty limit throttle begin */
throttle_us_per_full = 0;
@@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void)
}
/* Start migrate */
- migrate_qmp(from, uri, "{}");
+ migrate_qmp(from, uri, NULL, "{}");
/* Wait for dirty limit throttle begin */
throttle_us_per_full = 0;
--
2.22.3On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
> Introduce support for adding a 'channels' argument to migrate_qmp_fail
> and migrate_qmp functions within the migration qtest framework, enabling
> enhanced control over migration scenarios.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> tests/qtest/dbus-vmstate-test.c | 2 +-
> tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
> tests/qtest/migration-helpers.h | 11 ++--
> tests/qtest/migration-test.c | 33 ++++++------
> 4 files changed, 112 insertions(+), 27 deletions(-)
>
> diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
> index 6c990864e3..0ca572e29b 100644
> --- a/tests/qtest/dbus-vmstate-test.c
> +++ b/tests/qtest/dbus-vmstate-test.c
> @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
>
> thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
>
> - migrate_qmp(src_qemu, uri, "{}");
> + migrate_qmp(src_qemu, uri, NULL, "{}");
> test->src_qemu = src_qemu;
> if (test->migrate_fail) {
> wait_for_migration_fail(src_qemu, true);
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index e451dbdbed..d153677887 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -13,6 +13,7 @@
> #include "qemu/osdep.h"
> #include "qemu/ctype.h"
> #include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qlist.h"
>
> #include "migration-helpers.h"
>
> @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
> return false;
> }
>
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> +static char *socketAddressType_to_str(SocketAddressType type)
> +{
> + switch (type) {
> + case SOCKET_ADDRESS_TYPE_INET:
> + return g_strdup("inet");
> + case SOCKET_ADDRESS_TYPE_UNIX:
> + return g_strdup("unix");
> + case SOCKET_ADDRESS_TYPE_FD:
> + return g_strdup("fd");
> + case SOCKET_ADDRESS_TYPE_VSOCK:
> + return g_strdup("vsock");
> + default:
> + return g_strdup("unknown address type");
> + }
> +}
Use SocketAddressType_lookup?
> +
> +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
> +{
> + MigrationChannel *channel = NULL;
> + MigrationAddress *addr = NULL;
> + SocketAddress *saddr = NULL;
> + g_autofree const char *addr_type = NULL;
> + QList *channelList = qlist_new();
> + QDict *channelDict = qdict_new();
> + QDict *addrDict = qdict_new();
> +
> + channel = channels->value;
> + if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
> + fprintf(stderr, "%s: Channel or its type is NULL\n",
> + __func__);
> + }
> + g_assert(channel);
> + if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
> + qdict_put_str(channelDict, "channel-type", g_strdup("main"));
> + }
> +
> + addr = channel->addr;
> + if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
> + fprintf(stderr, "%s: addr or its transport is NULL\n",
> + __func__);
> + }
> + g_assert(addr);
> + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> + qdict_put_str(addrDict, "transport", g_strdup("socket"));
> + }
> +
> + saddr = &addr->u.socket;
> + if (!saddr) {
> + fprintf(stderr, "%s: saddr is NULL\n", __func__);
> + }
> + g_assert(saddr);
> + addr_type = socketAddressType_to_str(saddr->type);
> + qdict_put_str(addrDict, "type", addr_type);
> + qdict_put_str(addrDict, "port", saddr->u.inet.port);
> + qdict_put_str(addrDict, "host", saddr->u.inet.host);
> +
> + qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
> + qlist_append_obj(channelList, QOBJECT(channelDict));
> +
> + return channelList;
> +}
> +
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...)
> {
> va_list ap;
> QDict *args, *err;
> @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> args = qdict_from_vjsonf_nofail(fmt, ap);
> va_end(ap);
>
> - g_assert(!qdict_haskey(args, "uri"));
> - qdict_put_str(args, "uri", uri);
> + if (uri) {
> + g_assert(!qdict_haskey(args, "uri"));
IMHO we don't need to assert here?
Rather than doing this, we can also have tests covering when both are set,
or when none are set, to make sure we fail properly in those wrong cases.
> + qdict_put_str(args, "uri", uri);
> + }
> +
> + if (channels) {
> + g_assert(!qdict_haskey(args, "channels"));
> + QList *channelList = MigrationChannelList_to_QList(channels);
> + qdict_put_obj(args, "channels", QOBJECT(channelList));
> + }
>
> err = qtest_qmp_assert_failure_ref(
> who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> * Arguments are built from @fmt... (formatted like
> * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
> */
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> +void migrate_qmp(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...)
> {
> va_list ap;
> QDict *args;
> @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> args = qdict_from_vjsonf_nofail(fmt, ap);
> va_end(ap);
>
> - g_assert(!qdict_haskey(args, "uri"));
> - qdict_put_str(args, "uri", uri);
> + if (uri) {
> + g_assert(!qdict_haskey(args, "uri"));
> + qdict_put_str(args, "uri", uri);
> + }
> +
> + if (channels) {
> + g_assert(!qdict_haskey(args, "channels"));
> + QList *channelList = MigrationChannelList_to_QList(channels);
> + qdict_put_obj(args, "channels", QOBJECT(channelList));
> + }
Duplicated chunks; sign of adding some helper?
>
> qtest_qmp_assert_success(who,
> "{ 'execute': 'migrate', 'arguments': %p}", args);
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 3bf7ded1b9..52243bd2df 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -14,6 +14,7 @@
> #define MIGRATION_HELPERS_H
>
> #include "libqtest.h"
> +#include "migration/migration.h"
>
> typedef struct QTestMigrationState {
> bool stop_seen;
> @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
> bool migrate_watch_for_events(QTestState *who, const char *name,
> QDict *event, void *opaque);
>
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...);
>
> G_GNUC_PRINTF(3, 4)
> void migrate_incoming_qmp(QTestState *who, const char *uri,
> const char *fmt, ...);
>
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...);
>
> void migrate_set_capability(QTestState *who, const char *capability,
> bool value);
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 8a5bb1752e..e7f2719dcf 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -18,6 +18,7 @@
> #include "qemu/module.h"
> #include "qemu/option.h"
> #include "qemu/range.h"
> +#include "migration/migration.h"
> #include "qemu/sockets.h"
> #include "chardev/char.h"
> #include "qapi/qapi-visit-sockets.h"
> @@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
> wait_for_suspend(from, &src_state);
>
> g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to);
>
> @@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to)
> g_assert_cmpint(ret, ==, 1);
>
> migrate_recover(to, "fd:fd-mig");
> - migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
> + migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}");
>
> /*
> * Make sure both QEMU instances will go into RECOVER stage, then test
> @@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args)
> * Try to rebuild the migration channel using the resume flag and
> * the newly created channel
> */
> - migrate_qmp(from, uri, "{'resume': true}");
> + migrate_qmp(from, uri, NULL, "{'resume': true}");
>
> /* Restore the postcopy bandwidth to unlimited */
> migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
> @@ -1669,7 +1670,7 @@ static void test_baddest(void)
> if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
> return;
> }
> - migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
> + migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}");
> wait_for_migration_fail(from, false);
> test_migrate_end(from, to, false);
> }
> @@ -1708,7 +1709,7 @@ static void test_analyze_script(void)
> uri = g_strdup_printf("exec:cat > %s", file);
>
> migrate_ensure_converge(from);
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
> wait_for_migration_complete(from);
>
> pid = fork();
> @@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args)
> }
>
> if (args->result == MIG_TEST_QMP_ERROR) {
> - migrate_qmp_fail(from, connect_uri, "{}");
> + migrate_qmp_fail(from, connect_uri, NULL, "{}");
> goto finish;
> }
>
> - migrate_qmp(from, connect_uri, "{}");
> + migrate_qmp(from, connect_uri, NULL, "{}");
>
> if (args->result != MIG_TEST_SUCCEED) {
> bool allow_active = args->result == MIG_TEST_FAIL;
> @@ -1869,11 +1870,11 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
> }
>
> if (args->result == MIG_TEST_QMP_ERROR) {
> - migrate_qmp_fail(from, connect_uri, "{}");
> + migrate_qmp_fail(from, connect_uri, NULL, "{}");
> goto finish;
> }
>
> - migrate_qmp(from, connect_uri, "{}");
> + migrate_qmp(from, connect_uri, NULL, "{}");
> wait_for_migration_complete(from);
>
> /*
> @@ -2029,7 +2030,7 @@ static void test_ignore_shared(void)
> /* Wait for the first serial output from the source */
> wait_for_serial("src_serial");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to);
>
> @@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
> /* Wait for the first serial output from the source */
> wait_for_serial("src_serial");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> if (should_fail) {
> qtest_set_expected_status(to, EXIT_FAILURE);
> @@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void)
> /* Wait for the first serial output from the source */
> wait_for_serial("src_serial");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> /* Wait for throttling begins */
> percentage = 0;
> @@ -2869,7 +2870,7 @@ static void test_multifd_tcp_cancel(void)
>
> uri = migrate_get_socket_address(to, "socket-address");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to);
>
> @@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void)
>
> migrate_ensure_non_converge(from);
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to2);
>
> @@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void)
> migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
>
> /* Start migrate */
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> /* Wait for dirty limit throttle begin */
> throttle_us_per_full = 0;
> @@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void)
> }
>
> /* Start migrate */
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> /* Wait for dirty limit throttle begin */
> throttle_us_per_full = 0;
> --
> 2.22.3
>
--
Peter Xu
From: Peter Xu <peterx@redhat.com>
Date: Tuesday, 20 February 2024 at 11:33 AM
To: Het Gala <het.gala@nutanix.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, armbru@redhat.com <armbru@redhat.com>, berrange@redhat.com <berrange@redhat.com>, farosas@suse.de <farosas@suse.de>
Subject: Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
> Introduce support for adding a 'channels' argument to migrate_qmp_fail
> and migrate_qmp functions within the migration qtest framework, enabling
> enhanced control over migration scenarios.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> tests/qtest/dbus-vmstate-test.c | 2 +-
> tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
> tests/qtest/migration-helpers.h | 11 ++--
> tests/qtest/migration-test.c | 33 ++++++------
> 4 files changed, 112 insertions(+), 27 deletions(-)
>
> diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
> index 6c990864e3..0ca572e29b 100644
> --- a/tests/qtest/dbus-vmstate-test.c
> +++ b/tests/qtest/dbus-vmstate-test.c
> @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
>
> thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
>
> - migrate_qmp(src_qemu, uri, "{}");
> + migrate_qmp(src_qemu, uri, NULL, "{}");
> test->src_qemu = src_qemu;
> if (test->migrate_fail) {
> wait_for_migration_fail(src_qemu, true);
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index e451dbdbed..d153677887 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -13,6 +13,7 @@
> #include "qemu/osdep.h"
> #include "qemu/ctype.h"
> #include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qlist.h"
>
> #include "migration-helpers.h"
>
> @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
> return false;
> }
>
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> +static char *socketAddressType_to_str(SocketAddressType type)
> +{
> + switch (type) {
> + case SOCKET_ADDRESS_TYPE_INET:
> + return g_strdup("inet");
> + case SOCKET_ADDRESS_TYPE_UNIX:
> + return g_strdup("unix");
> + case SOCKET_ADDRESS_TYPE_FD:
> + return g_strdup("fd");
> + case SOCKET_ADDRESS_TYPE_VSOCK:
> + return g_strdup("vsock");
> + default:
> + return g_strdup("unknown address type");
> + }
> +}
Use SocketAddressType_lookup?
Ack, I guess combination of using qapi_enum_parse() and qapi_enum_lookup() would help me eliminate the need for creating this function itself. Let me do the changes in the next patch. Thanks!
> +
> +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
> +{
> + MigrationChannel *channel = NULL;
> + MigrationAddress *addr = NULL;
> + SocketAddress *saddr = NULL;
> + g_autofree const char *addr_type = NULL;
> + QList *channelList = qlist_new();
> + QDict *channelDict = qdict_new();
> + QDict *addrDict = qdict_new();
> +
> + channel = channels->value;
> + if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
> + fprintf(stderr, "%s: Channel or its type is NULL\n",
> + __func__);
> + }
> + g_assert(channel);
> + if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
> + qdict_put_str(channelDict, "channel-type", g_strdup("main"));
> + }
> +
> + addr = channel->addr;
> + if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
> + fprintf(stderr, "%s: addr or its transport is NULL\n",
> + __func__);
> + }
> + g_assert(addr);
> + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> + qdict_put_str(addrDict, "transport", g_strdup("socket"));
> + }
> +
> + saddr = &addr->u.socket;
> + if (!saddr) {
> + fprintf(stderr, "%s: saddr is NULL\n", __func__);
> + }
> + g_assert(saddr);
> + addr_type = socketAddressType_to_str(saddr->type);
> + qdict_put_str(addrDict, "type", addr_type);
> + qdict_put_str(addrDict, "port", saddr->u.inet.port);
> + qdict_put_str(addrDict, "host", saddr->u.inet.host);
> +
> + qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
> + qlist_append_obj(channelList, QOBJECT(channelDict));
> +
> + return channelList;
> +}
> +
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...)
> {
> va_list ap;
> QDict *args, *err;
> @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> args = qdict_from_vjsonf_nofail(fmt, ap);
> va_end(ap);
>
> - g_assert(!qdict_haskey(args, "uri"));
> - qdict_put_str(args, "uri", uri);
> + if (uri) {
> + g_assert(!qdict_haskey(args, "uri"));
IMHO we don't need to assert here?
Rather than doing this, we can also have tests covering when both are set,
or when none are set, to make sure we fail properly in those wrong cases.
(Neglecting your comments here based on Patch 3/3 comment).
> + qdict_put_str(args, "uri", uri);
> + }
> +
> + if (channels) {
> + g_assert(!qdict_haskey(args, "channels"));
> + QList *channelList = MigrationChannelList_to_QList(channels);
> + qdict_put_obj(args, "channels", QOBJECT(channelList));
> + }
>
> err = qtest_qmp_assert_failure_ref(
> who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> * Arguments are built from @fmt... (formatted like
> * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
> */
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> +void migrate_qmp(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...)
> {
> va_list ap;
> QDict *args;
> @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> args = qdict_from_vjsonf_nofail(fmt, ap);
> va_end(ap);
>
> - g_assert(!qdict_haskey(args, "uri"));
> - qdict_put_str(args, "uri", uri);
> + if (uri) {
> + g_assert(!qdict_haskey(args, "uri"));
> + qdict_put_str(args, "uri", uri);
> + }
> +
> + if (channels) {
> + g_assert(!qdict_haskey(args, "channels"));
> + QList *channelList = MigrationChannelList_to_QList(channels);
> + qdict_put_obj(args, "channels", QOBJECT(channelList));
> + }
Duplicated chunks; sign of adding some helper?
I didn’t think of adding a helper function here as it was just 2 lines of code, already calling MigrationChannelList_to_QList() to avoid duplication. Even then if you have something in your mind to create some helper function, please suggest :)
>
> qtest_qmp_assert_success(who,
> "{ 'execute': 'migrate', 'arguments': %p}", args);
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 3bf7ded1b9..52243bd2df 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -14,6 +14,7 @@
> #define MIGRATION_HELPERS_H
>
> #include "libqtest.h"
> +#include "migration/migration.h"
>
> typedef struct QTestMigrationState {
> bool stop_seen;
> @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
> bool migrate_watch_for_events(QTestState *who, const char *name,
> QDict *event, void *opaque);
>
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...);
>
> G_GNUC_PRINTF(3, 4)
> void migrate_incoming_qmp(QTestState *who, const char *uri,
> const char *fmt, ...);
Just thinking, should also add ‘channels’ argument here I guess, would be helpful in future to add some tests where only ‘channels’ parameter wants to be added to the function ?
>
> -G_GNUC_PRINTF(3, 4)
> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
> +G_GNUC_PRINTF(4, 5)
> +void migrate_qmp_fail(QTestState *who, const char *uri,
> + MigrationChannelList *channels, const char *fmt, ...);
>
> void migrate_set_capability(QTestState *who, const char *capability,
> bool value);
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 8a5bb1752e..e7f2719dcf 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -18,6 +18,7 @@
> #include "qemu/module.h"
> #include "qemu/option.h"
> #include "qemu/range.h"
> +#include "migration/migration.h"
> #include "qemu/sockets.h"
> #include "chardev/char.h"
> #include "qapi/qapi-visit-sockets.h"
> @@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
> wait_for_suspend(from, &src_state);
>
> g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to);
>
> @@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to)
> g_assert_cmpint(ret, ==, 1);
>
> migrate_recover(to, "fd:fd-mig");
> - migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
> + migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}");
>
> /*
> * Make sure both QEMU instances will go into RECOVER stage, then test
> @@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args)
> * Try to rebuild the migration channel using the resume flag and
> * the newly created channel
> */
> - migrate_qmp(from, uri, "{'resume': true}");
> + migrate_qmp(from, uri, NULL, "{'resume': true}");
>
> /* Restore the postcopy bandwidth to unlimited */
> migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
> @@ -1669,7 +1670,7 @@ static void test_baddest(void)
> if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
> return;
> }
> - migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
> + migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}");
> wait_for_migration_fail(from, false);
> test_migrate_end(from, to, false);
> }
> @@ -1708,7 +1709,7 @@ static void test_analyze_script(void)
> uri = g_strdup_printf("exec:cat > %s", file);
>
> migrate_ensure_converge(from);
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
> wait_for_migration_complete(from);
>
> pid = fork();
> @@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args)
> }
>
> if (args->result == MIG_TEST_QMP_ERROR) {
> - migrate_qmp_fail(from, connect_uri, "{}");
> + migrate_qmp_fail(from, connect_uri, NULL, "{}");
> goto finish;
> }
>
> - migrate_qmp(from, connect_uri, "{}");
> + migrate_qmp(from, connect_uri, NULL, "{}");
>
> if (args->result != MIG_TEST_SUCCEED) {
> bool allow_active = args->result == MIG_TEST_FAIL;
> @@ -1869,11 +1870,11 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
> }
>
> if (args->result == MIG_TEST_QMP_ERROR) {
> - migrate_qmp_fail(from, connect_uri, "{}");
> + migrate_qmp_fail(from, connect_uri, NULL, "{}");
> goto finish;
> }
>
> - migrate_qmp(from, connect_uri, "{}");
> + migrate_qmp(from, connect_uri, NULL, "{}");
> wait_for_migration_complete(from);
>
> /*
> @@ -2029,7 +2030,7 @@ static void test_ignore_shared(void)
> /* Wait for the first serial output from the source */
> wait_for_serial("src_serial");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to);
>
> @@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
> /* Wait for the first serial output from the source */
> wait_for_serial("src_serial");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> if (should_fail) {
> qtest_set_expected_status(to, EXIT_FAILURE);
> @@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void)
> /* Wait for the first serial output from the source */
> wait_for_serial("src_serial");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> /* Wait for throttling begins */
> percentage = 0;
> @@ -2869,7 +2870,7 @@ static void test_multifd_tcp_cancel(void)
>
> uri = migrate_get_socket_address(to, "socket-address");
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to);
>
> @@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void)
>
> migrate_ensure_non_converge(from);
>
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> migrate_wait_for_dirty_mem(from, to2);
>
> @@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void)
> migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
>
> /* Start migrate */
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> /* Wait for dirty limit throttle begin */
> throttle_us_per_full = 0;
> @@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void)
> }
>
> /* Start migrate */
> - migrate_qmp(from, uri, "{}");
> + migrate_qmp(from, uri, NULL, "{}");
>
> /* Wait for dirty limit throttle begin */
> throttle_us_per_full = 0;
> --
> 2.22.3
>
--
Peter Xu
Regards,
Het Gala
On Tue, Feb 20, 2024 at 06:14:46PM +0000, Het Gala wrote:
>
>
> From: Peter Xu <peterx@redhat.com>
> Date: Tuesday, 20 February 2024 at 11:33 AM
> To: Het Gala <het.gala@nutanix.com>
> Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, armbru@redhat.com <armbru@redhat.com>, berrange@redhat.com <berrange@redhat.com>, farosas@suse.de <farosas@suse.de>
> Subject: Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
> On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
> > Introduce support for adding a 'channels' argument to migrate_qmp_fail
> > and migrate_qmp functions within the migration qtest framework, enabling
> > enhanced control over migration scenarios.
> >
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> > tests/qtest/dbus-vmstate-test.c | 2 +-
> > tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
> > tests/qtest/migration-helpers.h | 11 ++--
> > tests/qtest/migration-test.c | 33 ++++++------
> > 4 files changed, 112 insertions(+), 27 deletions(-)
> >
> > diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
> > index 6c990864e3..0ca572e29b 100644
> > --- a/tests/qtest/dbus-vmstate-test.c
> > +++ b/tests/qtest/dbus-vmstate-test.c
> > @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
> >
> > thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
> >
> > - migrate_qmp(src_qemu, uri, "{}");
> > + migrate_qmp(src_qemu, uri, NULL, "{}");
> > test->src_qemu = src_qemu;
> > if (test->migrate_fail) {
> > wait_for_migration_fail(src_qemu, true);
> > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> > index e451dbdbed..d153677887 100644
> > --- a/tests/qtest/migration-helpers.c
> > +++ b/tests/qtest/migration-helpers.c
> > @@ -13,6 +13,7 @@
> > #include "qemu/osdep.h"
> > #include "qemu/ctype.h"
> > #include "qapi/qmp/qjson.h"
> > +#include "qapi/qmp/qlist.h"
> >
> > #include "migration-helpers.h"
> >
> > @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
> > return false;
> > }
> >
> > -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> > +static char *socketAddressType_to_str(SocketAddressType type)
> > +{
> > + switch (type) {
> > + case SOCKET_ADDRESS_TYPE_INET:
> > + return g_strdup("inet");
> > + case SOCKET_ADDRESS_TYPE_UNIX:
> > + return g_strdup("unix");
> > + case SOCKET_ADDRESS_TYPE_FD:
> > + return g_strdup("fd");
> > + case SOCKET_ADDRESS_TYPE_VSOCK:
> > + return g_strdup("vsock");
> > + default:
> > + return g_strdup("unknown address type");
> > + }
> > +}
>
> Use SocketAddressType_lookup?
> Ack, I guess combination of using qapi_enum_parse() and qapi_enum_lookup() would help me eliminate the need for creating this function itself. Let me do the changes in the next patch. Thanks!
Ah, what I wanted to say was actually SocketAddressType_str.
>
> > +
> > +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
> > +{
> > + MigrationChannel *channel = NULL;
> > + MigrationAddress *addr = NULL;
> > + SocketAddress *saddr = NULL;
> > + g_autofree const char *addr_type = NULL;
> > + QList *channelList = qlist_new();
> > + QDict *channelDict = qdict_new();
> > + QDict *addrDict = qdict_new();
> > +
> > + channel = channels->value;
> > + if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
> > + fprintf(stderr, "%s: Channel or its type is NULL\n",
> > + __func__);
> > + }
> > + g_assert(channel);
> > + if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
> > + qdict_put_str(channelDict, "channel-type", g_strdup("main"));
> > + }
> > +
> > + addr = channel->addr;
> > + if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
> > + fprintf(stderr, "%s: addr or its transport is NULL\n",
> > + __func__);
> > + }
> > + g_assert(addr);
> > + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> > + qdict_put_str(addrDict, "transport", g_strdup("socket"));
> > + }
> > +
> > + saddr = &addr->u.socket;
> > + if (!saddr) {
> > + fprintf(stderr, "%s: saddr is NULL\n", __func__);
> > + }
> > + g_assert(saddr);
> > + addr_type = socketAddressType_to_str(saddr->type);
> > + qdict_put_str(addrDict, "type", addr_type);
> > + qdict_put_str(addrDict, "port", saddr->u.inet.port);
> > + qdict_put_str(addrDict, "host", saddr->u.inet.host);
> > +
> > + qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
> > + qlist_append_obj(channelList, QOBJECT(channelDict));
> > +
> > + return channelList;
> > +}
> > +
> > +void migrate_qmp_fail(QTestState *who, const char *uri,
> > + MigrationChannelList *channels, const char *fmt, ...)
> > {
> > va_list ap;
> > QDict *args, *err;
> > @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> > args = qdict_from_vjsonf_nofail(fmt, ap);
> > va_end(ap);
> >
> > - g_assert(!qdict_haskey(args, "uri"));
> > - qdict_put_str(args, "uri", uri);
> > + if (uri) {
> > + g_assert(!qdict_haskey(args, "uri"));
>
> IMHO we don't need to assert here?
>
> Rather than doing this, we can also have tests covering when both are set,
> or when none are set, to make sure we fail properly in those wrong cases.
> (Neglecting your comments here based on Patch 3/3 comment).
>
> > + qdict_put_str(args, "uri", uri);
> > + }
> > +
> > + if (channels) {
> > + g_assert(!qdict_haskey(args, "channels"));
> > + QList *channelList = MigrationChannelList_to_QList(channels);
> > + qdict_put_obj(args, "channels", QOBJECT(channelList));
> > + }
> >
> > err = qtest_qmp_assert_failure_ref(
> > who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> > @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
> > * Arguments are built from @fmt... (formatted like
> > * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
> > */
> > -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> > +void migrate_qmp(QTestState *who, const char *uri,
> > + MigrationChannelList *channels, const char *fmt, ...)
> > {
> > va_list ap;
> > QDict *args;
> > @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
> > args = qdict_from_vjsonf_nofail(fmt, ap);
> > va_end(ap);
> >
> > - g_assert(!qdict_haskey(args, "uri"));
> > - qdict_put_str(args, "uri", uri);
> > + if (uri) {
> > + g_assert(!qdict_haskey(args, "uri"));
> > + qdict_put_str(args, "uri", uri);
> > + }
> > +
> > + if (channels) {
> > + g_assert(!qdict_haskey(args, "channels"));
> > + QList *channelList = MigrationChannelList_to_QList(channels);
> > + qdict_put_obj(args, "channels", QOBJECT(channelList));
> > + }
>
> Duplicated chunks; sign of adding some helper?
> I didn’t think of adding a helper function here as it was just 2 lines of code, already calling MigrationChannelList_to_QList() to avoid duplication. Even then if you have something in your mind to create some helper function, please suggest :)
migrate_qmp_attach_ports(QDict *args, const char *uri,
MigrationChannelList *channels)
{
if (uri) {
g_assert(!qdict_haskey(args, "uri"));
qdict_put_str(args, "uri", uri);
}
if (channels) {
g_assert(!qdict_haskey(args, "channels"));
QList *channelList = MigrationChannelList_to_QList(channels);
qdict_put_obj(args, "channels", QOBJECT(channelList));
}
}
If you plan to work on migrate_incoming_qmp(), it'll also be reusable
there.
>
> >
> > qtest_qmp_assert_success(who,
> > "{ 'execute': 'migrate', 'arguments': %p}", args);
> > diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> > index 3bf7ded1b9..52243bd2df 100644
> > --- a/tests/qtest/migration-helpers.h
> > +++ b/tests/qtest/migration-helpers.h
> > @@ -14,6 +14,7 @@
> > #define MIGRATION_HELPERS_H
> >
> > #include "libqtest.h"
> > +#include "migration/migration.h"
> >
> > typedef struct QTestMigrationState {
> > bool stop_seen;
> > @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
> > bool migrate_watch_for_events(QTestState *who, const char *name,
> > QDict *event, void *opaque);
> >
> > -G_GNUC_PRINTF(3, 4)
> > -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
> > +G_GNUC_PRINTF(4, 5)
> > +void migrate_qmp(QTestState *who, const char *uri,
> > + MigrationChannelList *channels, const char *fmt, ...);
> >
> > G_GNUC_PRINTF(3, 4)
> > void migrate_incoming_qmp(QTestState *who, const char *uri,
> > const char *fmt, ...);
> Just thinking, should also add ‘channels’ argument here I guess, would be helpful in future to add some tests where only ‘channels’ parameter wants to be added to the function ?
Sounds good.
Thanks,
--
Peter Xu
On 21/02/24 7:54 am, Peter Xu wrote:
> On Tue, Feb 20, 2024 at 06:14:46PM +0000, Het Gala wrote:
>>
>> From: Peter Xu<peterx@redhat.com>
>> Date: Tuesday, 20 February 2024 at 11:33 AM
>> To: Het Gala<het.gala@nutanix.com>
>> Cc:qemu-devel@nongnu.org <qemu-devel@nongnu.org>,armbru@redhat.com <armbru@redhat.com>,berrange@redhat.com <berrange@redhat.com>,farosas@suse.de <farosas@suse.de>
>> Subject: Re: [PATCH 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
>> On Fri, Feb 16, 2024 at 09:06:22AM +0000, Het Gala wrote:
>>> Introduce support for adding a 'channels' argument to migrate_qmp_fail
>>> and migrate_qmp functions within the migration qtest framework, enabling
>>> enhanced control over migration scenarios.
>>>
>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>> ---
>>> tests/qtest/dbus-vmstate-test.c | 2 +-
>>> tests/qtest/migration-helpers.c | 93 ++++++++++++++++++++++++++++++---
>>> tests/qtest/migration-helpers.h | 11 ++--
>>> tests/qtest/migration-test.c | 33 ++++++------
>>> 4 files changed, 112 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c
>>> index 6c990864e3..0ca572e29b 100644
>>> --- a/tests/qtest/dbus-vmstate-test.c
>>> +++ b/tests/qtest/dbus-vmstate-test.c
>>> @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test)
>>>
>>> thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop);
>>>
>>> - migrate_qmp(src_qemu, uri, "{}");
>>> + migrate_qmp(src_qemu, uri, NULL, "{}");
>>> test->src_qemu = src_qemu;
>>> if (test->migrate_fail) {
>>> wait_for_migration_fail(src_qemu, true);
>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>> index e451dbdbed..d153677887 100644
>>> --- a/tests/qtest/migration-helpers.c
>>> +++ b/tests/qtest/migration-helpers.c
>>> @@ -13,6 +13,7 @@
>>> #include "qemu/osdep.h"
>>> #include "qemu/ctype.h"
>>> #include "qapi/qmp/qjson.h"
>>> +#include "qapi/qmp/qlist.h"
>>>
>>> #include "migration-helpers.h"
>>>
>>> @@ -43,7 +44,70 @@ bool migrate_watch_for_events(QTestState *who, const char *name,
>>> return false;
>>> }
>>>
>>> -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>>> +static char *socketAddressType_to_str(SocketAddressType type)
>>> +{
>>> + switch (type) {
>>> + case SOCKET_ADDRESS_TYPE_INET:
>>> + return g_strdup("inet");
>>> + case SOCKET_ADDRESS_TYPE_UNIX:
>>> + return g_strdup("unix");
>>> + case SOCKET_ADDRESS_TYPE_FD:
>>> + return g_strdup("fd");
>>> + case SOCKET_ADDRESS_TYPE_VSOCK:
>>> + return g_strdup("vsock");
>>> + default:
>>> + return g_strdup("unknown address type");
>>> + }
>>> +}
>> Use SocketAddressType_lookup?
>> Ack, I guess combination of using qapi_enum_parse() and qapi_enum_lookup() would help me eliminate the need for creating this function itself. Let me do the changes in the next patch. Thanks!
> Ah, what I wanted to say was actually SocketAddressType_str.
Oh okay, got it !
>>> +
>>> +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels)
>>> +{
>>> + MigrationChannel *channel = NULL;
>>> + MigrationAddress *addr = NULL;
>>> + SocketAddress *saddr = NULL;
>>> + g_autofree const char *addr_type = NULL;
>>> + QList *channelList = qlist_new();
>>> + QDict *channelDict = qdict_new();
>>> + QDict *addrDict = qdict_new();
>>> +
>>> + channel = channels->value;
>>> + if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) {
>>> + fprintf(stderr, "%s: Channel or its type is NULL\n",
>>> + __func__);
>>> + }
>>> + g_assert(channel);
>>> + if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) {
>>> + qdict_put_str(channelDict, "channel-type", g_strdup("main"));
>>> + }
>>> +
>>> + addr = channel->addr;
>>> + if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) {
>>> + fprintf(stderr, "%s: addr or its transport is NULL\n",
>>> + __func__);
>>> + }
>>> + g_assert(addr);
>>> + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>>> + qdict_put_str(addrDict, "transport", g_strdup("socket"));
>>> + }
>>> +
>>> + saddr = &addr->u.socket;
>>> + if (!saddr) {
>>> + fprintf(stderr, "%s: saddr is NULL\n", __func__);
>>> + }
>>> + g_assert(saddr);
>>> + addr_type = socketAddressType_to_str(saddr->type);
>>> + qdict_put_str(addrDict, "type", addr_type);
>>> + qdict_put_str(addrDict, "port", saddr->u.inet.port);
>>> + qdict_put_str(addrDict, "host", saddr->u.inet.host);
>>> +
>>> + qdict_put_obj(channelDict, "addr", QOBJECT(addrDict));
>>> + qlist_append_obj(channelList, QOBJECT(channelDict));
>>> +
>>> + return channelList;
>>> +}
>>> +
>>> +void migrate_qmp_fail(QTestState *who, const char *uri,
>>> + MigrationChannelList *channels, const char *fmt, ...)
>>> {
>>> va_list ap;
>>> QDict *args, *err;
>>> @@ -52,8 +116,16 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>>> args = qdict_from_vjsonf_nofail(fmt, ap);
>>> va_end(ap);
>>>
>>> - g_assert(!qdict_haskey(args, "uri"));
>>> - qdict_put_str(args, "uri", uri);
>>> + if (uri) {
>>> + g_assert(!qdict_haskey(args, "uri"));
>> IMHO we don't need to assert here?
>>
>> Rather than doing this, we can also have tests covering when both are set,
>> or when none are set, to make sure we fail properly in those wrong cases.
>> (Neglecting your comments here based on Patch 3/3 comment).
>>
>>> + qdict_put_str(args, "uri", uri);
>>> + }
>>> +
>>> + if (channels) {
>>> + g_assert(!qdict_haskey(args, "channels"));
>>> + QList *channelList = MigrationChannelList_to_QList(channels);
>>> + qdict_put_obj(args, "channels", QOBJECT(channelList));
>>> + }
>>>
>>> err = qtest_qmp_assert_failure_ref(
>>> who, "{ 'execute': 'migrate', 'arguments': %p}", args);
>>> @@ -68,7 +140,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
>>> * Arguments are built from @fmt... (formatted like
>>> * qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
>>> */
>>> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
>>> +void migrate_qmp(QTestState *who, const char *uri,
>>> + MigrationChannelList *channels, const char *fmt, ...)
>>> {
>>> va_list ap;
>>> QDict *args;
>>> @@ -77,8 +150,16 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
>>> args = qdict_from_vjsonf_nofail(fmt, ap);
>>> va_end(ap);
>>>
>>> - g_assert(!qdict_haskey(args, "uri"));
>>> - qdict_put_str(args, "uri", uri);
>>> + if (uri) {
>>> + g_assert(!qdict_haskey(args, "uri"));
>>> + qdict_put_str(args, "uri", uri);
>>> + }
>>> +
>>> + if (channels) {
>>> + g_assert(!qdict_haskey(args, "channels"));
>>> + QList *channelList = MigrationChannelList_to_QList(channels);
>>> + qdict_put_obj(args, "channels", QOBJECT(channelList));
>>> + }
>> Duplicated chunks; sign of adding some helper?
>> I didn’t think of adding a helper function here as it was just 2 lines of code, already calling MigrationChannelList_to_QList() to avoid duplication. Even then if you have something in your mind to create some helper function, please suggest :)
> migrate_qmp_attach_ports(QDict *args, const char *uri,
> MigrationChannelList *channels)
> {
> if (uri) {
> g_assert(!qdict_haskey(args, "uri"));
> qdict_put_str(args, "uri", uri);
> }
>
> if (channels) {
> g_assert(!qdict_haskey(args, "channels"));
> QList *channelList = MigrationChannelList_to_QList(channels);
> qdict_put_obj(args, "channels", QOBJECT(channelList));
> }
> }
>
> If you plan to work on migrate_incoming_qmp(), it'll also be reusable
> there.
Ack, thanks!
>>> qtest_qmp_assert_success(who,
>>> "{ 'execute': 'migrate', 'arguments': %p}", args);
>>> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
>>> index 3bf7ded1b9..52243bd2df 100644
>>> --- a/tests/qtest/migration-helpers.h
>>> +++ b/tests/qtest/migration-helpers.h
>>> @@ -14,6 +14,7 @@
>>> #define MIGRATION_HELPERS_H
>>>
>>> #include "libqtest.h"
>>> +#include "migration/migration.h"
>>>
>>> typedef struct QTestMigrationState {
>>> bool stop_seen;
>>> @@ -25,15 +26,17 @@ typedef struct QTestMigrationState {
>>> bool migrate_watch_for_events(QTestState *who, const char *name,
>>> QDict *event, void *opaque);
>>>
>>> -G_GNUC_PRINTF(3, 4)
>>> -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
>>> +G_GNUC_PRINTF(4, 5)
>>> +void migrate_qmp(QTestState *who, const char *uri,
>>> + MigrationChannelList *channels, const char *fmt, ...);
>>>
>>> G_GNUC_PRINTF(3, 4)
>>> void migrate_incoming_qmp(QTestState *who, const char *uri,
>>> const char *fmt, ...);
>> Just thinking, should also add ‘channels’ argument here I guess, would be helpful in future to add some tests where only ‘channels’ parameter wants to be added to the function ?
> Sounds good.
>
> Thanks,
Regards,
Het Gala
© 2016 - 2024 Red Hat, Inc.