Re: [PATCH v5 3/8] tap: refactor packets handling functions

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 12 Jun 2024 16:18:59 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 12, 2024 at 12:09:50AM +0200, Stefano Brivio wrote:
> > On Wed,  5 Jun 2024 17:21:24 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> > > Consolidate pool_tap4() and pool_tap6() into pool_flush_all(),
> > > and tap4_handler() and tap6_handler() into tap_handler_all().
> > > Create a generic packet_add_all() to consolidate packet
> > > addition logic and reduce code duplication.
> > > 
> > > The purpose is to ease the export of these functions to use
> > > them with the vhost-user backend.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > >  tap.c | 113 +++++++++++++++++++++++++++++++++-------------------------
> > >  tap.h |   7 ++++
> > >  2 files changed, 71 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/tap.c b/tap.c
> > > index 2ea08491a51f..5fb3cb83f3d2 100644
> > > --- a/tap.c
> > > +++ b/tap.c
> > > @@ -920,6 +920,61 @@ append:
> > >  	return in->count;
> > >  }
> > >  
> > > +/**
> > > + * pool_flush() - Flush both IPv4 and IPv6 packet pools
> > > + */
> > > +void pool_flush_all(void)
> > > +{
> > > +	pool_flush(pool_tap4);
> > > +	pool_flush(pool_tap6);
> > > +}
> > > +
> > > +/**
> > > + * tap_handler_all() - IPv4/IPv4 and ARP packet handler for tap file descriptor  
> > 
> > IPv4/IPv6
> >   
> > > + * @c:		Execution context
> > > + * @now:	Current timestamp
> > > + */
> > > +void tap_handler_all(struct ctx *c, const struct timespec *now)  
> > 
> > I wonder if this shouldn't be named tap_handler() instead. As we
> > already have tap_handler_passt() and tap_handler_pasta(), it's not
> > immediately clear what "all" refers to.  
> 
> I concur, I think tap_handler() is a better name.
> 
> > > +{
> > > +	tap4_handler(c, pool_tap4, now);
> > > +	tap6_handler(c, pool_tap6, now);
> > > +}
> > > +
> > > +/**
> > > + * packet_add_all_do() - Add a packet to the appropriate TAP pool  
> > 
> > A couple of remarks here:
> > 
> > - it's a bit unexpected that this is still in tap.c (it adds packets to
> >   a pool, it should be in packet.c judging by this name/description).
> >   If we call it tap_queue_packet(), then it probably makes more sense?
> > 
> > - this does more than adding a packet to a pool. It's probably useless
> >   to describe in detail what this does, as the function body is anyway
> >   rather short and clear, but the current description could be a bit
> >   misleading. What about "Queue/capture packet, update notion of
> >   guest MAC address"?  
> 
> So, given this, I think it does make more sense for this to be in
> tap.c than packet.c.  How about calling it tap_add_packets().

It's a single packet it adds, so perhaps tap_add_packet()?

> > - what happens if you just call packet_add() from here, without dealing
> >   with 'func' and 'line'? I think it's fine to print in tracing output
> >   name and lines from this function, instead of the ones from the
> >   caller. It's obvious who the caller is  
> 
> It is as of this patch, but I believe the idea is this will also be
> called from VU code down the line.

Okay, but even then, it would be obvious from previous tracing output
who the caller is. What's relevant here is to log that something went
wrong while adding a packet to an IPv4 or IPv6 pool. I don't think we
should bother passing around function name and line to log anything
else.

> > > + * @c:		Execution context
> > > + * @l2len:	Total L2 packet length
> > > + * @p:		Packet buffer
> > > + * @func:	For tracing: name of calling function, NULL means no trace()
> > > + * @line:	For tracing: caller line of function call
> > > + */
> > > +void packet_add_all_do(struct ctx *c, ssize_t l2len, char *p,
> > > +		       const char *func, int line)
> > > +{
> > > +	const struct ethhdr *eh;
> > > +
> > > +	pcap(p, l2len);
> > > +
> > > +	eh = (struct ethhdr *)p;
> > > +
> > > +	if (memcmp(c->mac_guest, eh->h_source, ETH_ALEN)) {
> > > +		memcpy(c->mac_guest, eh->h_source, ETH_ALEN);
> > > +		proto_update_l2_buf(c->mac_guest, NULL);
> > > +	}
> > > +
> > > +	switch (ntohs(eh->h_proto)) {
> > > +	case ETH_P_ARP:
> > > +	case ETH_P_IP:
> > > +		packet_add_do(pool_tap4, l2len, p, func, line);
> > > +		break;
> > > +	case ETH_P_IPV6:
> > > +		packet_add_do(pool_tap6, l2len, p, func, line);
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +}
> > > +
> > >  /**
> > >   * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
> > >   * @c:		Execution context
> > > @@ -946,7 +1001,6 @@ static void tap_sock_reset(struct ctx *c)
> > >  void tap_handler_passt(struct ctx *c, uint32_t events,
> > >  		       const struct timespec *now)
> > >  {
> > > -	const struct ethhdr *eh;
> > >  	ssize_t n, rem;
> > >  	char *p;
> > >  
> > > @@ -959,8 +1013,7 @@ redo:
> > >  	p = pkt_buf;
> > >  	rem = 0;
> > >  
> > > -	pool_flush(pool_tap4);
> > > -	pool_flush(pool_tap6);
> > > +	pool_flush_all();
> > >  
> > >  	n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT);
> > >  	if (n < 0) {
> > > @@ -987,38 +1040,18 @@ redo:
> > >  		/* Complete the partial read above before discarding a malformed
> > >  		 * frame, otherwise the stream will be inconsistent.
> > >  		 */
> > > -		if (l2len < (ssize_t)sizeof(*eh) ||
> > > +		if (l2len < (ssize_t)sizeof(struct ethhdr) ||
> > >  		    l2len > (ssize_t)ETH_MAX_MTU)
> > >  			goto next;
> > >  
> > > -		pcap(p, l2len);
> > > -
> > > -		eh = (struct ethhdr *)p;
> > > -
> > > -		if (memcmp(c->mac_guest, eh->h_source, ETH_ALEN)) {
> > > -			memcpy(c->mac_guest, eh->h_source, ETH_ALEN);
> > > -			proto_update_l2_buf(c->mac_guest, NULL);
> > > -		}
> > > -
> > > -		switch (ntohs(eh->h_proto)) {
> > > -		case ETH_P_ARP:
> > > -		case ETH_P_IP:
> > > -			packet_add(pool_tap4, l2len, p);
> > > -			break;
> > > -		case ETH_P_IPV6:
> > > -			packet_add(pool_tap6, l2len, p);
> > > -			break;
> > > -		default:
> > > -			break;
> > > -		}
> > > +		packet_add_all(c, l2len, p);
> > >  
> > >  next:
> > >  		p += l2len;
> > >  		n -= l2len;
> > >  	}
> > >  
> > > -	tap4_handler(c, pool_tap4, now);
> > > -	tap6_handler(c, pool_tap6, now);
> > > +	tap_handler_all(c, now);
> > >  
> > >  	/* We can't use EPOLLET otherwise. */
> > >  	if (rem)
> > > @@ -1043,35 +1076,18 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
> > >  redo:
> > >  	n = 0;
> > >  
> > > -	pool_flush(pool_tap4);
> > > -	pool_flush(pool_tap6);
> > > +	pool_flush_all();
> > >  restart:
> > >  	while ((len = read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n)) > 0) {
> > > -		const struct ethhdr *eh = (struct ethhdr *)(pkt_buf + n);
> > >  
> > > -		if (len < (ssize_t)sizeof(*eh) || len > (ssize_t)ETH_MAX_MTU) {
> > > +		if (len < (ssize_t)sizeof(struct ethhdr) ||
> > > +		    len > (ssize_t)ETH_MAX_MTU) {
> > >  			n += len;
> > >  			continue;
> > >  		}
> > >  
> > > -		pcap(pkt_buf + n, len);
> > >  
> > > -		if (memcmp(c->mac_guest, eh->h_source, ETH_ALEN)) {
> > > -			memcpy(c->mac_guest, eh->h_source, ETH_ALEN);
> > > -			proto_update_l2_buf(c->mac_guest, NULL);
> > > -		}
> > > -
> > > -		switch (ntohs(eh->h_proto)) {
> > > -		case ETH_P_ARP:
> > > -		case ETH_P_IP:
> > > -			packet_add(pool_tap4, len, pkt_buf + n);
> > > -			break;
> > > -		case ETH_P_IPV6:
> > > -			packet_add(pool_tap6, len, pkt_buf + n);
> > > -			break;
> > > -		default:
> > > -			break;
> > > -		}
> > > +		packet_add_all(c, len, pkt_buf + n);
> > >  
> > >  		if ((n += len) == TAP_BUF_BYTES)
> > >  			break;
> > > @@ -1082,8 +1098,7 @@ restart:
> > >  
> > >  	ret = errno;
> > >  
> > > -	tap4_handler(c, pool_tap4, now);
> > > -	tap6_handler(c, pool_tap6, now);
> > > +	tap_handler_all(c, now);
> > >  
> > >  	if (len > 0 || ret == EAGAIN)
> > >  		return;
> > > diff --git a/tap.h b/tap.h
> > > index 2285a87093f9..3ffb7d6c3a91 100644
> > > --- a/tap.h
> > > +++ b/tap.h
> > > @@ -70,5 +70,12 @@ void tap_handler_passt(struct ctx *c, uint32_t events,
> > >  		       const struct timespec *now);
> > >  int tap_sock_unix_open(char *sock_path);
> > >  void tap_sock_init(struct ctx *c);
> > > +void pool_flush_all(void);
> > > +void tap_handler_all(struct ctx *c, const struct timespec *now);
> > > +
> > > +void packet_add_all_do(struct ctx *c, ssize_t l2len, char *p,
> > > +		       const char *func, int line);
> > > +#define packet_add_all(p, l2len, start)					\
> > > +	packet_add_all_do(p, l2len, start, __func__, __LINE__)
> > >  
> > >  #endif /* TAP_H */  
> >   
> 

-- 
Stefano