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.

[tgross35]

Fyi, no need to permalink these kind of files since we do want to see updates, and the file locations are usually stable enough. As long as there's a permalink in the commit message to show us what it actually looked like at that point in time.

View changes since the review

[dybucc]

You mean the files for the upstream directories only, or also the files
referring to the upstream header files?

[tgross35]

For any of the files in new, regular links rather than permalinks are fine. Just makes it easier to see the most recent version without them going out of sync.

[dybucc]

Done.

[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.)

[tgross35]

I haven't tried this but do things just work™️ if the semver tests have lines like netlink::NETLINK_ADD_MEMBERSHIP?

[dybucc]

I haven't tried this but do things just work™️ if the semver tests have lines
like netlink::NETLINK_ADD_MEMBERSHIP?

@tgross35 They do. Just cleaned up commit history as all tests seem to pass now.

[tgross35]

Does this work?

pub(crate) use freebsd::*;
pub use netlink;

View changes since the review

[dybucc]

Not quite, but something similar does work. The current reexports have been
changed to instead have first the "regular" modules, followed by a reexport of
the freebsd::netlink module.

The latter itself contains two submodules, netlink and netlink_generic,
which are now also used in the SemVer tests, as well as being the only ones
exposed to downstream crates.

[tgross35]

I think it would be preferable to have a single module since that's how most of the crate is. If there's no easy then it would be fine with just:

pub mod netlink {
    use super::freebsd;
    pub use freebsd::netlink::*;
    pub use freebsd::netlink_generic::*;
}

[dybucc]

Done. For the sake of keeping a close matching between our module tree and
upstream's directory tree under sys, I've gone with the above solution.

[tgross35]

It would be better if we can keep all this directly within a netlink module. In the exports, perhaps an in-file mod netlink { pub use ... }

View changes since the review

[dybucc]

I believe that's now fixed. The freebsd::netlink module exposed both netlink
and netlink_generic submodules, so I instead changed the reexport in
src/new/mod.rs to have the inner two modules exported.

Not everything is within a single netlink module, but at least we have a
shortened path.

[tgross35]

Sharing skips isn't intentional, why does this happen?

View changes since the review

[dybucc]

A prior review comment ¹ mentioned a recent patch that allows sharing skips
between instances of TestGenerator. I assumed you wanted me to use the now
shallow copy semantics of that type to avoid calling anew ctest_cfg() for
netlink_cfg.

This then needs resetting to the "defaults," because otherwise some skipping
functions that we do call with netlink_cfg but not with cfg end up affecting
cfg.

Footnotes

1. https://github.com/rust-lang/libc/pull/5326#discussion_r3747219428 ↩

[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) ...

[dybucc]

Done, but I've still got questions.

How is there not? TestGenerator automatically derives Clone and that would
make the inner Vec<Skip> use its Clone implementation, which itself depends
on the Clone impl of Skip. Skip is just an alias to a Rc<...>, so it
would call into the Clone impl for Rc<T: ?Sized>, which will yield a shallow
copy of T.

Granted, you'd still need some form of interior mutability to change those
skips, but I don't think skips are made to be "modified" once "registered."

What am I missing?

[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) ...

[tgross35]

Nit: put this in a is_netlink_struct function. Not super useful on its own but it's consistent with is_netlink_const and easier to expand if needed, and will the "sockaddr_nl" => true, more obvious.

[dybucc]

Done.

[tgross35]

For any of the files in new, regular links rather than permalinks are fine. Just makes it easier to see the most recent version without them going out of sync.

[tgross35]

Looks like these could all be c_enum! to match source

[tgross35]

I think it would be preferable to have a single module since that's how most of the crate is. If there's no easy then it would be fine with just:

pub mod netlink {
    use super::freebsd;
    pub use freebsd::netlink::*;
    pub use freebsd::netlink_generic::*;
}

[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.