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 <arm...@redhat.com>
Reviewed-by: Li Zhijian <lizhij...@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;
> }
>
