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

nix::sys::resource::getrlimit doesn't actually return None #1666

Closed
Arnavion opened this issue Feb 24, 2022 · 3 comments · Fixed by #1668
Closed

nix::sys::resource::getrlimit doesn't actually return None #1666

Arnavion opened this issue Feb 24, 2022 · 3 comments · Fixed by #1668

Comments

@Arnavion
Copy link
Contributor

Documentation says:

nix/src/sys/resource.rs

Lines 176 to 177 in 6123083

/// A value of `None` indicates the value equals to `RLIM_INFINITY` which means
/// there is no limit.

but code unconditionally returns Some():

nix/src/sys/resource.rs

Lines 209 to 212 in 6123083

Errno::result(res).map(|_| {
let rlimit { rlim_cur, rlim_max } = unsafe { old_rlim.assume_init() };
(Some(rlim_cur), Some(rlim_max))
})

This documentation is leftover from the old PR code which did return None:

    Errno::result(res).map(|_| {
        (if rlim.rlim_cur != RLIM_INFINITY { Some(rlim.rlim_cur) } else { None },
         if rlim.rlim_max != RLIM_INFINITY { Some(rlim.rlim_max) } else { None })
    })

... but the new PR removed that branch.

It would be nice to not return Option from getrlimit at least, since it makes the caller code more annoying when we only want to call setrlimit when we need to, which requires comparing the returned soft limit vs the returned hard limit. And if getrlimit is changed to not special-case INFINITY, the special-case might as well be removed from setrlimit too so that one doesn't need to Some()-wrap the values when passing them from getrlimit to setrlimit.

@asomers
Copy link
Member

asomers commented Mar 5, 2022

Good catch. Care to submit a PR to fix it?

@Arnavion
Copy link
Contributor Author

Arnavion commented Mar 5, 2022

What do you want the fix to be? Just fix the documentation? Or change the two functions to remove the Options entirely?

@asomers
Copy link
Member

asomers commented Mar 5, 2022

Probably should remove Option.

bors bot added a commit that referenced this issue Mar 14, 2022
1668: Change getrlimit and setrlimit to use rlim_t directly. r=asomers a=Arnavion

Fixes #1666

Co-authored-by: Arnavion <me@arnavion.dev>
bors bot added a commit that referenced this issue Mar 14, 2022
1668: Change getrlimit and setrlimit to use rlim_t directly. r=asomers a=Arnavion

Fixes #1666

Co-authored-by: Arnavion <me@arnavion.dev>
@bors bors bot closed this as completed in cf628ca Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants