Skip to content

Commit

Permalink
migration/rdma: Fix or document problematic uses of errno
Browse files Browse the repository at this point in the history
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>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
Message-ID: <20230928132019.2544702-17-armbru@redhat.com>
  • Loading branch information
Markus Armbruster authored and Juan Quintela committed Oct 11, 2023
1 parent 89997ac commit 0bc2604
Showing 1 changed file with 39 additions and 6 deletions.
45 changes: 39 additions & 6 deletions migration/rdma.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
}

Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -2210,6 +2233,11 @@ static int qemu_rdma_write_one(RDMAContext *rdma,
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;
}
Expand Down Expand Up @@ -2579,6 +2607,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;
Expand Down

0 comments on commit 0bc2604

Please sign in to comment.