qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: qemu-devel@nongnu.org
Cc: quintela@redhat.com, peterx@redhat.com, leobras@redhat.com,
	farosas@suse.de, lizhijian@fujitsu.com, eblake@redhat.com
Subject: [PATCH v2 16/53] migration/rdma: Fix or document problematic uses of errno
Date: Thu, 28 Sep 2023 15:19:42 +0200	[thread overview]
Message-ID: <20230928132019.2544702-17-armbru@redhat.com> (raw)
In-Reply-To: <20230928132019.2544702-1-armbru@redhat.com>

We use errno after calling Libibverbs functions that are not
documented to set errno (manual page does not mention errno), or where
the documentation is unclear ("returns [...] the value of errno on
failure").  While this could be read as "sets errno and returns it",
a glance at the source code[*] kills that hope:

    static inline int ibv_post_send(struct ibv_qp *qp, struct ibv_send_wr *wr,
                                    struct ibv_send_wr **bad_wr)
    {
            return qp->context->ops.post_send(qp, wr, bad_wr);
    }

The callback can be

    static int mana_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
                              struct ibv_send_wr **bad)
    {
            /* This version of driver supports RAW QP only.
             * Posting WR is done directly in the application.
             */
            return EOPNOTSUPP;
    }

Neither of them touches errno.

One of these errno uses is easy to fix, so do that now.  Several more
will go away later in the series; add temporary FIXME commments.
Three will remain; add TODO comments.  TODO, not FIXME, because the
bug might be in Libibverbs documentation.

[*] https://github.com/linux-rdma/rdma-core.git
    commit 55fa316b4b18f258d8ac1ceb4aa5a7a35b094dcf

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/rdma.c | 45 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 28097ce604..bba8c99fa9 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -853,6 +853,12 @@ static int qemu_rdma_broken_ipv6_kernel(struct ibv_context *verbs, Error **errp)
 
         for (x = 0; x < num_devices; x++) {
             verbs = ibv_open_device(dev_list[x]);
+            /*
+             * ibv_open_device() is not documented to set errno.  If
+             * it does, it's somebody else's doc bug.  If it doesn't,
+             * the use of errno below is wrong.
+             * TODO Find out whether ibv_open_device() sets errno.
+             */
             if (!verbs) {
                 if (errno == EPERM) {
                     continue;
@@ -1162,11 +1168,7 @@ static void qemu_rdma_advise_prefetch_mr(struct ibv_pd *pd, uint64_t addr,
     ret = ibv_advise_mr(pd, advice,
                         IBV_ADVISE_MR_FLAG_FLUSH, &sg_list, 1);
     /* ignore the error */
-    if (ret) {
-        trace_qemu_rdma_advise_mr(name, len, addr, strerror(errno));
-    } else {
-        trace_qemu_rdma_advise_mr(name, len, addr, "successed");
-    }
+    trace_qemu_rdma_advise_mr(name, len, addr, strerror(ret));
 #endif
 }
 
@@ -1183,7 +1185,12 @@ static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma)
                     local->block[i].local_host_addr,
                     local->block[i].length, access
                     );
-
+        /*
+         * ibv_reg_mr() is not documented to set errno.  If it does,
+         * it's somebody else's doc bug.  If it doesn't, the use of
+         * errno below is wrong.
+         * TODO Find out whether ibv_reg_mr() sets errno.
+         */
         if (!local->block[i].mr &&
             errno == ENOTSUP && rdma_support_odp(rdma->verbs)) {
                 access |= IBV_ACCESS_ON_DEMAND;
@@ -1291,6 +1298,12 @@ static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
         trace_qemu_rdma_register_and_get_keys(len, chunk_start);
 
         block->pmr[chunk] = ibv_reg_mr(rdma->pd, chunk_start, len, access);
+        /*
+         * ibv_reg_mr() is not documented to set errno.  If it does,
+         * it's somebody else's doc bug.  If it doesn't, the use of
+         * errno below is wrong.
+         * TODO Find out whether ibv_reg_mr() sets errno.
+         */
         if (!block->pmr[chunk] &&
             errno == ENOTSUP && rdma_support_odp(rdma->verbs)) {
             access |= IBV_ACCESS_ON_DEMAND;
@@ -1408,6 +1421,11 @@ static int qemu_rdma_unregister_waiting(RDMAContext *rdma)
         block->remote_keys[chunk] = 0;
 
         if (ret != 0) {
+            /*
+             * FIXME perror() is problematic, bcause ibv_dereg_mr() is
+             * not documented to set errno.  Will go away later in
+             * this series.
+             */
             perror("unregistration chunk failed");
             return -ret;
         }
@@ -1658,6 +1676,11 @@ static int qemu_rdma_block_for_wrid(RDMAContext *rdma,
 
         ret = ibv_get_cq_event(ch, &cq, &cq_ctx);
         if (ret) {
+            /*
+             * FIXME perror() is problematic, because ibv_reg_mr() is
+             * not documented to set errno.  Will go away later in
+             * this series.
+             */
             perror("ibv_get_cq_event");
             goto err_block_for_wrid;
         }
@@ -2199,6 +2222,11 @@ retry:
         goto retry;
 
     } else if (ret > 0) {
+        /*
+         * FIXME perror() is problematic, because whether
+         * ibv_post_send() sets errno is unclear.  Will go away later
+         * in this series.
+         */
         perror("rdma migration: post rdma write failed");
         return -ret;
     }
@@ -2559,6 +2587,11 @@ static int qemu_rdma_connect(RDMAContext *rdma, bool return_path,
         ret = rdma_get_cm_event(rdma->channel, &cm_event);
     }
     if (ret) {
+        /*
+         * FIXME perror() is wrong, because
+         * qemu_get_cm_event_timeout() can fail without setting errno.
+         * Will go away later in this series.
+         */
         perror("rdma_get_cm_event after rdma_connect");
         ERROR(errp, "connecting to destination!");
         goto err_rdma_source_connect;
-- 
2.41.0



  parent reply	other threads:[~2023-09-28 13:26 UTC|newest]

Thread overview: 121+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-28 13:19 [PATCH v2 00/53] migration/rdma: Error handling fixes Markus Armbruster
2023-09-28 13:19 ` [PATCH v2 01/53] migration/rdma: Clean up qemu_rdma_poll()'s return type Markus Armbruster
2023-10-04 14:26   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 02/53] migration/rdma: Clean up qemu_rdma_data_init()'s " Markus Armbruster
2023-10-04 14:35   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 03/53] migration/rdma: Clean up rdma_delete_block()'s " Markus Armbruster
2023-10-04 14:36   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 04/53] migration/rdma: Drop fragile wr_id formatting Markus Armbruster
2023-10-04 14:38   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 05/53] migration/rdma: Consistently use uint64_t for work request IDs Markus Armbruster
2023-10-04 14:39   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 06/53] migration/rdma: Fix unwanted integer truncation Markus Armbruster
2023-09-28 14:20   ` Fabiano Rosas
2023-10-04 14:41   ` Juan Quintela
2023-10-07  1:53   ` Zhijian Li (Fujitsu)
2023-09-28 13:19 ` [PATCH v2 07/53] migration/rdma: Clean up two more harmless signed vs. unsigned issues Markus Armbruster
2023-10-04 14:44   ` Juan Quintela
2023-10-07  2:38   ` Zhijian Li (Fujitsu)
2023-09-28 13:19 ` [PATCH v2 08/53] migration/rdma: Give qio_channel_rdma_source_funcs internal linkage Markus Armbruster
2023-10-04 14:50   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 09/53] migration/rdma: Fix qemu_rdma_accept() to return failure on errors Markus Armbruster
2023-10-04 14:51   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 10/53] migration/rdma: Put @errp parameter last Markus Armbruster
2023-10-04 14:54   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 11/53] migration/rdma: Eliminate error_propagate() Markus Armbruster
2023-10-04 14:58   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 12/53] migration/rdma: Drop rdma_add_block() error handling Markus Armbruster
2023-10-04 14:58   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 13/53] migration/rdma: Drop qemu_rdma_search_ram_block() " Markus Armbruster
2023-10-04 15:00   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 14/53] migration/rdma: Make qemu_rdma_buffer_mergeable() return bool Markus Armbruster
2023-10-04 15:01   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 15/53] migration/rdma: Use bool for two RDMAContext flags Markus Armbruster
2023-10-04 15:56   ` Juan Quintela
2023-09-28 13:19 ` Markus Armbruster [this message]
2023-09-29 15:09   ` [PATCH v2 16/53] migration/rdma: Fix or document problematic uses of errno Fabiano Rosas
2023-10-04 11:12     ` Markus Armbruster
2023-10-05  6:46   ` Juan Quintela
2023-10-07  5:34   ` Zhijian Li (Fujitsu)
2023-09-28 13:19 ` [PATCH v2 17/53] migration/rdma: Ditch useless numeric error codes in error messages Markus Armbruster
2023-10-04 15:06   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 18/53] migration/rdma: Fix io_writev(), io_readv() methods to obey contract Markus Armbruster
2023-10-04 15:09   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 19/53] migration/rdma: Replace dangerous macro CHECK_ERROR_STATE() Markus Armbruster
2023-10-04 15:10   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 20/53] migration/rdma: Fix qemu_rdma_broken_ipv6_kernel() to set error Markus Armbruster
2023-10-04 15:10   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 21/53] migration/rdma: Fix qemu_get_cm_event_timeout() to always " Markus Armbruster
2023-10-04 15:25   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 22/53] migration/rdma: Drop dead qemu_rdma_data_init() code for !@host_port Markus Armbruster
2023-09-29 15:10   ` Fabiano Rosas
2023-10-04 15:24   ` Juan Quintela
2023-10-07  5:36   ` Zhijian Li (Fujitsu)
2023-09-28 13:19 ` [PATCH v2 23/53] migration/rdma: Fix QEMUFileHooks method return values Markus Armbruster
2023-10-04 15:28   ` Juan Quintela
2023-10-04 16:22   ` Juan Quintela
2023-10-04 16:37     ` Markus Armbruster
2023-09-28 13:19 ` [PATCH v2 24/53] migration/rdma: Fix rdma_getaddrinfo() error checking Markus Armbruster
2023-10-04 15:30   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 25/53] migration/rdma: Return -1 instead of negative errno code Markus Armbruster
2023-10-04 16:19   ` Juan Quintela
2023-10-04 16:23   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 26/53] migration/rdma: Dumb down remaining int error values to -1 Markus Armbruster
2023-10-04 16:25   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 27/53] migration/rdma: Replace int error_state by bool errored Markus Armbruster
2023-10-04 16:25   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 28/53] migration/rdma: Drop superfluous assignments to @ret Markus Armbruster
2023-10-04 16:27   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 29/53] migration/rdma: Check negative error values the same way everywhere Markus Armbruster
2023-09-29 15:28   ` Fabiano Rosas
2023-10-04 16:33   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 30/53] migration/rdma: Plug a memory leak and improve a message Markus Armbruster
2023-10-04 16:27   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 31/53] migration/rdma: Delete inappropriate error_report() in macro ERROR() Markus Armbruster
2023-10-04 16:50   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 32/53] migration/rdma: Retire " Markus Armbruster
2023-10-04 16:50   ` Juan Quintela
2023-09-28 13:19 ` [PATCH v2 33/53] migration/rdma: Fix error handling around rdma_getaddrinfo() Markus Armbruster
2023-10-04 16:51   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 34/53] migration/rdma: Drop "@errp is clear" guards around error_setg() Markus Armbruster
2023-10-04 16:52   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 35/53] migration/rdma: Convert qemu_rdma_exchange_recv() to Error Markus Armbruster
2023-10-04 16:53   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 36/53] migration/rdma: Convert qemu_rdma_exchange_send() " Markus Armbruster
2023-10-04 16:55   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 37/53] migration/rdma: Convert qemu_rdma_exchange_get_response() " Markus Armbruster
2023-10-04 16:55   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 38/53] migration/rdma: Convert qemu_rdma_reg_whole_ram_blocks() " Markus Armbruster
2023-10-04 16:56   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 39/53] migration/rdma: Convert qemu_rdma_write_flush() " Markus Armbruster
2023-10-04 16:56   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 40/53] migration/rdma: Convert qemu_rdma_write_one() " Markus Armbruster
2023-10-04 16:56   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 41/53] migration/rdma: Convert qemu_rdma_write() " Markus Armbruster
2023-10-04 17:23   ` Juan Quintela
2023-09-28 13:20 ` [PATCH v2 42/53] migration/rdma: Convert qemu_rdma_post_send_control() " Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 43/53] migration/rdma: Convert qemu_rdma_post_recv_control() " Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 44/53] migration/rdma: Convert qemu_rdma_alloc_pd_cq() " Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 45/53] migration/rdma: Silence qemu_rdma_resolve_host() Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 46/53] migration/rdma: Silence qemu_rdma_connect() Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 47/53] migration/rdma: Silence qemu_rdma_reg_control() Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 48/53] migration/rdma: Don't report received completion events as error Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 49/53] migration/rdma: Silence qemu_rdma_block_for_wrid() Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 50/53] migration/rdma: Silence qemu_rdma_register_and_get_keys() Markus Armbruster
2023-09-28 13:20 ` [PATCH v2 51/53] migration/rdma: Downgrade qemu_rdma_cleanup() errors to warnings Markus Armbruster
2023-09-29 15:29   ` Fabiano Rosas
2023-10-04 17:47   ` Juan Quintela
2023-10-07  3:50   ` Zhijian Li (Fujitsu)
2023-09-28 13:20 ` [PATCH v2 52/53] migration/rdma: Use error_report() & friends instead of stderr Markus Armbruster
2023-09-29 15:36   ` Fabiano Rosas
2023-10-04 11:15     ` Markus Armbruster
2023-10-04 13:52       ` Fabiano Rosas
2023-10-05  7:24   ` Juan Quintela
2023-10-07  3:56   ` Zhijian Li (Fujitsu)
2023-09-28 13:20 ` [PATCH v2 53/53] migration/rdma: Replace flawed device detail dump by tracing Markus Armbruster
2023-09-29 17:05   ` Fabiano Rosas
2023-10-04 17:50   ` Juan Quintela
2023-10-07  3:57   ` Zhijian Li (Fujitsu)
2023-10-04 17:52 ` [PATCH v2 00/53] migration/rdma: Error handling fixes Juan Quintela
2023-10-05  5:07   ` Markus Armbruster
2023-10-05  6:37     ` Juan Quintela

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230928132019.2544702-17-armbru@redhat.com \
    --to=armbru@redhat.com \
    --cc=eblake@redhat.com \
    --cc=farosas@suse.de \
    --cc=leobras@redhat.com \
    --cc=lizhijian@fujitsu.com \
    --cc=peterx@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).