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

[Het Gala]

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 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.

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