Re: [PATCH v2 00/53] migration/rdma: Error handling fixes

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Markus Armbruster <armbru@redhat.com> wrote:
>>> Oh dear, where to start.  There's so much wrong, and in pretty obvious
>>> ways.  This code should never have passed review.  I'm refraining from
>>> saying more; see the commit messages instead.
>>>
>>> Issues remaining after this series include:
>>>
>>> * Terrible error messages
>>>
>>> * Some error message cascades remain
>>>
>>> * There is no written contract for QEMUFileHooks, and the
>>>   responsibility for reporting errors is unclear
>>>
>>> * There seem to be no tests whatsoever
>>>
>>> PATCH 29 is arguably a matter of taste.  I made my case for it during
>>> review of v1.  If maintainers don't want it, I'll drop it.
>>>
>>> Related: [PATCH 1/7] migration/rdma: Fix save_page method to fail on
>>> polling error
>>
>> Hi Markus
>>
>> I integrated everything except:
>>
>>>   migration/rdma: Fix or document problematic uses of errno
>>
>> Most of them are dropped on following patches.
>
> The hunks that are dropped in later patches are:
>
> * Four FIXME comments about incorrect or problematic use of perror().
>
>   If you drop the patch, you have to adjust the later patches that
>   remove these hunks.  Resolving the conflicts is *not* enough; you also
>   have to correct the commit messages.
>
> The hunks that are not dropped are:
>
> * Three comments about bugs (either library doc bug or incorrect use of
>   @errno here).  I'd hate to lose them.
>
> * One bug fix, in qemu_rdma_advise_prefetch_mr().  Losing this one would
>   be foolish.
>
> Please consider keeping the patch.

And here I am, having to redo the merge from this patch O:-)

>>>   migration/rdma: Use error_report() & friends instead of stderr
>>
>> You said you have to resend this one.
>
> Can do, but since the change is trivial, perhaps you could make it in
> your tree without a resend.  Change the line
>
>                 warn_report("WARN: migrations may fail:"
>
> to
>
>                 warn_report("migrations may fail:"

I thought it was more complicated.

Ok doing both.

Thanks, Juan.