On Thu, 15 Aug 2019 at 19:34, Eric Blake <ebl...@redhat.com> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>
> It's needed to provide keepalive for nbd client to track server
> availability.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> Message-Id: <20190725094937.32454-1-vsement...@virtuozzo.com>
> Reviewed-by: Markus Armbruster <arm...@redhat.com>
> Acked-by: Daniel P. Berrangé <berra...@redhat.com>
> [eblake: Fix error message typo]
> Signed-off-by: Eric Blake <ebl...@redhat.com>
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
