Re: [PATCH v3 01/15] flow: Common data structures for tracking flow addresses

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

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

On Sat, Jan 13, 2024 at 11:50:40PM +0100, Stefano Brivio wrote:
> On Thu, 21 Dec 2023 18:02:23 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Handling of each protocol needs some degree of tracking of the addresses
> > and ports at the end of each connection or flow.  Sometimes that's explicit
> > (as in the guest visible addresses for TCP connections), sometimes implicit
> > (the bound and connected addresses of sockets).
> > 
> > To allow more general and robust handling, and more consistency across
> > protocols we want to uniformly track the address and port at each end of
> > the connection.  Furthermore, because we allow port remapping, and we
> > sometimes need to apply NAT, the addresses and ports can be different as
> > seen by the guest/namespace and as by the host.
> > 
> > Introduce 'struct flowside' to keep track of common information related to
> > one side of each flow.  For now that's the addresses, ports and the pif id.
> > Store two of these in the common fields of a flow to track that information
> > for both sides.
> > 
> > For now we just introduce the structure and fields themselves, along with
> > a simple helper.  Later patches will actually use these to store useful
> > information.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.h     | 33 +++++++++++++++++++++++++++++++++
> >  passt.h    |  2 ++
> >  tcp_conn.h |  1 -
> >  3 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/flow.h b/flow.h
> > index 48a0ab4..e090ba0 100644
> > --- a/flow.h
> > +++ b/flow.h
> > @@ -27,11 +27,44 @@ extern const char *flow_type_str[];
> >  #define FLOW_TYPE(f)							\
> >          ((f)->type < FLOW_NUM_TYPES ? flow_type_str[(f)->type] : "?")
> >  
> > +/**
> > + * struct flowside - Common information for one side of a flow
> > + * @eaddr:	Endpoint address (remote address from passt's PoV)
> > + * @faddr:	Forwarding address (local address from passt's PoV)
> > + * @eport:	Endpoint port
> > + * @fport:	Forwarding port
> > + * @pif:	pif ID on which this side of the flow exists
> > + */
> > +struct flowside {
> > +	union inany_addr	faddr;
> > +	union inany_addr	eaddr;
> > +	in_port_t		fport;
> > +	in_port_t		eport;
> > +	uint8_t			pif;
> > +};
> > +static_assert(_Alignof(struct flowside) == _Alignof(uint32_t),
> > +	      "Unexpected alignment for struct flowside");
> > +
> 
> Nits:
> 
> > +/** flowside_complete - Check if flowside is fully initialized
> 
> flowside_complete(). By the way, shouldn't we call it something more
> descriptive such as flowside_is_complete()? It's not obvious this is
> a check otherwise.
> 
> > + * @fside:	flowside to check
> 
> * Return: true if pif, addresses and ports are set
> 
> ...or something of that sort.
> 
> > + */
> > +static inline bool flowside_complete(const struct flowside *fside)
> > +{
> > +	return fside->pif != PIF_NONE &&
> > +		!IN6_IS_ADDR_UNSPECIFIED(&fside->faddr) &&
> > +		!IN6_IS_ADDR_UNSPECIFIED(&fside->eaddr) &&
> > +		fside->fport != 0 && fside->eport != 0;
> 
> I would align everything after 'return ', that is:
> 
> 	return fside->pif != PIF_NONE &&
> 	       !IN6_IS_ADDR_UNSPECIFIED(&fside->faddr) &&
> 	       !IN6_IS_ADDR_UNSPECIFIED(&fside->eaddr) &&
> 	       fside->fport != 0 && fside->eport != 0;
> 
> mostly for consistency -- not a strong preference.

All seems reasonable, updated accordingly.  I've also updated
'flow_complete' to 'flow_is_complete' later in the series.

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