Re: [PATCH 34/52] migration/rdma: Convert qemu_rdma_exchange_recv() to Error

[Zhijian Li (Fujitsu)]

On 18/09/2023 22:41, Markus Armbruster wrote:
> Functions that use an Error **errp parameter to return errors should
> not also report them to the user, because reporting is the caller's
> job.  When the caller does, the error is reported twice.  When it
> doesn't (because it recovered from the error), there is no error to
> report, i.e. the report is bogus.
> 
> qio_channel_rdma_readv() violates this principle: it calls
> error_report() via qemu_rdma_exchange_recv().  I elected not to
> investigate how callers handle the error, i.e. precise impact is not
> known.
> 
> Clean this up by converting qemu_rdma_exchange_recv() to Error.
> 
> Necessitates setting an error when qemu_rdma_exchange_get_response()
> failed.  Since this error will go away later in this series, simply
> use "FIXME temporary error message" there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


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


> ---
>   migration/rdma.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index c88cd1f468..50546b3a27 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -1957,7 +1957,7 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, 
> RDMAControlHeader *head,
>    * control-channel message.
>    */
>   static int qemu_rdma_exchange_recv(RDMAContext *rdma, RDMAControlHeader 
> *head,
> -                                   uint32_t expecting)
> +                                   uint32_t expecting, Error **errp)
>   {
>       RDMAControlHeader ready = {
>                                   .len = 0,
> @@ -1972,7 +1972,7 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, 
> RDMAControlHeader *head,
>       ret = qemu_rdma_post_send_control(rdma, NULL, &ready);
>   
>       if (ret < 0) {
> -        error_report("Failed to send control buffer!");
> +        error_setg(errp, "Failed to send control buffer!");
>           return -1;
>       }
>   
> @@ -1983,6 +1983,7 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, 
> RDMAControlHeader *head,
>                                             expecting, RDMA_WRID_READY);
>   
>       if (ret < 0) {
> +        error_setg(errp, "FIXME temporary error message");
>           return -1;
>       }
>   
> @@ -1993,7 +1994,7 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, 
> RDMAControlHeader *head,
>        */
>       ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
>       if (ret < 0) {
> -        error_report("rdma migration: error posting second control recv!");
> +        error_setg(errp, "rdma migration: error posting second control 
> recv!");
>           return -1;
>       }
>   
> @@ -2908,11 +2909,11 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
>           /* We've got nothing at all, so lets wait for
>            * more to arrive
>            */
> -        ret = qemu_rdma_exchange_recv(rdma, &head, RDMA_CONTROL_QEMU_FILE);
> +        ret = qemu_rdma_exchange_recv(rdma, &head, RDMA_CONTROL_QEMU_FILE,
> +                                      errp);
>   
>           if (ret < 0) {
>               rdma->errored = true;
> -            error_setg(errp, "qemu_rdma_exchange_recv failed");
>               return -1;
>           }
>   
> @@ -3536,6 +3537,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f)
>       RDMAControlHeader blocks = { .type = RDMA_CONTROL_RAM_BLOCKS_RESULT,
>                                    .repeat = 1 };
>       QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
> +    Error *err = NULL;
>       RDMAContext *rdma;
>       RDMALocalBlocks *local;
>       RDMAControlHeader head;
> @@ -3565,9 +3567,10 @@ static int qemu_rdma_registration_handle(QEMUFile *f)
>       do {
>           trace_qemu_rdma_registration_handle_wait();
>   
> -        ret = qemu_rdma_exchange_recv(rdma, &head, RDMA_CONTROL_NONE);
> +        ret = qemu_rdma_exchange_recv(rdma, &head, RDMA_CONTROL_NONE, &err);
>   
>           if (ret < 0) {
> +            error_report_err(err);
>               break;
>           }
>