freebsd(13.2): add netlink/netlink.h support

[dybucc]

Description

This PR updates #3201 with merge conflicts resolved and follows the new plan at
¹.

The patch adds support for netlink.h interfaces in OpenBSD, where there's an
item resolution conflict if we expose the Rust bindings alongside those of
if_mib.h. This set of APIs is "scoped" in C because they live on separate
headers. In rust-lang/libc, we reexport all items at the root crate level, which
makes item resolution fail.

Note this depends on #5325. It won't pass tests but it will build. This is
because the test templates will gather all items in a single file, so item
resolution fails. We can't really skip these items altogether from the tests, so
it may just be necessary to extend ctest to allow skipping module-specific
Rust items.

Checklist

• Relevant tests in libc-test/semver have been updated
• Commit messages permalink to headers for added or changed API
• Placeholder or unstable values like *LAST or *MAX have the standard
  doc comment
• Tested locally (cargo test -p libc-test --target mytarget); especially
  relevant for platforms that may not be checked in CI

@rustbot label +stable-nominated

Footnotes

1. https://github.com/rust-lang/libc/pull/3201#issuecomment-4736374182 ↩

[tgross35]

The API looks fine from a quick skim, but since there is no hurry, I think it may be worth trying to add support to ctest first so the tricky test setup isn't needed. (It's useful otherwise too.)

Sketched some of that up at #5344

[dybucc]

Noted. Since you already pinged some contributor on that issue, I'll wait and
see whether they want to implement it. Otherwise, I'll do it.

[tgross35]

Could you try adding a separate TestGenerator instance like @mbyx recommended in that issue? Since it's split off, that should avoid the need to reexport libc::netlink at test root too.

[tgross35]

Either author or blocked, depending on whether that works.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[dybucc]

It seems having separate invocations gets the job done just fine. Still, that
was the same thing I tried in the initial patch. The added complexity comes when
we need to get the SemVer tests to pass.

Those seem to me like they're going to need extending (though it's notably less
work than with ctest as it's all in libc-test's build script.)

[dybucc]

@rustbot ready

[tgross35]

Cc @asomers, could you take a look when you get the chance?

View changes since this review

[tgross35]

Sort of, but there's no shallow copy. I guess the Rc<dyn Fn()> might look that way but it's just letting closures be used from multiple places like function pointers - it's not like cloning to a TestGenerator then adding skips to one means that the other TestGenerator gets those same skips.

(The pattern for something like that would be field: Rc<RefCell<Box<dyn Fn()>>> or field: Rc<RefCell<Vec<Box<dyn Fn()>>>.)

So you can delete the .skip_union(|_| false) ...

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dybucc]

@rustbot ready

[tgross35]

Also it looks like CI isn't yet passing, currently stuck on style but make sure the FreeBSD tests pass too.

View changes since this review

[tgross35]

Drop #[inline], no need to really worried about that kind of performance optimizations in tests and this isn't a bottleneck.

[dybucc]

Done.

[tgross35]

This can now be pub(crate) right?

[dybucc]

Yup. Done.

[tgross35]

Maybe just rename src/new/freebsd/netlink/netlink.rs to netlink_.rs or something, so we can keep the glob export? Think this might be done elsewhere as well.

Just add a comment by the mod netlink_ explaining why it has that name.

[dybucc]

I don't think this is a good idea. The new module's directory layout is meant to provide a fairly accurate mapping between upstream and our Rust module tree. Starting to allow this type of "exceptions" seems to me like we're just muddying things up (to eventually be back at having something as hard to grok as we have now outside new.)

I don't know of prior precedent to this, so we could very well be already allowing this. Still, considering this is simply to replace an inline module with the containing reexports, it doesn't seem worth it.

[tgross35]

These shouldn't be needed anymore right?

[dybucc]

I don't think so. IIUC, TestGenerator will parse the Rust files, which we presently can't restrict to a subset of the codebase. If these are removed, we'd be setting up test for all the types that are not part of the netlink module, even if we've only provided the #include for the netlink interfaces (a few lines above this comment.)

[tgross35]

IME the term "shallow copy" typically means (1) there exists a real or theoretical corresponding deep copy, and (2) the difference between a shallow and deep copy is observable. With this code:

let cfg1 = TestGenerator::new();
let cfg2 = cfg1.clone();
cfg1.skip_struct(|ty| ty.ident() == "foo");
// ...

I'd definitely consider it a shallow copy if both cfg1 and cfg2 skip foo as a result (would be pretty surprising API). I thought that's what you were expecting at first based on the comments and is why I mentioned the RefCell patterns, but perhaps this wasn't correct?

That shouldn't be what happens, cfg1 and cfg2 are decoupled. Existing skips do stick around when cloning, but it's more like cloning a Vec to get the same base items then being able to add on independently.

There is a "shallow copy" with the Rc<dyn Fn()> and it wouldn't be technically incorrect to refer to it as such, but I'd find it an unintuitive use of the term because you need to go out of your way and use interior mutability to observe it. It's not too far off from duplicating a fn() or &u32—technically shallow and observable, but the distinction isn't especially meaningful.

This is mostly just opinion, though, I'm sure others may look at it differently.

[dybucc]

CI fails now because of a recent unused-group lint that was added in rust-lang/rust#157036. We happen to have macros that add this by default, but that don't merge them when munching the repr attributes. Maybe that is the solution, but I have other PR reviews to go through first, so I can't work on that just yet.

@rustbot ready