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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]

On Wed, Feb 15, 2023 at 01:17:25PM +0100, Stefano Brivio wrote:
> 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.

True.. that only applies for the pasta case, though.  For passt we
always send a whole frame, or the stream would get out of sync.

> 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.

Fixed.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]