Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing OpenBSD sysctl(2) name definitions #2458

Closed
wants to merge 1 commit into from

Conversation

n1000
Copy link

@n1000 n1000 commented Oct 16, 2021

Add a bunch of missing sysctl(2) related definitions. These definitions expand the number of sysctl parameters that can be queried without using directly hard coded constants.

Tested with libc-test on OpenBSD 6.9.

@rust-highfive
Copy link

Some changes occurred in OpenBSD module

cc @semarie

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@@ -145,8 +163,8 @@ ELAST
EMEDIUMTYPE
ENEEDAUTH
ENOATTR
ENOTBLK
Copy link
Author

@n1000 n1000 Oct 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sorted this file alphabetically after adding the new entries, and noticed some existing entries shifted a bit.

Is sorting this file the right thing to do? Just wanted to confirm I didn't blow anything up by having these definitions shift in the input file.

@semarie
Copy link
Contributor

semarie commented Oct 16, 2021

do you really intented to use all of them ?

I tend to be reluctant to add such constants just because it is possible: OpenBSD could discard them from one release to another and recycle them later, while Rust libc crate doesn't want to remove or change them (Rust libc follows linux policy where breaking changes doesn't happens, whereas OpenBSD doesn't follows such rule). So trying to be exhaustive will only add problems for later.

Regarding the PR itself, it built fine on OpenBSD 7.0-current (7.1 upcoming)

@n1000
Copy link
Author

n1000 commented Oct 16, 2021

do you really intented to use all of them ?

That is sort of a hard question to answer.. :)

The motivation for doing this was that I was tinkering on some sysctl-rs project for fun (to continue learning Rust). I was looking into what it would take to add OpenBSD support to this existing project.

As part of that I realized that most of the sysctl definitions were missing, and thought to add them.

I'm not even completely sure I would ever finish the sysctl-rs OpenBSD changes, but thought these libc changes may be useful regardless.

However, if there is a big concern on versioning issues in libc, we can drop this PR as well, as I am not requesting it for any serious reason...

@semarie
Copy link
Contributor

semarie commented Oct 17, 2021

Again, I am not against adding things, but it doesn't fit with the way OpenBSD and Rust are working togther.

What I mean is adding things now could means that later it will not be possible to changes things which would be really required for someone else.

The problem with Rust libc is there is no support for versioning: once declared it is expected that the value will exist forever (whereas OpenBSD remove and replace obsolete things). Currently, we are dealing with that with deprecation notice and ignoring values in testsuite, but it is polluting namespace and it adds (small) maintenance overhead. Problems will be bigger when conflictious things will surfaced : coexistence will not be possible without a big overhead (you could look how FreeBSD is dealing with FreeBSD 11 to 12 transition regarding some type definitions).

@JohnTitor
Copy link
Member

I'm thinking of this and yes, currently, due to the versioning, it's not a good idea to declare stuff that potentially required by someone at once. We could introduce breaking changes incrementing a minor version just like other 0.x.y crates but it may be painful to users that use unrelated platforms. Thus, I'm going to close this PR for now, but at some point, we should discuss versioning along with #547.

@JohnTitor JohnTitor closed this Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants