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

fix(cgroup): fix parse zero or multi optional fields in mountinfo #113

Merged
merged 3 commits into from Dec 20, 2021

Conversation

PureWhiteWu
Copy link
Sponsor Contributor

@PureWhiteWu PureWhiteWu commented Oct 21, 2021

There could be zero or more optional fields in mountinfo according to https://man7.org/linux/man-pages/man5/proc.5.html

And also there's some minor code cleanup.

@PureWhiteWu
Copy link
Sponsor Contributor Author

In addition, I think this is a critical bug, which may cause performance greatly affected in containers(in fact, we are encountering this bug in our production environment).
Could you please publish a new version after this is merged?
Thanks a lot.

@seanmonstar
Copy link
Owner

Thanks for the PR! A problem we currently have is that the CI is no longer active on new PRs. We need to move off Travis-CI, and probably onto Github Actions.

@PureWhiteWu
Copy link
Sponsor Contributor Author

Thanks for the PR! A problem we currently have is that the CI is no longer active on new PRs. We need to move off Travis-CI, and probably onto Github Actions.

Okay, I've already sent another PR #114 to set up github actions.
You may need to enable the workflows and remove travis from your config.

@seanmonstar
Copy link
Owner

Ok, so I think if you rebase this branch off master, we'll get the proper CI going...

@PureWhiteWu
Copy link
Sponsor Contributor Author

Seems that Rust 1.13 is too old, can we update it to a newer version?

@PureWhiteWu
Copy link
Sponsor Contributor Author

I've tested and found that this requires a minimal version of 1.22, so I changed to version in ci to 1.22.
This can make code much more easier to read and write, so I think this upgrade is worthwhile.

@seanmonstar
Copy link
Owner

In general, I've taken a conservative approach to MSRV upgrades, and only done so when a new thing in Rust was needed to provide essential functionality. What is needed here that's new? Is it just if let Some(..) = syntax? We could just use match and keep the MSRV.

@PureWhiteWu
Copy link
Sponsor Contributor Author

PureWhiteWu commented Dec 16, 2021

I've fixed that on 1.13 and it should be fine now.
FYI, the feature I used that doesn't support 1.13 is the two below:

  1. https://github.com/seanmonstar/num_cpus/pull/113/files#diff-8eb8a5676963047d3171ac963ed5ade2b14abf02ba028c01b9dfee133a70c0cbR243
  2. https://github.com/seanmonstar/num_cpus/pull/113/files#diff-8eb8a5676963047d3171ac963ed5ade2b14abf02ba028c01b9dfee133a70c0cbR302

After this is merged, could you please release a new version?

@seanmonstar seanmonstar merged commit a1c3827 into seanmonstar:master Dec 20, 2021
@seanmonstar
Copy link
Owner

And release as v1.13.1. Thanks for the work!

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.

None yet

2 participants