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

Functions that use an Error **errp parameter to return errors should
not also report them to the user, because reporting is the caller's
job.  When the caller does, the error is reported twice.  When it
doesn't (because it recovered from the error), there is no error to
report, i.e. the report is bogus.

qemu_rdma_write_flush() violates this principle: it calls
error_report() via qemu_rdma_write_one().  I elected not to
investigate how callers handle the error, i.e. precise impact is not
known.

Clean this up by converting qemu_rdma_write_one() to Error.

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

On 18/09/2023 22:41, Markus Armbruster wrote:
> Functions that use an Error **errp parameter to return errors should
> not also report them to the user, because reporting is the caller's
> job.  When the caller does, the error is reported twice.  When it
> doesn't (because it recovered from the error), there is no error to
> report, i.e. the report is bogus.
> 
> qemu_rdma_write_flush() violates this principle: it calls
> error_report() via qemu_rdma_write_one().  I elected not to
> investigate how callers handle the error, i.e. precise impact is not
> known.
> 
> Clean this up by converting qemu_rdma_write_one() to Error.
> 
> Signed-off-by: Markus Armbruster<armbru@redhat.com>


Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>

On 18/09/2023 22:41, Markus Armbruster wrote:
> Functions that use an Error **errp parameter to return errors should
> not also report them to the user, because reporting is the caller's
> job.  When the caller does, the error is reported twice.  When it
> doesn't (because it recovered from the error), there is no error to
> report, i.e. the report is bogus.
> 
> qemu_rdma_write_flush() violates this principle: it calls
> error_report() via qemu_rdma_write_one().  I elected not to
> investigate how callers handle the error, i.e. precise impact is not
> known.
> 
> Clean this up by converting qemu_rdma_write_one() to Error.
> 
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   migration/rdma.c | 25 +++++++++++--------------
>   1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index c3c33fe242..9b8cbadfcd 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2019,9 +2019,8 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, RDMAControlHeader *head,
>    */
>   static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
>                                  int current_index, uint64_t current_addr,
> -                               uint64_t length)
> +                               uint64_t length, Error **errp)
>   {
> -    Error *err = NULL;
>       struct ibv_sge sge;
>       struct ibv_send_wr send_wr = { 0 };
>       struct ibv_send_wr *bad_wr;

[...]

>           }
> @@ -2219,7 +2216,7 @@ retry:
>           goto retry;
>   
>       } else if (ret > 0) {
> -        perror("rdma migration: post rdma write failed");
> +        error_setg(errp, "rdma migration: post rdma write failed");

It reminds that do you miss to use error_setg_errno() instead.





>           return -1;
>       }

On 26/09/2023 13:50, Li Zhijian wrote:
> 
> 
> On 18/09/2023 22:41, Markus Armbruster wrote:
>> Functions that use an Error **errp parameter to return errors should
>> not also report them to the user, because reporting is the caller's
>> job.  When the caller does, the error is reported twice.  When it
>> doesn't (because it recovered from the error), there is no error to
>> report, i.e. the report is bogus.
>>
>> qemu_rdma_write_flush() violates this principle: it calls
>> error_report() via qemu_rdma_write_one().  I elected not to
>> investigate how callers handle the error, i.e. precise impact is not
>> known.
>>
>> Clean this up by converting qemu_rdma_write_one() to Error.
>>
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>> ---
>>   migration/rdma.c | 25 +++++++++++--------------
>>   1 file changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index c3c33fe242..9b8cbadfcd 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -2019,9 +2019,8 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, RDMAControlHeader *head,
>>    */
>>   static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
>>                                  int current_index, uint64_t current_addr,
>> -                               uint64_t length)
>> +                               uint64_t length, Error **errp)
>>   {
>> -    Error *err = NULL;
>>       struct ibv_sge sge;
>>       struct ibv_send_wr send_wr = { 0 };
>>       struct ibv_send_wr *bad_wr;
> 
> [...]
> 
>>           }
>> @@ -2219,7 +2216,7 @@ retry:
>>           goto retry;
>>       } else if (ret > 0) {
>> -        perror("rdma migration: post rdma write failed");
>> +        error_setg(errp, "rdma migration: post rdma write failed");
> 
> It reminds that do you miss to use error_setg_errno() instead.
> 

Answer it myself:
ibv_post_send(3) says:

RETURN VALUE
        ibv_post_send() returns 0 on success, or the value of errno on failure (which indicates the failure reason).


the global error is not defined here.



> 
> 
> 
> 
>>           return -1;
>>       }

"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:

> On 18/09/2023 22:41, Markus Armbruster wrote:
>> Functions that use an Error **errp parameter to return errors should
>> not also report them to the user, because reporting is the caller's
>> job.  When the caller does, the error is reported twice.  When it
>> doesn't (because it recovered from the error), there is no error to
>> report, i.e. the report is bogus.
>> 
>> qemu_rdma_write_flush() violates this principle: it calls
>> error_report() via qemu_rdma_write_one().  I elected not to
>> investigate how callers handle the error, i.e. precise impact is not
>> known.
>> 
>> Clean this up by converting qemu_rdma_write_one() to Error.
>> 
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>> ---
>>   migration/rdma.c | 25 +++++++++++--------------
>>   1 file changed, 11 insertions(+), 14 deletions(-)
>> 
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index c3c33fe242..9b8cbadfcd 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -2019,9 +2019,8 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, RDMAControlHeader *head,
>>    */
>>   static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
>>                                  int current_index, uint64_t current_addr,
>> -                               uint64_t length)
>> +                               uint64_t length, Error **errp)
>>   {
>> -    Error *err = NULL;
>>       struct ibv_sge sge;
>>       struct ibv_send_wr send_wr = { 0 };
>>       struct ibv_send_wr *bad_wr;
>
> [...]
>
>>           }
>> @@ -2219,7 +2216,7 @@ retry:
>>           goto retry;
>>   
>>       } else if (ret > 0) {
>> -        perror("rdma migration: post rdma write failed");
>> +        error_setg(errp, "rdma migration: post rdma write failed");
>
> It reminds that do you miss to use error_setg_errno() instead.

Indeed!

I'll adjust the commit message as well.

>>           return -1;
>>       }

"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:

> On 26/09/2023 13:50, Li Zhijian wrote:
>> 
>> 
>> On 18/09/2023 22:41, Markus Armbruster wrote:
>>> Functions that use an Error **errp parameter to return errors should
>>> not also report them to the user, because reporting is the caller's
>>> job.  When the caller does, the error is reported twice.  When it
>>> doesn't (because it recovered from the error), there is no error to
>>> report, i.e. the report is bogus.
>>>
>>> qemu_rdma_write_flush() violates this principle: it calls
>>> error_report() via qemu_rdma_write_one().  I elected not to
>>> investigate how callers handle the error, i.e. precise impact is not
>>> known.
>>>
>>> Clean this up by converting qemu_rdma_write_one() to Error.
>>>
>>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>>> ---
>>>   migration/rdma.c | 25 +++++++++++--------------
>>>   1 file changed, 11 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>> index c3c33fe242..9b8cbadfcd 100644
>>> --- a/migration/rdma.c
>>> +++ b/migration/rdma.c
>>> @@ -2019,9 +2019,8 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, RDMAControlHeader *head,
>>>    */
>>>   static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
>>>                                  int current_index, uint64_t current_addr,
>>> -                               uint64_t length)
>>> +                               uint64_t length, Error **errp)
>>>   {
>>> -    Error *err = NULL;
>>>       struct ibv_sge sge;
>>>       struct ibv_send_wr send_wr = { 0 };
>>>       struct ibv_send_wr *bad_wr;
>> 
>> [...]
>> 
>>>           }
>>> @@ -2219,7 +2216,7 @@ retry:
>>>           goto retry;
>>>       } else if (ret > 0) {
>>> -        perror("rdma migration: post rdma write failed");
>>> +        error_setg(errp, "rdma migration: post rdma write failed");
>> 
>> It reminds that do you miss to use error_setg_errno() instead.
>> 
>
> Answer it myself:
> ibv_post_send(3) says:
>
> RETURN VALUE
>         ibv_post_send() returns 0 on success, or the value of errno on failure (which indicates the failure reason).

I read this as "assign error code to errno and return it."  But...

> the global error is not defined here.

... your assertion made me check the source code, and it looks like it
does *not* assign to errno, at least not reliably.  Which means perror()
prints garbage.

I'll delete the perror() in a separate patch.

>>>           return -1;
>>>       }

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

Let me try to map solutions.

Markus Armbruster <armbru@redhat.com> writes:

> migration/rdma.c uses errno directly or via perror() after the following
> functions:
>
> * poll()
>
>   POSIX specifies errno is set on error.  Good.

Nothing wrong, nothing to do.

> * rdma_get_cm_event(), rdma_connect(), rdma_get_cm_event()
>
>   Manual page promises "if an error occurs, errno will be set".  Good.

Nothing wrong, nothing to do.

> * 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?

1. Investigate whether ibv_open_device() sets errno.  I can't spare time
   for that.

2. Add a comment pointing out the problem, in the hope somebody
   investigates later.

3. Do nothing.

> * 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?

Likewise.

> * 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.

1. Add a comment pointing out the problem, remove it in PATCH 48.

2. Nothing wrong after the series, nothing to do.

> * 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.

1. Add a comment pointing out the problem, remove it in PATCH 39.

2. Pass @ret, not @errno to error_setg_errno().

3. Nothing wrong after the series, nothing to do.

Since 2. is easy, let's do it.

> * 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?

1. Change sterror(errno) to strerror(ret)

2. Drop strerror(errno)

3. Do nothing.

Since 1. is easy, let's do it.

> * 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.

1. Add a comment pointing out the problem, remove it in PATCH 51.

2. Nothing wrong after the series, nothing to do.

> * qemu_get_cm_event_timeout()
>
>   Can fail without setting errno.
>
>   qemu_rdma_connect() calls perror().  Removed in PATCH 45.  Good
>   enough.
>
> Thoughts?

Considering all of the above...  I'd like to stick a patch documenting
problematic errno use early in the series, and fix all the easy ones
later in the series, leaving just the two difficult ones in
qemu_rdma_broken_ipv6_kernel() and qemu_rdma_reg_whole_ram_blocks().

Makes sense?

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

+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
>

On Sat, Oct 07, 2023 at 03:40:50AM +0000, Zhijian Li (Fujitsu) wrote:
> +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)."

I think the intention of this language was that an errno constant is
returned, the caller should not assume it is stored in errno.

errno is difficult, many things overwrite it, you can loose its value
during error handling.

Jason

On 16/10/2023 20:11, Jason Gunthorpe wrote:
> On Sat, Oct 07, 2023 at 03:40:50AM +0000, Zhijian Li (Fujitsu) wrote:
>> +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)."
> 
> I think the intention of this language was that an errno constant is
> returned, the caller should not assume it is stored in errno.
> 
Understood.
If that's the case, I think some pieces of code were misunderstood in QEMU before.


Thanks
Zhijian

> errno is difficult, many things overwrite it, you can loose its value
> during error handling.
> 
> Jason