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

Add getrlimit and setrlimit #1302

Merged
merged 1 commit into from
Aug 1, 2021
Merged

Add getrlimit and setrlimit #1302

merged 1 commit into from
Aug 1, 2021

Conversation

LMJW
Copy link
Contributor

@LMJW LMJW commented Oct 3, 2020

This PR is based on the previous PR #1190, which has not been updated for a very long time.

I have fixed the segfault and compilation error of the original PR and rebased the changes to the latest master branch.

Please let me know if anything need to been changed.

@asomers
Copy link
Member

asomers commented Oct 3, 2020

You need to fix the test failures. First of all, use libc::self is unnecessary in Rust 2018. Just remove it. I don't know if that's related to the other test failures.

@LMJW LMJW closed this Oct 3, 2020
@LMJW LMJW reopened this Oct 3, 2020
@LMJW
Copy link
Contributor Author

LMJW commented Oct 3, 2020

Clicked the wrong button... Will fix the issues.

@LMJW LMJW force-pushed the add_rlimit branch 2 times, most recently from 323b5a4 to e85d4ed Compare October 4, 2020 22:28
@LMJW
Copy link
Contributor Author

LMJW commented Oct 5, 2020

Fixed the test errors. Please review. Thank you @asomers

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than define Resource twice for different platforms, which doesn't even handle all platform-specificities, I think you should define Resource only once, and gate each enum item with an appropriate #[cfg(...)].

I haven't reviewed the tests yet. ENOTIME.

src/sys/resource.rs Outdated Show resolved Hide resolved
src/sys/resource.rs Outdated Show resolved Hide resolved
src/sys/resource.rs Outdated Show resolved Hide resolved
src/sys/resource.rs Outdated Show resolved Hide resolved
src/sys/resource.rs Outdated Show resolved Hide resolved
src/sys/resource.rs Outdated Show resolved Hide resolved
src/sys/resource.rs Outdated Show resolved Hide resolved
test/test.rs Outdated Show resolved Hide resolved
@LMJW
Copy link
Contributor Author

LMJW commented Oct 6, 2020

Rather than define Resource twice for different platforms, which doesn't even handle all platform-specificities, I think you should define Resource only once, and gate each enum item with an appropriate #[cfg(...)].

I haven't reviewed the tests yet. ENOTIME.

Thank you for the suggestion. But I am not sure when enum can be both #[repr(u32)] and #[repr(i32)], how could I do this. (The reason why we have both u32 and i32 is explained in one of comments above) The closest case I can think of is using something like this:

libc_enum!{
    pub enum ResourceDemo {
        #[cfg(all(target_os = "linux", target_env = "gnu"))] #[repr(u32)]
        #[cfg(any(
            target_os = "freebsd",
            target_os = "openbsd",
            target_os = "netbsd",
            target_os = "macos",
            target_os = "ios",
            target_os = "android",
            target_os = "dragonfly",
            target_os = "linux"))] #[repr(i32)]
        RLIMIT_AS,

        #[cfg(all(target_os = "linux", target_env = "gnu"))] #[repr(u32)]
        #[cfg(any(
            target_os = "freebsd",
            target_os = "openbsd",
            target_os = "netbsd",
            target_os = "macos",
            target_os = "ios",
            target_os = "android",
            target_os = "dragonfly",
            target_os = "linux"))] #[repr(i32)]
        RLIMIT_CORE,
        RLIMIT_CPU,
        RLIMIT_DATA,
        RLIMIT_FSIZE
    }
}

But this will have compile error for linux-musl series toolchain as both cfg will be compiled thus causing the compile error. The idea I have came out is using cfg_if, but if there is better ways to do this. I am happy to adopt. Thank you very much.

@asomers
Copy link
Member

asomers commented Oct 8, 2020

Thank you for the suggestion. But I am not sure when enum can be both #[repr(u32)] and #[repr(i32)], how could I do this.
You can do it like this:

#[cfg_attr(any(target_os="linux", ...), repr(u32))]
#[cfg_attr(not(any(target_os="linux", ...), repr(u32)))]

@LMJW
Copy link
Contributor Author

LMJW commented Oct 9, 2020

Thanks for given me suggestions. It is very helpful for. The new PR will be pushed out shortly. However, I have few doubts about the API of the current setrlimit and getrlimit function.

The existing api for getrlimit and setrlimit is like this

pub fn getrlimit(resource: Resource) -> Result<(Option<rlim_t>, Option<rlim_t>)>;
pub fn setrlimit(resource: Resource, soft_limit: Option<rlim_t>, hard_limit: Option<rlim_t>) -> Result<()>;

My current implementation is inherit from previous implementation, which treate None as RLIM_INFINITY. However, for some os environment, like linux_gnu, use setrlimit(Resource::RLIMIT_NOFILE, Some(1024), None) will results an EPERM error whereas other os environment will treat it is okay (see build). The EPERM error is due to None converted to RLIM_INFINITY and it is bigger than the default value1048576 , causing kernel to throw EPERM error.

The question I have is: should we treat None as RLIM_INFINITY or we should treate None as not change this value. If we decide None as not change the current value, we will need to get the orginal value first, which means we will need to call getrlimit in setrlimit method.

I feel the later case is more natural for me. But it might be my personal preference.

@LMJW LMJW mentioned this pull request Oct 9, 2020
@LMJW
Copy link
Contributor Author

LMJW commented Oct 9, 2020

Also, I feel like the previous implementation does not have enough test. But I am not sure if it is necessary to test each flag(eg RLIMIT_AS, RLIMIT_CORE ...). Eventually, it is testing libc...

@LMJW
Copy link
Contributor Author

LMJW commented Oct 9, 2020

Looks like the test failure is a fake failure. Can we rerun it?

install.sh: Installing to: /home/travis/.cargo/bin
gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now

@asomers
Copy link
Member

asomers commented Oct 10, 2020

Thanks for given me suggestions. It is very helpful for. The new PR will be pushed out shortly. However, I have few doubts about the API of the current setrlimit and getrlimit function.

The existing api for getrlimit and setrlimit is like this

pub fn getrlimit(resource: Resource) -> Result<(Option<rlim_t>, Option<rlim_t>)>;
pub fn setrlimit(resource: Resource, soft_limit: Option<rlim_t>, hard_limit: Option<rlim_t>) -> Result<()>;

My current implementation is inherit from previous implementation, which treate None as RLIM_INFINITY. However, for some os environment, like linux_gnu, use setrlimit(Resource::RLIMIT_NOFILE, Some(1024), None) will results an EPERM error whereas other os environment will treat it is okay (see build). The EPERM error is due to None converted to RLIM_INFINITY and it is bigger than the default value1048576 , causing kernel to throw EPERM error.

The question I have is: should we treat None as RLIM_INFINITY or we should treate None as not change this value. If we decide None as not change the current value, we will need to get the orginal value first, which means we will need to call getrlimit in setrlimit method.

I feel the later case is more natural for me. But it might be my personal preference.

I agree that treating None as "no change" would be most natural. That would be similar to how chown(2) treats an owner or group of -1. HOWEVER, that is not the API that settlimit and getrlimit provide. There is no race-free way to use setrlimit to change one limit but not the other. The goal of Nix is to provide idiomatically Rusty access to libc, but nothing more. So I don't think we should try to provide an interface like chown here.

@LMJW
Copy link
Contributor Author

LMJW commented Oct 12, 2020

Hi @asomers. I think I have addressed your review comments. Please let me know if you feel like further improvements.

Thank you.

src/sys/resource.rs Outdated Show resolved Hide resolved
@LMJW LMJW requested a review from asomers October 18, 2020 10:02
@Nugine
Copy link

Nugine commented Oct 20, 2020

https://github.com/rust-lang/libc/blob/bc30283752bae5918f40e6d63f858203c81303c7/src/unix/bsd/apple/mod.rs#L1914

pub const RLIMIT_RSS: ::c_int = RLIMIT_AS;

RLIMIT_RSS is same with RLIMIT_AS on macos and ios. It means that the two variants will conflict when targeting apple.

Is it correct to select only one of them?

@LMJW
Copy link
Contributor Author

LMJW commented Oct 21, 2020

https://github.com/rust-lang/libc/blob/bc30283752bae5918f40e6d63f858203c81303c7/src/unix/bsd/apple/mod.rs#L1914

pub const RLIMIT_RSS: ::c_int = RLIMIT_AS;

RLIMIT_RSS is same with RLIMIT_AS on macos and ios. It means that the two variants will conflict when targeting apple.

Is it correct to select only one of them?

@Nugine The reason why I select one of them is because they have the same value. If I select two of them, there will be a compile error like the following:

error[E0425]: cannot find value RLIMIT_AS in module libc
--> src/sys/resource.rs:53:17
|
53 | RLIMIT_AS,
| ^^^^^^^^^
help: a constant with a similar name exists
|
53 | RLIMIT_RSS,
| ^^^^^^^^^^
help: possible candidate is found in another module, you can import it into scope
|
2 | use crate::sys::resource::Resource::RLIMIT_AS;
|
error[E0081]: discriminant value 0 already exists
--> src/macros.rs:164:26
|
164 | $entry = libc::$entry,
| ^^^^
| |
| first use of 0
| enum already has 0
|
::: src/sys/resource.rs:48:9
|
48 | / libc_enum! {
49 | | #[repr(i32)]
50 | | pub enum Resource {
51 | | /// See detail of each Resource https://man7.org/linux/man-pages/man2/getrlimit.2.html
... |
103 | | }
104 | | }
| |_________- in this macro invocation
error: aborting due to 2 previous errors

@LMJW
Copy link
Contributor Author

LMJW commented Nov 15, 2020

@asomers Do you have any further suggestions on the improvement?

@Nugine
Copy link

Nugine commented Jul 26, 2021

Eight months have passed. Is there any progress?
(It is the 3rd pull request for setrlimit)

@LMJW
Copy link
Contributor Author

LMJW commented Jul 26, 2021

I am happy to keep work on it. Just needs more inputs on the changes

/// earier reference (non-exhaustive).
///
/// * [Linux](https://man7.org/linux/man-pages/man2/getrlimit.2.html)
/// * [freeBSD](https://www.freebsd.org/cgi/man.cgi?query=setrlimit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preferred spelling is "FreeBSD" (note the capital F)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments. Corrected :)

RLIMIT_NOFILE,
RLIMIT_STACK,

// platform specific
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this comment here? It seems like it should apply to every entry that has a #[cfg(...)]. But #[cfg(...)] is self-explanatory. Better just to leave it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

/// [`Resource`]: enum.Resource.html
///
/// Note: `setrlimit` provides a safe wrapper to libc's `setrlimit`. For the
/// platform that are not compatible, try using `prlimit` to set rlimit.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment obsolete now? prlimit is no longer included in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the obsolete comments.

pub fn test_resource_limits_nofile() {
let (soft_limit, hard_limit) = getrlimit(Resource::RLIMIT_NOFILE).unwrap();

// make sure the soft limit and hard limit are not equal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What ensures that the soft limit is not equal to the hard limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually obsolete comments from the very early version of the test, see LMJW@4f8b468

For this particular test, I believe we don't need to have soft limit to equal to hard limit

@LMJW LMJW force-pushed the add_rlimit branch 7 times, most recently from 0fb6859 to daa86d7 Compare July 31, 2021 15:24
@LMJW LMJW requested a review from asomers July 31, 2021 15:40
@LMJW
Copy link
Contributor Author

LMJW commented Jul 31, 2021

Hi @asomers. I have made some updates. Please let me know if you have any concerns. Thanks

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it looks good. Just add a CHANGELOG entry and you'll be done.

This work is a continutation on previou contribution by @kpcyrd and @j1ah0ng.
@LMJW
Copy link
Contributor Author

LMJW commented Aug 1, 2021

Hi @asomers. I have updated the CHANGELOG. Thanks for reviewing the pull request :)

@LMJW LMJW requested a review from asomers August 1, 2021 00:40
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors bors bot merged commit ba42d04 into nix-rust:master Aug 1, 2021
@yihuaf
Copy link

yihuaf commented Aug 2, 2021

When would the next release be that includes this change? Thanks.

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 this pull request may close these issues.

4 participants