Match BSD-family IPv4 multicast TTL/loopback ABI to system headers by lopopolo · Pull Request #647 · rust-lang/socket2

lopopolo · 2026-04-05T16:34:22Z

Refs #646.

BSD-family system headers declare IP_MULTICAST_TTL and IP_MULTICAST_LOOP as u_char / unsigned char.

socket2 was using c_int for these options on all targets. On FreeBSD 15 this is observable: std::net::UdpSocket::set_multicast_ttl_v4(258) succeeds and stores 2, while socket2::Socket::set_multicast_ttl_v4(258) returns EINVAL.

Thomasdezeeuw · 2026-04-06T17:35:04Z

src/socket.rs

 from!(Socket, net::UdpSocket);
+
+#[cfg(test)]
+mod tests {


Can you move this to the other tests in the tests directory?

thanks, I was wondering why there were so few unit tests in this crate 😅

I've moved this into tests/socket.rs.

Thomasdezeeuw · 2026-04-06T17:35:19Z

src/socket.rs

+#[cfg(test)]
+mod tests {
+    #[test]
+    #[cfg(target_os = "freebsd")]


Why run it on FreeBSD only?

I mentioned this in the comment I left on the original issue #646 (comment), but in case you missed it:

The regression test only is gated on for freebsd because that is the VM I happened to have available for reproduction. I can widen this cfg if you'd like so it covers all of the relevant BSD OSes.

I've widened the cfg so it tests on all relevant BSD OSes.

Thomasdezeeuw · 2026-04-06T17:35:34Z

src/socket.rs

+mod tests {
+    #[test]
+    #[cfg(target_os = "freebsd")]
+    fn set_multicast_ttl_v4_matches_std_for_values_above_u8_range() {


Can we please fix these terrible AI generated names?

I've renamed the test to multicast_v4_bsd_abi.

Thomasdezeeuw · 2026-04-06T17:35:55Z

src/socket.rs

+
+        let socket2_socket = Socket::new(Domain::IPV4, Type::DGRAM, Some(Protocol::UDP)).unwrap();
+        socket2_socket.set_multicast_ttl_v4(258).unwrap();
+        assert_eq!(socket2_socket.multicast_ttl_v4().unwrap(), std_ttl);


Can you use the test macro for this? See the other tests.

I've factored this out into an assertion helper named assert_multicast_ttl_v4.

To maintain the part of the test that ensures behavior matches std, the helper accepts either a socket2 Socket or a std UdpSocket via a SocketRef enum which I am using to model an "either" enum. If this isn't acceptable, please let me know what you'd prefer.

lopopolo

thanks for the feedback, new patch incoming.

lopopolo · 2026-04-06T20:54:28Z

src/socket.rs

+mod tests {
+    #[test]
+    #[cfg(target_os = "freebsd")]
+    fn set_multicast_ttl_v4_matches_std_for_values_above_u8_range() {


I've renamed the test to multicast_v4_bsd_abi.

lopopolo · 2026-04-06T20:55:48Z

src/socket.rs

+#[cfg(test)]
+mod tests {
+    #[test]
+    #[cfg(target_os = "freebsd")]


I mentioned this in the comment I left on the original issue #646 (comment), but in case you missed it:

The regression test only is gated on for freebsd because that is the VM I happened to have available for reproduction. I can widen this cfg if you'd like so it covers all of the relevant BSD OSes.

I've widened the cfg so it tests on all relevant BSD OSes.

lopopolo · 2026-04-06T20:56:38Z

src/socket.rs

 from!(Socket, net::UdpSocket);
+
+#[cfg(test)]
+mod tests {


thanks, I was wondering why there were so few unit tests in this crate 😅

I've moved this into tests/socket.rs.

lopopolo · 2026-04-06T21:16:41Z

src/socket.rs

+
+        let socket2_socket = Socket::new(Domain::IPV4, Type::DGRAM, Some(Protocol::UDP)).unwrap();
+        socket2_socket.set_multicast_ttl_v4(258).unwrap();
+        assert_eq!(socket2_socket.multicast_ttl_v4().unwrap(), std_ttl);


I've factored this out into an assertion helper named assert_multicast_ttl_v4.

lopopolo · 2026-04-06T21:20:47Z

src/socket.rs

+
+        let socket2_socket = Socket::new(Domain::IPV4, Type::DGRAM, Some(Protocol::UDP)).unwrap();
+        socket2_socket.set_multicast_ttl_v4(258).unwrap();
+        assert_eq!(socket2_socket.multicast_ttl_v4().unwrap(), std_ttl);


To maintain the part of the test that ensures behavior matches std, the helper accepts either a socket2 Socket or a std UdpSocket via a SocketRef enum which I am using to model an "either" enum. If this isn't acceptable, please let me know what you'd prefer.

lopopolo · 2026-04-06T21:41:14Z

thanks for the feedback @Thomasdezeeuw. I have tried my best to address it and believe this PR is ready for another review.

Thomasdezeeuw

I'm sorry, but this is not at all what I wanted and I don't want to review this AI generated code any longer.

lopopolo · 2026-04-07T17:36:52Z

I'm sorry, but this is not at all what I wanted and I don't want to review this AI generated code any longer.

Hi Thomas, this is disappointing to hear. Thanks for your time.

lopopolo · 2026-04-07T18:40:08Z

I'm sorry, but this is not at all what I wanted and I don't want to review this AI generated code any longer.

I am trying to debug what was not acceptable here and I think it is because I did not construct the tests like this:

socket2/tests/socket.rs

Lines 1602 to 1607 in 642df44

    
           #[cfg(all(feature = "all", any(target_os = "linux", target_os = "android")))] 
        
           test!( 
        
               #[ignore = "setting `IPV6_TRANSPARENT` requires the `CAP_NET_ADMIN` capability (works when running as root)"] 
        
               IPv6 ip_transparent_v6, 
        
               set_ip_transparent_v6(true) 
        
           );

I did not find this macro because #647 (comment) was not very specific in terms of which tests to match. Additionally, @Thomasdezeeuw had been providing dismissive feedback in response to AI assistance, so I wrote the commits 92be67d and 7ddafc0 by hand.

As someone familiar to Rust but new to this crate, test macro most obviously meant #[test] and not a macro_rules! macro named test. I did find this inCONTRIBUTING.md:

Tests should be added to tests/socket.rs, following (roughly) the same order in which the methods are defined on a type. At the bottom of this file it has a macro to create a simple get/set socket option test, more complex API however needs a manually written test.

but the docs do not match the reality of the test file; the macro is defined ~600 lines before the end of the file. When scanning the tests/socket.rs source in vim, I missed it. I attempted to find something approximating an assert! macro and found several assertion helpers like this one:

socket2/tests/socket.rs

Lines 381 to 390 in 642df44

    
           /// Assert that `CLOEXEC` is set on `socket`. 
        
           #[cfg(unix)] 
        
           #[track_caller] 
        
           pub fn assert_close_on_exec<S>(socket: &S, want: bool) 
        
           where 
        
               S: AsRawFd, 
        
           { 
        
               let flags = unsafe { libc::fcntl(socket.as_raw_fd(), libc::F_GETFD) }; 
        
               assert_eq!(flags & libc::FD_CLOEXEC != 0, want, "CLOEXEC option"); 
        
           }

which is what I used to model how I addressed the feedback. I also believed divergence from std was part of the bug here which warranted a dedicated test.

Based on my understanding now, I am guessing that the acceptable test would have been the following:

#[cfg(any(
    target_os = "dragonfly",
    target_os = "freebsd",
    target_os = "openbsd",
    target_os = "netbsd",
    target_os = "solaris",
    target_os = "illumos",
    target_os = "nto",
))]
test!(IPv4 multicast_ttl_v4, set_multicast_ttl_v4(258), 2);

I would have hoped that my good faith contributions could have been either:

given constructive review feedback with the goal of getting the code mergable.
merged and then fixed up by the maintainers with a followup commit.
written directly by the maintainers in the first place after I reported the bug initially.
engaged with in the context of the correctness issue this work identifies, reproduces, and fixes.

For the avoidance of doubt, this comment (and all of them on #647 and #646) are human-authored.

Add FreeBSD multicast TTL regression test

86ee96a

lopopolo mentioned this pull request Apr 5, 2026

IPv4 multicast TTL and loopback use a different ABI than std and system headers on BSD-family targets #646

Closed

Match BSD multicast ABI to system headers

7475821

lopopolo force-pushed the codex/bsd-multicast-abi branch from 5e7192c to 7475821 Compare April 5, 2026 16:37

lopopolo marked this pull request as ready for review April 5, 2026 16:37

Thomasdezeeuw reviewed Apr 6, 2026

View reviewed changes

lopopolo commented Apr 6, 2026

View reviewed changes

Incorporate review feedback

92be67d

lopopolo force-pushed the codex/bsd-multicast-abi branch from c07d43c to 92be67d Compare April 6, 2026 21:32

Remove cfgs from assert helpers, use platform independent 'want' type

7ddafc0

lopopolo requested a review from Thomasdezeeuw April 6, 2026 21:40

Thomasdezeeuw requested changes Apr 7, 2026

View reviewed changes

Thomasdezeeuw closed this Apr 7, 2026

Conversation

lopopolo commented Apr 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lopopolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lopopolo commented Apr 6, 2026

Uh oh!

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Uh oh!

lopopolo commented Apr 7, 2026

Uh oh!

lopopolo commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants