Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/mount: add Change.Perform #3138
Conversation
zyga
added some commits
Apr 3, 2017
Some checks were not successful
All checks have passed
zyga
requested a review
from
jdstrand
Apr 12, 2017
| @@ -42,6 +44,29 @@ type Change struct { | ||
| Action Action | ||
| } | ||
| +// Perform executes the desired mount or unmount change. |
jdstrand
Apr 12, 2017
Contributor
I'm not clear on the ramifications of this change to this backend. I see OptsToFlags was added to this backend sometime before, but it isn't used by anything yet. Seems like we are working towards something, but I don't know what that is; as such, it is hard to comment on if this is doing the right thing.
My understanding of the mount backend is that it is supposed to manage files that snap-confine (or possibly other helpers) to consume. Adding OptsToFlags and Perform makes it seem like snapd is going to start manipulating the mount table directly, which would regress on a hardening feature: snap-confine runs under confinement and is only allowed to use the mount command in very specific ways (though a lot of them) and we do this to guard against coding errors. If snapd (or another unconfined helper) is performing the mount operations, we lose the guard against programming errors.
Taken at face value, Perform requires that caller handle the error condition of the syscall. While Perform is not currently a method anywhere else in the codebase, it is so general in name that it seems like the intent is that it is an implementation of a future Perform() backend primitive. As such, having the caller have to check for specific syscall error conditions for a function that might be performed generically seems like a potential problem. Perhaps just check the return values here and return an appropriate error?
jdstrand
Apr 12, 2017
Contributor
Is the mount backend the correct place for this then? I mean, it is topically similar, but functionally in the codebase, it is different....
zyga
Apr 13, 2017
Contributor
We import code from all over the tree. I think it is okay to keep it here but if you feel strongly about it I can move it elsewhere.
jdstrand
Apr 13, 2017
Contributor
It isn't about importing from all over, it is about putting the code where it belongs. The mount backend deals with mounts, yes, but it deals with them insofar as to create files for snap-confine to consume and not to perform mounts. If this doesn't resonate with you or other reviewers, I won't block on this point.
I do think that my comment regarding return syscall still holds. As written, a caller of Perform() may decide to not care about the error but since this is for snap-update-ns, it seems that you'd want to die here. Is there a particular reason you aren't doing this?
zyga
Apr 13, 2017
Contributor
The die part will be in the update-ns code itself. This is just a library routine. We don't panic() in any other code that was written for update-ns.
jdstrand
Apr 13, 2017
Contributor
Ok, well, this still suggests to me that this code should live somewhere else (perhaps even just mount_utils.go file in this directory) or somewhere since you are mixing library code meant to be used only by update-ns with code meant to be used by the interfaces system.
I've said what I have to say on this; if you feel strongly the code is in the correct place, then perhaps at the very least group the update-ns library code functions together with a comment that these are helpers for update-ns.
If other reviewers are fine with this PR, I won't block on it any more.
zyga
Apr 13, 2017
Contributor
Thanks for the review. My main goal here was to get some comments on the mount syscall vs mount command (and libmount). The particular details of error handling are not that important. They are just consistent with go and in this specific case I don't believe it is appropriate to panic.
zyga
added some commits
Apr 24, 2017
All checks have failed
All checks have passed
| +// not handled. Note that some behaviors of the mount program are actually | ||
| +// implemented with multiple calls to the mount system call and are thus not | ||
| +// atomic (e.g. --make-shared). Such features are not supported. You get what | ||
| +// the kernel gives you. |
niemeyer
Apr 25, 2017
Contributor
Some content but a bit more condensed:
// Perform executes the desired mount or unmount change using system calls.
// Filesystems that depend on helper programs or multiple independent calls to
// the kernel (--make-shared, for example) are unsupported.
| + } | ||
| + return syscall.Mount(c.Entry.Name, c.Entry.Dir, c.Entry.Type, uintptr(flags), "") | ||
| + case Unmount: | ||
| + const UmountNoFollow = 8 // This is UMOUNT_NOFOLLOW |
zyga
added some commits
Apr 25, 2017
zyga commentedApr 4, 2017
This patch adds a trivial function for performing changes to the mount
table. It is more of an RFC as we may also use the mount command
directly but this has different properties.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com