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

[Stefano Brivio <sbrivio@redhat.com>]

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.

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

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.

-- 
Stefano