Add struct ip_mreqn: ip multicast req with intf id #1555

flocknetworks · 2019-10-16T16:53:17Z

The ip_mreqn struct has an additional interface id parameter. This
allows programming IPv4 multicast groups using the interface id
alongside the src IPv4 address.
Linux has supported the ip_mreqn struct since Linux 2.2
ref: man ip.7

rust-highfive · 2019-10-16T16:53:20Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

flocknetworks · 2019-10-16T19:06:22Z

rust-lang.libc spar64 + mips64 tests failed but I cant see anything in the logs specific to this diff set ?

gnzlbg · 2019-10-17T08:36:45Z

There are sparc and mips linux targets, and the ABI of the type you added does not match the C ABI on those targets. Why, I don't know. Maybe the struct definition that you have added does not match the struct definition on those targets ?

gnzlbg · 2019-10-17T15:27:56Z

src/unix/linux_like/mod.rs

    }

+    #[cfg(not(any(target_arch = "mips64", target_arch = "sparc64")))]
    pub struct ip_mreqn {


This won't work. If you don't want to add this to all linux targets, you need to manually add this to the targets you want. I do however recommend finding the source code for this and checking out what mips and sparc do.

i cant find any alternate versions of the struct in glibc, any ideas where else to look ?

flocknetworks · 2019-10-17T15:28:07Z

in glibc I cant find any ip_mreqn struct specific to an architecture. I assume this struct is just not in the sparc64 / mips64 builds ?

gnzlbg · 2019-10-17T15:30:15Z

Which header file does expose it?

flocknetworks · 2019-10-17T15:31:55Z

struct ip_mreq is defined in inet/netinet/in.h
struct ip_mreqn is defined in sysdeps/unix/sysv/linux/bits/in.h

gnzlbg · 2019-10-17T15:36:16Z

So that's in this header: https://elixir.bootlin.com/linux/v5.4-rc2/source/include/uapi/linux/in.h#L173

I assume this struct is just not in the sparc64 / mips64 builds ?

It is available - we know because the C tests for it on sparc64 and mips64 use the type and they compile successfully, so a type definition must be available.

gnzlbg · 2019-10-17T16:01:02Z

So looking at the error, the test that's failing is the following.

This struct has 3 fields, where each field is 4 bytes wide. The first two fields are a struct { u32 }, while the last fields is an i32.

The test initializes the struct bytes in Rust to the following values: [0, 1, 2, 3, ..., 11]. It then passes the struct by value to a C function expecting it. This function then reads the bytes of the struct and verifies that they match the values set on the Rust side. This fails, reporting that the last 4 bytes (the ones of the i32) are read as [0, 0, 0, 0] instead if [8, 9, 10, 11].

That's weird. In particular, this is apparently the only test that fails there. That is, the struct sizes must match, so there can't be any padding between the in_addr and the c_int (that would explain the zeroes in the C side).

Maybe you can just modify the test_linux function in the libc-test/build.rs file to skip the roundtrip test for this type ? (there is a cfg.skip_roundtrip there where you can specify that). It would be interesting to know if all other tests pass.

flocknetworks · 2019-10-17T16:07:07Z

thanks for looking into this, weird indeed. I will try modifying the build.rs

gnzlbg · 2019-10-17T16:14:46Z

If you need these now and don't have time to investigate that's ok, just add these structs to the platforms you need them, e.g., instead of to linux/, maybe only linux/gnu/x86_64 - copy pasting them isn't a problem.

What we try to avoid is adding types that might be subtly broken on some platforms. In this case, it would cost a mips64 user quite a bit of time to figure out why C is crashing when passing it these types without actually writing some code that tries to print their value in C, because on the Rust side, everything looks fine.

flocknetworks · 2019-10-17T17:54:23Z

all the other tests pass (when the roundtrip test is skipped)

The ip_mreqn struct has an additional interface id parameter. This allows programming IPv4 multicast groups using the interface id alongside the src IPv4 address. Linux has supported the ip_mreqn struct since Linux 2.2 ref: man ip.7 [Adding to linux x86_64 only as adding to linux_like/mod.rs caused failures in the sparc64 and mips64 C regression tests]

gnzlbg · 2019-10-17T19:01:55Z

all the other tests pass (when the roundtrip test is skipped)

Damn, so I don't know what might be going on. Maybe our ABI implementation on mips64 and sparc64 is borked somehow? cc @nagisa @nikic

I'm going to open an issue to track investigating what's going on here. In the meantime, we can land this for x86_64, thanks @flocknetworks

@bors: r+

bors · 2019-10-17T19:01:56Z

📌 Commit 44b9022 has been approved by gnzlbg

bors · 2019-10-17T19:02:03Z

⌛ Testing commit 44b9022 with merge b728e0f...

Add struct ip_mreqn: ip multicast req with intf id The ip_mreqn struct has an additional interface id parameter. This allows programming IPv4 multicast groups using the interface id alongside the src IPv4 address. Linux has supported the ip_mreqn struct since Linux 2.2 ref: man ip.7

bors · 2019-10-17T21:32:03Z

💥 Test timed out

gnzlbg · 2019-10-18T07:44:41Z

@bors: retry

bors · 2019-10-18T09:29:06Z

⌛ Testing commit 44b9022 with merge 636b5eb...

Add struct ip_mreqn: ip multicast req with intf id The ip_mreqn struct has an additional interface id parameter. This allows programming IPv4 multicast groups using the interface id alongside the src IPv4 address. Linux has supported the ip_mreqn struct since Linux 2.2 ref: man ip.7

bors · 2019-10-18T10:58:48Z

💔 Test failed - status-azure

flocknetworks · 2019-10-21T13:01:27Z

I'm assuming these failures are due to something in the CI infra rather than my diffs ?

bors · 2019-11-18T09:26:55Z

⌛ Testing commit 44b9022 with merge a813015...

Add struct ip_mreqn: ip multicast req with intf id The ip_mreqn struct has an additional interface id parameter. This allows programming IPv4 multicast groups using the interface id alongside the src IPv4 address. Linux has supported the ip_mreqn struct since Linux 2.2 ref: man ip.7

gnzlbg · 2019-11-18T09:27:18Z

@bors r- retry

gnzlbg · 2019-11-18T16:28:11Z

@bors: r+

bors · 2019-11-18T16:28:13Z

📌 Commit 44b9022 has been approved by gnzlbg

bors · 2019-11-18T22:28:20Z

⌛ Testing commit 44b9022 with merge e9f6339...

Add struct ip_mreqn: ip multicast req with intf id The ip_mreqn struct has an additional interface id parameter. This allows programming IPv4 multicast groups using the interface id alongside the src IPv4 address. Linux has supported the ip_mreqn struct since Linux 2.2 ref: man ip.7

bors · 2019-11-18T23:56:42Z

☀️ Test successful - checks-cirrus-freebsd-10, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, status-azure
Approved by: gnzlbg
Pushing e9f6339 to master...

rust-highfive assigned gnzlbg Oct 16, 2019

gnzlbg reviewed Oct 17, 2019

View reviewed changes

flocknetworks force-pushed the ip_mreqn branch from 523758f to 666d3c7 Compare October 17, 2019 16:17

flocknetworks force-pushed the ip_mreqn branch from 666d3c7 to 909bdfe Compare October 17, 2019 17:55

flocknetworks force-pushed the ip_mreqn branch from 909bdfe to 44b9022 Compare October 17, 2019 18:59

gnzlbg mentioned this pull request Oct 17, 2019

ip_mreqn roundtrip test fails for mips64 and sparc64 #1558

Open

bors merged commit 44b9022 into rust-lang:master Nov 18, 2019

Add struct ip_mreqn: ip multicast req with intf id #1555

Add struct ip_mreqn: ip multicast req with intf id #1555

Uh oh!

Conversation

flocknetworks commented Oct 16, 2019

Uh oh!

rust-highfive commented Oct 16, 2019

Uh oh!

flocknetworks commented Oct 16, 2019

Uh oh!

gnzlbg commented Oct 17, 2019

Uh oh!

gnzlbg Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

flocknetworks Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

flocknetworks commented Oct 17, 2019

Uh oh!

gnzlbg commented Oct 17, 2019

Uh oh!

flocknetworks commented Oct 17, 2019

Uh oh!

gnzlbg commented Oct 17, 2019

Uh oh!

gnzlbg commented Oct 17, 2019

Uh oh!

flocknetworks commented Oct 17, 2019

Uh oh!

gnzlbg commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flocknetworks commented Oct 17, 2019

Uh oh!

gnzlbg commented Oct 17, 2019

Uh oh!

bors commented Oct 17, 2019

Uh oh!

bors commented Oct 17, 2019

Uh oh!

bors commented Oct 17, 2019

Uh oh!

gnzlbg commented Oct 18, 2019

Uh oh!

bors commented Oct 18, 2019

Uh oh!

bors commented Oct 18, 2019

Uh oh!

flocknetworks commented Oct 21, 2019

Uh oh!

bors commented Nov 18, 2019

Uh oh!

gnzlbg commented Nov 18, 2019

Uh oh!

gnzlbg commented Nov 18, 2019

Uh oh!

bors commented Nov 18, 2019

Uh oh!

bors commented Nov 18, 2019

Uh oh!

bors commented Nov 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gnzlbg commented Oct 17, 2019 •

edited

Loading