Add getrlimit(2) and setrlimit(2) #879

kpcyrd · 2018-04-01T17:10:02Z

This is a followup of #364 by @kamalmarhubi. The api has been changed to (Option<rlim_t>, Option<rlim_t>) to hide the rlim struct. This allows:

let (rlim_cur, rlim_max) = getrlimit()?;

Resolves #878.

Susurrus · 2018-04-01T17:17:34Z

src/sys/resource.rs

+#[cfg(any(target_os = "linux",
+          target_os = "openbsd",
+          target_os = "netbsd",
+          target_os = "bitrig"))]


Bitrig no longer exists, do go ahead and drop this. But please add fuchsia and empscripten, as they also support this API.

Additionally these targets should be alphabetized.

Done, removed the import.

Susurrus · 2018-04-01T17:18:05Z

src/sys/resource.rs

+
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+#[repr(i32)]
+pub enum Resource {


Please use the libc_enum! macro instead.

Susurrus · 2018-04-01T17:18:44Z

src/sys/resource.rs

+    }
+}
+
+pub fn getrlimit(resource: Resource) -> Result<(Option<rlim_t>, Option<rlim_t>)> {


Needs a doccomment, should have an example as well.

Susurrus · 2018-04-01T17:22:10Z

src/sys/resource.rs

+    Ok((rlim_to_option(rlim.rlim_cur), rlim_to_option(rlim.rlim_max)))
+}
+
+pub fn setrlimit(resource: Resource, limit: (Option<rlim_t>, Option<rlim_t>)) -> Result<()> {


I'm not a fan of this API because it's hard to know what limit means. I'd make this take a soft_limit and a hard_limit which are both Option<rlim_t>s so it's more explicit.

Additionally this should have doccomments and an example showing how this would be used (can be same as the one for getrlimit.

I've changed the api to

setrlimit(resource: Resource, soft_limit: Option<rlim_t>, hard_limit: Option<rlim_t>) -> Result<()>

I'd prefer to delay documentation until the api itself is approved.

Susurrus · 2018-04-01T17:22:53Z

src/sys/resource.rs

+    rlim.rlim_max = limit.1.unwrap_or(RLIM_INFINITY);
+
+    let res = unsafe { libc::setrlimit(resource as c_int, &rlim as *const _) };
+    Errno::result(res).map(drop)


Please do Errno::result(res).map(|_| ()) as it's more clear about what it's actually doing.

Susurrus · 2018-04-01T17:25:34Z

src/sys/resource.rs

+    RLIMIT_STACK = libc::RLIMIT_STACK,
+    RLIMIT_AS = libc::RLIMIT_AS,
+    // BSDs and Linux
+    #[cfg(all(unix, not(target_os = "solaris")))]


Please don't use not, explicitly list targets here instead.

Susurrus · 2018-04-01T17:28:47Z

src/sys/resource.rs

+}
+
+#[inline]
+fn rlim_to_option(rlim: rlim_t) -> Option<rlim_t> {


Is there not a better API to do this? I would think this would be a common enough idiom that there's a function somewhere in the Rust API to do this already.

I've changed it to a rust-style ternary expression. Option.filter would work, but is nightly only.

Susurrus · 2018-04-01T17:29:17Z

src/sys/resource.rs

+    #[cfg(all(unix, not(target_os = "solaris")))]
+    RLIMIT_RSS = libc::RLIMIT_RSS,
+    // Linux-only
+    #[cfg(any(target_os = "linux", target_os = "android"))]


These also need to be alphabetized.

Susurrus · 2018-04-01T17:29:28Z

src/sys/resource.rs

+    RLIMIT_NPROC = libc::RLIMIT_NPROC,
+    #[cfg(all(unix, not(target_os = "solaris")))]
+    RLIMIT_RSS = libc::RLIMIT_RSS,
+    // Linux-only


"Android and Linux only"

asomers · 2018-04-01T18:17:36Z

test/test_resource.rs

+#[test]
+pub fn test_resource_limits() {
+    let mut limit = getrlimit(Resource::RLIMIT_STACK).unwrap();
+    assert!(limit.0 != limit.1);


This assertion need not hold universally. On FreeBSD for example, the default hard limits and soft limits seem to be identical.

asomers · 2018-04-01T18:22:42Z

src/sys/resource.rs

+use std::mem;
+
+use libc::{self, c_int};
+pub use libc::{rlimit, rlim_t, RLIM_INFINITY};


I understand why you're reexporting rlim_t, but why reexport the others?

Done, limited the reexport to rlim_t

asomers · 2018-04-01T18:23:12Z

src/sys/resource.rs

+          target_os = "openbsd",
+          target_os = "netbsd",
+          target_os = "bitrig"))]
+pub use libc::{RLIM_SAVED_CUR, RLIM_SAVED_MAX};


Why reexport these symbols? For that matter, why import them? It seems like nothing is using them.

Done, removed the import.

asomers · 2018-04-01T18:25:36Z

src/sys/resource.rs

+    RLIMIT_RTTIME = libc::RLIMIT_RTTIME,
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    RLIMIT_SIGPENDING = libc::RLIMIT_SIGPENDING,
+}


There are more resources on non-Linux platforms, such as RLIMIT_KQUEUES, RLIMIT_NPTS, RLIMIT_SBSIZE, and RLIMIT_SWAP.

asomers

I'm happy with the API now.

asomers · 2018-04-01T20:16:00Z

src/sys/resource.rs

-    match rlim {
-        RLIM_INFINITY => None,
-        rlim => Some(rlim),
+        // Non-Linux


The comment makes it sound like these symbols are available everywhere except Linux.

Changed to // Available on some BSD

asomers · 2018-04-01T20:16:19Z

src/sys/resource.rs

+        RLIMIT_KQUEUES,
+        #[cfg(target_os = "freebsd")]
+        RLIMIT_NPTS,
+        #[cfg(target_os = "freebsd")]


RLIMIT_SBSIZE is also available on Dragonfly.

asomers · 2018-04-01T20:24:19Z

test/test_resource.rs

-    let mut limit = getrlimit(Resource::RLIMIT_STACK).unwrap();
-    assert!(limit.0 != limit.1);
+    let (mut soft_limit, hard_limit) = getrlimit(Resource::RLIMIT_STACK).unwrap();
+    let orig_limit = (soft_limit, hard_limit);


Your changes to the test do fix the previous failure, but the coverage isn't very good, since the soft and hard limits are often identical. How about setting RLIMIT_NOFILE to a hard-coded value of 1024 if the current soft limit is unlimited, or the current softlimit - 1 otherwise?

Do we really need two tests? Does the stack test provide any extra coverage over the RLIMIT_NOFILE test?

asomers · 2018-04-01T23:35:53Z

src/sys/resource.rs

+/// # Examples
+///
+/// ```
+/// use nix::sys::resource::{getrlimit, Resource};


In the doc test, you should put a # before the use, fn main, and final } lines so they will be hidden from the documentation. Likewise, omit the blank line before main.

asomers · 2018-04-01T23:37:44Z

src/sys/resource.rs

+/// # Parameters
+///
+/// * `resource`: The [`Resource`] that we want to set the limits of.
+/// * `soft_limit`: The value that the kenrel enforces for the corresponding resource.


s/kenrel/kernel

asomers · 2018-04-01T23:39:45Z

test/test_resource.rs

+    setrlimit(Resource::RLIMIT_NOFILE, soft_limit, hard_limit).unwrap();
+
+    let (new_soft_limit, new_hard_limit) = getrlimit(Resource::RLIMIT_NOFILE).unwrap();
+    assert!(new_soft_limit != new_hard_limit);


This should be assert!(new_soft_limit, soft_limit)

asomers · 2018-04-01T23:40:28Z

test/test_resource.rs

-    let mut limit = getrlimit(Resource::RLIMIT_STACK).unwrap();
-    assert!(limit.0 != limit.1);
+    let (mut soft_limit, hard_limit) = getrlimit(Resource::RLIMIT_STACK).unwrap();
+    let orig_limit = (soft_limit, hard_limit);


Do we really need two tests? Does the stack test provide any extra coverage over the RLIMIT_NOFILE test?

Susurrus · 2018-04-03T04:16:25Z

src/sys/resource.rs

+libc_enum!{
+    #[repr(i32)]
+    pub enum Resource {
+        // POSIX


Please provide doc comments for every entry here to give users an understanding of what they mean.

Susurrus · 2018-04-03T04:16:33Z

src/sys/resource.rs

+
+libc_enum!{
+    #[repr(i32)]
+    pub enum Resource {


This enum needs a doccomment as well.

Susurrus · 2018-04-03T04:17:35Z

src/sys/resource.rs

+
+/// Get the current processes resource limits
+///
+/// A value of `None` corresponds to `RLIM_INFINITY`, which means there's no limit.


I'd just remove the RLIM_INFINITY reference. For the user that implementation detail is completely hidden: "A value of None indicates that there's no limit."

Susurrus · 2018-04-03T04:18:06Z

src/sys/resource.rs

+///
+/// ```
+/// # use nix::sys::resource::{getrlimit, Resource};
+/// # fn main() {


You shouldn't need to declare main() here.

Susurrus · 2018-04-03T04:18:47Z

src/sys/resource.rs

+    rlim.rlim_max = hard_limit.unwrap_or(RLIM_INFINITY);
+
+    let res = unsafe { libc::setrlimit(resource as c_int, &rlim as *const _) };
+    Errno::result(res).map(drop)


This should be map(|_| ()).

Susurrus · 2018-04-03T04:21:00Z

src/sys/resource.rs

+pub fn getrlimit(resource: Resource) -> Result<(Option<rlim_t>, Option<rlim_t>)> {
+    let mut rlim: rlimit = unsafe { mem::uninitialized() };
+    let res = unsafe { libc::getrlimit(resource as c_int, &mut rlim as *mut _) };
+    Errno::result(res).map(|_| {


Ugly, but it works. Can you put a TODO in here to switch over to that other API you mentioned once it hits stable?

Susurrus · 2018-04-03T04:21:58Z

test/test_resource.rs

+
+    // make sure the soft limit and hard limit are not equal
+    let soft_limit = match soft_limit {
+        Some(nofile) => Some(nofile -1),


Need a space after the -

Susurrus · 2018-04-03T04:22:21Z

test/test_resource.rs

+use nix::sys::resource::{Resource, getrlimit, setrlimit};
+
+#[test]
+pub fn test_resource_limits_nofile() {


Can you summarize what this test is doing? Makes it easier to fix or modify them later.

Susurrus · 2018-04-03T04:24:33Z

src/sys/resource.rs

+/// # fn main() {
+/// let soft_limit = Some(1024);
+/// let hard_limit = None;
+/// setrlimit(Resource::RLIMIT_NOFILE, soft_limit, hard_limit).unwrap();


Is this a realistic use case auser might actually do? If not, we should try to find one more common to use as an example and provide a nice description of it before the codeblock.

Susurrus · 2018-04-03T04:25:32Z

test/test_resource.rs

+}
+
+#[test]
+pub fn test_resource_limits_stack() {


Could you provide some comments or a general summarizing comment here? It makes it easier for us reviewers to understand and to modify/fix them later.

kpcyrd · 2018-04-14T16:51:07Z

I'm having trouble coming up with a cross plattform compatible test that is also a realistic test case. Could you suggest one?

Susurrus · 2018-04-15T18:47:37Z

A test doesn't need to run on all platforms, though it's nice if it does. It'd be good to get at least a test working on one of our Tier 1 platforms so it'll run regularly in CI tho.

kamalmarhubi · 2018-04-15T18:47:15Z

src/sys/resource.rs

+/// [setrlimit(2)](https://linux.die.net/man/2/setrlimit)
+///
+/// [`Resource`]: enum.Resource.html
+pub fn setrlimit(resource: Resource, soft_limit: Option<rlim_t>, hard_limit: Option<rlim_t>) -> Result<()> {


I came in here to suggest changing this to

pub fn setrlimit<S=rlim_t, H=rlim_t>(resource: Resource, soft_limit: Option<rlim_t>, hard_limit: Option<rlim_t>) -> Result<()> where S: Into<Option<rlim_t>>, H: Into<Option<rlim_t>> { // ...

playground example

to take advantage for the impl<T> From<T> for Option<T> impl added in 1.12: rust-lang/rust#34828.

but it turns out we can't use default type params in functions on stable. There's a WG looking into this, so maybe we can open it up later this year. (This would allow setrlimit(RLIMIT_RSS, 1024 * 1024, None) without the Some wrapping on the first arg.)

kamalmarhubi · 2018-04-15T18:48:21Z

src/sys/resource.rs

+/// [`Resource`]: enum.Resource.html
+pub fn setrlimit(resource: Resource, soft_limit: Option<rlim_t>, hard_limit: Option<rlim_t>) -> Result<()> {
+    let mut rlim: rlimit = unsafe { mem::uninitialized() };
+    rlim.rlim_cur = soft_limit.unwrap_or(RLIM_INFINITY);


I would prefer to leave this to the kernel.

Susurrus · 2018-06-02T19:20:36Z

@kpcyrd friendly pin on this.

kpcyrd · 2018-06-19T22:21:28Z

This issue is up for grabs if anybody is interested in it, but I might come back to it later. I'm currently using my own solution in the program I was working on when filing this PR.

rtzoeller · 2021-11-19T03:46:02Z

Implemented by #1302.

Add getrlimit(2) and setrlimit(2)

63c438f

Susurrus suggested changes Apr 1, 2018

View reviewed changes

asomers requested changes Apr 1, 2018

View reviewed changes

kpcyrd added 4 commits April 1, 2018 21:46

Update API

aaf89e4

Use libc_enum

f69bedb

Add freebsd specific rlimits

2c4d804

Use explicit target_os list

13f75c1

asomers requested changes Apr 1, 2018

View reviewed changes

kpcyrd added 3 commits April 2, 2018 00:55

Add docs

7b537e3

Increase test coverage

8c18615

Add examples

de89045

asomers requested changes Apr 1, 2018

View reviewed changes

Update documentation and tests

7242607

Susurrus suggested changes Apr 3, 2018

View reviewed changes

kpcyrd added 2 commits April 4, 2018 18:42

Codestyle and docs

1c629ad

Document Resource enum

f87978b

kamalmarhubi reviewed Apr 15, 2018

View reviewed changes

j1ah0ng referenced this pull request in j1ah0ng/nix Mar 6, 2020

Commenting for clarity

88c0581

j1ah0ng mentioned this pull request Mar 6, 2020

Add getrlimit(2) and setrlimit(2) #1190

Closed

rtzoeller closed this Nov 19, 2021

Add getrlimit(2) and setrlimit(2) #879

Add getrlimit(2) and setrlimit(2) #879

Conversation

kpcyrd commented Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpcyrd commented Apr 14, 2018

Susurrus commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jun 2, 2018

kpcyrd commented Jun 19, 2018

rtzoeller commented Nov 19, 2021

kpcyrd commented Apr 1, 2018 •

edited

Loading