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

[Fabiano Rosas <farosas@suse.de>]

Het Gala <het.gala@nutanix.com> writes:

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

exec because we intend to deprecate it, so no one is paying too much
attention to it.

rdma because no one wants to write them.

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

The latter. It's easier to work with.

static QDict *SocketAddress_to_qdict(SocketAddress *addr)
{
    QDict *dict = qdict_new();

    switch (addr->type) {
    case SOCKET_ADDRESS_TYPE_INET:
        qdict_put_str(dict, "type", "inet");
        qdict_put_str(dict, "host", addr->u.inet.host);
        qdict_put_str(dict, "port", addr->u.inet.port);

        break;
    case SOCKET_ADDRESS_TYPE_UNIX:
        qdict_put_str(dict, "type", "unix");
        qdict_put_str(dict, "path", addr->u.q_unix.path);

        break;
    case SOCKET_ADDRESS_TYPE_FD:
        qdict_put_str(dict, "type", "fd");
        qdict_put_str(dict, "str", addr->u.fd.str);

        break;
    case SOCKET_ADDRESS_TYPE_VSOCK:
        qdict_put_str(dict, "type", "vsock");
        qdict_put_str(dict, "cid", addr->u.vsock.cid);
        qdict_put_str(dict, "port", addr->u.vsock.port);

        break;
    default:
        g_assert_not_reached();
        break;
    }

    return dict;
}

>
> Regards,
>
> Het Gala