tcp: fix ports

[tuexen]

Summary

inline is only supported in C99 and newer. However, some ports use -ansi, which is equivalent to -std=c89, and therefore fail to compile. Using __inline was suggested by dim@.

[imp]

This looks right to me...

[dim]

I think this is OK as a workaround, but as @kib said, do we really need these symbols in the global namespace for tcp.h? Are user space applications really supposed to use these interfaces?

[tuexen]

In D43231#985299, @dim wrote:

I think this is OK as a workaround, but as @kib said, do we really need these symbols in the global namespace for tcp.h? Are user space applications really supposed to use these interfaces?

I would suggest that. If not, they need to get the handling of th_flags and th_x2 right by using functionality like the one provided by the two functions.

[rscheff]

In D43231#985299, @dim wrote:

I think this is OK as a workaround, but as @kib said, do we really need these symbols in the global namespace for tcp.h? Are user space applications really supposed to use these interfaces?

With TCP over UDP, a userspace application can choose to implement it's own TCP stack; the "th_x2" field is an artifact from when there was little expectation to have any more than the usual 6 flags - but formally, the TCP header always featured a 12 bit field for Flags, which were never split up into "th_flags" and "th_x2". Also, the ipfw code internally uses one of these (currently reserved) flags for local purposes, and "normalizes" some header fields.

For examples in userspace, look at tcpdump, read_bbrlog (decodes TCP black box logging), wireshark - having the TCP flags as a contingent field of 12 bits, and canoncical names certainly helps portability...

[kib]

It is fine to enforce use of the accessors for our kernel, but I do not imagine the procedure how would you force it over the third-party developers.

IMO we must not pollute user namespace even more. Either hide the accessors under _KERNEL, or put them into the implementation namespace (__tcp_get_flags).

[tuexen]

In D43231#985388, @kib wrote:

It is fine to enforce use of the accessors for our kernel, but I do not imagine the procedure how would you force it over the third-party developers.

It is not so much about enforcing them to use something, it is, in my view, more something about offering what they need to do anyway.

IMO we must not pollute user namespace even more. Either hide the accessors under _KERNEL, or put them into the implementation namespace (__tcp_get_flags).

I'm fine with putting it under _KERNEL, but find it strange to make the flags available to userland, but keep the accessors private. Please note that you cannot use the TH_AE flag like any other TCP flags:

th->th_flags |= TH_AE;

will just not work.

Before the introduction of TH_AE this was not a problem. All flags could be accessed simply by using th_flags. However, this is not true anymore.

[kib]

In D43231#985415, @tuexen wrote:

In D43231#985388, @kib wrote:

It is fine to enforce use of the accessors for our kernel, but I do not imagine the procedure how would you force it over the third-party developers.

It is not so much about enforcing them to use something, it is, in my view, more something about offering what they need to do anyway.

IMO we must not pollute user namespace even more. Either hide the accessors under _KERNEL, or put them into the implementation namespace (__tcp_get_flags).

I'm fine with putting it under _KERNEL, but find it strange to make the flags available to userland, but keep the accessors private.

With __get/__set names, they are not private, but are in the impl namespace.

Please note that you cannot use the TH_AE flag like any other TCP flags:

th->th_flags |= TH_AE;

will just not work.

Before the introduction of TH_AE this was not a problem. All flags could be accessed simply by using th_flags. However, this is not true anymore.

[• karels]

Most of netinet/tcp.h is already inside #ifdef __BSD_VISIBLE, so it would be "legal" to use tcp_[gs]et_flags. It would be better to avoid intruding into the BSD namespace, though, using __tcp_[gs]et_flags for user space, and providing aliases for the kernel. I can imagine network tools using these names.

[rscheff]

In D43231#985388, @kib wrote:

It is fine to enforce use of the accessors for our kernel, but I do not imagine the procedure how would you force it over the third-party developers.

IMO we must not pollute user namespace even more. Either hide the accessors under _KERNEL, or put them into the implementation namespace (__tcp_get_flags).

We've had these accessors for some time in the _KERNEL. (D34130 see for some history)

However, as shown in D43172, not all the places in the core OS components which deal with TCP headers are in _KERNEL scope.

Extending the tcphdr.th_flags to the full 12 bits, and making a union mapping of the tcphdr.th_x2 would have been one option - but there are no easy ways to convert from network order to host order in a portable way that I could see for all eventualities (not all access to the tcphdr struct use a ntoh* function call, or in a consistent manner; also, ntoh* functions map to the usual suspects of multiple-of-8 fields, not 4-bit nibble wide fields...)

The final alternative could be to let every userspace application blissfully ignore that there are actually up to 12 TCP header flags (since rfc793, 1981), and replicate the accessor locally when deemed necessary....

[tuexen]

In D43231#985422, @kib wrote:

In D43231#985415, @tuexen wrote:

In D43231#985388, @kib wrote:

It is fine to enforce use of the accessors for our kernel, but I do not imagine the procedure how would you force it over the third-party developers.

It is not so much about enforcing them to use something, it is, in my view, more something about offering what they need to do anyway.

IMO we must not pollute user namespace even more. Either hide the accessors under _KERNEL, or put them into the implementation namespace (__tcp_get_flags).

I'm fine with putting it under _KERNEL, but find it strange to make the flags available to userland, but keep the accessors private.

With __get/__set names, they are not private, but are in the impl namespace.

OK. I can do that. But the intention of this patch was to un-break the ports. This part can be done by replacing inline with __inline.
I think we all agree on this.
In addition to the fix, you are proposing to rename the functions, which requires a larger patch, since we also have to change all occurrences of the accessors. Do you think it is a good idea to do this in the same commit? In my view fixing ports and renaming things are two different things, which should be done separately.

Please note that you cannot use the TH_AE flag like any other TCP flags:

th->th_flags |= TH_AE;

will just not work.

Before the introduction of TH_AE this was not a problem. All flags could be accessed simply by using th_flags. However, this is not true anymore.

[tuexen]

In D43231#985443, @rscheff wrote:

In D43231#985388, @kib wrote:

It is fine to enforce use of the accessors for our kernel, but I do not imagine the procedure how would you force it over the third-party developers.

IMO we must not pollute user namespace even more. Either hide the accessors under _KERNEL, or put them into the implementation namespace (__tcp_get_flags).

We've had these accessors for some time in the _KERNEL. (D34130 see for some history)

However, as shown in D43172, not all the places in the core OS components which deal with TCP headers are in _KERNEL scope.

Which ones are not? Aren't they all kernel modules?

Extending the tcphdr.th_flags to the full 12 bits, and making a union mapping of the tcphdr.th_x2 would have been one option - but there are no easy ways to convert from network order to host order in a portable way that I could see for all eventualities (not all access to the tcphdr struct use a ntoh* function call, or in a consistent manner; also, ntoh* functions map to the usual suspects of multiple-of-8 fields, not 4-bit nibble wide fields...)

The final alternative could be to let every userspace application blissfully ignore that there are actually up to 12 TCP header flags (since rfc793, 1981), and replicate the accessor locally when deemed necessary....

[tuexen]

In D43231#985436, @karels wrote:

Most of netinet/tcp.h is already inside #ifdef __BSD_VISIBLE, so it would be "legal" to use tcp_[gs]et_flags. It would be better to avoid intruding into the BSD namespace, though, using __tcp_[gs]et_flags for user space, and providing aliases for the kernel. I can imagine network tools using these names.

What do you mean by "aliases for the kernel"? I'm not familiar with the impl. namespace...

[kib]

I believe the mention of the aliases is a suggestion to do something like

#ifdef _KERNEL
#define tcp_get_flags(a) __tcp_get_flags(a)
#endif

which eliminates the need to do the pass over the kernel sources.

Of course s/inline/__inline/ should be a separate change. It just that the whole state of the namespace pollution and standard obeying by this header was not too good that triggered the discussion and then extended its scope.

[rscheff]

In D43231#985447, @tuexen wrote:

In D43231#985443, @rscheff wrote:

However, as shown in D43172, not all the places in the core OS components which deal with TCP headers are in _KERNEL scope.

Which ones are not? Aren't they all kernel modules?

To access tcphdr.th_flags / th_x2:

usr.sbin/traceroute6
usr.sbin/ppp (debugging should print full tcp flags)
(share/dtrace/tcpdebug example should print full tcp flags)
sbin/ipf/ipsend set's up a tcphdr, also should print full tcp flags
sbin/ipf/libipf should support and print full tcp flags
sbin/ipf/ipmon should support full tcp flags

ideally, contrib/tcpdump and contrib/traceroute would be using this (but for portability, a local accessor is probably better)

cddl/lib/libdtrace/tcp.d (but that's probably better served by direct tcphdr.th_x2 access to expand it...

[tuexen]

In D43231#985459, @kib wrote:

I believe the mention of the aliases is a suggestion to do something like

#ifdef _KERNEL
#define tcp_get_flags(a) __tcp_get_flags(a)
#endif

which eliminates the need to do the pass over the kernel sources.

Of course s/inline/__inline/ should be a separate change. It just that the whole state of the namespace pollution and standard obeying by this header was not too good that triggered the discussion and then extended its scope.

The fix has been committed. The cleanup is now discussed in D43245.