Page MenuHomeFreeBSD

Add header definition for RFC4340, Datagram Congestion Control Protocol
ClosedPublic

Authored by thj on Aug 7 2019, 11:46 AM.
Tags
None
Referenced Files
F102600122: D21179.diff
Thu, Nov 14, 3:11 PM
Unknown Object (File)
Tue, Nov 12, 11:54 PM
Unknown Object (File)
Tue, Nov 12, 10:09 PM
Unknown Object (File)
Fri, Oct 18, 7:45 PM
Unknown Object (File)
Fri, Oct 18, 3:05 AM
Unknown Object (File)
Fri, Oct 18, 2:39 AM
Unknown Object (File)
Fri, Oct 18, 2:07 AM
Unknown Object (File)
Fri, Oct 18, 2:07 AM

Details

Summary

Split out from https://reviews.freebsd.org/D16851

Add header definition for RFC4340, Datagram Congestion Control Protocol

Add a header definition for DCCP as defined in RFC4340. This header definition
is required to perform validation when receiving and forwarding DCCP packets.
We do not currently support DCCP.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26585
Build 24969: arc lint + arc unit

Event Timeline

rscheff added inline comments.
sys/netinet/dccp.h
42

#elseif

46

#else #warning BYTE_ORDER unexpected or not set

58

This definition doesn't match either the short or long header here...
For the long header, you are missing a uint8_t reserved before the 6-byte sequence, while for the short header, the sequence number would only be 4 byte here...

Its not clear if you want this header structure to be a univeral struct, but even then, mapping to the long header (if the reserved bits get used eventually) seems to be more future-proof.

Maybe point the intent out as a comment?

sys/netinet/dccp.h
42

Reply to both comments about the ENDIAN preprocess.

This is the way that ip.h, ip6.h and tcp.h set up bit fields like this. Should we be different in dccp?

58

The intention is to have enough header space to hold either a long or short header. I will add a comment to clarify.

Add comment to explain the seq field is sized to hold either the short or the long sequence number

Extend the sequence number field to include a reserved byte in the long header.

sys/netinet/dccp.h
47

shouldn't be the checksum field a uint16_t type, instead of a 2-byte array?

sys/netinet/dccp.h
47

The opposite was a comment before I split out this change from D16851

"Due to alignment, it seems likely that an extra byte will be added before d_cksum. The easiest way to get around this is to turn this into a 2-byte array (e.g. uint8_t d_cksum[2])."

https://reviews.freebsd.org/D16851#inline-130434

Remove extra white space around union

Fix struct so that will build, last rev I seem to have updated the review from
the wrong tree.

glebius added inline comments.
sys/netinet/dccp.h
69

Any reason not to use C11 anonymous structs and unions? Is it expected the code will be compiled by legacy compilers?

  • Use named union and struct in header definition
This revision is now accepted and ready to land.Oct 6 2019, 6:32 PM

Approved by: bz (co-mentor)

If you move the $FreeBSD$ then this is still good to go I think.

sys/netinet/dccp.h
32

Given this is a head file, the $FreeBSD$ should go at the end of the license comment; like in the template: https://svnweb.freebsd.org/base/head/share/examples/etc/bsd-style-copyright?annotate=333391#l27 as __FBSDID() needs the cdefs.h include. etc.