Merge remote-tracking branch 'upstream/master' into validate-experimental-optionsvalidate-experimental-options

7 files changed, 124 insertions, 26 deletions

diff --git a/cmd/snap-update-ns/utils.go b/cmd/snap-update-ns/utils.go
index 85c5d460a5..9d3daefefd 100644
--- a/cmd/snap-update-ns/utils.go
+++ b/cmd/snap-update-ns/utils.go
@@ -485,7 +485,10 @@ func planWritableMimic(dir, neededBy string) ([]*Change, error) {
	changes = append(changes, &Change{
	Action: Mount, Entry: osutil.MountEntry{
	Name: "tmpfs", Dir: dir, Type: "tmpfs",
-	Options: []string{"x-snapd.synthetic", fmt.Sprintf("x-snapd.needed-by=%s", neededBy)},
+	Options: []string{
+	osutil.XSnapdSynthetic(),
+	osutil.XSnapdNeededBy(neededBy),
+	},
	},
	})
	// Iterate over the items in the original directory (nothing is mounted _yet_).
@@ -506,10 +509,10 @@ func planWritableMimic(dir, neededBy string) ([]*Change, error) {
	case m.IsDir():
	ch.Entry.Options = []string{"rbind"}
	case m.IsRegular():
-	ch.Entry.Options = []string{"bind", "x-snapd.kind=file"}
+	ch.Entry.Options = []string{"bind", osutil.XSnapdKindFile()}
	case m&os.ModeSymlink != 0:
	if target, err := osReadlink(filepath.Join(dir, fi.Name())); err == nil {
-	ch.Entry.Options = []string{"x-snapd.kind=symlink", fmt.Sprintf("x-snapd.symlink=%s", target)}
+	ch.Entry.Options = []string{osutil.XSnapdKindSymlink(), osutil.XSnapdSymlink(target)}
	} else {
	continue
	}
@@ -517,13 +520,13 @@ func planWritableMimic(dir, neededBy string) ([]*Change, error) {
	logger.Noticef("skipping unsupported file %s", fi)
	continue
	}
-	ch.Entry.Options = append(ch.Entry.Options, "x-snapd.synthetic")
-	ch.Entry.Options = append(ch.Entry.Options, fmt.Sprintf("x-snapd.needed-by=%s", neededBy))
+	ch.Entry.Options = append(ch.Entry.Options, osutil.XSnapdSynthetic())
+	ch.Entry.Options = append(ch.Entry.Options, osutil.XSnapdNeededBy(neededBy))
	changes = append(changes, ch)
	}
	// Finally unbind the safe-keeping directory as we don't need it anymore.
	changes = append(changes, &Change{
-	Action: Unmount, Entry: osutil.MountEntry{Name: "none", Dir: safeKeepingDir, Options: []string{"x-snapd.detach"}},
+	Action: Unmount, Entry: osutil.MountEntry{Name: "none", Dir: safeKeepingDir, Options: []string{osutil.XSnapdDetach()}},
	})
	return changes, nil
 }
@@ -580,7 +583,7 @@ func execWritableMimic(plan []*Change, sec *Secure) ([]*Change, error) {
	// for how to undo" so we need to flip the actions.
	recoveryUndoChange.Action = Unmount
	if recoveryUndoChange.Entry.OptBool("rbind") {
-	recoveryUndoChange.Entry.Options = append(recoveryUndoChange.Entry.Options, "x-snapd.detach")
+	recoveryUndoChange.Entry.Options = append(recoveryUndoChange.Entry.Options, osutil.XSnapdDetach())
	}
	if _, err2 := changePerform(recoveryUndoChange, sec); err2 != nil {
	// Drat, we failed when trying to recover from an error.
@@ -596,7 +599,7 @@ func execWritableMimic(plan []*Change, sec *Secure) ([]*Change, error) {
	// change is the safe-keeping unmount.
	continue
	}
-	if kind, _ := change.Entry.OptStr("x-snapd.kind"); kind == "symlink" {
+	if change.Entry.XSnapdKind() == "symlink" {
	// Don't represent symlinks in the undo plan. They are removed when
	// the tmpfs is unmounted.
	continue
diff --git a/interfaces/mount/spec.go b/interfaces/mount/spec.go
index 08bdc78f89..793cf1b648 100644
--- a/interfaces/mount/spec.go
+++ b/interfaces/mount/spec.go
@@ -66,7 +66,7 @@ func mountEntryFromLayout(layout *snap.Layout) osutil.MountEntry {
	}
	if layout.BindFile != "" {
	mountSource := layout.Snap.ExpandSnapVariables(layout.BindFile)
-	entry.Options = []string{"bind", "rw", "x-snapd.kind=file"}
+	entry.Options = []string{"bind", "rw", osutil.XSnapdKindFile()}
	entry.Name = mountSource
	}
 
diff --git a/osutil/mountentry.go b/osutil/mountentry.go
index 84e80e2d8e..f294d57829 100644
--- a/osutil/mountentry.go
+++ b/osutil/mountentry.go
@@ -356,6 +356,43 @@ func (e *MountEntry) XSnapdSynthetic() bool {
	return e.OptBool("x-snapd.synthetic")
 }
 
+// XSnapdKind returns the kind of a given mount entry.
+//
+// There are three kinds of mount entries today: one for directories, one for
+// files and one for symlinks. The values are "", "file" and "symlink" respectively.
+//
+// Directories use the empty string (in fact they don't need the option at
+// all) as this was the default and is retained for backwards compatibility.
+func (e *MountEntry) XSnapdKind() string {
+	val, _ := e.OptStr("x-snapd.kind")
+	return val
+}
+
+// XSnapdDetach returns true if a mount entry should be detached rather than unmounted.
+//
+// Whenever we create a recursive bind mount we don't want to just unmount it
+// as it may have replicated additional mount entries. For simplicity and
+// race-free behavior we just detach such mount entries and let the kernel do
+// the rest.
+func (e *MountEntry) XSnapdDetach() bool {
+	return e.OptBool("x-snapd.detach")
+}
+
+// XSnapdNeededBy returns the string "x-snapd.needed-by=..." with the given path appended.
+func XSnapdNeededBy(path string) string {
+	return fmt.Sprintf("x-snapd.needed-by=%s", path)
+}
+
+// XSnapdSynthetic returns the string "x-snapd.synthetic".
+func XSnapdSynthetic() string {
+	return "x-snapd.synthetic"
+}
+
+// XSnapdDetach returns the string "x-snapd.detach".
+func XSnapdDetach() string {
+	return "x-snapd.detach"
+}
+
 // XSnapdKindSymlink returns the string "x-snapd.kind=symlink".
 func XSnapdKindSymlink() string {
	return "x-snapd.kind=symlink"
diff --git a/osutil/mountentry_test.go b/osutil/mountentry_test.go
index 66e8fdff08..de6027d638 100644
--- a/osutil/mountentry_test.go
+++ b/osutil/mountentry_test.go
@@ -233,8 +233,6 @@ func (s *entrySuite) TestOptBool(c *C) {
 }
 
 func (s *entrySuite) TestOptionHelpers(c *C) {
-	c.Assert(osutil.XSnapdKindSymlink(), Equals, "x-snapd.kind=symlink")
-	c.Assert(osutil.XSnapdKindFile(), Equals, "x-snapd.kind=file")
	c.Assert(osutil.XSnapdUser(1000), Equals, "x-snapd.user=1000")
	c.Assert(osutil.XSnapdGroup(1000), Equals, "x-snapd.group=1000")
	c.Assert(osutil.XSnapdMode(0755), Equals, "x-snapd.mode=0755")
@@ -340,6 +338,9 @@ func (s *entrySuite) TestXSnapdNeededBy(c *C) {
	// The needed-by attribute parsed from the x-snapd.needed-by= option.
	e = &osutil.MountEntry{Options: []string{"x-snap.id=foo", "x-snapd.needed-by=bar"}}
	c.Assert(e.XSnapdNeededBy(), Equals, "bar")
+
+	// There's a helper function that returns this option string.
+	c.Assert(osutil.XSnapdNeededBy("foo"), Equals, "x-snapd.needed-by=foo")
 }
 
 func (s *entrySuite) TestXSnapdSynthetic(c *C) {
@@ -350,10 +351,9 @@ func (s *entrySuite) TestXSnapdSynthetic(c *C) {
	// Tagging is done with x-snapd.synthetic option.
	e = &osutil.MountEntry{Options: []string{"x-snapd.synthetic"}}
	c.Assert(e.XSnapdSynthetic(), Equals, true)
-}
 
-func (s *entrySuite) TextXSnapdOriginLayout(c *C) {
-	c.Assert(osutil.XSnapdOriginLayout(), Equals, "x-snapd.origin=layout")
+	// There's a helper function that returns this option string.
+	c.Assert(osutil.XSnapdSynthetic(), Equals, "x-snapd.synthetic")
 }
 
 func (s *entrySuite) TestXSnapdOrigin(c *C) {
@@ -361,11 +361,44 @@ func (s *entrySuite) TestXSnapdOrigin(c *C) {
	e := &osutil.MountEntry{}
	c.Assert(e.XSnapdOrigin(), Equals, "")
 
-	// Origin can be indicated with the x-snapd.origin= option
-	e = &osutil.MountEntry{Options: []string{"x-snapd.origin=layout"}}
-	c.Assert(e.XSnapdOrigin(), Equals, "layout")
-
-	// The helpful helper for this constant actually works.
+	// Origin can be indicated with the x-snapd.origin= option.
	e = &osutil.MountEntry{Options: []string{osutil.XSnapdOriginLayout()}}
	c.Assert(e.XSnapdOrigin(), Equals, "layout")
+
+	// There's a helper function that returns this option string.
+	c.Assert(osutil.XSnapdOriginLayout(), Equals, "x-snapd.origin=layout")
+}
+
+func (s *entrySuite) TestXSnapdDetach(c *C) {
+	// Entries are not detached by default.
+	e := &osutil.MountEntry{}
+	c.Assert(e.XSnapdDetach(), Equals, false)
+
+	// Detach can be requested with the x-snapd.detach option.
+	e = &osutil.MountEntry{Options: []string{osutil.XSnapdDetach()}}
+	c.Assert(e.XSnapdDetach(), Equals, true)
+
+	// There's a helper function that returns this option string.
+	c.Assert(osutil.XSnapdDetach(), Equals, "x-snapd.detach")
+}
+
+func (s *entrySuite) TestXSnapdKind(c *C) {
+	// Entries have a kind (directory, file or symlink). Directory is spelled
+	// as an empty string though, for backwards compatibility.
+	e := &osutil.MountEntry{}
+	c.Assert(e.XSnapdKind(), Equals, "")
+
+	// A bind mount entry can refer to a file using the x-snapd.kind=file option string.
+	e = &osutil.MountEntry{Options: []string{osutil.XSnapdKindFile()}}
+	c.Assert(e.XSnapdKind(), Equals, "file")
+
+	// There's a helper function that returns this option string.
+	c.Assert(osutil.XSnapdKindFile(), Equals, "x-snapd.kind=file")
+
+	// A mount entry can create a symlink by using the x-snapd.kind=symlink option string.
+	e = &osutil.MountEntry{Options: []string{osutil.XSnapdKindSymlink()}}
+	c.Assert(e.XSnapdKind(), Equals, "symlink")
+
+	// There's a helper function that returns this option string.
+	c.Assert(osutil.XSnapdKindSymlink(), Equals, "x-snapd.kind=symlink")
 }
diff --git a/testutil/exec_test.go b/testutil/exec_test.go
index 369723900a..4605764927 100644
--- a/testutil/exec_test.go
+++ b/testutil/exec_test.go
@@ -29,6 +29,10 @@ type mockCommandSuite struct{}
 
 var _ = Suite(&mockCommandSuite{})
 
+const (
+	UmountNoFollow = umountNoFollow
+)
+
 func (s *mockCommandSuite) TestMockCommand(c *C) {
	mock := MockCommand(c, "cmd", "true")
	defer mock.Restore()
diff --git a/testutil/lowlevel.go b/testutil/lowlevel.go
index 195541a96f..9cf8329b21 100644
--- a/testutil/lowlevel.go
+++ b/testutil/lowlevel.go
@@ -26,12 +26,12 @@ import (
	"syscall"
	"time"
 
-	. "gopkg.in/check.v1"
+	"gopkg.in/check.v1"
 
	"github.com/snapcore/snapd/osutil/sys"
 )
 
-const UMOUNT_NOFOLLOW = 8
+const umountNoFollow = 8
 
 // fakeFileInfo implements os.FileInfo for testing.
 //
@@ -49,6 +49,7 @@ func (*fakeFileInfo) ModTime() time.Time { panic("unexpected call") }
 func (fi *fakeFileInfo) IsDir() bool { return fi.Mode().IsDir() }
 func (*fakeFileInfo) Sys() interface{} { panic("unexpected call") }
 
+// FakeFileInfo returns a fake object implementing os.FileInfo
 func FakeFileInfo(name string, mode os.FileMode) os.FileInfo {
	return &fakeFileInfo{name: name, mode: mode}
 }
@@ -140,8 +141,8 @@ func formatMountFlags(flags int) string {
 // Please expand the set of recognized flags as tests require.
 func formatUnmountFlags(flags int) string {
	var fl []string
-	if flags&UMOUNT_NOFOLLOW == UMOUNT_NOFOLLOW {
-	flags ^= UMOUNT_NOFOLLOW
+	if flags&umountNoFollow == umountNoFollow {
+	flags ^= umountNoFollow
	fl = append(fl, "UMOUNT_NOFOLLOW")
	}
	if flags&syscall.MNT_DETACH == syscall.MNT_DETACH {
@@ -203,6 +204,10 @@ func (sys *SyscallRecorder) InsertFault(call string, errors ...error) {
	}
 }
 
+// InsertFaultFunc arranges given function to be called whenever given call is made.
+//
+// The main purpose is to allow to vary the behavior of a given system call over time.
+// The provided function can return an error or nil to indicate success.
 func (sys *SyscallRecorder) InsertFaultFunc(call string, fn func() error) {
	if sys.errors == nil {
	sys.errors = make(map[string]func() error)
@@ -257,10 +262,12 @@ func (sys *SyscallRecorder) StrayDescriptorsError() error {
	return nil
 }
 
-func (sys *SyscallRecorder) CheckForStrayDescriptors(c *C) {
-	c.Assert(sys.StrayDescriptorsError(), IsNil)
+// CheckForStrayDescriptors ensures that all fake file descriptors are closed.
+func (sys *SyscallRecorder) CheckForStrayDescriptors(c *check.C) {
+	c.Assert(sys.StrayDescriptorsError(), check.IsNil)
 }
 
+// Open is a fake implementation of syscall.Open
 func (sys *SyscallRecorder) Open(path string, flags int, mode uint32) (int, error) {
	call := fmt.Sprintf("open %q %s %#o", path, formatOpenFlags(flags), mode)
	if err := sys.call(call); err != nil {
@@ -269,6 +276,7 @@ func (sys *SyscallRecorder) Open(path string, flags int, mode uint32) (int, erro
	return sys.allocFd(call), nil
 }
 
+// Openat is a fake implementation of syscall.Openat
 func (sys *SyscallRecorder) Openat(dirfd int, path string, flags int, mode uint32) (int, error) {
	call := fmt.Sprintf("openat %d %q %s %#o", dirfd, path, formatOpenFlags(flags), mode)
	if _, ok := sys.fds[dirfd]; !ok {
@@ -281,6 +289,7 @@ func (sys *SyscallRecorder) Openat(dirfd int, path string, flags int, mode uint3
	return sys.allocFd(call), nil
 }
 
+// Close is a fake implementation of syscall.Close
 func (sys *SyscallRecorder) Close(fd int) error {
	if err := sys.call(fmt.Sprintf("close %d", fd)); err != nil {
	return err
@@ -288,6 +297,7 @@ func (sys *SyscallRecorder) Close(fd int) error {
	return sys.freeFd(fd)
 }
 
+// Fchown is a fake implementation of syscall.Fchown
 func (sys *SyscallRecorder) Fchown(fd int, uid sys.UserID, gid sys.GroupID) error {
	call := fmt.Sprintf("fchown %d %d %d", fd, uid, gid)
	if _, ok := sys.fds[fd]; !ok {
@@ -297,6 +307,7 @@ func (sys *SyscallRecorder) Fchown(fd int, uid sys.UserID, gid sys.GroupID) erro
	return sys.call(call)
 }
 
+// Mkdirat is a fake implementation of syscall.Mkdirat
 func (sys *SyscallRecorder) Mkdirat(dirfd int, path string, mode uint32) error {
	call := fmt.Sprintf("mkdirat %d %q %#o", dirfd, path, mode)
	if _, ok := sys.fds[dirfd]; !ok {
@@ -306,10 +317,12 @@ func (sys *SyscallRecorder) Mkdirat(dirfd int, path string, mode uint32) error {
	return sys.call(call)
 }
 
+// Mount is a fake implementation of syscall.Mount
 func (sys *SyscallRecorder) Mount(source string, target string, fstype string, flags uintptr, data string) (err error) {
	return sys.call(fmt.Sprintf("mount %q %q %q %s %q", source, target, fstype, formatMountFlags(int(flags)), data))
 }
 
+// Unmount is a fake implementation of syscall.Unmount
 func (sys *SyscallRecorder) Unmount(target string, flags int) (err error) {
	return sys.call(fmt.Sprintf("unmount %q %s", target, formatUnmountFlags(flags)))
 }
@@ -322,6 +335,7 @@ func (sys *SyscallRecorder) InsertLstatResult(call string, fi os.FileInfo) {
	sys.lstats[call] = fi
 }
 
+// Lstat is a fake implementation of os.Lstat
 func (sys *SyscallRecorder) Lstat(name string) (os.FileInfo, error) {
	call := fmt.Sprintf("lstat %q", name)
	if err := sys.call(call); err != nil {
@@ -341,6 +355,7 @@ func (sys *SyscallRecorder) InsertFstatResult(call string, buf syscall.Stat_t) {
	sys.fstats[call] = buf
 }
 
+// Fstat is a fake implementation of syscall.Fstat
 func (sys *SyscallRecorder) Fstat(fd int, buf *syscall.Stat_t) error {
	call := fmt.Sprintf("fstat %d <ptr>", fd)
	if _, ok := sys.fds[fd]; !ok {
@@ -365,6 +380,7 @@ func (sys *SyscallRecorder) InsertReadDirResult(call string, infos []os.FileInfo
	sys.readdirs[call] = infos
 }
 
+// ReadDir is a fake implementation of os.ReadDir
 func (sys *SyscallRecorder) ReadDir(dirname string) ([]os.FileInfo, error) {
	call := fmt.Sprintf("readdir %q", dirname)
	if err := sys.call(call); err != nil {
@@ -376,11 +392,13 @@ func (sys *SyscallRecorder) ReadDir(dirname string) ([]os.FileInfo, error) {
	panic(fmt.Sprintf("one of InsertReadDirResult() or InsertFault() for %s must be used", call))
 }
 
+// Symlink is a fake implementation of syscall.Symlink
 func (sys *SyscallRecorder) Symlink(oldname, newname string) error {
	call := fmt.Sprintf("symlink %q -> %q", newname, oldname)
	return sys.call(call)
 }
 
+// Symlinkat is a fake implementation of osutil.Symlinkat (syscall.Symlinkat is not exposed)
 func (sys *SyscallRecorder) Symlinkat(oldname string, dirfd int, newname string) error {
	call := fmt.Sprintf("symlinkat %q %d %q", oldname, dirfd, newname)
	if _, ok := sys.fds[dirfd]; !ok {
@@ -398,6 +416,7 @@ func (sys *SyscallRecorder) InsertReadlinkatResult(call, oldname string) {
	sys.readlinkats[call] = oldname
 }
 
+// Readlinkat is a fake implementation of osutil.Readlinkat (syscall.Readlinkat is not exposed)
 func (sys *SyscallRecorder) Readlinkat(dirfd int, path string, buf []byte) (int, error) {
	call := fmt.Sprintf("readlinkat %d %q <ptr>", dirfd, path)
	if _, ok := sys.fds[dirfd]; !ok {
@@ -414,11 +433,13 @@ func (sys *SyscallRecorder) Readlinkat(dirfd int, path string, buf []byte) (int,
	panic(fmt.Sprintf("one of InsertReadlinkatResult() or InsertFault() for %s must be used", call))
 }
 
+// Remove is a fake implementation of os.Remove
 func (sys *SyscallRecorder) Remove(name string) error {
	call := fmt.Sprintf("remove %q", name)
	return sys.call(call)
 }
 
+// Fchdir is a fake implementation of syscall.Fchdir
 func (sys *SyscallRecorder) Fchdir(fd int) error {
	call := fmt.Sprintf("fchdir %d", fd)
	if _, ok := sys.fds[fd]; !ok {
diff --git a/testutil/lowlevel_test.go b/testutil/lowlevel_test.go
index 9649abec35..30672347d4 100644
--- a/testutil/lowlevel_test.go
+++ b/testutil/lowlevel_test.go
@@ -282,7 +282,7 @@ func (s *lowLevelSuite) TestMountFailure(c *C) {
 }
 
 func (s *lowLevelSuite) TestUnmountSuccess(c *C) {
-	err := s.sys.Unmount("target", testutil.UMOUNT_NOFOLLOW|syscall.MNT_DETACH)
+	err := s.sys.Unmount("target", testutil.UmountNoFollow|syscall.MNT_DETACH)
	c.Assert(err, IsNil)
	c.Assert(s.sys.Calls(), DeepEquals, []string{`unmount "target" UMOUNT_NOFOLLOW|MNT_DETACH`})
 }