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

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

[Michael Tokarev]

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.

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

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

[Dr. David Alan Gilbert]

* 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

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

[Michael Tokarev]

19.02.2015 13:25, Dr. David Alan Gilbert wrote:
> * 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.

No, I don't have any RDMA hw, I only compile-tested this code, and
double-checked the changes does not change behavour.

Thanks,

/mjt

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

[Dr. David Alan Gilbert]

* 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.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Juan/Amit: There's a space at the end of a line that got in, so
           that just needs fixing in the merge;  it's after the 'break;'
but the patch is a big improvement on what's there:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

also tested on an RDMA system.

Thanks,

Dave

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

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

[Michael Tokarev]

04.03.2015 15:16, Dr. David Alan Gilbert wrote:
> * 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.
>>
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> 
> Juan/Amit: There's a space at the end of a line that got in, so
>            that just needs fixing in the merge;  it's after the 'break;'

I already fixed it in my tree two days ago.

> but the patch is a big improvement on what's there:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> also tested on an RDMA system.

Thank you!

I think it's okay to merge it using the -trivial tree, I'm about to
send a pull request today.

/mjt

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

[Dr. David Alan Gilbert]

* Michael Tokarev (mjt@tls.msk.ru) wrote:
> 04.03.2015 15:16, Dr. David Alan Gilbert wrote:
> > * 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.
> >>
> >> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > 
> > Juan/Amit: There's a space at the end of a line that got in, so
> >            that just needs fixing in the merge;  it's after the 'break;'
> 
> I already fixed it in my tree two days ago.
> 
> > but the patch is a big improvement on what's there:
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > also tested on an RDMA system.
> 
> Thank you!
> 
> I think it's okay to merge it using the -trivial tree, I'm about to
> send a pull request today.

Yes, OK, although I'd generally prefer the RDMA stuff to go
through migration unless it's really trivial.

Dave

> 
> /mjt
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Michael Tokarev]

04.03.2015 16:32, Dr. David Alan Gilbert wrote:
> * Michael Tokarev (mjt@tls.msk.ru) wrote:
[]
>> I think it's okay to merge it using the -trivial tree, I'm about to
>> send a pull request today.
> 
> Yes, OK, although I'd generally prefer the RDMA stuff to go
> through migration unless it's really trivial.

I've no prob with either of these.  It is just that I'm preparing
a pull request for -trivial tree now... ;)  I can drop the patch
from there.

Thanks,

/mjt

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

[Juan Quintela]

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.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Applied, thanks.