Add methods to create and parse cmsghdr structured data. #1590

PeterAdams-A · 2020-07-13T13:30:56Z

Motivation:

cmsghdrs can be used to send an receive extra data on UDP packets.
For example ECN data.

Modifications:

Map in Linux and Darwin versions of cmsghdr macros as functions.
Create strctures for holding a received collection; parsing ecn
data from a received collection and building a collection suitable
for sending.

Result:

Functions to manipulate cmsghdr and data exist.

Motivation: cmsghdrs can be used to send an receive extra data on UDP packets. For example ECN data. Modifications: Map in Linux and Darwin versions of cmsghdr macros as functions. Create strctures for holding a received collection; parsing ecn data from a received collection and building a collection suitable for sending. Result: Functions to manipulate cmsghdr and data exist.

PeterAdams-A · 2020-07-13T13:32:32Z

@Lukasa The obvious points for discussion are

Send and receive collections are currently different - this is because I felt more confortable using the msghdr based functions when I had received them that way.
No windows support - should I implement blank functions which just assert on windows?

Lukasa

Great, looks like a good first start. Notes in the diff.

Lukasa · 2020-07-13T13:33:13Z

Sources/NIO/ControlMessage.swift

+/// Unsafe as captures pointers and must not escape the scope where those pointers are valid.
+struct UnsafeControlMessage {
+    var level: Int32
+    var type: Int32


These should probably be CInt, not Int32.

Lukasa · 2020-07-13T13:34:42Z

Sources/NIO/ControlMessage.swift

+/// Collection representation of `cmsghdr` structures and associated data from `recvmsg`
+/// Unsafe as captures pointers held in msghdr structure which must not escape scope of validity.
+struct UnsafeControlMessageCollection: Collection {
+    typealias Index = ControlMessageIndex


You can elide this typealias by renaming ControlMessageIndex to Index.

Sources/NIO/ControlMessage.swift

Lukasa · 2020-07-13T13:40:38Z

Sources/NIO/ControlMessage.swift

+                                    data: Posix.cmsgData(for: cmsg))
+    }
+
+    private var messageHeader: msghdr


It's quite confusing to have stored properties, computed properties, and functions scattered around this type definition. Can we refactor it? Something like:

struct UnsafeControlMessageCollection { private var messageHeader: msghdr init(messageHeader: msghdr) } extension UnsafeControlMessageCollection: Collection { var startIndex: Index var endIndex: Index // This should not be a stored property as it's a global static, let the compiler handle it func index(after:) subscript }

This would much more clearly separate concerns.

Lukasa · 2020-07-13T14:40:52Z

Tests/NIOTests/SystemTest.swift

+
+    func testCMsgNextHeader() {
+        var exampleCmsgHrd = SystemTest.cmsghdrExample
+        exampleCmsgHrd.withUnsafeMutableBytes { pCmsgHdr in


Same mutable note here too.

same reason.

Tests/NIOTests/SystemTest.swift

Lukasa · 2020-07-13T14:43:13Z

Tests/NIOTests/SystemTest.swift

+                let second = Posix.cmsgNextHeader(inside: pMsgHdr, from: first)
+                let expectedSecondSlice = UnsafeMutableRawBufferPointer(
+                    rebasing: pCmsgHdr[SystemTest.cmsghdr_secondStartPosition...])
+                XCTAssertEqual(expectedSecondSlice.baseAddress, second)


Let's not bother creating a slice if we don't need one, let's just use pCmsgHdr.baseAddress! + SystemTest.cmsghdr_secondStartPosition

Tests/NIOTests/SystemTest.swift

PeterAdams-A · 2020-07-13T17:34:15Z

@Lukasa I have taken most of your suggested changes. There are a few where I have not done but have replied to your comment.

Lukasa · 2020-07-14T07:30:32Z

There are no visible comment replies @PeterAdams-A: did you submit them?

Lukasa · 2020-07-14T08:21:25Z

Sources/NIO/ControlMessage.swift

+    private static let ipv4TosType = IP_TOS    // Linux
+    #endif
+
+    static func readCInt(data: UnsafeRawBufferPointer) -> CInt {


Prefix with an underscore to indicate this is not supposed to be part of the public API of this type.

Lukasa · 2020-07-14T08:22:15Z

Sources/NIO/ControlMessage.swift

+
+extension CInt {
+    #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
+    fileprivate static let notCapableValue = IPTOS_ECN_NOTECT


This can be private.

Lukasa · 2020-07-14T08:23:02Z

Sources/NIO/ControlMessage.swift

+
+struct UnsafeOutboundControlBytes {
+    private var controlBytes: UnsafeMutableRawBufferPointer
+    private var writePosition: UnsafeMutableRawBufferPointer.Index = 0


For the sake of keeping this as correct as possible, let's initialise this in init with self.controlBytes.startIndex.

good thinking.

Lukasa · 2020-07-14T08:23:56Z

Sources/NIO/ControlMessage.swift

+        cmsghdrPtr.pointee.cmsg_type = type
+        cmsghdrPtr.pointee.cmsg_len = .init(Posix.cmsgLen(payloadSize: MemoryLayout.size(ofValue: payload)))
+
+        let dataPointer = Posix.cmsgData(for: cmsghdrPtr)


We can bring the force-unwrap to here and save ourselves two more below.

Lukasa · 2020-07-14T08:26:04Z

Sources/NIO/ControlMessage.swift

+    /// It's assumed the caller has checked that congestion information is required before calling.
+    internal init(from controlMessagesReceived: UnsafeControlMessageCollection) {
+        var controlMessageReceiver = ControlMessageParser()
+        controlMessagesReceived.forEach { controlMessage in controlMessageReceiver.receiveMessage(controlMessage) }


for message in controlMessagesReceived { controlMessageReceiver.receiveMessage(message) }

In general forEach is not really worth using: it's kinda needless sugar over the more straightforward looping operation.

Also, should this loop move into an init on ControlMessageParser? That object can really only be used with one thing anyway.

Sources/NIO/ControlMessage.swift

Lukasa

One note, otherwise good.

Lukasa · 2020-07-14T10:24:59Z

Sources/NIO/ControlMessage.swift

+    var endIndex: Index { return Index(cmsgPointer: nil) }
+
+    func index(after: Index) -> Index {
+        precondition(after.cmsgPointer != nil)


This precondition is unnecessary: we force-unwrap this pointer before its first use anyway, so the correctness of the code isn't enforced here. If you wanted to match the message below you can move the force-unwrap to this line instead, if that helps.

Motivation: cmsghdrs can be used to send an receive extra data on UDP packets. For example ECN data. Modifications: Map in Linux and Darwin versions of cmsghdr macros as functions. Create strctures for holding a received collection; parsing ecn data from a received collection and building a collection suitable for sending. Result: Functions to manipulate cmsghdr and data exist.

PeterAdams-A requested a review from Lukasa July 13, 2020 13:30

Lukasa requested changes Jul 13, 2020

View reviewed changes

PeterAdams-A added 2 commits July 13, 2020 18:25

Cory review comments.

4be601f

Missed Cory review comment.

866660a

PeterAdams-A requested a review from Lukasa July 13, 2020 17:33

Lukasa added the 🔨 semver/patch No public API change. label Jul 14, 2020

Lukasa added this to the 2.20.0 milestone Jul 14, 2020

Lukasa reviewed Jul 14, 2020

View reviewed changes

glbrntt reviewed Jul 14, 2020

View reviewed changes

Sources/NIO/ControlMessage.swift Show resolved Hide resolved

PeterAdams-A added 2 commits July 14, 2020 10:20

More Cory and George reviewing.

3ad5e06

Bring back mismatched pre-condition which Cory and I are ok with.

a590a98

PeterAdams-A requested a review from Lukasa July 14, 2020 09:50

Lukasa approved these changes Jul 14, 2020

View reviewed changes

Remove unnecessary precondition.

db4f639

PeterAdams-A merged commit f12803d into apple:master Jul 14, 2020

PeterAdams-A deleted the cmsg_rw branch July 14, 2020 11:29

PeterAdams-A mentioned this pull request Jul 23, 2020

Support propagating ECN information on AddressedEnvelopes #1472

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods to create and parse cmsghdr structured data. #1590

Add methods to create and parse cmsghdr structured data. #1590

PeterAdams-A commented Jul 13, 2020

PeterAdams-A commented Jul 13, 2020

Lukasa left a comment

Lukasa Jul 13, 2020

PeterAdams-A Jul 13, 2020

Lukasa Jul 13, 2020

PeterAdams-A Jul 13, 2020

Lukasa Jul 13, 2020

PeterAdams-A Jul 13, 2020

Lukasa Jul 13, 2020

PeterAdams-A Jul 13, 2020

Lukasa Jul 13, 2020

PeterAdams-A Jul 13, 2020

PeterAdams-A commented Jul 13, 2020

Lukasa commented Jul 14, 2020

Lukasa Jul 14, 2020

PeterAdams-A Jul 14, 2020

Lukasa Jul 14, 2020

PeterAdams-A Jul 14, 2020

Lukasa Jul 14, 2020

PeterAdams-A Jul 14, 2020

Lukasa Jul 14, 2020

PeterAdams-A Jul 14, 2020

Lukasa Jul 14, 2020

Lukasa Jul 14, 2020

PeterAdams-A Jul 14, 2020

Lukasa left a comment

Lukasa Jul 14, 2020

Add methods to create and parse cmsghdr structured data. #1590

Add methods to create and parse cmsghdr structured data. #1590

Conversation

PeterAdams-A commented Jul 13, 2020

PeterAdams-A commented Jul 13, 2020

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterAdams-A commented Jul 13, 2020

Lukasa commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment