Re: [Qemu-devel] [PULL 1/9] qapi: Add InetSocketAddress member keep-alive

[Peter Maydell]

On Thu, 15 Aug 2019 at 19:34, Eric Blake <address@hidden> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
>
> It's needed to provide keepalive for nbd client to track server
> availability.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Message-Id: <address@hidden>
> Reviewed-by: Markus Armbruster <address@hidden>
> Acked-by: Daniel P. Berrangé <address@hidden>
> [eblake: Fix error message typo]
> Signed-off-by: Eric Blake <address@hidden>

Hi; Coverity spots a bug in this change (CID 1405300):

> @@ -458,6 +464,19 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error 
> **errp)
>      }

At this point in the function, we might be in one of
two cases:
 (1) sock >= 0 : the connect succeeded
 (2) sock < 0 : connect failed, we have just called
     error_propagate() and are using the same end-of-function
     codepath to free 'res' before returning the error code.

>
>      freeaddrinfo(res);
> +
> +    if (saddr->keep_alive) {
> +        int val = 1;
> +        int ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> +                                  &val, sizeof(val));

...but here we use 'sock' assuming it is valid.

> +
> +        if (ret < 0) {
> +            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> +            close(sock);
> +            return -1;
> +        }
> +    }
> +
>      return sock;
>  }

If we want to add more "actual work" at this point in
the function we should probably separate out the failed-case
codepath, eg by changing


    if (sock < 0) {
        error_propagate(errp, local_err);
    }

    freeaddrinfo(res);

into

    freeaddrinfo(res);

    if (sock < 0) {
        error_propagate(errp, local_err);
        return sock;
    }



thanks
-- PMM