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

[Het Gala]

On 20/03/24 12:33 am, Fabiano Rosas wrote:

> Het Gala <het.gala@nutanix.com> writes:
>
> > 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.
> > > > > >
> > > > > > -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'
> >       
> For now, there's nothing else. Let's just ignore when socket-address is
> missing in the reply so we don't break future tests that use a
> non-socket type.
>     
    Okay, Done. Can find the build here:
      https://gitlab.com/galahet/Qemu/-/pipelines/1219841944

> > > > > 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.
> >       
> You could add the parameter to migrate_incoming_qmp and use NULL when
> calling. The callbacks don't need to be changed. When we add more tests
> then we'd alter the callbacks accordingly.
>
> I might convert the file tests soon, you can leave that part to me if
> you want.
>     
    Okay, sure. Will leave those changes to you.
> > > > 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 */
> > > > > >
> > > > > >
> > > > > >               
> > > > >             
> > > >           
> > >         
> >       
>     
    Regards,
      Het Gala