Re: git: 220ee18f1964 - main - netinet/tcp_var.h: always define IS_FASTOPEN() for kernel compilation env

On Wed, Mar 13, 2024 at 11:21:23PM +0000, Konstantin Belousov wrote:
K> diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h
K> index 6f7f7115c2f4..7b5c57d39213 100644
K> --- a/sys/netinet/tcp_var.h
K> +++ b/sys/netinet/tcp_var.h
K> @@ -812,11 +812,13 @@ tcp_packets_this_ack(struct tcpcb *tp, tcp_seq ack)
K>  #define	ENTER_RECOVERY(t_flags) t_flags |= (TF_CONGRECOVERY | TF_FASTRECOVERY)
K>  #define	EXIT_RECOVERY(t_flags) t_flags &= ~(TF_CONGRECOVERY | TF_FASTRECOVERY)
K>  
K> -#if defined(_KERNEL) && !defined(TCP_RFC7413)
K> +#if defined(_KERNEL)
K> +#if !defined(TCP_RFC7413)
K>  #define	IS_FASTOPEN(t_flags)		(false)
K>  #else
K>  #define	IS_FASTOPEN(t_flags)		(t_flags & TF_FASTOPEN)
K>  #endif
K> +#endif
K>  
K>  #define	BYTES_THIS_ACK(tp, th)	(th->th_ack - tp->snd_una)

I know Konstantin in doing that to clear path for IPSEC changes, and the patch
does improve code.  So the message isn't addressed to him, rather it is for
other src-committers.

Using ifdefs that come from the kernel config inside include files that are not
ephemeral opt_foo.h is laying out a minefield for the future. Best case - for
yourself, worst case - for somebody else.

In the best case this ends in cryptic kernel failure builds, where your custom
kernel doesn't build, but GENERIC builds and it is not clear why. In the worst
case this creates runtime failures even more cryptic in their nature.

In this particular case TCP_RFC7413 comes via opt_inet.h.  Thus, if you got a
userland tool, e.g. netstat(1) using IS_FASTOPEN() it will always be false.
Again, netstat would be a best case.  A worst case would be netinet6 kernel
compilation unit that does not include opt_inet.h, but uses IS_FASTOPEN().

Other than that, we got 32 flags for t_flags and only one is obfuscated via a
macro.  I'd really like to remove the macro and test the flag directly.  Any
objections?

-- 
Gleb Smirnoff