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]

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]

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?

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

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

[tgross35]

This can now be pub(crate) right?

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

[tgross35]

These shouldn't be needed anymore right?

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