Re: [PATCH v2 52/53] migration/rdma: Use error_report() & friends instead of stderr

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> error_report() obeys -msg, reports the current error location if any,
>>> and reports to the current monitor if any.  Reporting to stderr
>>> directly with fprintf() or perror() is wrong, because it loses all
>>> this.
>>>
>>> Fix the offenders.  Bonus: resolves a FIXME about problematic use of
>>> errno.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  migration/rdma.c | 44 +++++++++++++++++++++-----------------------
>>>  1 file changed, 21 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>> index 54b59d12b1..dba0802fca 100644
>>> --- a/migration/rdma.c
>>> +++ b/migration/rdma.c
>>> @@ -877,12 +877,12 @@ static int qemu_rdma_broken_ipv6_kernel(struct 
>>> ibv_context *verbs, Error **errp)
>>>  
>>>          if (roce_found) {
>>>              if (ib_found) {
>>> -                fprintf(stderr, "WARN: migrations may fail:"
>>> -                                " IPv6 over RoCE / iWARP in linux"
>>> -                                " is broken. But since you appear to have 
>>> a"
>>> -                                " mixed RoCE / IB environment, be sure to 
>>> only"
>>> -                                " migrate over the IB fabric until the 
>>> kernel "
>>> -                                " fixes the bug.\n");
>>> +                warn_report("WARN: migrations may fail:"
>>> +                            " IPv6 over RoCE / iWARP in linux"
>>> +                            " is broken. But since you appear to have a"
>>> +                            " mixed RoCE / IB environment, be sure to only"
>>> +                            " migrate over the IB fabric until the kernel "
>>> +                            " fixes the bug.");
>>
>> Won't this become "warning: WARN:"?
>
> It will.  I'll drop the "WARN: " prefix.
>
>>>              } else {
>>>                  error_setg(errp, "RDMA ERROR: "
>>>                             "You only have RoCE / iWARP devices in your 
>>> systems"
>>> @@ -1418,12 +1418,8 @@ 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");
>>> +            error_report("unregistration chunk failed: %s",
>>> +                         strerror(ret));
>>
>> Doesn't seem to fix the issue, ret might still not be an errno. Am I
>> missing something?
>
> Yes :)
>
> ibv_dereg_mr(3) section RETURN VALUE has:
>
>        ibv_dereg_mr()  returns  0 on success, or the value of errno on failure
>        (which indicates the failure reason).
>
> Clearer now?

Yep, thank you.