Re: [PATCH v2 04/13] tap: Fold reset handling into tap_handler_passt()

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 10 Aug 2023 12:33:06 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We call tap_sock_reset() if tap_handler_passt() fails, or if we get an
> error event on the socket.  Fold that logic into tap_handler() passt itself
> which simplifies the caller.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tap.c | 63 ++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 866ca4d..501af33 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -891,19 +891,41 @@ append:
>  	return in->count;
>  }
>  
> +/**
> + * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
> + * @c:		Execution context
> + */
> +static void tap_sock_reset(struct ctx *c)
> +{
> +	if (c->one_off) {
> +		info("Client closed connection, exiting");
> +		exit(EXIT_SUCCESS);
> +	}
> +
> +	/* Close the connected socket, wait for a new connection */
> +	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
> +	close(c->fd_tap);
> +	c->fd_tap = -1;
> +}
> +
>  /**
>   * tap_handler_passt() - Packet handler for AF_UNIX file descriptor
>   * @c:		Execution context
> + * @events:	epoll events
>   * @now:	Current timestamp
> - *
> - * Return: -ECONNRESET on receive error, 0 otherwise
>   */
> -static int tap_handler_passt(struct ctx *c, const struct timespec *now)
> +static void tap_handler_passt(struct ctx *c, uint32_t events,
> +			      const struct timespec *now)
>  {
>  	struct ethhdr *eh;
>  	ssize_t n, rem;
>  	char *p;
>  
> +	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) {
> +		tap_sock_reset(c);
> +		return;
> +	}
> +
>  redo:
>  	p = pkt_buf;
>  	rem = 0;
> @@ -914,12 +936,13 @@ redo:
>  	n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT);
>  	if (n < 0) {
>  		if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK)
> -			return 0;
> +			return;
>  
>  		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
>  		close(c->fd_tap);
>  
> -		return -ECONNRESET;
> +		tap_sock_reset(c);

...also reported by covscan: close() before this is redundant now.

-- 
Stefano