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

[Stefano Brivio <sbrivio@redhat.com>]

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.

-- 
Stefano