Re: [PATCH v5 01/19] flow: Clarify and enforce flow state transitions

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

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

On Fri, May 17, 2024 at 01:00:44PM +0200, Stefano Brivio wrote:
> On Fri, 17 May 2024 13:57:58 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, May 16, 2024 at 11:30:58AM +0200, Stefano Brivio wrote:
> > > On Tue, 14 May 2024 11:03:19 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > [...]
> > >
> > > >  /**
> > > >   * struct flow_common - Common fields for packet flows
> > > > + * @state:	State of the flow table entry
> > > >   * @type:	Type of packet flow
> > > >   */
> > > >  struct flow_common {
> > > > +	uint8_t		state;  
> > > 
> > > In this case, I would typically do
> > > (https://seitan.rocks/seitan/tree/common/gluten.h?id=5a9302bab9c9bb3d1577f04678d074fb7af4115f#n53):
> > > 
> > > #ifdef __GNUC__
> > > 	enum flow_state		state:8;
> > > #else
> > > 	uint8_t			state;
> > > #endif  
> > 
> > I don't object to that, but I have two questions
> > 
> >   - What's the advantage to using the explicit enum?  Is that for the
> >     benefit of static checkers and/or compiler diagnostics?
> 
> Yes: if we assign a value that's not in the enum, I expect static
> checkers to complain. But also for humans: even with that ifdef, a
> reader would know right away what values that might have.
> 
> >     AFAIK C
> >     itself doesn't really treat enums any differently to integer
> >     types.
> 
> Right.
> 
> >   - What's the need for GNUC?  Are enum bitfields a gnu extension?
> 
> They're rather permitted by gcc's interpretation of the standard:
> 
>   https://gcc.gnu.org/onlinedocs/gcc-14.1.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
> 
>   Allowable bit-field types other than _Bool, signed int, and unsigned
>   int (C99 and C11 6.7.2.1).
> 
>   Other integer types, such as long int, and enumerated types are permitted
>   even in strictly conforming mode.
> 
> but C11 says (6.7.2.1):
> 
>   A bit-field shall have a type that is a qualified or unqualified
>   version of _Bool, signed int, unsigned int, or some other
>   implementation-defined type. It is implementation-defined whether
>   atomic types are permitted.
> 
> Is an enum a "version" of those? Maybe not. That was at least the
> interpretation adopted by older gcc versions, up to 4.7.4:
> 
>   https://gcc.gnu.org/onlinedocs/gcc-4.7.4/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
> 
>   Allowable bit-field types other than _Bool, signed int, and unsigned
>   int (C99 6.7.2.1).
> 
>   No other types are permitted in strictly conforming mode.
> 
> Did that change from C99? Not really:
> 
>   A bit-field shall have a type that is a qualified or unqualified
>   version of _Bool, signed int, unsigned int, or some other
>   implementation-defined type.
> 
> >     Even versus C11, which we already require?
> 
> Yes, I would say it doesn't change things. Only C23 would improve that:
>   https://open-std.org/JTC1/SC22/WG14/www/docs/n3030.htm#design-constant.type
> 
> by allowing us to define the underlying type.

Ah, ok.  Makes sense, I've made that change.

> > > ...and in any case we need to make sure to assign single values in the
> > > enum above: there are no guarantees that FLOW_STATE_ACTIVE is 3
> > > otherwise (except for that static_assert(), but that's not its purpose).  
> > 
> > I'm not clear how this comment relates to the one before.
> 
> It's unrelated, but:
> 
> > AFAIK
> > nothing in here (or the rest of the series) relies on the specific
> > numeric values of the flow state values (although we do rely on them
> > being ordered as written in some places).
> 
> while we rely on the fact that no value is bigger than 255, I realised
> that the standards already guarantee that values start from 0 and every
> subsequent constant is defined as one more than the previous one, all
> the way from C90 to C11, so this would actually be fine.

Right, I was expecting that behaviour - the way we define
FLOW_NUM_STATES and similar things in a bunch of places relies on
this.

> Sorry, I don't know exactly why I thought that wouldn't be the case, I
> was pretty sure of the opposite until I checked.

Ok.  I've scattered in some extra static_assert()s in the vicinity, too.

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