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:

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