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
cmd/snap-confine: use snap-discard-ns ns to discard stale namespaces #6147
Conversation
This comment has been minimized.
This comment has been minimized.
During startup snap-confine may decide to discard a preserved mount namespace that became stale. This used to involve unmounting one file. Then we realized we had a bug because the .fstab file was not unlinked. With the introduction of per-user mount namespaces we need to do a little bit more. Instead of duplicating this logic or providing library code I believe the easiest way is to execute snap-discard-ns itself. The same patch contains a small refactoring for opening internal tools. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
5bd2d43
to
90a4d79
Compare
@@ -307,18 +307,59 @@ enum sc_discard_vote { | |||
SC_DISCARD_YES = 2, | |||
}; | |||
|
|||
static void call_snap_discard_ns(int snap_discard_ns_fd, const char *snap_name) |
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.
Any chance this could get unified into a single function with sc_setup_mount_profiles
?
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.
Hmm, not sure why? Can you explain please?
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.
Discussed on IRC. In the hope of saving time between iterations, we'll treat it as a proposal for refactoring to be revisited 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.
To clarify what the discussion included, the part where snap-confine forks, execs and waits could be factored out to a new helper. The leftovers, argument and environment handling, would stay in the current functions.
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 did this refactoring but it turned out to be larger than I hoped. I will propose this separately next.
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.
looks good. Couple of q's.
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
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, +1
During startup snap-confine may decide to discard a preserved mount
namespace that became stale. This used to involve unmounting one file.
Then we realized we had a bug because the .fstab file was not unlinked.
With the introduction of per-user mount namespaces we need to do a little
bit more. Instead of duplicating this logic or providing library code
I believe the easiest way is to execute snap-discard-ns itself.
The same patch contains a small refactoring for opening internal tools.
Signed-off-by: Zygmunt Krynicki me@zygoon.pl