Replace parts of ffi module by libc functions ind sched.rs #402
Conversation
| flags: c_int, | ||
| arg: *mut super::CloneCb, | ||
| ...) | ||
| -> c_int; |
There was a problem hiding this comment.
Can this be replaced with the libc clone?
There was a problem hiding this comment.
Probably, but understanding what is happning and how clone is supposed to be used, was to much for me yesterday evening, so given that it does not share any types or code with the other functions in that file, I left it for another commit.
There was a problem hiding this comment.
Yeah there's some fanciness going on here to allow passing a Rust closure as the function to run in the new process. There's also #360 which is concerning.
|
Overall looks good. Interested to hear feedback on the comments but neither of those seem gating. Your commit message has a typo ("ind") that you might as well amend to fix. |
|
Test failures look legitimate (typo). Once that is fixed, looks good r=me. |
|
|
||
| // Structure representing the CPU set to apply | ||
| #[repr(C)] | ||
| #[derive(Clone, Copy)] | ||
| pub struct CpuSet { | ||
| cpu_mask: [CpuMask; cpuset_attribs::CPU_SETSIZE/cpuset_attribs::CPU_MASK_BITS] | ||
| cpu_mask: [CpuMask; CPU_SETSIZE as usize / cpuset_attribs::CPU_MASK_BITS], |
There was a problem hiding this comment.
would it make more sense to wrap libc::cpu_set_t here?
There was a problem hiding this comment.
Since the field in cpu_set_t is private, it would complicate the set and get methods below needlessly.
There was a problem hiding this comment.
When we use theCPU_ functions (macros), the previous comment becomes obsolete.
|
Few more comments, mostly to do with using even more libc :-) |
|
Thank you for your comments. #392 is also awaiting a merge (or more comments). |
|
I changed the PR based on @kamalmarhubi's comments. However, I did not use Maybe someone can come up with a macro, that captures the three very similar functions. I could not get around implementing it in at least two rules in order to differentiate between What also should be looked at are the return values. Since we want to provide safe interfaces, I felt that such a check was necessary. |
|
Given that @posborne was fine with the changes and I addressed (almost) all of @kamalmarhubi's concerns and in the intrest of getting forward with this project, I'll take @posborne's invitation: How about a release in the next few weeks? @kamalmarhubi could write up how it is done in the RELEASE_PROCEDURE.md, and then I could do it in the future in regular intervals. |
|
📌 Commit 899a130 has been approved by |
Replace parts of ffi module by libc functions ind sched.rs Not exporting `CpuMask` anymore, should be the only API change of this PR. Since the type is not used in any other exported signature or type, I think it is fine to no longer export it. r? @kamalmarhubi @posborne
Absolutely, it may make sense to push for it earlier. I saw some requests for it on nix and I think @carllerche said he was planning to cut a mio release soon. |
|
☀️ Test successful - status |
Not exporting
CpuMaskanymore, should be the only API change of this PR. Since the type is not used in any other exported signature or type, I think it is fine to no longer export it.r? @kamalmarhubi @posborne