Re: [PATCH 50/52] migration/rdma: Silence qemu_rdma_cleanup()

[Markus Armbruster]

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

> On 26/09/2023 19:52, Markus Armbruster wrote:
>> "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:
>> 
>>> On 18/09/2023 22:42, 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_source_init(), qemu_rdma_connect(),
>>>> rdma_start_incoming_migration(), and rdma_start_outgoing_migration()
>>>> violate this principle: they call error_report() via
>>>> qemu_rdma_cleanup().
>>>>
>>>> Moreover, qemu_rdma_cleanup() can't fail.  It is called on error
>>>> paths, and QIOChannel close and finalization.  Are the conditions it
>>>> reports really errors?  I doubt it.
>>>
>>> I'm not very sure, it's fine if it's call from the error path. but when
>>> the caller is migration_cancle from HMP/QMP, shall we report something more
>>> though we know QEMU can recover.
>>>
>>> maybe change to warning etc...
>> 
>> The part I'm sure about is that reporting an error to the user is wrong
>> when we actually recover from the error.  Which qemu_rdma_cleanup()
>> does.
>
> Yes, i have no doubt about this.
>
>
>> 
>> I'm not sure whether the (complicated!) condition that triggers
>> qemu_rdma_cleanup()'s ill-advised error report needs to be reported in
>> some other form.  The remainder of the function ignores failure...
>> 
>> If you think we should to downgrade the error to a warning, and no
>> maintainer disagrees, then I'll downgrade.  Do you?
>
> Yes, I'd like downgrade error to a warning.

Got it, thanks!