Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number

[Het Gala]

On 06/03/24 9:31 pm, Fabiano Rosas wrote:

> Het Gala <het.gala@nutanix.com> writes:
>
> > On 06/03/24 8:06 pm, Fabiano Rosas wrote:
> > > Het Gala<het.gala@nutanix.com>  writes:
> > >
> > > > Add a migrate_set_ports() function that from each QDict, fills in
> > > > the port in case it was 0 in the test.
> > > > Handle a list of channels so we can add a negative test that
> > > > passes more than one channel.
> > > >
> > > > Signed-off-by: Het Gala<het.gala@nutanix.com>
> > > > Suggested-by: Fabiano Rosas<farosas@suse.de>
> > > > ---
> > > >   tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
> > > >   1 file changed, 26 insertions(+)
> > > >
> > > > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> > > > index 478c1f259b..df4978bf17 100644
> > > > --- a/tests/qtest/migration-helpers.c
> > > > +++ b/tests/qtest/migration-helpers.c
> > > > @@ -17,6 +17,8 @@
> > > >   #include "qapi/qapi-visit-sockets.h"
> > > >   #include "qapi/qobject-input-visitor.h"
> > > >   #include "qapi/error.h"
> > > > +#include "qapi/qmp/qlist.h"
> > > > +
> > > >           
> > > Extra line here. This is unwanted because it sometimes trips git into
> > > thinking there's a conflict here when another patch changes the
> > > surrounding lines.
> > >         
> > Ack, that makes sense
> > > >   
> > > >   #include "migration-helpers.h"
> > > >   
> > > > @@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
> > > >       return result;
> > > >   }
> > > >   
> > > > +static void migrate_set_ports(QTestState *to, QList *channelList)
> > > > +{
> > > > +    g_autofree char *addr = NULL;
> > > > +    g_autofree char *addr_port = NULL;
> > > > +    QListEntry *entry;
> > > > +
> > > > +    addr = migrate_get_socket_address(to, "socket-address");
> > > > +    addr_port = g_strsplit(addr, ":", 3)[2];
> > > >           
> > > Will this always do the right thing when the src/dst use different types
> > > of channels? If there is some kind of mismatch (say one side uses vsock
> > > and the other inet), it's better that this function doesn't touch the
> > > channels dict instead of putting garbage in the port field.
> > >         
> > Yes you are right. This will fail if there is a mismatch in type of 
> > channels.
> >
> > Better idea would be to check if 'port' key is present in both, i.e. in 
> > 'addr'
> > as well as 'addrdict' and only then change the port ?
> >
> >       
> Yep, either parse the type from string or add a version of
> migrate_get_socket_address that returns a dict. Then check if type
> matches and port exists.
>

one silly question here, why are we not having tests for exec and rdma specifically ?

Another suggestion required: Parsing uri to qdict is easy to implement but (little)
messy codewise, and the other hand migrate_get_qdict looks clean, but under the hood we would convert it to socketaddress and then call SocketAddress_to_qdict. Which one we can prefer more here ?

Regards,

Het Gala