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

[Markus Armbruster]

"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;
>>>       }