Add getrlimit(2) and setrlimit(2) #364
Conversation
| use {Errno, Result}; | ||
|
|
||
| #[repr(i32)] | ||
| pub enum Resource { |
There was a problem hiding this comment.
should this be Resource or Rlimit / RLimit?
There was a problem hiding this comment.
@fiveop thoughts? since you added the convention on enums...
There was a problem hiding this comment.
I would prefer Resource, because it very clearly expresses what it stands for.
There was a problem hiding this comment.
Resource as a name still feels a little off give the values in the enum. You are not really getting the rlimit of a resource, you are getting the value of a specific resource limit in the system (one "resource" may have multiple related limits).
I'm not completely sold on this, but I could see naming the enum ResourceLimit and allowing get/set on the limit. E.g.
let limit = ResourceLimit::RLIMIT_STACK.get().unwrap();
ResourceLimit::RLIMIT_STACK.set(limit).unwrap();There was a problem hiding this comment.
yeah I thought of this kind of approach but I don't know yet how I feel about going so far from the C API.
In related news: I just created https://github.com/nix-rust/rfcs and will create RFC 0000 soon (probably tomorrow) to bootstrap the process. Then we'll have a good place to discuss stuff like this.
There was a problem hiding this comment.
If we add the get and set methods to the type, the name ResourceLimit does make more sense. However, if we don't do that, the variants of the enum actually represent the Resource, whose limit is set/get by the methods setrlimit, getrlimit.
The more I think about it, the more I like @posborne's proposal.
| use std::mem; | ||
|
|
||
| use libc::{self, c_int}; | ||
| pub use libc::{rlimit, RLIM_INFINITY}; |
There was a problem hiding this comment.
I think these should be reexported, but I'm not sure if we are consistently doing that.
There was a problem hiding this comment.
What about RLIM_SAVED_CUR and RLIM_SAVED_MAX?
Why don't we wrap rlimit and provide proper accessor methods? I admit that it would be purely cosmetic in this case, as far as I can see.
There was a problem hiding this comment.
What about the rlimit wrapper?
There was a problem hiding this comment.
oh I was deciding not to do that since we aren't doing anything with the type. I wanted it to be easy to specify inline, eg
setrlimit(RLIMIT_STACK, rlimit { rlim_cur: 1024*1024, rlim_max: RLIM_INFINITY }).unwrap()
There was a problem hiding this comment.
What about ResourceLimit::new(1024 * 1024, RLIM_INFINITY) ?
|
Apple failure is because |
|
With the new libc version, this should be mergeable. Could you address the open discussion topics, so that we can get this merged. |
|
I've been busy but should get to this in the next few days. Thanks for the ping! |
|
Any chance for an update? |
|
@kamalmarhubi You still planning on merging this? |
|
I experimented with this today, and ran into strange behavior when trying to change RLIMIT_STACK inside |
|
@ccope It appears that |
|
I was only specifically testing testing setrlimit with RLIMIT_STACK, and while it doesn't work in rust < 1.20.0 inside the main thread (due to the main thread guard page), I am unable to find any issues* with rust 1.20.0+. *: No matter what version of rust you use, you need to make sure that variables larger than the initial stack size don't get inlined into the main function before you can adjust the stack size. |
|
@ccope Can we expose these failures at run time or will things just Not Work and the user will be confused? Additionally we should make sure to clearly document all the invariants involved with using this functionality. @kamalmarhubi are you still willing to work on this PR and get it merged? If not, @ccope, you willing to step in here? |
|
@Susurrus rust will exit with a stack overflow error in rust 1.20+, and the user will have to figure out why RLIMIT_STACK didin't take effect. I'm not sure there's a way to implement automatic checking for this, but documentation would help. |
|
@ccope As long as this is clearly documented, I think it'll be fine, though I wonder if this falls under Rust's definition of Additionally, I'm not certain I like the API we expose here. I think we're mimicking the C API a little more closely than we should. For example I'd see us replacing the use of |
|
@kamalmarhubi I'm going to go ahead and close this PR because you haven't touched this in over a year. @ccope If you're interested in pushing this forward, please clone into your repo and file your own PR that we can then iterate on. |
No description provided.