unix: Extend UpdSocket to send and receive TTL for a specific packet #82858

LinkTed · 2021-03-07T11:35:02Z

Add the function recv_vectored_with_ancillary_from,
recv_vectored_with_ancillary, send_vectored_with_ancillary_to and
send_vectored_with_ancillary to UdpSocket for Unix platforms. Also add
set_recvttl and recvttl to UdpSocket to tell the kernel to receive the
TTL. Therefore, the AncillaryData enum had to be extended.

rust-highfive · 2021-03-07T11:35:05Z

r? @KodrAus

(rust-highfive has picked a reviewer for you, use r? to override)

joshtriplett · 2021-03-18T20:17:04Z

Can you force-push to fix the typo in the commit message (UpdSocket should be UdpSocket), please?

library/std/src/net/udp.rs

joshtriplett · 2021-03-18T20:24:45Z

I posted comments with a few very minor issues. With those fixed, this LGTM.

bors · 2021-03-30T04:01:40Z

☔ The latest upstream changes (presumably #83664) made this pull request unmergeable. Please resolve the merge conflicts.

JohnTitor · 2021-04-19T00:22:30Z

r? @joshtriplett

bors · 2021-05-05T16:54:17Z

☔ The latest upstream changes (presumably #84200) made this pull request unmergeable. Please resolve the merge conflicts.

LinkTed · 2021-05-09T17:18:38Z

Hi @joshtriplett, could you approve this PR?

library/std/src/os/unix/net/ancillary.rs

LinkTed · 2021-07-25T13:03:52Z

The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Finished release [optimized] target(s) in 8.34s
tidy check
tidy: Skipping binary file check, read-only filesystem
Checking which error codes lack tests...
tidy error: /checkout/library/std/src/net/udp.rs:6: platform-specific cfg: cfg(any(
    target_os = "android",
    target_os = "dragonfly",
    target_os = "emscripten",
    target_os = "freebsd",
    target_os = "linux",
    target_os = "netbsd",
    target_os = "openbsd",
))
tidy error: /checkout/library/std/src/net/udp.rs:17: platform-specific cfg: cfg(any(
    target_os = "android",
    target_os = "dragonfly",
    target_os = "emscripten",
    target_os = "freebsd",
    target_os = "linux",
    target_os = "netbsd",
    target_os = "openbsd",
))
tidy error: /checkout/library/std/src/net/udp.rs:159: platform-specific cfg: cfg(any(
        target_os = "android",
        target_os = "dragonfly",
        target_os = "emscripten",
        target_os = "freebsd",
        target_os = "linux",
        target_os = "netbsd",
        target_os = "openbsd",
    ))
tidy error: /checkout/library/std/src/net/udp.rs:178: platform-specific cfg: cfg(any(
        target_os = "android",
        target_os = "dragonfly",
        target_os = "emscripten",
        target_os = "freebsd",
        target_os = "linux",
        target_os = "netbsd",
        target_os = "openbsd",
    ))
tidy error: /checkout/library/std/src/net/udp.rs:227: platform-specific cfg: cfg(any(
        target_os = "android",
        target_os = "dragonfly",
        target_os = "emscripten",
        target_os = "freebsd",
        target_os = "linux",
        target_os = "netbsd",
        target_os = "openbsd",
    ))
tidy error: /checkout/library/std/src/net/udp.rs:282: platform-specific cfg: cfg(any(
        target_os = "android",
        target_os = "dragonfly",
        target_os = "emscripten",
        target_os = "freebsd",
        target_os = "linux",
        target_os = "netbsd",
        target_os = "openbsd",
    ))
tidy error: /checkout/library/std/src/net/udp.rs:392: platform-specific cfg: cfg(any(
        target_os = "android",
        target_os = "dragonfly",
        target_os = "emscripten",
        target_os = "freebsd",
        target_os = "linux",
        target_os = "netbsd",
        target_os = "openbsd",
    ))
tidy error: /checkout/library/std/src/net/udp.rs:440: platform-specific cfg: cfg(any(
        target_os = "android",
        target_os = "dragonfly",
        target_os = "emscripten",
        target_os = "freebsd",
        target_os = "linux",
        target_os = "netbsd",
        target_os = "openbsd",
* 625 error codes
* highest error code: E0783
Found 499 error codes
Found 0 error codes with no tests
Found 0 error codes with no tests
Done!
* 340 features
some tidy checks failed


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build" "16"


Build completed unsuccessfully in 0:00:10

Does somebody know how to fix this tidy error?

the8472 · 2021-07-25T13:38:13Z

Afaik platform-specific code should be in the sys (internal) or os (public) modules.

LinkTed · 2021-07-25T17:24:45Z

Afaik platform-specific code should be in the sys (internal) or os (public) modules.

Did not know that, thanks.

LinkTed · 2021-07-27T19:52:57Z

Hi @joshtriplett,
If you have some free time, could you review my PR?

the8472 · 2021-08-11T20:24:31Z

Since josh seems busy and this concerns unstable stuff which will need some more revisions anyway I hope it's ok if I take over review. I'll go over it on the weekend.

r? @the8472

Add the function recv_vectored_with_ancillary_from, recv_vectored_with_ancillary, send_vectored_with_ancillary_to and send_vectored_with_ancillary to UdpSocket for Unix platforms. Also add set_recvttl and recvttl to UdpSocket to tell the kernel to receive the TTL. Therefore, rename the Ancillary(Data) to UnixAncillary(Data) for the UnixDatagram and UnixStream and also add the IpAncillary(Data) for UdpSocket.

the8472

First pass, haven't reviewed everything yet.

Overall I am a bit concerned that the split into IpAncillary and UnixAncillary will lead to some duplication especially if we want to expose raw ancillary messages (parsed into header + raw data) so that other libraries can do their own decoding. Although that discussion belongs in #76915 i'm just noting that the changes here may need to be revised.

library/std/src/os/unix/net/ancillary/ip.rs

the8472 · 2021-08-15T23:36:38Z

library/std/src/os/unix/net/ancillary/ip.rs

+    /// and type `IPV6_HOPLIMIT`.
+    ///
+    /// # Example
+    /// ```no_run


Any reason why the doc test is no_run? Obviously the receiving parts would hang if they never received data but the sending ones could send the data into a black hole.

But it would fail because of the expect("send_vectored_with_ancillary function failed").

Why would sending fail though? UDP is fire-and-forget so you can still send even if nothing is listening on the other side.
If it's failing due to the lack of ipv6 you could try 127.0.0.1 instead.

library/std/src/os/unix/net/ancillary/ip.rs

the8472 · 2021-08-15T23:43:48Z

library/std/src/os/unix/net/ancillary/ip.rs

+    ///
+    /// The function returns `true` if there was enough space in the buffer.
+    /// If there was not enough space then no file descriptors was appended.
+    /// Technically, that means this operation adds a control message with the level `IPPROTO_IPV6`


"Technically" seems like an odd choice here (and in the comment below too). The following would probably do

This adds a control message [...]

Since this is standardized maybe link to the relevant section in RFC 3542

Where and how to add the RFC 3542 link?

the8472 · 2021-08-15T23:51:53Z

library/std/src/sys_common/net.rs

+    target_os = "linux",
+    target_os = "netbsd",
+    target_os = "openbsd",
+))]


these additions run afoul of this rule:

rust/src/tools/tidy/src/pal.rs

Line 27 in 4d90635

//! `std/sys_common` should _not_ contain platform-specific code.

It currently isn't fully enforced by tidy, but the goal is to reduce that amount, not add more. Instead the module-reuse trick used a few times by hermit might help.

https://github.com/rust-lang/rust/blob/master/library/std/src/sys/hermit/mod.rs#L33-L38

the8472 · 2021-08-16T00:04:26Z

library/std/src/os/unix/net/udp.rs

+    sys_common::AsInner,
+};
+
+impl UdpSocket {


Unless I missed a policy change simply adding platform-specific methods to a cross-platform struct isn't allowed and sealed extension traits should be used instead.

the8472 · 2021-08-16T00:10:55Z

library/std/src/os/unix/net/udp.rs

+        self.as_inner().recvttl()
+    }
+
+    /// Receives data and ancillary data from socket.


ancilliary data should link to IpAncillary's docs.

How do I add this link?

Check the Ipv4Addr doc comments for an example.

the8472 · 2021-08-16T00:19:45Z

library/std/src/os/unix/net/ancillary/ip.rs

+    }
+}
+
+/// A net IP Ancillary data struct.


This needs several paragraphs of text and links to external documentation (e.g. RFC 3542, cmsg manpages or posix specs)

library/std/src/os/unix/net/tests.rs

LinkTed · 2021-08-21T08:02:53Z

First pass, haven't reviewed everything yet.

Overall I am a bit concerned that the split into IpAncillary and UnixAncillary will lead to some duplication especially if we want to expose raw ancillary messages (parsed into header + raw data) so that other libraries can do their own decoding. Although that discussion belongs in #76915 i'm just noting that the changes here may need to be revised.

This was suggested here. I think we should keep IpAncillary and UnixAncillary because a Unix socket does not have a TTL. I try to avoid code duplication as much as possible.

If other libraries should use their own decoding, then we have to define the interface.

Extend UdpSocket to send and receive hop-limit.

the8472 · 2021-08-22T14:50:26Z

First pass, haven't reviewed everything yet.
Overall I am a bit concerned that the split into IpAncillary and UnixAncillary will lead to some duplication especially if we want to expose raw ancillary messages (parsed into header + raw data) so that other libraries can do their own decoding. Although that discussion belongs in #76915 i'm just noting that the changes here may need to be revised.

This was suggested here. I think we should keep IpAncillary and UnixAncillary because a Unix socket does not have a TTL. I try to avoid code duplication as much as possible.

If other libraries should use their own decoding, then we have to define the interface.

Having separate enums makes sense. But do we need separate ancillary types and all those setters too?

Imo it would make more sense to align more closely with native concept of arbitrary messages. Some of those happen to be of types known to the standard library but not all of them.

I think it could look roughly look like this:

trait AncillaryMessage {
   fn to_raw(&self) -> (c_int, c_int, &[u8]);

   // and some unsafe helper methods do the memcpy to custom types properly
}

struct RawAncillaryMessage {
   // I'm not 100% sure, but I think for raw accessors to platform-specific values using `c_int` should be fine.
   fn from_raw(level: c_int, type: c_int, body: &[u8]) -> Self;
}

impl AncillaryMessage for RawAncillaryMessage {
  ...
}

impl AncillaryMessage for IpAncillaryData {
  ...
}

impl TryFrom<RawAncillaryMessage> for IpAncillaryData {
  ...
}

impl SocketAncillary {
  pub fn addMessage(&mut self, impl AncillaryMessage) {
     ...
  }

  pub fn getMessages(&self) -> impl Iterator<Item = RawAncillaryMessage> {
     ...
  }
}

Then you could add any kind of message and iterator over any kind of message and then try_into() each message into IpAncillaryData and ignore those out that the standard library doesn't know or process them yourself by accessing the raw data.

If you pass in a UnixAncillaryData into an inet socket then the send will fail with an error, so it's not something we need to guard against. The separation of the enums would only exist as a weak separation so that users of try_into() would have fewer enum variants to deal with as long as they use the correct one for the socket.

the8472 · 2021-08-22T14:56:12Z

library/std/src/os/unix/net/ancillary/unix.rs

+#[doc(cfg(any(target_os = "android", target_os = "linux",)))]
+#[cfg(any(doc, target_os = "android", target_os = "linux",))]
+#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
+pub struct ScmCredentials<'a>(AncillaryDataIter<'a, libc::ucred>);


If these structs are linux-specific they should be moved to os::linux, or are there any other unix platforms that support this too and they're just not implemented yet? There's #88025 which looks similar and yet different, I haven't checked if they can be unified.

bors · 2021-08-23T23:22:22Z

☔ The latest upstream changes (presumably #88265) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2021-09-27T05:32:01Z

@LinkTed
Ping from triage - can you please address the merge conflicts?

JohnCSimon · 2021-11-08T04:59:57Z

@LinkTed
I'm closing this PR is inactive, please feel free to reopen when you're ready to continue and address the comments and merge conflicts.
@rustbot label: +S-inactive

rust-highfive assigned KodrAus Mar 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2021

LinkTed mentioned this pull request Mar 7, 2021

Tracking Issue for feature(unix_socket_ancillary_data) #76915

Open

This comment has been minimized.

Sign in to view

LinkTed force-pushed the master branch from d370839 to 1469c86 Compare March 13, 2021 11:35

This comment has been minimized.

Sign in to view

LinkTed force-pushed the master branch 2 times, most recently from 062c56f to 1447222 Compare March 13, 2021 14:25

joshtriplett reviewed Mar 18, 2021

View reviewed changes

library/std/src/net/udp.rs Outdated Show resolved Hide resolved

joshtriplett reviewed Mar 18, 2021

View reviewed changes

library/std/src/net/udp.rs Outdated Show resolved Hide resolved

LinkTed force-pushed the master branch from 1447222 to 2ef5353 Compare March 20, 2021 11:54

LinkTed force-pushed the master branch from 2ef5353 to 8385d10 Compare April 1, 2021 16:59

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2021

rust-highfive assigned joshtriplett and unassigned KodrAus Apr 19, 2021

LinkTed force-pushed the master branch from 8385d10 to 6a1169c Compare May 9, 2021 17:17

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021

LinkTed mentioned this pull request Jun 19, 2021

Tweak SocketAncillary API #86432

Closed

de-vri-es reviewed Jun 23, 2021

View reviewed changes

library/std/src/os/unix/net/ancillary.rs Outdated Show resolved Hide resolved

LinkTed force-pushed the master branch from 0449d28 to 6aec4be Compare July 25, 2021 14:54

This comment has been minimized.

Sign in to view

LinkTed force-pushed the master branch 2 times, most recently from 8d9d6b4 to 605bf81 Compare July 26, 2021 12:36

rust-highfive assigned the8472 and unassigned joshtriplett Aug 11, 2021

the8472 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 11, 2021

LinkTed force-pushed the master branch from 605bf81 to bf0b98c Compare August 14, 2021 16:35

the8472 requested changes Aug 16, 2021

View reviewed changes

unix: Add HopLimit to IpAncillary

c40097a

Extend UdpSocket to send and receive hop-limit.

LinkTed force-pushed the master branch from bf0b98c to c40097a Compare August 21, 2021 08:11

the8472 reviewed Aug 22, 2021

View reviewed changes

the8472 mentioned this pull request Aug 28, 2021

ancillary data size and doc improvement #88425

Closed

the8472 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2021

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 8, 2021

JohnCSimon closed this Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix: Extend UpdSocket to send and receive TTL for a specific packet #82858

unix: Extend UpdSocket to send and receive TTL for a specific packet #82858

LinkTed commented Mar 7, 2021

rust-highfive commented Mar 7, 2021

This comment has been minimized.

This comment has been minimized.

joshtriplett commented Mar 18, 2021

joshtriplett commented Mar 18, 2021

bors commented Mar 30, 2021

JohnTitor commented Apr 19, 2021

bors commented May 5, 2021

LinkTed commented May 9, 2021

LinkTed commented Jul 25, 2021

the8472 commented Jul 25, 2021

This comment has been minimized.

LinkTed commented Jul 25, 2021 •

edited

Loading

LinkTed commented Jul 27, 2021

the8472 commented Aug 11, 2021

the8472 left a comment

the8472 Aug 15, 2021

LinkTed Aug 21, 2021

the8472 Aug 22, 2021

the8472 Aug 15, 2021

LinkTed Aug 21, 2021

the8472 Aug 15, 2021

the8472 Aug 16, 2021

the8472 Aug 16, 2021

LinkTed Aug 21, 2021

the8472 Aug 22, 2021

the8472 Aug 16, 2021

LinkTed commented Aug 21, 2021

the8472 commented Aug 22, 2021 •

edited

Loading

the8472 Aug 22, 2021

bors commented Aug 23, 2021

JohnCSimon commented Sep 27, 2021

JohnCSimon commented Nov 8, 2021

unix: Extend UpdSocket to send and receive TTL for a specific packet #82858

unix: Extend UpdSocket to send and receive TTL for a specific packet #82858

Conversation

LinkTed commented Mar 7, 2021

rust-highfive commented Mar 7, 2021

This comment has been minimized.

This comment has been minimized.

joshtriplett commented Mar 18, 2021

joshtriplett commented Mar 18, 2021

bors commented Mar 30, 2021

JohnTitor commented Apr 19, 2021

bors commented May 5, 2021

LinkTed commented May 9, 2021

LinkTed commented Jul 25, 2021

the8472 commented Jul 25, 2021

This comment has been minimized.

LinkTed commented Jul 25, 2021 • edited Loading

LinkTed commented Jul 27, 2021

the8472 commented Aug 11, 2021

the8472 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

Choose a reason for hiding this comment

LinkTed commented Aug 21, 2021

the8472 commented Aug 22, 2021 • edited Loading

Choose a reason for hiding this comment

bors commented Aug 23, 2021

JohnCSimon commented Sep 27, 2021

JohnCSimon commented Nov 8, 2021

LinkTed commented Jul 25, 2021 •

edited

Loading

the8472 commented Aug 22, 2021 •

edited

Loading