Re: [PATCH v3 26/26] migration/rdma: Plug memory leaks in qemu_rdma_registration_stop()

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> qemu_rdma_registration_stop() uses the ERROR() macro to create, report
> to stderr, and store an Error object.  The stored Error object is
> never used, and its memory is leaked.
> 
> Even where ERROR() doesn't leak, it is ill-advised.  The whole point
> of passing an Error to the caller is letting the caller handle the
> error.  Error handling may report to stderr, to somewhere else, or not
> at all.  Also reporting in the callee mixes up concerns that should be
> kept separate.  Since I don't know what reporting to stderr is
> supposed to accomplish, I'm not touching it.
> 
> Commit 2a1bc8bde7 "migration/rdma: rdma_accept_incoming_migration fix
> error handling" plugged the same leak in
> rdma_accept_incoming_migration().
> 
> Plug the memory leak the same way: keep the report part, delete the
> store part.
> 
> The report part uses fprintf().  If it's truly an error, it should use
> error_report() instead.  But I don't know, so I leave it alone, just
> like commit 2a1bc8bde7 did.
> 
> Fixes: 2da776db4846eadcb808598a5d3484d149773c05
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks,


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

> ---
>  migration/rdma.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index ec45d33ba3..3b18823268 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3787,7 +3787,6 @@ static int qemu_rdma_registration_start(QEMUFile *f, 
> void *opaque,
>  static int qemu_rdma_registration_stop(QEMUFile *f, void *opaque,
>                                         uint64_t flags, void *data)
>  {
> -    Error *local_err = NULL, **errp = &local_err;
>      QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(opaque);
>      RDMAContext *rdma;
>      RDMAControlHeader head = { .len = 0, .repeat = 1 };
> @@ -3832,7 +3831,7 @@ static int qemu_rdma_registration_stop(QEMUFile *f, 
> void *opaque,
>                      &reg_result_idx, rdma->pin_all ?
>                      qemu_rdma_reg_whole_ram_blocks : NULL);
>          if (ret < 0) {
> -            ERROR(errp, "receiving remote info!");
> +            fprintf(stderr, "receiving remote info!");
>              return ret;
>          }
>  
> @@ -3851,10 +3850,10 @@ static int qemu_rdma_registration_stop(QEMUFile *f, 
> void *opaque,
>           */
>  
>          if (local->nb_blocks != nb_dest_blocks) {
> -            ERROR(errp, "ram blocks mismatch (Number of blocks %d vs %d) "
> -                        "Your QEMU command line parameters are probably "
> -                        "not identical on both the source and destination.",
> -                        local->nb_blocks, nb_dest_blocks);
> +            fprintf(stderr, "ram blocks mismatch (Number of blocks %d vs %d) 
> "
> +                    "Your QEMU command line parameters are probably "
> +                    "not identical on both the source and destination.",
> +                    local->nb_blocks, nb_dest_blocks);
>              rdma->error_state = -EINVAL;
>              return -EINVAL;
>          }
> @@ -3867,10 +3866,10 @@ static int qemu_rdma_registration_stop(QEMUFile *f, 
> void *opaque,
>  
>              /* We require that the blocks are in the same order */
>              if (rdma->dest_blocks[i].length != local->block[i].length) {
> -                ERROR(errp, "Block %s/%d has a different length %" PRIu64
> -                            "vs %" PRIu64, local->block[i].block_name, i,
> -                            local->block[i].length,
> -                            rdma->dest_blocks[i].length);
> +                fprintf(stderr, "Block %s/%d has a different length %" PRIu64
> +                        "vs %" PRIu64, local->block[i].block_name, i,
> +                        local->block[i].length,
> +                        rdma->dest_blocks[i].length);
>                  rdma->error_state = -EINVAL;
>                  return -EINVAL;
>              }
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK