[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 43/52] migration/rdma: Convert qemu_rdma_alloc_pd_cq() to Err
From: |
Zhijian Li (Fujitsu) |
Subject: |
Re: [PATCH 43/52] migration/rdma: Convert qemu_rdma_alloc_pd_cq() to Error |
Date: |
Tue, 26 Sep 2023 06:55:20 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 |
On 26/09/2023 14:41, Markus Armbruster wrote:
> "Zhijian Li (Fujitsu)" <lizhijian@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<armbru@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 <lizhijian@fujitsu.com>
>>
>>
>>> goto err_rdma_source_init;
>>> }
>
> Thanks!
>
- Re: [PATCH 10/52] migration/rdma: Eliminate error_propagate(), (continued)
- [PATCH 18/52] migration/rdma: Fix qemu_rdma_broken_ipv6_kernel() to set error, Markus Armbruster, 2023/09/18
- [PATCH 31/52] migration/rdma: Retire macro ERROR(), Markus Armbruster, 2023/09/18
- [PATCH 44/52] migration/rdma: Silence qemu_rdma_resolve_host(), Markus Armbruster, 2023/09/18
- [PATCH 29/52] migration/rdma: Plug a memory leak and improve a message, Markus Armbruster, 2023/09/18
- [PATCH 08/52] migration/rdma: Fix qemu_rdma_accept() to return failure on errors, Markus Armbruster, 2023/09/18