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 current values to rlimit retrieval #346

Merged
merged 1 commit into from
Aug 31, 2017
Merged

add current values to rlimit retrieval #346

merged 1 commit into from
Aug 31, 2017

Conversation

phemmer
Copy link

@phemmer phemmer commented Apr 5, 2017

This implements the proposal in #344

During development of this, I uncovered a few issues with the current state of things.

  1. RlimitStat uses int32 for the limit values. This is insufficient as limits can overflow an int32.
  2. Rlimit() uses math.MaxInt32 to denote unlimited. It's possible for this to be the actual value of a limit. RLIMIT_INFINITY is typically -1 for this reason. Edit: This is really related to the first issue. The kernel uses -1, but once we're using uint64, the max value of the type is accurate as the -1 underflows to the max uint64 value.

There is one deficiency in this implementation. There is no way to differentiate between "no current value" and "current value is 0". We could use int64(-1) to indicate "no current value). This might make sense if we fix the RlimitStat issue to also use int64(-1) for unlimited. Open to suggestions.

Also from a consumer point of view, working with the rlimits is very cumbersome. If the consumer of gopsutil wants to get a specific rlimit, they have to iterate through every one to find the one they want. A map[int]RlimitStat (with the int key being RLIMIT_* constants) would be much easier to work with.
From a consumer standpoint, it would also be convenient if there were a way to get the string name for a RLIMIT_* constant. Currently the consumer needs to maintain a map of the int values to their string names.

Closes #344

@phemmer
Copy link
Author

phemmer commented May 3, 2017

Ping? Any feedback?

@shirou shirou mentioned this pull request May 4, 2017
9 tasks
@shirou
Copy link
Owner

shirou commented May 4, 2017

Sorry, I have not noticed.

But this PR seems to change Rlimit API. API change is not acceptable until v3 (and v3 release is not scheduled). How about implement as other function?

@phemmer
Copy link
Author

phemmer commented Jun 1, 2017

PR updated to introduce functionality as a new function. Original Rlimit() function is just a passthrough to new function with static arg.

@phemmer
Copy link
Author

phemmer commented Aug 30, 2017

ping?

@shirou
Copy link
Owner

shirou commented Aug 31, 2017

Sorry late, and thank you so much for your cool contribution!

@shirou shirou merged commit a452de7 into shirou:master Aug 31, 2017
@shirou shirou mentioned this pull request Sep 7, 2020
14 tasks
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.

Provide live values corresponding to Process.Rlimit()
2 participants