many: fix review comments from PR canonical#3177 (canonical#3244)

Author: morphis

Diff for: cmd/snap/cmd_run.go

@@ -272,7 +272,7 @@ func migrateXauthority(info *snap.Info) (string, error) {
 	// it point to exactly the file we just opened (no symlinks,
 	// no funny "../.." etc)
 	if fin.Name() != xauthPathCan {
-		logger.Noticef("WARNING: %s != %s\n", fin.Name(), xauthPathCan)
+		logger.Noticef("WARNING: XAUTHORITY environment value is not a clean path: %q", xauthPathCan)
 		return "", nil
 	}
 
@@ -297,7 +297,7 @@ func migrateXauthority(info *snap.Info) (string, error) {
 	}
 	sys := fi.Sys()
 	if sys == nil {
-		return "", fmt.Errorf(i18n.G("Can't validate file owner"))
+		return "", fmt.Errorf(i18n.G("cannot validate owner of file %s"), fin.Name())
 	}
 	// cheap comparison as the current uid is only available as a string
 	// but it is better to convert the uid from the stat result to a
@@ -318,14 +318,13 @@ func migrateXauthority(info *snap.Info) (string, error) {
 		if fout, err = os.Open(targetPath); err != nil {
 			return "", err
 		}
-		if osutil.FileStreamsEqual(fin, fout) {
+		if osutil.StreamsEqual(fin, fout) {
 			fout.Close()
 			return targetPath, nil
 		}
 
 		fout.Close()
 		if err := os.Remove(targetPath); err != nil {
-			logger.Noticef("WARNING: failed to remove existing file %s", targetPath)
 			return "", err
 		}
 
@@ -338,9 +337,8 @@ func migrateXauthority(info *snap.Info) (string, error) {
 	// To guard against setting XAUTHORITY to non-xauth files, check
 	// that we have a valid Xauthority. Specifically, the file must be
 	// parseable as an Xauthority file and not be empty.
-	if err := x11.ValidateXauthorityFromFile(fin); err != nil {
-		logger.Noticef("WARNING: invalid Xauthority file: %s", err)
-		return "", nil
+	if err := x11.ValidateXauthority(fin); err != nil {
+		return "", err
 	}
 
 	// Read data from the beginning of the file
@@ -357,9 +355,9 @@ func migrateXauthority(info *snap.Info) (string, error) {
 	// Read and write validated Xauthority file to its right location
 	if _, err = io.Copy(fout, fin); err != nil {
 		if err := os.Remove(targetPath); err != nil {
-			logger.Noticef("WARNING: failed to remove file at %s", targetPath)
+			logger.Noticef("WARNING: cannot remove file at %s: %s", targetPath, err)
 		}
-		return "", fmt.Errorf(i18n.G("Failed to write new Xauthority file at %s"), targetPath)
+		return "", fmt.Errorf(i18n.G("cannot write new Xauthority file at %s: %s"), targetPath, err)
 	}
 
 	return targetPath, nil

Diff for: cmd/snap/cmd_run_test.go

@@ -563,6 +563,6 @@ func (s *SnapSuite) TestSnapRunXauthorityMigration(c *check.C) {
 	c.Assert(err, check.IsNil)
 	c.Assert(info.Mode().Perm(), check.Equals, os.FileMode(0600))
 
-	err = x11.ValidateXauthority(expectedXauthPath)
+	err = x11.ValidateXauthorityFile(expectedXauthPath)
 	c.Assert(err, check.IsNil)
 }

Diff for: osutil/cmp.go

@@ -58,17 +58,17 @@ func FilesAreEqual(a, b string) bool {
 		return false
 	}
 
-	return FileStreamsEqual(fa, fb)
+	return StreamsEqual(fa, fb)
 }
 
-// FileStreamsEqual compares two file streams and returns true if both
+// StreamsEqual compares two streams and returns true if both
 // have the same content.
-func FileStreamsEqual(fa, fb io.Reader) bool {
+func StreamsEqual(a, b io.Reader) bool {
 	bufa := make([]byte, bufsz)
 	bufb := make([]byte, bufsz)
 	for {
-		ra, erra := io.ReadAtLeast(fa, bufa, bufsz)
-		rb, errb := io.ReadAtLeast(fb, bufb, bufsz)
+		ra, erra := io.ReadAtLeast(a, bufa, bufsz)
+		rb, errb := io.ReadAtLeast(b, bufb, bufsz)
 		if erra == io.EOF && errb == io.EOF {
 			return true
 		}

Diff for: osutil/cmp_test.go

@@ -96,6 +96,6 @@ func (ts *CmpTestSuite) TestCmpStreams(c *C) {
 		{"hello", "world", false},
 		{"hello", "hell", false},
 	} {
-		c.Assert(FileStreamsEqual(strings.NewReader(x.a), strings.NewReader(x.b)), Equals, x.r)
+		c.Assert(StreamsEqual(strings.NewReader(x.a), strings.NewReader(x.b)), Equals, x.r)
 	}
 }

Diff for: snap/snapenv/snapenv.go

@@ -34,6 +34,9 @@ import (
 //
 // It merges it with the existing os.Environ() and ensures the SNAP_*
 // overrides the any pre-existing environment variables.
+//
+// With the extra parameter additional environment variables can be
+// supplied which will be set in the execution environment.
 func ExecEnv(info *snap.Info, extra map[string]string) []string {
 	// merge environment and the snap environment, note that the
 	// snap environment overrides pre-existing env entries

Diff for: x11/xauth.go

@@ -38,61 +38,48 @@ type xauth struct {
 	Data    []byte
 }
 
-func readBytes(f *os.File, data []byte) error {
-	n, err := f.Read(data)
-	if err != nil {
-		return err
-	}
-
-	if n != len(data) {
-		return fmt.Errorf("Could not read enough bytes")
-	}
-
-	return nil
-}
-
-func readChunk(f *os.File) ([]byte, error) {
-	// A chunk consists of a length encoded by two bytes and
-	// additional data which is the real value of the item
-	// we reading here from the file.
+func readChunk(r io.Reader) ([]byte, error) {
+	// A chunk consists of a length encoded by two bytes (so max 64K)
+	// and additional data which is the real value of the item we're
+	// reading here from the file.
 
 	b := [2]byte{}
-	if err := readBytes(f, b[:]); err != nil {
+	if _, err := io.ReadFull(r, b[:]); err != nil {
 		return nil, err
 	}
 
 	size := int(binary.BigEndian.Uint16(b[:]))
 	chunk := make([]byte, size)
-	if err := readBytes(f, chunk); err != nil {
+	if _, err := io.ReadFull(r, chunk); err != nil {
 		return nil, err
 	}
 
 	return chunk, nil
 }
 
-func (xa *xauth) readFromFile(f *os.File) error {
+func (xa *xauth) readFromFile(r io.Reader) error {
 	b := [2]byte{}
-	if err := readBytes(f, b[:]); err != nil {
+	if _, err := io.ReadFull(r, b[:]); err != nil {
 		return err
 	}
 	// The family field consists of two bytes
 	xa.Family = binary.BigEndian.Uint16(b[:])
 
 	var err error
 
-	if xa.Address, err = readChunk(f); err != nil {
+	if xa.Address, err = readChunk(r); err != nil {
 		return err
 	}
 
-	if xa.Number, err = readChunk(f); err != nil {
+	if xa.Number, err = readChunk(r); err != nil {
 		return err
 	}
 
-	if xa.Name, err = readChunk(f); err != nil {
+	if xa.Name, err = readChunk(r); err != nil {
 		return err
 	}
 
-	if xa.Data, err = readChunk(f); err != nil {
+	if xa.Data, err = readChunk(r); err != nil {
 		return err
 	}
 
@@ -101,22 +88,22 @@ func (xa *xauth) readFromFile(f *os.File) error {
 
 // ValidateXauthority validates a given Xauthority file. The file is valid
 // if it can be parsed and contains at least one cookie.
-func ValidateXauthority(path string) error {
+func ValidateXauthorityFile(path string) error {
 	f, err := os.Open(path)
 	if err != nil {
 		return err
 	}
 	defer f.Close()
-	return ValidateXauthorityFromFile(f)
+	return ValidateXauthority(f)
 }
 
 // ValidateXauthority validates a given Xauthority file. The file is valid
 // if it can be parsed and contains at least one cookie.
-func ValidateXauthorityFromFile(f *os.File) error {
+func ValidateXauthority(r io.Reader) error {
 	cookies := 0
 	for {
 		xa := &xauth{}
-		err := xa.readFromFile(f)
+		err := xa.readFromFile(r)
 		if err == io.EOF {
 			break
 		} else if err != nil {

Diff for: x11/xauth_test.go

@@ -37,7 +37,7 @@ type xauthTestSuite struct{}
 var _ = Suite(&xauthTestSuite{})
 
 func (s *xauthTestSuite) TestXauthFileNotAvailable(c *C) {
-	err := x11.ValidateXauthority("/does/not/exist")
+	err := x11.ValidateXauthorityFile("/does/not/exist")
 	c.Assert(err, NotNil)
 }
 
@@ -46,7 +46,7 @@ func (s *xauthTestSuite) TestXauthFileExistsButIsEmpty(c *C) {
 	c.Assert(err, IsNil)
 	defer os.Remove(xauthPath)
 
-	err = x11.ValidateXauthority(xauthPath)
+	err = x11.ValidateXauthorityFile(xauthPath)
 	c.Assert(err, ErrorMatches, "Xauthority file is invalid")
 }
 
@@ -60,15 +60,15 @@ func (s *xauthTestSuite) TestXauthFileExistsButHasInvalidContent(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(n, Equals, len(data))
 
-	err = x11.ValidateXauthority(f.Name())
-	c.Assert(err, ErrorMatches, "Could not read enough bytes")
+	err = x11.ValidateXauthorityFile(f.Name())
+	c.Assert(err, ErrorMatches, "unexpected EOF")
 }
 
 func (s *xauthTestSuite) TestValidXauthFile(c *C) {
 	for _, n := range []int{1, 2, 4} {
 		path, err := x11.MockXauthority(n)
 		c.Assert(err, IsNil)
-		err = x11.ValidateXauthority(path)
+		err = x11.ValidateXauthorityFile(path)
 		c.Assert(err, IsNil)
 	}
 }