Re: [PATCH 39/52] migration/rdma: Convert qemu_rdma_write_one() to Error

[Zhijian Li (Fujitsu)]

+rdma-core


Is global variable *errno* reliable when the documentation only states
"returns 0 on success, or the value of errno on failure (which indicates the 
failure reason)."

Someone read it as "assign error code to errno and return it.", I used to think 
the same way.
but ibv_post_send() doesn't always follow this rule. see ibv_post_send() -> 
mana_post_send()

Actually, QEMU are using errno after calling libibverbs APIs, so we hope the 
man page can be
more clear. like posix does:

RETURN VALUE
        Upon successful completion fopen(), fdopen() and freopen() return a 
FILE pointer.  Otherwise, NULL is returned and errno is set to indicate the 
error

Thanks
Zhijian


On 27/09/2023 19:46, Markus Armbruster wrote:
> migration/rdma.c uses errno directly or via perror() after the following
> functions:
> 
> * poll()
> 
>    POSIX specifies errno is set on error.  Good.
> 
> * rdma_get_cm_event(), rdma_connect(), rdma_get_cm_event()
> 
>    Manual page promises "if an error occurs, errno will be set".  Good.
> 
> * ibv_open_device()
> 
>    Manual page does not mention errno.  Using it seems ill-advised.
> 
>    qemu_rdma_broken_ipv6_kernel() recovers from EPERM by trying the next
>    device.  Wrong if ibv_open_device() doesn't actually set errno.
> 
>    What is to be done here?
> 
> * ibv_reg_mr()
> 
>    Manual page does not mention errno.  Using it seems ill-advised.
> 
>    qemu_rdma_reg_whole_ram_blocks() and qemu_rdma_register_and_get_keys()
>    recover from errno = ENOTSUP by retrying with modified @access
>    argument.  Wrong if ibv_reg_mr() doesn't actually set errno.
> 
>    What is to be done here?
> 
> * ibv_get_cq_event()
> 
>    Manual page does not mention errno.  Using it seems ill-advised.
> 
>    qemu_rdma_block_for_wrid() calls perror().  Removed in PATCH 48.  Good
>    enough.
> 
> * ibv_post_send()
> 
>    Manual page has the function return "the value of errno on failure".
>    Sounds like it sets errno to the value it returns.  However, the
>    rdma-core repository defines it as
> 
>      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);
>      }
> 
>    and at least one of the methods fails without setting errno:
> 
>      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;
>      }
> 
>    qemu_rdma_write_one() calls perror().  PATCH 39 (this one) replaces it
>    by error_setg(), not error_setg_errno().  Seems prudent, but should be
>    called out in the commit message.
> 
> * ibv_advise_mr()
> 
>    Manual page has the function return "the value of errno on failure".
>    Sounds like it sets errno to the value it returns, but my findings for
>    ibv_post_send() make me doubt it.
> 
>    qemu_rdma_advise_prefetch_mr() traces strerror(errno).  Could be
>    misleading.  Drop that part?
> 
> * ibv_dereg_mr()
> 
>    Manual page has the function return "the value of errno on failure".
>    Sounds like it sets errno to the value it returns, but my findings for
>    ibv_post_send() make me doubt it.
> 
>    qemu_rdma_unregister_waiting() calls perror().  Removed in PATCH 51.
>    Good enough.
> 
> * qemu_get_cm_event_timeout()
> 
>    Can fail without setting errno.
> 
>    qemu_rdma_connect() calls perror().  Removed in PATCH 45.  Good
>    enough.
> 
> Thoughts?
> 
> 
> [...]
> 
> [*] https://github.com/linux-rdma/rdma-core.git
>      commit 55fa316b4b18f258d8ac1ceb4aa5a7a35b094dcf
>