On 26/09/2023 14:41, Markus Armbruster wrote:
> "Zhijian Li (Fujitsu)" <lizhij...@fujitsu.com> writes:
>
>> 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_source_init() violates this principle: it calls
>>> error_report() via qemu_rdma_alloc_pd_cq(). I elected not to
>>> investigate how callers handle the error, i.e. precise impact is not
>>> known.
>>>
>>> Clean this up by converting qemu_rdma_alloc_pd_cq() to Error.
>>>
>>> Signed-off-by: Markus Armbruster<arm...@redhat.com>
>>> ---
>>> migration/rdma.c | 27 ++++++++++++++-------------
>>> 1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> [...]
>>
>>
>>> @@ -2451,6 +2451,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>>>
>>> static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error
>>> **errp)
>>> {
>>> + ERRP_GUARD();
>>> int ret, idx;
>>>
>>> /*
>>> @@ -2464,12 +2465,12 @@ static int qemu_rdma_source_init(RDMAContext *rdma,
>>> bool pin_all, Error **errp)
>>> goto err_rdma_source_init;
>>> }
>>>
>>> - ret = qemu_rdma_alloc_pd_cq(rdma);
>>> + ret = qemu_rdma_alloc_pd_cq(rdma, errp);
>>> if (ret < 0) {
>>> - error_setg(errp, "RDMA ERROR: "
>>> - "rdma migration: error allocating pd and cq! Your
>>> mlock()"
>>> - " limits may be too low. Please check $ ulimit -a # and
>>> "
>>> - "search for 'ulimit -l' in the output");
>>> + error_append_hint(errp,
>>> + "Your mlock() limits may be too low. "
>>> + "Please check $ ulimit -a # and "
>>> + "search for 'ulimit -l' in the output\n");
>>
>>
>> I think we could freely remove this error message as well, it may neither a
>> exact resolution
>> nor some one will take care. Just report the error qemu_rdma_alloc_pd_cq()
>> tell us.
>
> Double-checking: you recommend to drop error_append_hint()?
Yes
>
>> Anyway
>>
>> Reviewed-by: Li Zhijian <lizhij...@fujitsu.com>
>>
>>
>>> goto err_rdma_source_init;
>>> }
>
> Thanks!
>
