[v1] migration/rdma: Error handling fixes

[PATCH 23/52] migration/rdma: Clean up qemu_rdma_wait_comp_channel()'s error value

Posted by Markus Armbruster 11 months, 3 weeks ago

qemu_rdma_wait_comp_channel() returns 0 on success, and either -1 or
rdma->error_state on failure.  Callers actually expect a negative
error value.  I believe rdma->error_state can't be positive, but let's
make things more obvious by simply returning -1 on any failure.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 3421ae0796..efbb3c7754 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1588,7 +1588,7 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma,
     if (rdma->received_error) {
         return -EPIPE;
     }
-    return rdma->error_state;
+    return -!!rdma->error_state;
 }
 
 static struct ibv_comp_channel *to_channel(RDMAContext *rdma, uint64_t wrid)
-- 
2.41.0

Re: [PATCH 23/52] migration/rdma: Clean up qemu_rdma_wait_comp_channel()'s error value

Posted by Zhijian Li (Fujitsu) 11 months, 2 weeks ago


On 18/09/2023 22:41, Markus Armbruster wrote:
> qemu_rdma_wait_comp_channel() returns 0 on success, and either -1 or
> rdma->error_state on failure.  Callers actually expect a negative
> error value. 

I don't see the only one callers expect a negative error code.
migration/rdma.c:1654:        ret = qemu_rdma_wait_comp_channel(rdma, ch);
migration/rdma.c-1655-        if (ret) {
migration/rdma.c-1656-            goto err_block_for_wrid;


  I believe rdma->error_state can't be positive, but let's
> make things more obvious by simply returning -1 on any failure.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   migration/rdma.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 3421ae0796..efbb3c7754 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -1588,7 +1588,7 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma,
>       if (rdma->received_error) {
>           return -EPIPE;
>       }
> -    return rdma->error_state;
> +    return -!!rdma->error_state;

And i rarely see such things, below would be more clear.

return rdma->error_state ? -1 : 0:


>   }
>   
>   static struct ibv_comp_channel *to_channel(RDMAContext *rdma, uint64_t wrid)

Re: [PATCH 23/52] migration/rdma: Clean up qemu_rdma_wait_comp_channel()'s error value

Posted by Markus Armbruster 11 months, 2 weeks ago

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

> On 18/09/2023 22:41, Markus Armbruster wrote:
>> qemu_rdma_wait_comp_channel() returns 0 on success, and either -1 or
>> rdma->error_state on failure.  Callers actually expect a negative
>> error value. 
>
> I don't see the only one callers expect a negative error code.
> migration/rdma.c:1654:        ret = qemu_rdma_wait_comp_channel(rdma, ch);
> migration/rdma.c-1655-        if (ret) {
> migration/rdma.c-1656-            goto err_block_for_wrid;

You're right.

I want the change anyway, to let me simplify the code some.  I'll adjust
the commit message.

>   I believe rdma->error_state can't be positive, but let's
>> make things more obvious by simply returning -1 on any failure.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   migration/rdma.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 3421ae0796..efbb3c7754 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -1588,7 +1588,7 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma,
>>       if (rdma->received_error) {
>>           return -EPIPE;
>>       }
>> -    return rdma->error_state;
>> +    return -!!rdma->error_state;
>
> And i rarely see such things, below would be more clear.
>
> return rdma->error_state ? -1 : 0:

Goes away in PATCH 26:

   @@ -1588,7 +1588,7 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma,
        if (rdma->received_error) {
            return -1;
        }
   -    return -!!rdma->error_state;
   +    return -rdma->errored;
    }

    static struct ibv_comp_channel *to_channel(RDMAContext *rdma, uint64_t wrid)

>
>>   }
>>   
>>   static struct ibv_comp_channel *to_channel(RDMAContext *rdma, uint64_t wrid)