Re: [PATCH 2/6] tap: Improve handling of EINTR in tap_passt_input()

[Stefano Brivio <sbrivio@redhat.com>]

On Tue,  3 Sep 2024 22:02:31 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> When tap_passt_input() gets an error from recv() it (correctly) does not
> print any error message for EINTR, EAGAIN or EWOULDBLOCK.  However in all
> three cases it returns from the function.  That makes sense for EAGAIN and
> EWOULDBLOCK, since we then want to wait for the next EPOLLIN event before
> trying again.  For EINTR, however, it makes more sense to retry immediately
> - as it stands we're likely to get a renewer EPOLLIN event immediately in
> that case, since we're using level triggered signalling.
> 
> So, handle EINTR separately by immediately retrying until we succeed or
> get a different type of error.

I don't see an actual improvement: we don't know why we would get EINTR
(because of signals) so repeating the recv() right away isn't
necessarily a better choice.

I'd say whatever way we have of carrying on on EINTR is fine. If this
patch helps with brevity for the next ones, it makes sense, but
otherwise I don't see a real advantage.

Well, it's more consistent with the way we handle EINTR on other calls,
but that's about it.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 14c88871..9ee59faa 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1003,10 +1003,13 @@ static void tap_passt_input(struct ctx *c, const struct timespec *now)
>  		memmove(pkt_buf, partial_frame, partial_len);
>  	}
>  
> -	n = recv(c->fd_tap, pkt_buf + partial_len, TAP_BUF_BYTES - partial_len,
> -		 MSG_DONTWAIT);
> +	do {
> +		n = recv(c->fd_tap, pkt_buf + partial_len,
> +			 TAP_BUF_BYTES - partial_len, MSG_DONTWAIT);
> +	} while ((n < 0) && errno == EINTR);
> +
>  	if (n < 0) {
> -		if (errno != EINTR && errno != EAGAIN && errno != EWOULDBLOCK) {
> +		if (errno != EAGAIN && errno != EWOULDBLOCK) {
>  			err_perror("Receive error on guest connection, reset");
>  			tap_sock_reset(c);
>  		}

-- 
Stefano