| Message ID | 20230918144206.560120-11-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/rdma: Error handling fixes | expand |
Markus Armbruster <armbru@redhat.com> writes: > When all we do with an Error we receive into a local variable is > propagating to somewhere else, we can just as well receive it there > right away. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
On 18/09/2023 22:41, Markus Armbruster wrote: > When all we do with an Error we receive into a local variable is > propagating to somewhere else, we can just as well receive it there > right away. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> > --- > migration/rdma.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/migration/rdma.c b/migration/rdma.c > index 2b40bbcbb0..960fff5860 100644 > --- a/migration/rdma.c > +++ b/migration/rdma.c > @@ -2445,7 +2445,6 @@ static void qemu_rdma_cleanup(RDMAContext *rdma) > static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) > { > int ret, idx; > - Error *local_err = NULL, **temp = &local_err; > > /* > * Will be validated against destination's actual capabilities > @@ -2453,14 +2452,14 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) > */ > rdma->pin_all = pin_all; > > - ret = qemu_rdma_resolve_host(rdma, temp); > + ret = qemu_rdma_resolve_host(rdma, errp); > if (ret) { > goto err_rdma_source_init; > } > > ret = qemu_rdma_alloc_pd_cq(rdma); > if (ret) { > - ERROR(temp, "rdma migration: error allocating pd and cq! Your mlock()" > + ERROR(errp, "rdma migration: error allocating pd and cq! Your mlock()" > " limits may be too low. Please check $ ulimit -a # and " > "search for 'ulimit -l' in the output"); > goto err_rdma_source_init; > @@ -2468,13 +2467,13 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) > > ret = qemu_rdma_alloc_qp(rdma); > if (ret) { > - ERROR(temp, "rdma migration: error allocating qp!"); > + ERROR(errp, "rdma migration: error allocating qp!"); > goto err_rdma_source_init; > } > > ret = qemu_rdma_init_ram_blocks(rdma); > if (ret) { > - ERROR(temp, "rdma migration: error initializing ram blocks!"); > + ERROR(errp, "rdma migration: error initializing ram blocks!"); > goto err_rdma_source_init; > } > > @@ -2489,7 +2488,7 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) > for (idx = 0; idx < RDMA_WRID_MAX; idx++) { > ret = qemu_rdma_reg_control(rdma, idx); > if (ret) { > - ERROR(temp, "rdma migration: error registering %d control!", > + ERROR(errp, "rdma migration: error registering %d control!", > idx); > goto err_rdma_source_init; > } > @@ -2498,7 +2497,6 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) > return 0; > > err_rdma_source_init: > - error_propagate(errp, local_err); > qemu_rdma_cleanup(rdma); > return -1; > } > @@ -4103,7 +4101,6 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp) > { > int ret; > RDMAContext *rdma; > - Error *local_err = NULL; > > trace_rdma_start_incoming_migration(); > > @@ -4113,13 +4110,12 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp) > return; > } > > - rdma = qemu_rdma_data_init(host_port, &local_err); > + rdma = qemu_rdma_data_init(host_port, errp); > if (rdma == NULL) { > goto err; > } > > - ret = qemu_rdma_dest_init(rdma, &local_err); > - > + ret = qemu_rdma_dest_init(rdma, errp); > if (ret) { > goto err; > } > @@ -4142,7 +4138,6 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp) > cleanup_rdma: > qemu_rdma_cleanup(rdma); > err: > - error_propagate(errp, local_err); > if (rdma) { > g_free(rdma->host); > g_free(rdma->host_port);
On Mon, Sep 18, 2023 at 04:41:24PM +0200, Markus Armbruster wrote: > When all we do with an Error we receive into a local variable is > propagating to somewhere else, we can just as well receive it there > right away. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > > ret = qemu_rdma_alloc_pd_cq(rdma); > if (ret) { > - ERROR(temp, "rdma migration: error allocating pd and cq! Your mlock()" > + ERROR(errp, "rdma migration: error allocating pd and cq! Your mlock()" > " limits may be too low. Please check $ ulimit -a # and " > "search for 'ulimit -l' in the output"); Not this patch's problem, but noticing it while here: it would help if we had a consistent style on whether to break long strings after the space instead of carrying the space to the next line, rather than using both styles in the same concatenated string.
Eric Blake <eblake@redhat.com> writes: > On Mon, Sep 18, 2023 at 04:41:24PM +0200, Markus Armbruster wrote: >> When all we do with an Error we receive into a local variable is >> propagating to somewhere else, we can just as well receive it there >> right away. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> >> ret = qemu_rdma_alloc_pd_cq(rdma); >> if (ret) { >> - ERROR(temp, "rdma migration: error allocating pd and cq! Your mlock()" >> + ERROR(errp, "rdma migration: error allocating pd and cq! Your mlock()" >> " limits may be too low. Please check $ ulimit -a # and " >> "search for 'ulimit -l' in the output"); > > Not this patch's problem, but noticing it while here: > > it would help if we had a consistent style on whether to break long > strings after the space instead of carrying the space to the next > line, rather than using both styles in the same concatenated string. Oh yes. I prefer to break lines before space, because leading space is more visible than trailing space. However, I elected to refrain from touching error messages in this series. It's long enough as it is.
diff --git a/migration/rdma.c b/migration/rdma.c index 2b40bbcbb0..960fff5860 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -2445,7 +2445,6 @@ static void qemu_rdma_cleanup(RDMAContext *rdma) static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) { int ret, idx; - Error *local_err = NULL, **temp = &local_err; /* * Will be validated against destination's actual capabilities @@ -2453,14 +2452,14 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) */ rdma->pin_all = pin_all; - ret = qemu_rdma_resolve_host(rdma, temp); + ret = qemu_rdma_resolve_host(rdma, errp); if (ret) { goto err_rdma_source_init; } ret = qemu_rdma_alloc_pd_cq(rdma); if (ret) { - ERROR(temp, "rdma migration: error allocating pd and cq! Your mlock()" + ERROR(errp, "rdma migration: error allocating pd and cq! Your mlock()" " limits may be too low. Please check $ ulimit -a # and " "search for 'ulimit -l' in the output"); goto err_rdma_source_init; @@ -2468,13 +2467,13 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) ret = qemu_rdma_alloc_qp(rdma); if (ret) { - ERROR(temp, "rdma migration: error allocating qp!"); + ERROR(errp, "rdma migration: error allocating qp!"); goto err_rdma_source_init; } ret = qemu_rdma_init_ram_blocks(rdma); if (ret) { - ERROR(temp, "rdma migration: error initializing ram blocks!"); + ERROR(errp, "rdma migration: error initializing ram blocks!"); goto err_rdma_source_init; } @@ -2489,7 +2488,7 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) for (idx = 0; idx < RDMA_WRID_MAX; idx++) { ret = qemu_rdma_reg_control(rdma, idx); if (ret) { - ERROR(temp, "rdma migration: error registering %d control!", + ERROR(errp, "rdma migration: error registering %d control!", idx); goto err_rdma_source_init; } @@ -2498,7 +2497,6 @@ static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp) return 0; err_rdma_source_init: - error_propagate(errp, local_err); qemu_rdma_cleanup(rdma); return -1; } @@ -4103,7 +4101,6 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp) { int ret; RDMAContext *rdma; - Error *local_err = NULL; trace_rdma_start_incoming_migration(); @@ -4113,13 +4110,12 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp) return; } - rdma = qemu_rdma_data_init(host_port, &local_err); + rdma = qemu_rdma_data_init(host_port, errp); if (rdma == NULL) { goto err; } - ret = qemu_rdma_dest_init(rdma, &local_err); - + ret = qemu_rdma_dest_init(rdma, errp); if (ret) { goto err; } @@ -4142,7 +4138,6 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp) cleanup_rdma: qemu_rdma_cleanup(rdma); err: - error_propagate(errp, local_err); if (rdma) { g_free(rdma->host); g_free(rdma->host_port);
When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- migration/rdma.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)