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

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

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

On Wed, Jun 12, 2024 at 08:34:21AM +0200, Stefano Brivio wrote:
> 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()?

Good point.

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

Yeah, fair enough.

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