Re: [PATCH 22/52] migration/rdma: Fix rdma_getaddrinfo() error checking

[Zhijian Li (Fujitsu)]

On 18/09/2023 22:41, Markus Armbruster wrote:
> rdma_getaddrinfo() returns 0 on success.  On error, it returns one of
> the EAI_ error codes like getaddrinfo() does,


Good catch, It used to be -1 on error, rdma_getaddrinfo(3) updated it 2021.



  or -1 with errno set.
> This is broken by design: POSIX implicitly specifies the EAI_ error
> codes to be non-zero, no more.  They could clash with -1.  Nothing we
> can do about this design flaw.
> 
> Both callers of rdma_getaddrinfo() only recognize negative values as
> error.  Works only because systems elect to make the EAI_ error codes
> negative.
> 
> Best not to rely on that: change the callers to treat any non-zero
> value as failure.  Also change them to return -1 instead of the value
> received from getaddrinfo() on failure, to avoid positive error
> values.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>


> ---
>   migration/rdma.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 46b5859268..3421ae0796 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -935,14 +935,14 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, 
> Error **errp)
>   
>       if (rdma->host == NULL || !strcmp(rdma->host, "")) {
>           ERROR(errp, "RDMA hostname has not been set");
> -        return -EINVAL;
> +        return -1;
>       }
>   
>       /* create CM channel */
>       rdma->channel = rdma_create_event_channel();
>       if (!rdma->channel) {
>           ERROR(errp, "could not create CM channel");
> -        return -EINVAL;
> +        return -1;
>       }
>   
>       /* create CM id */
> @@ -956,7 +956,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, 
> Error **errp)
>       port_str[15] = '\0';
>   
>       ret = rdma_getaddrinfo(rdma->host, port_str, NULL, &res);
> -    if (ret < 0) {
> +    if (ret) {
>           ERROR(errp, "could not rdma_getaddrinfo address %s", rdma->host);
>           goto err_resolve_get_addr;
>       }
> @@ -998,7 +998,6 @@ route:
>                   rdma_event_str(cm_event->event));
>           error_report("rdma_resolve_addr");
>           rdma_ack_cm_event(cm_event);
> -        ret = -EINVAL;
>           goto err_resolve_get_addr;
>       }
>       rdma_ack_cm_event(cm_event);
> @@ -1019,7 +1018,6 @@ route:
>           ERROR(errp, "result not equal to event_route_resolved: %s",
>                           rdma_event_str(cm_event->event));
>           rdma_ack_cm_event(cm_event);
> -        ret = -EINVAL;
>           goto err_resolve_get_addr;
>       }
>       rdma_ack_cm_event(cm_event);
> @@ -1034,7 +1032,7 @@ err_resolve_get_addr:
>   err_resolve_create_id:
>       rdma_destroy_event_channel(rdma->channel);
>       rdma->channel = NULL;
> -    return ret;
> +    return -1;
>   }
>   
>   /*
> @@ -2644,7 +2642,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error 
> **errp)
>       port_str[15] = '\0';
>   
>       ret = rdma_getaddrinfo(rdma->host, port_str, NULL, &res);
> -    if (ret < 0) {
> +    if (ret) {
>           ERROR(errp, "could not rdma_getaddrinfo address %s", rdma->host);
>           goto err_dest_init_bind_addr;
>       }
> @@ -2688,7 +2686,7 @@ err_dest_init_create_listen_id:
>       rdma_destroy_event_channel(rdma->channel);
>       rdma->channel = NULL;
>       rdma->error_state = ret;
> -    return ret;
> +    return -1;
>   
>   }
>