Page MenuHomeFreeBSD

Access TCP Header Flags via accessor MACRO
ClosedPublic

Authored by rscheff on Feb 1 2022, 4:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 24 2024, 6:09 PM
Unknown Object (File)
Feb 20 2024, 6:06 AM
Unknown Object (File)
Feb 13 2024, 7:51 PM
Unknown Object (File)
Feb 11 2024, 5:32 AM
Unknown Object (File)
Feb 11 2024, 5:31 AM
Unknown Object (File)
Feb 10 2024, 5:29 PM
Unknown Object (File)
Feb 10 2024, 5:29 PM
Unknown Object (File)
Feb 10 2024, 5:29 PM
Subscribers

Details

Summary

In order to consistently provide access to all
(including reserved) TCP header flag bits,
use the accessor macros TCP_GET/SET_FLAGS, and
expand any uint8_t / char to at least 16 bit
width.

This is in preparation of AccECN (using one of
the reserved flags)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 44256
Build 41144: arc lint + arc unit

Event Timeline

Has anyone else an opinion on the accessor macros?

This revision is effectively replacing it all over the place - including where it is not strictly required.

I could also do a change which only makes use of there, where the ECN / AccECN flags are getting accessed, in order to make the change smaller, and if the compiler doesn't emit efficient code (th_x2 and th_flags may not actually be continous in memory, thus requiring a couple bit shifts wherever the macro is in use).

sys/netinet/tcp.h
77 ↗(On Diff #102219)

C question: th_x2 is part of a unsigned char. Doesn't shifting it 8 steps to the left basically clear it? In other words: Don't you need to cast it to uint16_t first? Why is this code working?

If we make these static inline functions instead of macros, we would be able to catch with compiler cases when improper type of flags is used.

  • use static inline function instead of macro
  • replace all macro references

@glebius Good hint.

I put the static inline function into tcp_var.h, where similar other functions already live.

All the accesses to th->th_flags should be covered, some function parameters got extended from uint8_t to uint16_t.

sys/netinet/tcp.h
77 ↗(On Diff #102219)

I guess it would cast it to int automagically. But I added an explicit cast in the inline function now. thx.

hselasky added inline comments.
sys/netinet/tcp_var.h
1262

const struct tcphdr *th ??

rscheff marked an inline comment as done.
  • use const struct for best compiler optimization in tcp_get_flags
sys/netinet/tcp_var.h
1261

Can we return uint16_t from this function? That of course may require changing type of the argument in several calling functions. But if we are on code improving crusade, why not? :)

  • make tcp_get_flags return an uint16_t
rscheff added inline comments.
sys/netinet/tcp_var.h
1261

Sure. Where the function parameter definitions matter, I addressed this already; the local "flags" or "thflags" is mostly defined as an int (not a uint16_t). If that shoud be addressed too, I'd probably do this in another diff, though.

sys/netinet/tcp_var.h
1264

The cast should also be uint16_t, as the whole function, its arguments and return type are unsigned. If I'm not missing anything that will also remove any implicit conversions to the return type.

rscheff marked an inline comment as done.
  • fix typecasting oversight
This revision is now accepted and ready to land.Feb 3 2022, 2:20 PM