[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 16/53] migration/rdma: Fix or document problematic uses of
From: |
Fabiano Rosas |
Subject: |
Re: [PATCH v2 16/53] migration/rdma: Fix or document problematic uses of errno |
Date: |
Fri, 29 Sep 2023 12:09:07 -0300 |
Markus Armbruster <armbru@redhat.com> writes:
> 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;
This function can call into glibc, so it's not unreasonable to expect
errno to be set at some point. We're not relying on errno to be set,
just taking an action if it happens to be.
I don't think someone would just decide to handle EPERM at this point
for no reason. Specially since the manual makes no mention to
errno. This was probably introduced after someone got bit by it.
... indeed the commit 5b61d57521 ("rdma: Fix qemu crash when IPv6
address is used for migration") introduced this to fix a crash.
- [PATCH v2 11/53] migration/rdma: Eliminate error_propagate(), (continued)
- [PATCH v2 11/53] migration/rdma: Eliminate error_propagate(), Markus Armbruster, 2023/09/28
- [PATCH v2 52/53] migration/rdma: Use error_report() & friends instead of stderr, Markus Armbruster, 2023/09/28
- [PATCH v2 22/53] migration/rdma: Drop dead qemu_rdma_data_init() code for !@host_port, Markus Armbruster, 2023/09/28
- [PATCH v2 45/53] migration/rdma: Silence qemu_rdma_resolve_host(), Markus Armbruster, 2023/09/28
- [PATCH v2 40/53] migration/rdma: Convert qemu_rdma_write_one() to Error, Markus Armbruster, 2023/09/28
- [PATCH v2 50/53] migration/rdma: Silence qemu_rdma_register_and_get_keys(), Markus Armbruster, 2023/09/28
- [PATCH v2 27/53] migration/rdma: Replace int error_state by bool errored, Markus Armbruster, 2023/09/28
- [PATCH v2 16/53] migration/rdma: Fix or document problematic uses of errno, Markus Armbruster, 2023/09/28
- Re: [PATCH v2 16/53] migration/rdma: Fix or document problematic uses of errno,
Fabiano Rosas <=
- [PATCH v2 14/53] migration/rdma: Make qemu_rdma_buffer_mergeable() return bool, Markus Armbruster, 2023/09/28
- [PATCH v2 44/53] migration/rdma: Convert qemu_rdma_alloc_pd_cq() to Error, Markus Armbruster, 2023/09/28
- [PATCH v2 48/53] migration/rdma: Don't report received completion events as error, Markus Armbruster, 2023/09/28
- [PATCH v2 37/53] migration/rdma: Convert qemu_rdma_exchange_get_response() to Error, Markus Armbruster, 2023/09/28
- [PATCH v2 51/53] migration/rdma: Downgrade qemu_rdma_cleanup() errors to warnings, Markus Armbruster, 2023/09/28
- [PATCH v2 53/53] migration/rdma: Replace flawed device detail dump by tracing, Markus Armbruster, 2023/09/28
- [PATCH v2 49/53] migration/rdma: Silence qemu_rdma_block_for_wrid(), Markus Armbruster, 2023/09/28
- [PATCH v2 47/53] migration/rdma: Silence qemu_rdma_reg_control(), Markus Armbruster, 2023/09/28