I just discussed this with Kristof offline, but now posting for wider discussion. I think this approach should be applied outside pf(4) and used all around the network stack where we select between IPv4 and IPv6. That means, that the selector type name shouldn't have the PF_ or pf_ prefix. That would allow to start using the same technique all around without renaming inside pf(4).

Suggested name (feel free to come with a better one!):

#ifndef AF_INET
/* For binary compatibility reasons assign ip_version_t values to sa_family_t.  This will allow to call into old style code from the new style code and vice versa. */
#define AF_INET         2              /* IPv4 */
#define AF_INET6        28             /* IPv6 */
#endif
typedef enum {
        IPv4 = AF_INET,
        IPv6 = AF_INET6,
} ip_version_t;

In D46683#1064617, @glebius wrote:

I just discussed this with Kristof offline, but now posting for wider discussion. I think this approach should be applied outside pf(4) and used all around the network stack where we select between IPv4 and IPv6. That means, that the selector type name shouldn't have the PF_ or pf_ prefix. That would allow to start using the same technique all around without renaming inside pf(4).

I'll see if I can come up with a draft patch for this for wider discussion in the next couple of days.

Also I'd suggest simply putting this in socket.h and be done with it.

This has nothing to do with sockets. And suggested code won't compile, of course, since after preprocessor it will end up as 2 = 2.

Bjoern, the whole idea is to make the code more readable for somebody, who doesn't have a 30 year old history of BSD stack in their mind. Some things we are used to, aren't good, they are just what we are used to. They can be made better.

In D46683#1066394, @glebius wrote:

Also I'd suggest simply putting this in socket.h and be done with it.

This has nothing to do with sockets. And suggested code won't compile, of course, since after preprocessor it will end up as 2 = 2.

Bjoern, the whole idea is to make the code more readable for somebody, who doesn't have a 30 year old history of BSD stack in their mind. Some things we are used to, aren't good, they are just what we are used to. They can be made better.

Why not? This should work, right?

diff --git sys/sys/socket.h sys/sys/socket.h
index a1b220ce4bae..f773f970b779 100644
--- sys/sys/socket.h
+++ sys/sys/socket.h
@@ -216,12 +216,14 @@ struct accept_filter_arg {
  * Address families.
  */
 #define        AF_UNSPEC       0               /* unspecified */
-#if __BSD_VISIBLE
-#define        AF_LOCAL        AF_UNIX         /* local to host (pipes, portals) */
-#endif
 #define        AF_UNIX         1               /* standardized name for AF_LOCAL */
-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;
+
 #if __BSD_VISIBLE
+#define        AF_LOCAL        AF_UNIX         /* local to host (pipes, portals) */
 #define        AF_IMPLINK      3               /* arpanet imp addresses */
 #define        AF_PUP          4               /* pup protocols: e.g. BSP */
 #define        AF_CHAOS        5               /* mit CHAOS protocols */
@@ -249,9 +251,6 @@ struct accept_filter_arg {
 #define        AF_ISDN         26              /* Integrated Services Digital Network*/
 #define        AF_E164         AF_ISDN         /* CCITT E.164 recommendation */
 #define        pseudo_AF_KEY   27              /* Internal key-management function */
-#endif
-#define        AF_INET6        28              /* IPv6 */
-#if __BSD_VISIBLE
 #define        AF_NATM         29              /* native ATM access */
 #define        AF_ATM          30              /* ATM */
 #define pseudo_AF_HDRCMPLT 31          /* Used by BPF to not rewrite headers

Otherwise if you wanted a real "IP Version Type" then you'd define IPV4 as 4 and IPV6 as 6 but that's not what you are actually after, so maybe the enum is misnamed and should really be ip_af_t (whether here or in in.h)? If you call it af_ip_t it also perfectly fits in the above context.

In D46683#1066400, @bz wrote:

diff --git sys/sys/socket.h sys/sys/socket.h
index a1b220ce4bae..f773f970b779 100644
--- sys/sys/socket.h
+++ sys/sys/socket.h
@@ -216,12 +216,14 @@ struct accept_filter_arg {
 #define        AF_UNIX         1               /* standardized name for AF_LOCAL */
-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;
+

Otherwise if you wanted a real "IP Version Type" then you'd define IPV4 as 4 and IPV6 as 6 but that's not what you are actually after, so maybe the enum is misnamed and should really be ip_af_t (whether here or in in.h)? If you call it af_ip_t it also perfectly fits in the above context.

That won't quite work, because the ability to not define the IPv4 version for NOINET or IPv6 version for NOINET6 kernels is important. Otherwise the compiler would end up complaining we don't cover all enum values in the switch block. (Or we'd have to keep a default case, the removal of which was the impetus for introducing this enum in the first place.)
We have to define AF_INET even in a NOINET kernel, so we can't just ifdef that out.

As for the other remarks, I'm happy to rename this, ip_af_t seems reasonable, and I'm also happy to separate out commits for the definition of the new type and the use of it in pf, but I'd like us to agree on the enum definition (and especially the ifdef-ing of non-relevant versions) before I do that work.

In D46683#1066486, @kp wrote:

In D46683#1066400, @bz wrote:

diff --git sys/sys/socket.h sys/sys/socket.h
index a1b220ce4bae..f773f970b779 100644
--- sys/sys/socket.h
+++ sys/sys/socket.h
@@ -216,12 +216,14 @@ struct accept_filter_arg {
 #define        AF_UNIX         1               /* standardized name for AF_LOCAL */
-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;
+

Otherwise if you wanted a real "IP Version Type" then you'd define IPV4 as 4 and IPV6 as 6 but that's not what you are actually after, so maybe the enum is misnamed and should really be ip_af_t (whether here or in in.h)? If you call it af_ip_t it also perfectly fits in the above context.

That won't quite work, because the ability to not define the IPv4 version for NOINET or IPv6 version for NOINET6 kernels is important. Otherwise the compiler would end up complaining we don't cover all enum values in the switch block. (Or we'd have to keep a default case, the removal of which was the impetus for introducing this enum in the first place.)
We have to define AF_INET even in a NOINET kernel, so we can't just ifdef that out.

As for the other remarks, I'm happy to rename this, ip_af_t seems reasonable, and I'm also happy to separate out commits for the definition of the new type and the use of it in pf, but I'd like us to agree on the enum definition (and especially the ifdef-ing of non-relevant versions) before I do that work.

Oh doh. Got you now. So the "IPv4" and "IPv6" are never to be used; this is more clever than I had gotten from the proposed commit message; can you leave a comment around the enum definition saying that and in that case we can name the "IPv4" and "IPv6" away to something a lot more complicated so we'll never have a conflict?

And thanks for offering to split this up.

-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;

This will break any third party software that checks for #ifdef AF_INET

Let's just do not touch user visible headers for this change. Let's forget about socket and about address families. Let's focus on that we need a enum to tell IPv4 packet header from IPv6 packet header, used by various kernel KPIs. There shall be no visible user changes.

The pf bits are in D46809

One issue is that this breaks the NOIP kernel. For some reason I did not pick this up on previous iterations (I was sure make universe passed on them, but I don't see how), but we can't have an empty enum definition. clang complains:

/usr/src/sys/netinet/in.h:445:1: error: use of empty enum
  445 | } ip_af_t;
      | ^
1 error generated.

I'm not sure what to do about that. Removing the enum entirely for NOIP configs would mean adding #if defined(INET) || defined(INET6) all over the place, and we're already adding a bunch of #ifdef INET as it is.

I see no good reason to ifdef pieces of enum. Makes sense to disable some code for NOINET or NOINET6 kernels, but what's the point in disabling a piece of declaration?

In D46683#1067412, @glebius wrote:

I see no good reason to ifdef pieces of enum. Makes sense to disable some code for NOINET or NOINET6 kernels, but what's the point in disabling a piece of declaration?

If we don't we upset the compiler in LINT-NOINET (and LINT-NOINET6) kernels:

/usr/src/sys/netpfil/pf/pf_lb.c:769:13: error: enumeration value 'IP_AF_4' not handled in switch [-Werror,-Wswitch]
  769 |                                 switch (pd->af) {
      |                                         ^~~~~~

It's fairly common to have code specific to one address family or another not be compiled in those kernels, so we end up with switch statements that don't handle all options. We can prevent this by adding a default: panic("Unknown af %d", af) case, but removing that was the reason to go down this path in the first place.

In D46683#1067664, @kp wrote:

In D46683#1067412, @glebius wrote:

I see no good reason to ifdef pieces of enum. Makes sense to disable some code for NOINET or NOINET6 kernels, but what's the point in disabling a piece of declaration?

If we don't we upset the compiler in LINT-NOINET (and LINT-NOINET6) kernels:

/usr/src/sys/netpfil/pf/pf_lb.c:769:13: error: enumeration value 'IP_AF_4' not handled in switch [-Werror,-Wswitch]
  769 |                                 switch (pd->af) {
      |                                         ^~~~~~

It's fairly common to have code specific to one address family or another not be compiled in those kernels, so we end up with switch statements that don't handle all options. We can prevent this by adding a default: panic("Unknown af %d", af) case, but removing that was the reason to go down this path in the first place.

Right, but then you absolutely need to handle the case that the code UNDER the switch statement not having its pre-requisites met!

It's no good to have a NOINET kernel compile and then you just run a bunch of address struct checks with invalid or zero data. Like, those code paths likely make no sense to be executed in the first place.

In D46683#1067664, @kp wrote:

If we don't we upset the compiler in LINT-NOINET (and LINT-NOINET6) kernels:

Damn, that's too bad :( Putting #ifdefs from opt_foo.h into installable headers is also planting a minefield. Let me think a bit more on possible options here.