Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-update-ns: add actual implementation #3216
Conversation
All checks have failed
zyga
closed this
Apr 24, 2017
|
I'll reopen in a moment, need to address the spread part |
| + } | ||
| + | ||
| + // Compute the needed changes and perform each change if needed, collecting | ||
| + // those that we managed to perform or that were performed already. |
jdstrand
May 1, 2017
Contributor
It is probably worth adding to this comment something about how the order of entries in the fstab files is critically important (something you mention in NeededChanges, but not here) and that you are relying on changesNeeded to be ordered in a particular way.
| + if err != nil { | ||
| + return fmt.Errorf("cannot read mount-info table: %s", err) | ||
| + } | ||
| + if !change.Needed(mounted) { |
jdstrand
May 1, 2017
Contributor
With what is currently in master, Needed() unconditionally returns 'true'. Also, this reads weird to me. if !change.Needed(mounted) reads as 'if no changes are needed, append to changesMade the change and continue', but you didn't make a change.... AIUI, what you are doing is saying that if the change isn't needed, you don't need to perform a mount, but you still want it listed in changesMade for the eventual save of the current profile.
At a minimum, you should add a comment as to why you are doing this because the naming of the variables doesn't make this obvious, but changing changesMade to something that implies less would help too.
zyga
May 4, 2017
Contributor
Will do, thanks! FYI: this is being tracked in another branch (feature/update-ns/tool)
zyga commentedApr 20, 2017
This patch adds a non-dummy implementation of snap-update-ns. There
are still three pieces missing. There's no locking so concurrently
running snap-confine is not synchronized. The function that determines
if a mount change is needed is dummy and always returns true.
The mount changes are not really performed yet as the Perform function
is just a stub.
The stubs will be addressed with separate PRs (one is already up), I just
wanted to land the general idea of how the tool operates.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com