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

[Het Gala]

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