Provide definition of tcp_info

[NeoLegends]

Follow-up to #1598 with better bitfield support.

Relevant open question: #1598 (comment)

[rust-highfive]

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.

[gnzlbg]

If the Rust struct does not contain these fields or if these fields are private, this should not be necessary.

[gnzlbg]

This is breaking all tests because it is replacing the closure above (previous cfg.skip_fields call) with this one. You need to add tcp_info as a case for the previous closure and not as a new closure here.

[gnzlbg]

This might work, so let's see what CI says first.

I'm not 100% sure what the C rules for bitsets are (e.g. whether C store the bitset in two u8 with an alignment requirement of 1 or in one u16 with an alignment requirement of 2 which might require padding before the first bitset). As long as these are private, it doesn't matter much, but you might want to add an impl in a future PR that looks like this:

impl tcp_info { 
    fn tcpi_snd_wscale(i: usize) -> bool { 
        assert!(i < 4); 
        get_bit(self.tcpi_wscale, i) 
    }
}

and then the layout for this "hole" within the struct would need to be correct for these methods to return the right values. Let's try to merge this as is first, so that we know that the struct has the general layout correct, and worry about how to access these private fields in a different PR.

[NeoLegends]

Can we use bindgen in libc? I think bindgen would handle all these cases (including accessing the bitfields) for us.

[NeoLegends]

By that I mean it would probably be feasible to copy whatever bindgen generates, but (in this case) this includes bindgen's bitfield helpers and I'm not sure whether you want to expose them. You could likely make them private though, and only expose accessor functions that do not expose bindgens internals.

[NeoLegends]

Regarding the C bitset rules, the bitsets in the C header are exposed as two distinct __u8, so I think that should clarify how they're laid out. Or rather, I'd find it weird if C merged both bitfields into one large-ish field.

[gnzlbg]

You can run bindgen yourself on the side and see what it does and try to replicate it here, but we can't use bindgen since it doesn't work well with cross-compiling. I hope doing this isn't two hard, you should just need a getter and a setter per bitfield, that does an assert that the bit index is in bounds, and just "test" or "sets" a bit in a u8. The more complicated part is going to be adding a test for this to libc-test. What we do there, is have some Rust code with a #[test] that creates a tcp_info, and uses the setters to set the bitfields to some pattern. Then it calls some C code, that verifies the pattern by accessing the struct in C, and modifies it, returning the tcp_info struct back to Rust. Finally, Rust uses the setters to verify the C patter, and the test succeeds. That's more or less how our roundtrip tests work, but for the getters and setters these would need to be written manually instead of being automatically generated.

[NeoLegends]

I‘ve read here https://stackoverflow.com/a/6044223 that the data layout of bitfields isn‘t all that well-defined. Do you think it‘s safe to just do it the intuitive way and write regular getters and setters that shift the data around?

[gnzlbg]

I don't see this as a choice for this type. Rust does not have bitfields, so we need to treat the storage where they are stored as raw bytes. If we want to give those raw bytes some meaning, the only safe way to do that is via getter and setters.

[NeoLegends]

Sure, I think I wasn't really clear on that. I was just wondering how to implement the getters and setters correctly, given that the StackOverflow article suggests the actual data layout in bitfields isn't all that well-defined. But I might also just be overly cautious on that. I just don't have much experience with C and try to be careful if I have to deal with it.

I fully agree the data has to be exposed in a proper implementation.

[gnzlbg]

Yeah, makes sense. Let's only try to do that later in a different PR, when this one is merged.

[gnzlbg]

You might need to include the header that contains tcp_info to the cfg.header list for the linux tests.

[gnzlbg]

Maybe the linux headers we are using for testing are too old:

error: 'struct tcp_info' has no member named 'tcpi_max_pacing_rate';

?

[NeoLegends]

Got the time to revive this now. :)

[NeoLegends]

Hmm the jobs fail because there seem to be superfluous parentheses around code I didn't touch. Wait until that's fixed + rebase?

[bors]

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

[JohnTitor]

Hey, sorry for the big delay, I totally overlooked this PR. If you're still interested in it, could you resolve the merge conflicts? Or, it's also fine to open a new PR as rebasing would be hard.

[JohnTitor]

Closing as inactive, feel free to open a new PR if you're still interested in that change!