Re: [PATCH 3/6] tap: Restructure in tap_pasta_input()

[Stefano Brivio <sbrivio@redhat.com>]

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

> tap_pasta_input() has a rather confusing structure, using two gotos.
> Remove these by restructuring the function to have the main loop condition
> based on filling our buffer space, with errors or running out of data
> treated as the exception, rather than the other way around.  This allows
> us to handle the EINTR which triggered the 'restart' goto with a continue.
> 
> The outer 'redo' was triggered if we completely filled our buffer, to flush
> it and do another pass.  This one is unnecessary since we don't (yet) use
> EPOLLET on the tap device: if there's still more data we'll get another
> event and re-enter the loop.
> 
> Along the way handle a couple of extra edge cases:
>  - Check for EWOULDBLOCK as well as EAGAIN for the benefit of any future
>    ports where those might not have the same value
>  - Detect EOF on the tap device and exit in that case
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tap.c | 42 +++++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 9ee59faa..71742748 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1073,42 +1073,34 @@ void tap_handler_passt(struct ctx *c, uint32_t events,
>  static void tap_pasta_input(struct ctx *c, const struct timespec *now)
>  {
>  	ssize_t n, len;
> -	int ret;
> -
> -redo:
> -	n = 0;
>  
>  	tap_flush_pools();
> -restart:
> -	while ((len = read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n)) > 0) {
>  
> -		if (len < (ssize_t)sizeof(struct ethhdr) ||
> -		    len > (ssize_t)ETH_MAX_MTU) {
> -			n += len;
> -			continue;
> +	for (n = 0; n < (ssize_t)TAP_BUF_BYTES; n += len) {
> +		len = read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n);
> +
> +		if (len <= 0) {
> +			if (errno == EINTR) {

You might be checking errno when read() returns 0, in which case it's
not set. I guess you should zero out errno before read() if you want to
keep this, or, alternatively, directly handle len == 0 here.

> +				len = 0;
> +				continue;
> +			}
> +			break;
>  		}
>  
> +		/* Ignore frames of bad length */
> +		if (len < (ssize_t)sizeof(struct ethhdr) ||
> +		    len > (ssize_t)ETH_MAX_MTU)
> +			continue;
>  
>  		tap_add_packet(c, len, pkt_buf + n);
> -
> -		if ((n += len) == TAP_BUF_BYTES)
> -			break;
>  	}
>  
> -	if (len < 0 && errno == EINTR)
> -		goto restart;
> -
> -	ret = errno;
> +	if (len < 0 && errno != EAGAIN && errno != EWOULDBLOCK)
> +		die("Error on tap device, exiting");
> +	else if (len == 0)
> +		die("EOF on tap device, exiting");
>  
>  	tap_handler(c, now);
> -
> -	if (len > 0 || ret == EAGAIN)
> -		return;
> -
> -	if (n == TAP_BUF_BYTES)
> -		goto redo;
> -
> -	die("Error on tap device, exiting");
>  }
>  
>  /**

-- 
Stefano