Re: [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work

[Daniel P. Berrange]

On Mon, Mar 13, 2017 at 01:44:27PM +0100, Juan Quintela wrote:
> We create new channels for each new thread created. We only send through
> them a character to be sure that we are creating the channels in the
> right order.
> 
> Signed-off-by: Juan Quintela <address@hidden>
> 
> --
> Split SocketArgs into incoming and outgoing args
> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  include/migration/migration.h |  7 +++++
>  migration/ram.c               | 35 ++++++++++++++++++++++
>  migration/socket.c            | 67 
> +++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 106 insertions(+), 3 deletions(-)
> 

> diff --git a/migration/socket.c b/migration/socket.c
> index 13966f1..58a16b5 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -24,6 +24,65 @@
>  #include "io/channel-socket.h"
>  #include "trace.h"
>  
> +struct SocketIncomingArgs {
> +    QIOChannelSocket *ioc;
> +} incoming_args;
> +
> +QIOChannel *socket_recv_channel_create(void)
> +{
> +    QIOChannelSocket *sioc;
> +    Error *err = NULL;
> +
> +    sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(incoming_args.ioc),
> +                                     &err);
> +    if (!sioc) {
> +        error_report("could not accept migration connection (%s)",
> +                     error_get_pretty(err));
> +        return NULL;
> +    }
> +    return QIO_CHANNEL(sioc);
> +}
> +
> +int socket_recv_channel_destroy(QIOChannel *recv)
> +{
> +    /* Remove channel */
> +    object_unref(OBJECT(send));
> +    return 0;
> +}
> +
> +/* we have created all the recv channels, we can close the main one */
> +int socket_recv_channel_close_listening(void)
> +{
> +    /* Close listening socket as its no longer needed */
> +    qio_channel_close(QIO_CHANNEL(incoming_args.ioc), NULL);
> +    return 0;
> +}
> +
> +struct SocketOutgoingArgs {
> +    SocketAddress *saddr;
> +    Error **errp;
> +} outgoing_args;
> +
> +QIOChannel *socket_send_channel_create(void)
> +{
> +    QIOChannelSocket *sioc = qio_channel_socket_new();
> +
> +    qio_channel_socket_connect_sync(sioc, outgoing_args.saddr,
> +                                    outgoing_args.errp);
> +    qio_channel_set_delay(QIO_CHANNEL(sioc), false);
> +    return QIO_CHANNEL(sioc);
> +}
> +
> +int socket_send_channel_destroy(QIOChannel *send)
> +{
> +    /* Remove channel */
> +    object_unref(OBJECT(send));
> +    if (outgoing_args.saddr) {
> +        qapi_free_SocketAddress(outgoing_args.saddr);
> +        outgoing_args.saddr = NULL;
> +    }
> +    return 0;
> +}
>  
>  static SocketAddress *tcp_build_address(const char *host_port, Error **errp)
>  {
> @@ -97,6 +156,10 @@ static void 
> socket_start_outgoing_migration(MigrationState *s,
>      struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
>  
>      data->s = s;
> +
> +    outgoing_args.saddr = saddr;
> +    outgoing_args.errp = errp;
> +
>      if (saddr->type == SOCKET_ADDRESS_KIND_INET) {
>          data->hostname = g_strdup(saddr->u.inet.data->host);
>      }
> @@ -107,7 +170,6 @@ static void 
> socket_start_outgoing_migration(MigrationState *s,
>                                       socket_outgoing_migration,
>                                       data,
>                                       socket_connect_data_free);
> -    qapi_free_SocketAddress(saddr);
>  }
>  
>  void tcp_start_outgoing_migration(MigrationState *s,
> @@ -154,8 +216,6 @@ static gboolean 
> socket_accept_incoming_migration(QIOChannel *ioc,
>      object_unref(OBJECT(sioc));
>  
>  out:
> -    /* Close listening socket as its no longer needed */
> -    qio_channel_close(ioc, NULL);
>      return FALSE; /* unregister */
>  }
>  
> @@ -164,6 +224,7 @@ static void socket_start_incoming_migration(SocketAddress 
> *saddr,
>                                              Error **errp)
>  {
>      QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> +    incoming_args.ioc = listen_ioc;
>  
>      qio_channel_set_name(QIO_CHANNEL(listen_ioc),
>                           "migration-socket-listener");

I still don't really like any of the changes in this file. We've now got
two sets of methods which connect to a remote host and two sets of methods
which accept incoming clients. I've got to think there's a better way to
refactor the existing code, such that we don't need two sets of methods
for the same actions


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|