Re: [Qemu-devel] [PATCH v5 2/5] migration: Set the migration tcp port

[Peter Xu]

On Mon, Jan 29, 2018 at 01:17:51PM +0100, Juan Quintela wrote:
> We can set the port parameter as zero.  This patch lets us know what
> port the system was choosen for us.  Now we can migrate to this place.
> 
> Signed-off-by: Juan Quintela <address@hidden>
> 
> --
> 
> This was migrate_set_uri(), but as we only need the tcp_port, change
> to that one.
> ---
>  migration/migration.c | 10 ++++++++++
>  migration/migration.h |  2 ++
>  migration/socket.c    | 35 ++++++++++++++++++++++++++++++-----
>  3 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index eb6958dcda..53818a87af 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -245,6 +245,16 @@ void migrate_send_rp_req_pages(MigrationIncomingState 
> *mis, const char *rbname,
>      }
>  }
>  
> +void migrate_set_port(const uint16_t port, Error **errp)
> +{
> +    MigrateSetParameters p = {
> +        .has_x_tcp_port = true,
> +        .x_tcp_port = port,
> +    };
> +
> +    qmp_migrate_set_parameters(&p, errp);

If we are calling qmp_migrate_set_parameters() here, does it mean that
user can also set this parameter via QMP?

> +}
> +
>  void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p;
> diff --git a/migration/migration.h b/migration/migration.h
> index d3b214e5ba..7c9ac3f00b 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -232,4 +232,6 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
>  void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* 
> rbname,
>                                ram_addr_t start, size_t len);
>  
> +void migrate_set_port(const uint16_t port, Error **errp);
> +
>  #endif
> diff --git a/migration/socket.c b/migration/socket.c
> index e090097077..08606c665d 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
> @@ -160,17 +161,24 @@ out:
>  }
>  
>  
> -static void socket_start_incoming_migration(SocketAddress *saddr,
> -                                            Error **errp)
> +static SocketAddress *socket_start_incoming_migration(SocketAddress *saddr,
> +                                                      Error **errp)
>  {
>      QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> +    SocketAddress *address;
>  
>      qio_channel_set_name(QIO_CHANNEL(listen_ioc),
>                           "migration-socket-listener");
>  
>      if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
>          object_unref(OBJECT(listen_ioc));
> -        return;
> +        return NULL;
> +    }
> +
> +    address = qio_channel_socket_get_local_address(listen_ioc, errp);

[1]

> +    if (address < 0) {
> +        object_unref(OBJECT(listen_ioc));
> +        return NULL;
>      }
>  
>      qio_channel_add_watch(QIO_CHANNEL(listen_ioc),

[2]

> @@ -178,14 +186,28 @@ static void 
> socket_start_incoming_migration(SocketAddress *saddr,
>                            socket_accept_incoming_migration,
>                            listen_ioc,
>                            (GDestroyNotify)object_unref);
> +    return address;
>  }
>  
>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
>  {
>      Error *err = NULL;
>      SocketAddress *saddr = tcp_build_address(host_port, &err);
> +
>      if (!err) {
> -        socket_start_incoming_migration(saddr, &err);
> +        SocketAddress *address = socket_start_incoming_migration(saddr, 
> &err);
> +
> +        if (address) {
> +            unsigned long long port;
> +
> +            if (parse_uint_full(address->u.inet.port, &port, 10) < 0) {
> +                error_setg(errp, "error parsing port in '%s'",
> +                           address->u.inet.port);
> +            } else {
> +                migrate_set_port(port, errp);
> +            }

If *errp is non-NULL when reach here (I don't know when this will
happen, though), would there be a dangling incoming task in main
thread (which was created during socket_start_incoming_migration at
[2])?

Not sure whether it can be fixed by moving the set port logic to [1]
above.  Then the watch won't be created only if set port succeeded.

Thanks,

> +            qapi_free_SocketAddress(address);
> +        }
>      }
>      qapi_free_SocketAddress(saddr);
>      error_propagate(errp, err);
> @@ -194,6 +216,9 @@ void tcp_start_incoming_migration(const char *host_port, 
> Error **errp)
>  void unix_start_incoming_migration(const char *path, Error **errp)
>  {
>      SocketAddress *saddr = unix_build_address(path);
> -    socket_start_incoming_migration(saddr, errp);
> +    SocketAddress *address;
> +
> +    address = socket_start_incoming_migration(saddr, errp);
> +    qapi_free_SocketAddress(address);
>      qapi_free_SocketAddress(saddr);
>  }
> -- 
> 2.14.3
> 

-- 
Peter Xu