[PATCH] migration/rdma: fix return value for qio_channel_rdma_{readv, writev}

[PATCH] migration/rdma: fix return value for qio_channel_rdma_{readv, writev}

[Fiona Ebner]

upon errors. As the documentation in include/io/channel.h states, only
-1 and QIO_CHANNEL_ERR_BLOCK should be returned upon error. Other
values have the potential to confuse the call sites.

error_setg is used rather than error_setg_errno, because there are
certain code paths where -1 (as a non-errno) is propagated up (e.g.
starting from qemu_rdma_block_for_wrid or qemu_rdma_post_recv_control)
all the way to qio_channel_rdma_{readv,writev}.

Similar to a216ec85b7 ("migration/channel-block: fix return value for
qio_channel_block_{readv,writev}").

Suggested-by: Zhang Chen <chen.zhang@intel.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

I did only compile-test the patch, because I have no experience with
RDMA.

@Zhang Chen: In [0], besides qio_channel_rdma_writev/readv, you
mentioned qio_channel_buffer_writev/readv and 'etc.', but I did only
find the issue in the RDMA-specific implementation.

[0]: https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg02636.html

 migration/rdma.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 94a55dd95b..0ba1668d70 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2785,7 +2785,8 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
     rdma = qatomic_rcu_read(&rioc->rdmaout);
 
     if (!rdma) {
-        return -EIO;
+        error_setg(errp, "RDMA control channel output is not set");
+        return -1;
     }
 
     CHECK_ERROR_STATE();
@@ -2797,7 +2798,8 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
     ret = qemu_rdma_write_flush(f, rdma);
     if (ret < 0) {
         rdma->error_state = ret;
-        return ret;
+        error_setg(errp, "qemu_rdma_write_flush returned %d", ret);
+        return -1;
     }
 
     for (i = 0; i < niov; i++) {
@@ -2816,7 +2818,8 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
 
             if (ret < 0) {
                 rdma->error_state = ret;
-                return ret;
+                error_setg(errp, "qemu_rdma_exchange_send returned %d", ret);
+                return -1;
             }
 
             data += len;
@@ -2867,7 +2870,8 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
     rdma = qatomic_rcu_read(&rioc->rdmain);
 
     if (!rdma) {
-        return -EIO;
+        error_setg(errp, "RDMA control channel input is not set");
+        return -1;
     }
 
     CHECK_ERROR_STATE();
@@ -2903,7 +2907,8 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
 
         if (ret < 0) {
             rdma->error_state = ret;
-            return ret;
+            error_setg(errp, "qemu_rdma_exchange_recv returned %d", ret);
+            return -1;
         }
 
         /*
-- 
2.30.2

Re: [PATCH] migration/rdma: fix return value for qio_channel_rdma_{readv,writev}

[Juan Quintela]

Fiona Ebner <f.ebner@proxmox.com> wrote:
> upon errors. As the documentation in include/io/channel.h states, only
> -1 and QIO_CHANNEL_ERR_BLOCK should be returned upon error. Other
> values have the potential to confuse the call sites.
>
> error_setg is used rather than error_setg_errno, because there are
> certain code paths where -1 (as a non-errno) is propagated up (e.g.
> starting from qemu_rdma_block_for_wrid or qemu_rdma_post_recv_control)
> all the way to qio_channel_rdma_{readv,writev}.
>
> Similar to a216ec85b7 ("migration/channel-block: fix return value for
> qio_channel_block_{readv,writev}").
>
> Suggested-by: Zhang Chen <chen.zhang@intel.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.