-
Notifications
You must be signed in to change notification settings - Fork 1.2k
procctl for freebsd 15 upgrade. #4657
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
base: main
Are you sure you want to change the base?
Conversation
e44b6d0
to
17b153c
Compare
Could you please rebase on the latest commit of the main branch? This will resolve some unrelated FreeBSD CI failures and thus make the CI results clearer.
c992ac6213276f54d868f317cc5092f8aed4ff54 seems to be a rather arbitrary recent commit. I'd suggest linking dabf006a638fdc44cdcf69731de8ac83959db731 instead, which is the commit that introduced the new constants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for your contribution! It looks good (to my eyes at least) for the most part
pub const PROC_NO_NEW_PRIVS_STATUS: c_int = 20; | ||
pub const PROC_WXMAP_CTL: c_int = 21; | ||
pub const PROC_WXMAP_STATUS: c_int = 22; | ||
pub const PROC_PROCCTL_MD_MIN: c_int = 0x10000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best to keep PROC_PROCCTL_MD_MIN
at the end, it doesn't really have a place between PROC_WXMAP_STATUS
and PROC_LOGSIGEXIT_CTL
in the original headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
PR_SCTP_POLICY(x) <= SCTP_PR_SCTP_MAX | ||
} | ||
|
||
pub {const} fn PPROT_OP(o: c_int) -> c_int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally would prefer the PPROT changes in a separate commit (same PR is ok), so they can get a more focused commit message (perhaps with a reference link as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but ultimately I will have to squash it so..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fyi, >1 commit per PR is fine if they're meaningful (the merge strategy here is rebase). We just don't want the progress commits like "fix abc" "update" etc.
865eb73
to
54943db
Compare
It looks like there are still some test failures with FreeBSD14, so |
Reminder, once the PR becomes ready for a review, use |
54943db
to
ce79713
Compare
adding missing PROC_SPROTECT helpers while at it. ref: freebsd/freebsd-src@dabf006#diff-769c07b1f90eef1351a20f40593b254f85925859273219036320baa1d086774cR68
ce79713
to
ed7f951
Compare
// Added in FreeBSD 15 | ||
"PROC_LOGSIGEXIT_CTL" | ||
| "PROC_LOGSIGEXIT_STATUS" | ||
| "PROC_LOGSIGEXIT_CTL_NOFORCE" | ||
| "PROC_LOGSIGEXIT_CTL_FORCE_ENABLE" | ||
| "PROC_LOGSIGEXIT_CTL_FORCE_DISABLE" | ||
if Some(15) > freebsd_ver => | ||
{ | ||
true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these added to FreeBSD < 15 in the first place? I think they could just live in src/unix/bsd/freebsdlike/freebsd/freebsd15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you refer to (ie freebsd < version >) is the place where data types change across releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand - freebsd15/mod.rs
also includes constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @asomers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a function or constant changes between releases, then we need to put it in the appropriately versioned file. That's so users don't, for example, call the FreeBSD-11-compat version of fstatat
with the FreeBSD 12+ struct stat
. That would be bad. But if a function, struct, or constant newly appears in some new FreeBSD release, then it's fine to put it into the main freebsd/mod.rs file. If a user tries to use it while running a too-old version of FreeBSD, they'll simply get an ENOSYS
error, which isn't too bad.
POSIX_SPAWN_SETSIGDEF | ||
POSIX_SPAWN_SETSIGMASK | ||
PPPDISC | ||
PPROT_CLEAR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the list of symbols you're adding here different than what you're adding in freebsd/mod.rs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added forgotten ones.
adding missing PROC_SPROTECT helpers while at it.
ref: freebsd/freebsd-src@dabf006#diff-769c07b1f90eef1351a20f40593b254f85925859273219036320baa1d086774cR68