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

[Het Gala]

On 29/02/24 6:47 am, Fabiano Rosas wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
> > On 27/02/24 1:04 am, Het Gala wrote:
> > > On 26/02/24 6:31 pm, Fabiano Rosas wrote:
> > > > Het Gala<het.gala@nutanix.com>  writes:
> > > >
> > > > > On 24/02/24 1:42 am, Fabiano Rosas wrote:
> > > > > this was the same first approach that I attempted. It won't work because
> > > > >
> > > > > The final 'migrate' QAPI with channels string would look like
> > > > >
> > > > > { "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
> > > > > "main", "addr": { "transport": "socket", "type": "inet", "host":
> > > > > "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
> > > > >
> > > > > instead of
> > > > >
> > > > > { "execute": "migrate", "arguments": { "channels": [ { "channel-type":
> > > > > "main", "addr": { "transport": "socket", "type": "inet", "host":
> > > > > "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
> > > > >
> > > > > It would complain, that channels should be an *array* and not a string.
> > > > >
> > > > > So, that's the reason parsing was required in qtest too.
> > > > >
> > > > > I would be glad to hear if there are any ideas to convert /string ->
> > > > > json object -> add it inside qdict along with uri/ ?
> > > > Isn't this what the various qobject_from_json do? How does it work with
> > > > the existing tests?
> > > >
> > > >       qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
> > > >                                "  'arguments': { "
> > > >                                "      'channels': [ { 'channel-type': 'main',"
> > > >                                "      'addr': { 'transport': 'socket',"
> > > >                                "                'type': 'inet',"
> > > >                                "                'host': '127.0.0.1',"
> > > >                                "                'port': '0' } } ] } }");
> > > >
> > > > We can pass this^ string successfully to QMP somehow...
> > > I think, here in qtest_qmp_assert_success, we actually can pass the
> > > whole QMP command, and it just asserts that return key is present in
> > > the response, though I am not very much familiar with qtest codebase
> > > to verify how swiftly we can convert string into an actual QObject.
> > >
> > > [...]
> > I tried with qobject_from_json type of utility functions and the error I
> > got was this :
> >
> > migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126:
> > qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type <
> > QTYPE__MAX' failed.
> >
> > And I suppose this was the case because, there are only limited types of
> > QTYPE available
> >
> > typedefenumQType{
> > QTYPE_NONE,
> > QTYPE_QNULL,
> > QTYPE_QNUM,
> > QTYPE_QSTRING,
> > QTYPE_QDICT,
> > QTYPE_QLIST,
> > QTYPE_QBOOL,
> > QTYPE__MAX,
> > } QType;
> >
> > And 'channels' is a mixture of QDICT and QLIST and hence it is not able
> > to easily convert from string to json.
> >
> > Thoughts on this ?
> I'm not sure what you tried. This works:
>
>      g_assert(!qdict_haskey(args, "channels"));
>      if (channels) {
>          channels_obj = qobject_from_json(channels, errp);
>          qdict_put_obj(args, "channels", channels_obj);
>      }

Are you sure the above works ?

Sorry I want to get this doubt cleared first, Let's me take a step back 
and just focus on the above part and not focus on port issue for now.

Adding a validation test for uri and channels both used together

migration_test_add("/migration/validate_uri/channels/both_set",
                   test_validate_uri_channels_both_set);
static void test_validate_uri_channels_both_set(void)
{

   QTestState *from, *to;

   MigrateCommon args = {
        .connect_channels = "'channels': [ { 'channel-type': 'main',"
                            "      'addr': { 'transport': 'socket',"
                            "                'type': 'inet',"
                            "                'host': '127.0.0.1',"
                            "                'port': '0' } } ]",
        .listen_uri = "tcp:127.0.0.1:0",
        .result = MIG_TEST_QMP_ERROR,

    };

    if (test_migrate_start(&from, &to, args.listen_uri, &args.start)) {
        return;
    }

    wait_for_serial("src_serial");

    if (args.result == MIG_TEST_QMP_ERROR) {
        migrate_qmp_fail(from, args.connect_uri, args.connect_channels, 
"{}");
    } else {
        migrate_qmp(from, args.connect_uri, args.connect_channels, "{}");
    }

    test_migrate_end(from, to, false);
}

In the migration-helpers.c file, inside migrate_qmp_fail function:

void migrate_qmp_fail(QTestState *who, const char *uri,
                      const char *channels, const char *fmt, ...)
{
    [...]

    Error **errp = NULL;
    QObject *channels_obj = NULL;

    [...]

    g_assert(!qdict_haskey(args, "channels"));
    if (channels) {
        channels_obj = qobject_from_json(channels, errp);
        if (!channels_obj) {
            fprintf(stdout, "Everything is right till here also ?\n");
            goto cleanup;
        }
        qdict_put_obj(args, "channels", channels_obj);
    }

    err = qtest_qmp_assert_failure_ref(
        who, "{ 'execute': 'migrate', 'arguments': %p}", args);

   [...]

cleanup:
    qobject_unref(channels_obj);
}

When I ran the test alone, test passed, but instead of giving output as

{"error": {"class": "GenericError", "desc": "need either 'uri' or 
'channels' argument"}}

It just passed, i.e. channels_obj for me was NULL and hence just ended 
the test. What wrong am I doing here according to you ? Because, IMO if 
qobject_from_json() is not working properly, this approach of passing 
channels as string might not work at all.


Regards,

Het Gala