cmd/snap-update-ns: add actual implementation #3216

Closed
wants to merge 1 commit into
from
Jump to file or symbol
Failed to load files and symbols.
+61 −2
Split
@@ -25,6 +25,9 @@ import (
"github.com/jessevdk/go-flags"
+ "github.com/snapcore/snapd/dirs"
+ "github.com/snapcore/snapd/interfaces/mount"
+ "github.com/snapcore/snapd/logger"
"github.com/snapcore/snapd/snap"
)
@@ -53,12 +56,68 @@ func run() error {
if err := parseArgs(os.Args[1:]); err != nil {
return err
}
+
// There is some C code that runs before main() is started.
// That code always runs and sets an error condition if it fails.
// Here we just check for the error.
if err := BootstrapError(); err != nil {
return err
}
- // TODO: implement this
- return fmt.Errorf("not implemented")
+
+ // TODO: lock the mount namespace so that concurrent invocations of
+ // snap-confine are synchronized. This should be done only after
+ // https://github.com/snapcore/snapd/pull/3149 lands so that we can use the
+ // lock at "/run/snapd/lock/$SNAP_NAME.lock"
+
+ // Read the current and desired mount profiles. Note that missing files
+ // count as empty profiles so that we can gracefully handle a mount
+ // interface connection/disconnection.
+ snapName := opts.Positionals.SnapName
+ currentProfilePath := fmt.Sprintf("%s/snap.%s.fstab", dirs.SnapMountPolicyDir, snapName)
+ desiredProfilePath := fmt.Sprintf("%s/snap.%s.fstab", dirs.SnapRunNsDir, snapName)
+ current, err := mount.LoadProfile(currentProfilePath)
+ if err != nil {
+ return fmt.Errorf("cannot load current mount profile: %s", err)
+ }
+ desired, err := mount.LoadProfile(desiredProfilePath)
+ if err != nil {
+ return fmt.Errorf("cannot load desired mount profile: %s", err)
+ }
+
+ // Compute the needed changes and perform each change if needed, collecting
+ // those that we managed to perform or that were performed already.
@jdstrand

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.

+ changesNeeded := mount.NeededChanges(current, desired)
+ var changesMade []mount.Change
+ for _, change := range changesNeeded {
+ // Read mount info each time as our operations may have unexpected
+ // consequences and we want to know the real state of what is mounted
+ // at each iteration.
+ mounted, err := mount.LoadMountInfo(mount.ProcSelfMountInfo)
+ if err != nil {
+ return fmt.Errorf("cannot read mount-info table: %s", err)
+ }
+ if !change.Needed(mounted) {
@jdstrand

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

zyga May 4, 2017

Contributor

Will do, thanks! FYI: this is being tracked in another branch (feature/update-ns/tool)

+ changesMade = append(changesMade, change)
+ continue
+ }
+ fmt.Printf("%s", change)
+ if err := change.Perform(); err != nil {
+ logger.Noticef("cannot perform mount change %s: %s", change, err)
+ continue
+ }
+ changesMade = append(changesMade, change)
+ }
+
+ // Compute the new current profile so that it contains only changes that were made
+ // and save it back for next runs.
+ current = &mount.Profile{
+ Entries: make([]mount.Entry, 0, len(changesMade)),
+ }
+ for _, change := range changesMade {
+ current.Entries = append(current.Entries, change.Entry)
+ }
+ if err := current.Save(currentProfilePath); err != nil {
+ return fmt.Errorf("cannot save current mount profile: %s", err)
+ }
+ return nil
}