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

[Peter Xu <peterx@redhat.com>]

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