Re: [PATCH v2 07/11] flow: Introduce 'sidx' type to represent one side of one flow

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

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

On Thu, Nov 30, 2023 at 10:21:16AM +0100, Stefano Brivio wrote:
> On Thu, 30 Nov 2023 11:37:40 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Nov 29, 2023 at 03:32:32PM +0100, Stefano Brivio wrote:
> > > On Mon, 27 Nov 2023 10:33:44 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > In a number of places, we use indices into the flow table to identify a
> > > > specific flow.  We also have cases where we need to identify a particular
> > > > side of a particular flow, and we expect those to become more common as
> > > > we generalise the flow table to cover more things.
> > > > 
> > > > To assist with that, introduces flow_sidx_t, an index type which identifies
> > > > a specific side of a specific flow in the table.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  flow.h       | 13 +++++++++++++
> > > >  flow_table.h | 36 ++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 49 insertions(+)
> > > > 
> > > > diff --git a/flow.h b/flow.h
> > > > index b6da516..3c90bbd 100644
> > > > --- a/flow.h
> > > > +++ b/flow.h
> > > > @@ -39,6 +39,19 @@ struct flow_common {
> > > >  #define FLOW_TABLE_PRESSURE		30	/* % of FLOW_MAX */
> > > >  #define FLOW_FILE_PRESSURE		30	/* % of c->nofile */
> > > >  
> > > > +/**
> > > > + * struct flow_sidx - ID for one side of a specific flow
> > > > + * @side:	Side referenced (0 or 1)
> > > > + * @flow:	Index of flow referenced
> > > > + */
> > > > +typedef struct flow_sidx {  
> > > 
> > > Implying my usual argument :) ...is there any advantage over using this
> > > simply as a struct?  
> > 
> > So, usually I too would prefer to use a struct as a struct, without a
> > typedef.  The reason I'm doing differently here, is that I want to
> > emphasise that for many purposes this can be treated like an index, in
> > particular that it's small and trivially copyable.  In particular it
> > should be passed by value, passing by reference would be silly.
> 
> Hmm, that was exactly my "not hiding" point though. The day somebody
> adds here:
> 
> 	char mood[RLIMIT_STACK_VAL + 1]; /* list of side emojis */
> 
> the typedef makes it still apparently okay to pass by value. If it's a
> struct, one surely has to check first.

Right.. and that's kind of the point.  If someone adds that, this
struct is no longer doing the job intended for it, and a wider
redesign is needed.  That's why there's also the static_assert()
verifying that flow_sidx_t fits in a u32.

> > That's kind of the opposite of what one tends to be conveying by
> > reminding users that they're working with a struct.
> 
> I see, but it's probably a matter of taste (passing structs by value
> doesn't personally make me nervous).
> 

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