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: optional fields for /proc/self/mountinfo #108

Closed
wants to merge 1 commit into from

Conversation

flisky
Copy link

@flisky flisky commented Jul 5, 2021

according to https://man7.org/linux/man-pages/man5/proc.5.html,
/proc/[pid]/mountinfo could have zero or more
fields and separated by a single hyphen.

@seanmonstar
Copy link
Owner

Thanks for the PR! Do you have an example that this fixes, to help me understand the reason for the code changes?

@flisky
Copy link
Author

flisky commented Jul 7, 2021

Thanks for investigating this!

       /proc/[pid]/mountinfo (since Linux 2.6.26)
              ...
              The file contains lines of the form:

              36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue
              (1)(2)(3)   (4)   (5)      (6)      (7)   (8) (9)   (10)         (11)

On our k8s instance, the mountinfo has none optional fields (part (7) in above form).
(Actually, it is sentenced as a private mount)

However, it's asked for exactly one optional field in the current implementation (shared:7 in fixtures).

@seanmonstar
Copy link
Owner

Oh ok, cool. Speaking of, can this make use of the existing fixtures (or add a new one if needed) to make some tests to catch this fix?

@flisky flisky force-pushed the master branch 4 times, most recently from 30cc35f to a73bf28 Compare July 7, 2021 09:08
@flisky
Copy link
Author

flisky commented Jul 7, 2021

Fixture is a bit cumbersome in this case, and I prefer some simple unittest :)

according to <https://man7.org/linux/man-pages/man5/proc.5.html>,
`/proc/[pid]/mountinfo` could have zero or more
fields and separated by a single hyphen.
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for the tests! And now I've just realized that this repo is still setup for Travis-CI, and that's no longer free. I need to get the tests ported over to GitHub Actions...

@flisky
Copy link
Author

flisky commented Jan 19, 2022

#113 was merged, closing.

@flisky flisky closed this Jan 19, 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 this pull request may close these issues.

None yet

2 participants