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

[Markus Armbruster]

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?

>>              return -1;
>>          }