Re: [PATCH v2 03/10] flow, tcp: Consolidate flow pointer<->index helpers

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 28 Aug 2023 15:41:39 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Both tcp.c and tcp_splice.c define CONN_IDX() variants to find the index
> of their connection structures in the connection table, now become the
> unified flow table.  We can easily combine these into a common helper.
> While we're there, add some trickery for some additional type safety.
> 
> They also define their own CONN() versions, which aren't so easily combined
> since they need to return different types, but we can have them use a
> common helper.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  flow_table.h | 20 ++++++++++++++++++++
>  tcp.c        | 49 ++++++++++++++++++++++++-------------------------
>  tcp_conn.h   |  2 +-
>  tcp_splice.c | 17 ++++++++---------
>  4 files changed, 53 insertions(+), 35 deletions(-)
> 
> diff --git a/flow_table.h b/flow_table.h
> index c4c646b..dd4875e 100644
> --- a/flow_table.h
> +++ b/flow_table.h
> @@ -22,4 +22,24 @@ union flow {
>  /* Global Flow Table */
>  extern union flow flowtab[];
>  
> +
> +/** flowk_idx - Index of flow from common structure

"flowk"

> + * @f:	Common flow fields pointer
> + *
> + * Return: index of @f in the flow table
> + */
> +static inline unsigned flow_idx(const struct flow_common *f)
> +{
> +	return (union flow *)f - flowtab;
> +}
> +
> +/** FLOW_IDX - Find the index of a flow
> + * @f_:	Flow pointer, either union flow * or protocol specific
> + *
> + * Return: index of @f in the flow table
> + */
> +#define FLOW_IDX(f_)		(flow_idx(&(f_)->f))
> +
> +#define FLOW(index)		(&flowtab[(index)])
> +
>  #endif /* FLOW_TABLE_H */
> diff --git a/tcp.c b/tcp.c
> index 7994197..7d2e89d 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -561,8 +561,7 @@ tcp6_l2_flags_buf[TCP_FRAMES_MEM];
>  
>  static unsigned int tcp6_l2_flags_buf_used;
>  
> -#define CONN(index)		(&flowtab[(index)].tcp)
> -#define CONN_IDX(conn)		((union flow *)(conn) - flowtab)
> +#define CONN(index)		(&FLOW(index)->tcp)

Extra parentheses are not needed, but I've been wondering for a bit why
you would use "&FLOW" here, as FLOW(x) already resolves to &flowtab[x]...
perhaps (&(FLOW(index)->tcp)) is actually easier to read?

-- 
Stefano