Split out changes from D47063 which are strictly
speaking changing the functionality.
Add support for the AE Flag in the TCP header to pf and ppp
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 59990 Build 56875: arc lint + arc unit
Event Timeline
Comment Actions
pf bits look good.
| usr.sbin/ppp/ip.c | ||
|---|---|---|
| 563 | It looks like we only use this in a loop that ends when mask == 0x40, so we're never going to see ECE, CWR or AE there, right? | |
| sys/netpfil/ipfilter/netinet/ip_compat.h | ||
|---|---|---|
| 698 | I found TCPF_ALL is used in printtcpflags() from sbin/ipf/libipf/printtcpflags.c. Would you please also add these in flagset[] and flags[] from sbin/ipf/libipf/flags.c? | |
Comment Actions
| usr.sbin/ppp/ip.c | ||
|---|---|---|
| 563 | Then let's remove the magic number, and decode all TCP header flags properly for logging instead... | |
Comment Actions
My current concern is that the definition and the usage of the super set macro TH_FLAGS or TCPF_ALL are inconsistent. For example, TH_ECE is in TH_FLAGS, but TH_ECN is in TCPF_ALL.
| sbin/ipf/ipsend/ipsend.c | ||
|---|---|---|
| 368–393 ↗ | (On Diff #144885) | Looks the same typo is among these lines, shall use __tcp_get_flags() inside, like this: __tcp_set_flags(tcp, __tcp_get_flags(tcp) | TH_SYN); |
| sbin/ipf/ipsend/iptests.c | ||
| 927 ↗ | (On Diff #144885) | I am thinking if we can just use the super set macro TH_FLAGS from sys/netinet/tcp.h or TCPF_ALL from sys/netpfil/ipfilter/netinet/ip_compat.h. But looks there is duplicated definition as TH_ECE == TH_ECN. I am confused about which one to use. But since you are using TH_ECE, so I take it for granted to use TH_FLAGS from sys/netinet/tcp.h. |
| sbin/ipf/libipf/printpacket.c | ||
| 103–105 | Shall be W for TH_CWR. | |
Comment Actions
Thanks for the thorough review!
I did build world, but the typo wasn't flagged - is that source code not compiled by GENERIC?
| sbin/ipf/ipsend/iptests.c | ||
|---|---|---|
| 927 ↗ | (On Diff #144885) | Yes, there is no point of having multiple independent definitions to sum up all the TCP header flags. Removing the redundant definitions and using TH_FLAGS is the better path. Thanks! |