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"
+
 
 #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];
+
+    QLIST_FOREACH_ENTRY(channelList, entry) {
+        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+        QObject *addr_obj = qdict_get(channel, "addr");
+
+        if (qobject_type(addr_obj) == QTYPE_QDICT) {
+            QDict *addrdict = qobject_to(QDict, addr_obj);
+            if (qdict_haskey(addrdict, "port") &&
+            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+                qdict_put_str(addrdict, "port", addr_port);
+            }
+        }
+    }
+}
+
 bool migrate_watch_for_events(QTestState *who, const char *name,
                               QDict *event, void *opaque)
 {
@@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
     if (!uri) {
         connect_uri = migrate_get_socket_address(to, "socket-address");
     }
+    migrate_set_ports(to, NULL);
     qdict_put_str(args, "uri", uri ? uri : connect_uri);
 
     qtest_qmp_assert_success(who,
-- 
2.22.3

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.

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

Also what happens if the dst is using unix: or fd:?

> +
> +    QLIST_FOREACH_ENTRY(channelList, entry) {
> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
> +        QObject *addr_obj = qdict_get(channel, "addr");
> +
> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
> +            QDict *addrdict = qobject_to(QDict, addr_obj);

You might not need these two lines if at the start you use:

QDict *addr = qdict_get_dict(channel, "addr");

> +            if (qdict_haskey(addrdict, "port") &&
> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
> +                qdict_put_str(addrdict, "port", addr_port);
> +            }
> +        }
> +    }
> +}
> +
>  bool migrate_watch_for_events(QTestState *who, const char *name,
>                                QDict *event, void *opaque)
>  {
> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>      if (!uri) {
>          connect_uri = migrate_get_socket_address(to, "socket-address");
>      }
> +    migrate_set_ports(to, NULL);

migrate_set_ports is not prepared to take NULL. This breaks the tests in
this commit. All individual commits should work, otherwise it will break
bisecting.

>      qdict_put_str(args, "uri", uri ? uri : connect_uri);
>  
>      qtest_qmp_assert_success(who,

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 ?

> Also what happens if the dst is using unix: or fd:?
yes in that case - how should the migration behaviour be ? src and dst 
should be of the same type right
>> +
>> +    QLIST_FOREACH_ENTRY(channelList, entry) {
>> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>> +        QObject *addr_obj = qdict_get(channel, "addr");
>> +
>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
> You might not need these two lines if at the start you use:
>
> QDict *addr = qdict_get_dict(channel, "addr");

If you are commenting regarding this two lines:

+        if (qobject_type(addr_obj) == QTYPE_QDICT) {
+            QDict *addrdict = qobject_to(QDict, addr_obj);

then, I am not sure, because addrdict and addr is different right? Also addrdict can also
be a QList, like in the case of exec migration, it can be a list instead of dict
ex:
# -> { "execute": "migrate",
#      "arguments": {
#          "channels": [ { "channel-type": "main",
#                          "addr": { "transport": "exec",
#                                    "args": [ "/bin/nc", "-p", "6000",
#                                              "/some/sock" ] } } ] } }

>
>> +            if (qdict_haskey(addrdict, "port") &&
>> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>> +                qdict_put_str(addrdict, "port", addr_port);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>>                                 QDict *event, void *opaque)
>>   {
>> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>       if (!uri) {
>>           connect_uri = migrate_get_socket_address(to, "socket-address");
>>       }
>> +    migrate_set_ports(to, NULL);
> migrate_set_ports is not prepared to take NULL. This breaks the tests in
> this commit. All individual commits should work, otherwise it will break
> bisecting.
Okay, so in that case, is it better to merge this with the next patch ? 
because if I just
define the migrate_set_ports function and not use it anywhere, it gives 
a warning/error
(depending upon what compiler is used)
>>       qdict_put_str(args, "uri", uri ? uri : connect_uri);
>>   
>>       qtest_qmp_assert_success(who,

Regards,

Het Gala

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.

>> Also what happens if the dst is using unix: or fd:?
> yes in that case - how should the migration behaviour be ? src and dst 
> should be of the same type right

Remember this is test code. If the test was written incorrectly either
by developer mistake or on purpose to test some condition, then it's not
this function's reponsibility to fix that.

Replace the port only if the transport type allows for a port, there is
a port in both addr and addrdict and port is 0. Anything else, leave the
channelList untouched and let QEMU deal with the bad input.

>>> +
>>> +    QLIST_FOREACH_ENTRY(channelList, entry) {
>>> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>>> +        QObject *addr_obj = qdict_get(channel, "addr");
>>> +
>>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>> You might not need these two lines if at the start you use:
>>
>> QDict *addr = qdict_get_dict(channel, "addr");
>
> If you are commenting regarding this two lines:
>
> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>
> then, I am not sure, because addrdict and addr is different right? Also addrdict can also
> be a QList, like in the case of exec migration, it can be a list instead of dict
> ex:
> # -> { "execute": "migrate",
> #      "arguments": {
> #          "channels": [ { "channel-type": "main",
> #                          "addr": { "transport": "exec",
> #                                    "args": [ "/bin/nc", "-p", "6000",
> #                                              "/some/sock" ] } } ] } }

"addr" is always a dict, no? Even in the example you just gave.

>
>>
>>> +            if (qdict_haskey(addrdict, "port") &&
>>> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>>> +                qdict_put_str(addrdict, "port", addr_port);
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>> +
>>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>>>                                 QDict *event, void *opaque)
>>>   {
>>> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>       if (!uri) {
>>>           connect_uri = migrate_get_socket_address(to, "socket-address");
>>>       }
>>> +    migrate_set_ports(to, NULL);
>> migrate_set_ports is not prepared to take NULL. This breaks the tests in
>> this commit. All individual commits should work, otherwise it will break
>> bisecting.
> Okay, so in that case, is it better to merge this with the next patch ? 
> because if I just
> define the migrate_set_ports function and not use it anywhere, it gives 
> a warning/error
> (depending upon what compiler is used)

You can return early at migrate_set_ports if channelList is NULL.

Also, I just noticed: s/channelList/channel_list/

>>>       qdict_put_str(args, "uri", uri ? uri : connect_uri);
>>>   
>>>       qtest_qmp_assert_success(who,
>
> Regards,
>
> 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

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

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.
>
>>> Also what happens if the dst is using unix: or fd:?
>> yes in that case - how should the migration behaviour be ? src and dst
>> should be of the same type right
> Remember this is test code. If the test was written incorrectly either
> by developer mistake or on purpose to test some condition, then it's not
> this function's reponsibility to fix that.
>
> Replace the port only if the transport type allows for a port, there is
> a port in both addr and addrdict and port is 0. Anything else, leave the
> channelList untouched and let QEMU deal with the bad input.
>
>>>> +
>>>> +    QLIST_FOREACH_ENTRY(channelList, entry) {
>>>> +        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>>>> +        QObject *addr_obj = qdict_get(channel, "addr");
>>>> +
>>>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>>>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>>> You might not need these two lines if at the start you use:
>>>
>>> QDict *addr = qdict_get_dict(channel, "addr");
>> If you are commenting regarding this two lines:
>>
>> +        if (qobject_type(addr_obj) == QTYPE_QDICT) {
>> +            QDict *addrdict = qobject_to(QDict, addr_obj);
>>
>> then, I am not sure, because addrdict and addr is different right? Also addrdict can also
>> be a QList, like in the case of exec migration, it can be a list instead of dict
>> ex:
>> # -> { "execute": "migrate",
>> #      "arguments": {
>> #          "channels": [ { "channel-type": "main",
>> #                          "addr": { "transport": "exec",
>> #                                    "args": [ "/bin/nc", "-p", "6000",
>> #                                              "/some/sock" ] } } ] } }
> "addr" is always a dict, no? Even in the example you just gave.

Sorry, my apologies. I had args <-> addrdict in back of my mind.
You are correct. Will make the changes in next patchset.

>>>> +            if (qdict_haskey(addrdict, "port") &&
>>>> +            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>>>> +                qdict_put_str(addrdict, "port", addr_port);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>>    bool migrate_watch_for_events(QTestState *who, const char *name,
>>>>                                  QDict *event, void *opaque)
>>>>    {
>>>> @@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>>        if (!uri) {
>>>>            connect_uri = migrate_get_socket_address(to, "socket-address");
>>>>        }
>>>> +    migrate_set_ports(to, NULL);
>>> migrate_set_ports is not prepared to take NULL. This breaks the tests in
>>> this commit. All individual commits should work, otherwise it will break
>>> bisecting.
>> Okay, so in that case, is it better to merge this with the next patch ?
>> because if I just
>> define the migrate_set_ports function and not use it anywhere, it gives
>> a warning/error
>> (depending upon what compiler is used)
> You can return early at migrate_set_ports if channelList is NULL.
>
> Also, I just noticed: s/channelList/channel_list/
Ack. Thanks


Regards,

Het Gala