Re: [PATCH 1/4] tap: Don't pcap frames that didn't get sent

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 27 Jan 2023 16:11:07 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In tap_send_frames() we send a number of frames to the tap device, then
> also write them to the pcap capture file (if configured).  However the tap
> send can partially fail (short write()s or similar), meaning that some
> of the requested frames weren't actually sent, but we still write those
> frames to the capture file.
> 
> We do give a debug message in this case, but it's misleading to add frames
> that we know weren't sent to the capture file.  Rework to avoid this.

To be really "correct", I guess we should also truncate messages in
captures if they were sent partially, by returning the number of bytes
sent from tap_send_frames_{pasta,passt}() and then modifying the
argument to pcap_frame() in the pcap_multiple() loop.

This is relevant because, if a packet has a checksum, we could consider
it lost while checking captures.

Still, it's a vast improvement on the original, so I would apply this
even like it is -- except for two nits, below:

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tap.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index af9bc15..dd22490 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -309,10 +309,12 @@ void tap_icmp6_send(const struct ctx *c,
>   * @iov:	Array of buffers, each containing one frame
>   * @n:		Number of buffers/frames in @iov
>   *
> + * Returns: number of frames successfully sent

For consistency: "Return:" -- I see now that one slipped through in
pcap_frame(). I can "fix" this on merge or in a follow-up patch, too.

> + *
>   * #syscalls:pasta write
>   */
> -static void tap_send_frames_pasta(struct ctx *c,
> -				  const struct iovec *iov, size_t n)
> +static size_t tap_send_frames_pasta(struct ctx *c,
> +				    const struct iovec *iov, size_t n)
>  {
>  	size_t i;
>  
> @@ -324,6 +326,8 @@ static void tap_send_frames_pasta(struct ctx *c,
>  			i--;
>  		}
>  	}
> +
> +	return n;
>  }
>  
>  /**
> @@ -356,10 +360,12 @@ static void tap_send_remainder(const struct ctx *c, const struct iovec *iov,
>   * @iov:	Array of buffers, each containing one frame
>   * @n:		Number of buffers/frames in @iov
>   *
> + * Returns: number of frames successfully sent

Same here.

-- 
Stefano