-
Notifications
You must be signed in to change notification settings - Fork 42
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
mountinfo: add MountedFast #100
Conversation
Instead of copy/pasting the code to check individual mountedBy* implementations, add a list and a loop to check them. The only special thing is, we have to check if the name of the function being tested is "mountedByStat", to skip erroring out on bind mounts. Otherwise the code is the same. Signed-off-by: Kir Kolyshkin <[email protected]>
mountinfo/mounted_linux.go
Outdated
if sure { | ||
return mounted, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly prefer having this check as (even when we have tests) as an extra check.
if sure && err == nil
Can this change be made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
Also, I just realized that if normalizePath
returns an error, it does not make sense to continue with parsing mountinfo, so this code needs to be changed anyway.
Will fix.
One nit and this looks awesome! thanks! |
Should we maybe rename this to |
I like |
bd6e12a
to
7cca805
Compare
@thaJeztah @cpuguy83 PTAL |
I agree; let's stick with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments/suggestions
mountinfo/mounted_linux_test.go
Outdated
} | ||
} else { | ||
if openat2Supported { | ||
if mounted != exp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is becoming quite complex; could we remove the local exp
variable, and instead use tc.isMount
? (I had to scroll up to learn what exp
was)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
mountinfo/mounted_linux_test.go
Outdated
// Check the public MountedFast() function as a whole. | ||
mounted, sure, err := MountedFast(m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see my other comment about this test becoming quite complex; wondering if we should just move this to a separate (TestMountedFast
) test. There will be some boilerplating / code duplication, but perhaps it's a good trade-off compared to the complexity we're ending up into now. i.e. using subtests
normally avoids having to include the function name in the t.Errorf
, but because we're now combining so many we have to do it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it, too, but decided against it, since test setup/cleanup is complex and I'd rather not duplicate it.
We can move mountedFast checks into a function maybe; let me see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks better. Updated.
7cca805
to
5044597
Compare
@manugupt1 @thaJeztah PTAL 🙏🏻 |
5044597
to
422ecb3
Compare
looks good to me after @manugupt1's suggestion above has been looked at |
Fixes: dbd468b Signed-off-by: Kir Kolyshkin <[email protected]>
MountedFast is a method of detecting a mount point without reading mountinfo from procfs. A caller can only trust the result if no error and sure == true are returned. Otherwise, other methods (e.g. parsing /proc/mounts) have to be used. If unsure, use Mounted instead (which uses MountedFast, but falls back to parsing mountinfo if needed). If a non-existent path is specified, an appropriate error (rather than "not mounted") is returned. In case the caller is not interested in this particular error, it should be handled separately using e.g. errors.Is(err, os.ErrNotExist). This function is only available on Linux. The implementation mostly relies on openat2(2) syscall, available since Linux kernel v5.6. On older kernels, this uses stat(2) syscall which can reliably detect normal (but not bind) mounts. Signed-off-by: Kir Kolyshkin <[email protected]>
1. Do not compare booleans to false and true. 2. Since there are only two values, no sense in printing the actual value. Signed-off-by: Kir Kolyshkin <[email protected]>
For clarity. Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
Test both MountedFast and Mounted. Signed-off-by: Kir Kolyshkin <[email protected]>
422ecb3
to
5d09d69
Compare
@thaJeztah @moby/moby-maintainers PTAL |
One thing I failed to document is shadowed mounts. For example, if
I am failing to describe all this in a nice yet compact way. In any case, this can be done later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for doing this in such a nice way and helping me. I learnt a lot from you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
we can further improve documentation in follow ups if we see a need 👍
@kolyshkin @thaJeztah Thanks |
Yes, I wanted to prose doing so. Needed to check again in which order things had to be released (as the mountinfo and mount packages have a dependency on each other) |
This is a carry of #97 -- mostly the same code, arranged slightly differently. I have also cleaned up tests a bit.
@manugupt1 PTAL