[v2] migration/rdma: Error handling fixes

[PATCH v2 22/53] migration/rdma: Drop dead qemu_rdma_data_init() code for !@host_port

Posted by Markus Armbruster 1 year, 7 months ago

qemu_rdma_data_init() neglects to set an Error when it fails because
@host_port is null.  Fortunately, no caller passes null, so this is
merely a latent bug.  Drop the flawed code handling null argument.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/rdma.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 1a0ad44411..1ae2f87906 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2747,25 +2747,22 @@ static RDMAContext *qemu_rdma_data_init(const char *host_port, Error **errp)
     RDMAContext *rdma = NULL;
     InetSocketAddress *addr;
 
-    if (host_port) {
-        rdma = g_new0(RDMAContext, 1);
-        rdma->current_index = -1;
-        rdma->current_chunk = -1;
+    rdma = g_new0(RDMAContext, 1);
+    rdma->current_index = -1;
+    rdma->current_chunk = -1;
 
-        addr = g_new(InetSocketAddress, 1);
-        if (!inet_parse(addr, host_port, NULL)) {
-            rdma->port = atoi(addr->port);
-            rdma->host = g_strdup(addr->host);
-            rdma->host_port = g_strdup(host_port);
-        } else {
-            ERROR(errp, "bad RDMA migration address '%s'", host_port);
-            g_free(rdma);
-            rdma = NULL;
-        }
-
-        qapi_free_InetSocketAddress(addr);
+    addr = g_new(InetSocketAddress, 1);
+    if (!inet_parse(addr, host_port, NULL)) {
+        rdma->port = atoi(addr->port);
+        rdma->host = g_strdup(addr->host);
+        rdma->host_port = g_strdup(host_port);
+    } else {
+        ERROR(errp, "bad RDMA migration address '%s'", host_port);
+        g_free(rdma);
+        rdma = NULL;
     }
 
+    qapi_free_InetSocketAddress(addr);
     return rdma;
 }
 
-- 
2.41.0

Re: [PATCH v2 22/53] migration/rdma: Drop dead qemu_rdma_data_init() code for !@host_port

Posted by Zhijian Li (Fujitsu) 1 year, 7 months ago


On 28/09/2023 21:19, Markus Armbruster wrote:
> qemu_rdma_data_init() neglects to set an Error when it fails because
> @host_port is null.  Fortunately, no caller passes null, so this is
> merely a latent bug.  Drop the flawed code handling null argument.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>


> ---
>   migration/rdma.c | 29 +++++++++++++----------------
>   1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 1a0ad44411..1ae2f87906 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2747,25 +2747,22 @@ static RDMAContext *qemu_rdma_data_init(const char *host_port, Error **errp)
>       RDMAContext *rdma = NULL;
>       InetSocketAddress *addr;
>   
> -    if (host_port) {
> -        rdma = g_new0(RDMAContext, 1);
> -        rdma->current_index = -1;
> -        rdma->current_chunk = -1;
> +    rdma = g_new0(RDMAContext, 1);
> +    rdma->current_index = -1;
> +    rdma->current_chunk = -1;
>   
> -        addr = g_new(InetSocketAddress, 1);
> -        if (!inet_parse(addr, host_port, NULL)) {
> -            rdma->port = atoi(addr->port);
> -            rdma->host = g_strdup(addr->host);
> -            rdma->host_port = g_strdup(host_port);
> -        } else {
> -            ERROR(errp, "bad RDMA migration address '%s'", host_port);
> -            g_free(rdma);
> -            rdma = NULL;
> -        }
> -
> -        qapi_free_InetSocketAddress(addr);
> +    addr = g_new(InetSocketAddress, 1);
> +    if (!inet_parse(addr, host_port, NULL)) {
> +        rdma->port = atoi(addr->port);
> +        rdma->host = g_strdup(addr->host);
> +        rdma->host_port = g_strdup(host_port);
> +    } else {
> +        ERROR(errp, "bad RDMA migration address '%s'", host_port);
> +        g_free(rdma);
> +        rdma = NULL;
>       }
>   
> +    qapi_free_InetSocketAddress(addr);
>       return rdma;
>   }
>

Re: [PATCH v2 22/53] migration/rdma: Drop dead qemu_rdma_data_init() code for !@host_port

Posted by Juan Quintela 1 year, 7 months ago

Markus Armbruster <armbru@redhat.com> wrote:
> qemu_rdma_data_init() neglects to set an Error when it fails because
> @host_port is null.  Fortunately, no caller passes null, so this is
> merely a latent bug.  Drop the flawed code handling null argument.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

As your discussion with Peter, I think that this is ok.  The only other
thing we could do is add an assert(), but I am not a big fan either.

Re: [PATCH v2 22/53] migration/rdma: Drop dead qemu_rdma_data_init() code for !@host_port

Posted by Fabiano Rosas 1 year, 7 months ago

Markus Armbruster <armbru@redhat.com> writes:

> qemu_rdma_data_init() neglects to set an Error when it fails because
> @host_port is null.  Fortunately, no caller passes null, so this is
> merely a latent bug.  Drop the flawed code handling null argument.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>