Re: [Qemu-devel] [RFC PATCH RDMA support v1: 4/5] connection-setup code between client/server

[Orit Wasserman]

Hi Michael,
It will be cleaner to create a new file migration-rdma.c and put all the
RDMA logic into it, this way you won't effect TCP migration code.

More comments inline

On 01/29/2013 12:01 AM, address@hidden wrote:
> From: "Michael R. Hines" <address@hidden>
> 
> 
> Signed-off-by: Michael R. Hines <address@hidden>
> ---
>  migration-tcp.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  migration.c     |   41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/migration-tcp.c b/migration-tcp.c
> index e78a296..b6c53ba 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -14,10 +14,12 @@
>   */
>  
>  #include "qemu-common.h"
> +#include "qemu/rdma.h"
>  #include "qemu/sockets.h"
>  #include "migration/migration.h"
>  #include "migration/qemu-file.h"
>  #include "block/block.h"
> +#include <poll.h>
>  
>  //#define DEBUG_MIGRATION_TCP
>  
> @@ -55,6 +57,9 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>  
>      if (fd < 0) {
>          DPRINTF("migrate connect error\n");
> +        if (qemu_use_rdma_migration()) {
> +            qemu_rdma_migration_cleanup(&rdma_mdata);
> +        }
>          s->fd = -1;
>          migrate_fd_error(s);
>      } else {
> @@ -62,6 +67,25 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>          s->fd = fd;
>          socket_set_block(s->fd);
>          migrate_fd_connect(s);
> +
> +        /* RDMA initailization */
> +        if (qemu_use_rdma_migration()) {
> +            if (qemu_rdma_migration_client_init(&rdma_mdata)) {
> +                migrate_fd_error(s);
> +                return;
> +            }
> +
> +            fprintf(stderr, "qemu_rdma_migration_client_init success\n");

Please use DPRINTF or trace for debug.
> +
> +            if (qemu_rdma_migration_client_connect(&rdma_mdata)) {
> +                migrate_fd_error(s);
> +                close(s->fd);
> +                return;
> +            }
> +
> +            fprintf(stderr, "qemu_rdma_migration_client_connect success\n");
> +        }
> +
>      }
>  }
>  
> @@ -101,6 +125,16 @@ static void tcp_accept_incoming_migration(void *opaque)
>          goto out;
>      }
>  
> +    if (qemu_use_rdma_migration()) {
> +        printf("listen on port started: %d\n", rdma_mdata.port);
> +
> +        if (qemu_rdma_migration_server_wait_for_client(&rdma_mdata)) {
> +            fprintf(stderr, "rdma migration: error waiting for client!\n");
> +            close(s);
> +            return;
> +        }
> +    }
> +
>      process_incoming_migration(f);
>      return;
>  
> @@ -117,6 +151,25 @@ void tcp_start_incoming_migration(const char *host_port, 
> Error **errp)
>          return;
>      }
>  
> +    if (qemu_use_rdma_migration()) {
> +        int ret;
> +
> +        ret = qemu_rdma_migration_server_init(&rdma_mdata);
> +        if (ret) {
> +            fprintf(stderr, "rdma migration: error init server!\n");

Please set errp in case of an error, I would suggest passing it into 
qemu_rdma_migration_server_init
that will set the relevant error.       
> +            close(s);
> +            return;
> +        }
> +
> +        ret = qemu_rdma_migration_server_prepare(&rdma_mdata);
> +        if (ret) {
> +            fprintf(stderr, "rdma migration: error preparing server!\n");
> +            close(s);
> +            return;
> +        }
> +
> +    }
> +
>      qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
>                           (void *)(intptr_t)s);
>  }
> diff --git a/migration.c b/migration.c
> index 77c1971..cffe16f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -22,6 +22,7 @@
>  #include "qemu/sockets.h"
>  #include "migration/block.h"
>  #include "qemu/thread.h"
> +#include "qemu/rdma.h"
>  #include "qmp-commands.h"
>  
>  //#define DEBUG_MIGRATION
> @@ -279,6 +280,11 @@ static int migrate_fd_cleanup(MigrationState *s)
>      }
>  
>      assert(s->fd == -1);
> +
> +    if (qemu_rdma_migration_enabled()) {
> +        qemu_rdma_migration_cleanup(&rdma_mdata);
> +    }
> +
>      return ret;
>  }
>  
> @@ -481,6 +487,41 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
>      return migrate_xbzrle_cache_size();
>  }
>  
> +void qmp_migrate_set_rdma_port(int64_t port, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
> +        return;
> +    }

Please return error here by setting errp.
> +    printf("rdma migration port: %" PRId64 "\n", port);
You forgot a printf here ...
> +    rdma_mdata.port = port;
> +}
> +
> +void qmp_migrate_set_rdma_host(const char *host, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
> +        return;
> +    }
> +    printf("rdma migration host name: %s\n", host);
> +    strncpy(rdma_mdata.host, host, 64);

Use g_strdup here

regards,
Orit
> +    rdma_mdata.host[63] = '\0';

> +}
> +
> +void qmp_migrate_rdma_prepare(Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    if (s && (s->state == MIG_STATE_ACTIVE)) {
> +        return;
> +    }
> +    qemu_rdma_migration_client_init(&rdma_mdata);
> +}
> +
> +void qmp_migrate_rdma_release(Error **errp)
> +{
> +    qemu_rdma_migration_cleanup(&rdma_mdata);
> +}
> +
>  void qmp_migrate_set_speed(int64_t value, Error **errp)
>  {
>      MigrationState *s;
>