Re: [Qemu-trivial] [Qemu-devel] [PATCH] migration/rdma: clean up qemu_rdma_dest_init a bit

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Michael Tokarev (mjt@tls.msk.ru) wrote:
> Do not check for rdma->host being empty twice.  This removes a large
> "if" block, so code indentation is changed.  While at it, remove an
> ugly goto from the loop, replacing it with a cleaner if logic.  And
> finally, there's no need to initialize `ret' variable since is always
> has a value.

This looks OK; have you got RDMA hardware to test with, if not I can
give it a go.

Dave

> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  migration/rdma.c | 51 ++++++++++++++++++++++-----------------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 6bee30c..76af724 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2366,10 +2366,10 @@ err_rdma_source_connect:
>  
>  static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>  {
> -    int ret = -EINVAL, idx;
> +    int ret, idx;
>      struct rdma_cm_id *listen_id;
>      char ip[40] = "unknown";
> -    struct rdma_addrinfo *res;
> +    struct rdma_addrinfo *res, *e;
>      char port_str[16];
>  
>      for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
> @@ -2377,7 +2377,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>          rdma->wr_data[idx].control_curr = NULL;
>      }
>  
> -    if (rdma->host == NULL) {
> +    if (!rdma->host || !rdma->host[0]) {
>          ERROR(errp, "RDMA host is not set!");
>          rdma->error_state = -EINVAL;
>          return -1;
> @@ -2400,40 +2400,33 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>      snprintf(port_str, 16, "%d", rdma->port);
>      port_str[15] = '\0';
>  
> -    if (rdma->host && strcmp("", rdma->host)) {
> -        struct rdma_addrinfo *e;
> +    ret = rdma_getaddrinfo(rdma->host, port_str, NULL, &res);
> +    if (ret < 0) {
> +        ERROR(errp, "could not rdma_getaddrinfo address %s", rdma->host);
> +        goto err_dest_init_bind_addr;
> +    }
>  
> -        ret = rdma_getaddrinfo(rdma->host, port_str, NULL, &res);
> -        if (ret < 0) {
> -            ERROR(errp, "could not rdma_getaddrinfo address %s", rdma->host);
> -            goto err_dest_init_bind_addr;
> +    for (e = res; e != NULL; e = e->ai_next) {
> +        inet_ntop(e->ai_family,
> +            &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
> +        trace_qemu_rdma_dest_init_trying(rdma->host, ip);
> +        ret = rdma_bind_addr(listen_id, e->ai_dst_addr);
> +        if (ret) {
> +            continue;
>          }
> -
> -        for (e = res; e != NULL; e = e->ai_next) {
> -            inet_ntop(e->ai_family,
> -                &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
> -            trace_qemu_rdma_dest_init_trying(rdma->host, ip);
> -            ret = rdma_bind_addr(listen_id, e->ai_dst_addr);
> -            if (!ret) {
> -                if (e->ai_family == AF_INET6) {
> -                    ret = qemu_rdma_broken_ipv6_kernel(errp, listen_id->verbs);
> -                    if (ret) {
> -                        continue;
> -                    }
> -                }
> -                    
> -                goto listen;
> +        if (e->ai_family == AF_INET6) {
> +            ret = qemu_rdma_broken_ipv6_kernel(errp, listen_id->verbs);
> +            if (ret) {
> +                continue;
>              }
>          }
> +        break; 
> +    }
>  
> +    if (!e) {
>          ERROR(errp, "Error: could not rdma_bind_addr!");
>          goto err_dest_init_bind_addr;
> -    } else {
> -        ERROR(errp, "migration host and port not specified!");
> -        ret = -EINVAL;
> -        goto err_dest_init_bind_addr;
>      }
> -listen:
>  
>      rdma->listen_id = listen_id;
>      qemu_rdma_dump_gid("dest_init", listen_id);
> -- 
> 2.1.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK