On Tue, Feb 20, 2024 at 06:14:46PM +0000, Het Gala wrote:
>
>
> From: Peter Xu <pet...@redhat.com>
> Date: Tuesday, 20 February 2024 at 11:33 AM
> To: Het Gala <het.g...@nutanix.com>
> Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, arm...@redhat.com
> <arm...@redhat.com>, berra...@redhat.com <berra...@redhat.com>,
> faro...@suse.de <faro...@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.g...@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
