| Message ID | 20230918144206.560120-21-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/rdma: Error handling fixes | expand |
On Mon, Sep 18, 2023 at 04:41:34PM +0200, 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 Indeed they all seem to be non-null. Before this patch, qemu_rdma_data_init() can still tolerant NULL, not setting errp but still returning NULL showing an error. After this patch, qemu_rdma_data_init() should crash at inet_parse() if it's null. Would it be simpler and clearer if we just set ERROR() for !host_port? Thanks, > 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 d3dc162363..cc59155a50 100644 > --- a/migration/rdma.c > +++ b/migration/rdma.c > @@ -2716,25 +2716,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 >
Peter Xu <peterx@redhat.com> writes: > On Mon, Sep 18, 2023 at 04:41:34PM +0200, 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 > > Indeed they all seem to be non-null. > > Before this patch, qemu_rdma_data_init() can still tolerant NULL, not > setting errp but still returning NULL showing an error. Returning failure without setting an error is wrong :) > After this patch, qemu_rdma_data_init() should crash at inet_parse() if > it's null. Yes. > Would it be simpler and clearer if we just set ERROR() for !host_port? I dislike impossible error paths, because they are untestable. > Thanks, > >> merely a latent bug. Drop the flawed code handling null argument. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
diff --git a/migration/rdma.c b/migration/rdma.c index d3dc162363..cc59155a50 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -2716,25 +2716,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; }
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(-)