osutil: call sync after cp if requested. overlord/snapstate/backend: …

…switch to use osutil instead of another buggy call to cp

osutil/cp.go

@@ -20,6 +20,7 @@
 package osutil
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"os/exec"
@@ -63,7 +64,14 @@ func CopyFile(src, dst string, flags CopyFlag) (err error) {
 		// Our native copy code does not preserve all attributes
 		// (yet). If the user needs this functionatlity we just
 		// fallback to use the system's "cp" binary to do the copy.
-		return runCpPreserveAll(src, dst)
+		err := runCpPreserveAll(src, dst, "copy all")
+		if err != nil {
+			return err
+		}
+		if flags&CopyFlagSync != 0 {
+			return runSync()
+		}
+		return nil
 	}
 
 	fin, err := openfile(src, os.O_RDONLY, 0)
@@ -109,16 +117,18 @@ func CopyFile(src, dst string, flags CopyFlag) (err error) {
 	return nil
 }
 
-func runCpPreserveAll(path, dest string) error {
-	cmd := exec.Command("cp", "-av", path, dest)
+func runCmd(cmd *exec.Cmd, errdesc string) error {
 	if output, err := cmd.CombinedOutput(); err != nil {
+		output = bytes.TrimSpace(output)
 		if exitCode, err := ExitCode(err); err == nil {
 			return &ErrCopySpecialFile{
+				desc:     errdesc,
 				exitCode: exitCode,
 				output:   output,
 			}
 		}
 		return &ErrCopySpecialFile{
+			desc:   errdesc,
 			err:    err,
 			output: output,
 		}
@@ -127,23 +137,32 @@ func runCpPreserveAll(path, dest string) error {
 	return nil
 }
 
+func runSync() error {
+	return runCmd(exec.Command("sync"), "sync")
+}
+
+func runCpPreserveAll(path, dest, errdesc string) error {
+	return runCmd(exec.Command("cp", "-av", path, dest), errdesc)
+}
+
 // CopySpecialFile is used to copy all the things that are not files
 // (like device nodes, named pipes etc)
 func CopySpecialFile(path, dest string) error {
-	return runCpPreserveAll(path, dest)
+	return runCpPreserveAll(path, dest, "copy device node")
 }
 
 // ErrCopySpecialFile is returned if a special file copy fails
 type ErrCopySpecialFile struct {
+	desc     string
 	exitCode int
 	output   []byte
 	err      error
 }
 
 func (e ErrCopySpecialFile) Error() string {
 	if e.err == nil {
-		return fmt.Sprintf("failed to copy device node: %q (%v)", e.output, e.exitCode)
+		return fmt.Sprintf("failed to %s: %q (%v)", e.desc, e.output, e.exitCode)
 	}
 
-	return fmt.Sprintf("failed to copy device node: %q (%v)", e.output, e.err)
+	return fmt.Sprintf("failed to %s: %q (%v)", e.desc, e.output, e.err)
 }

osutil/cp_test.go

@@ -21,6 +21,7 @@ package osutil
 
 import (
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -241,3 +242,89 @@ func (s *cpSuite) TestCopyPreserveAll(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(st1.ModTime(), Equals, st2.ModTime())
 }
+
+func (s *cpSuite) TestCopyPreserveAllSync(c *C) {
+	dir := c.MkDir()
+	cp := filepath.Join(dir, "cp")
+	sync := filepath.Join(dir, "sync")
+	log := filepath.Join(dir, "log")
+
+	sh := fmt.Sprintf(`#!/bin/sh
+echo "$0:$@" >> %s
+`, log)
+
+	c.Assert(ioutil.WriteFile(cp, []byte(sh), 0755), IsNil)
+	c.Assert(os.Symlink(cp, sync), IsNil)
+
+	oldPATH := os.Getenv("PATH")
+	defer os.Setenv("PATH", oldPATH)
+	os.Setenv("PATH", dir+":"+oldPATH)
+
+	src := filepath.Join(dir, "meep")
+	dst := filepath.Join(dir, "copied-meep")
+
+	err := ioutil.WriteFile(src, []byte(nil), 0644)
+	c.Assert(err, IsNil)
+
+	err = CopyFile(src, dst, CopyFlagPreserveAll|CopyFlagSync)
+	c.Assert(err, IsNil)
+
+	content, err := ioutil.ReadFile(log)
+	c.Assert(err, IsNil)
+	c.Check(string(content), Equals, fmt.Sprintf("%[1]s/cp:-av %[1]s/meep %[1]s/copied-meep\n%[1]s/sync:\n", dir))
+}
+
+func (s *cpSuite) TestCopyPreserveAllSyncCpFailure(c *C) {
+	dir := c.MkDir()
+	cp := filepath.Join(dir, "cp")
+	sync := filepath.Join(dir, "sync")
+
+	sh := `#!/bin/sh
+echo "OUCH: $(basename $0) failed." >&2
+exit 42
+`
+
+	c.Assert(ioutil.WriteFile(cp, []byte(sh), 0755), IsNil)
+	c.Assert(os.Symlink(cp, sync), IsNil)
+
+	oldPATH := os.Getenv("PATH")
+	defer os.Setenv("PATH", oldPATH)
+	os.Setenv("PATH", dir+":"+oldPATH)
+
+	src := filepath.Join(dir, "meep")
+	dst := filepath.Join(dir, "copied-meep")
+
+	err := ioutil.WriteFile(src, []byte(nil), 0644)
+	c.Assert(err, IsNil)
+
+	err = CopyFile(src, dst, CopyFlagPreserveAll|CopyFlagSync)
+	c.Assert(err, ErrorMatches, `failed to copy all: "OUCH: cp failed." \(42\)`)
+}
+
+func (s *cpSuite) TestCopyPreserveAllSyncSyncFailure(c *C) {
+	dir := c.MkDir()
+	cp := filepath.Join(dir, "cp")
+	sync := filepath.Join(dir, "sync")
+
+	sh := `#!/bin/sh
+echo
+echo "OUCH: $(basename $0) failed." >&2
+exit 42
+`
+
+	c.Assert(ioutil.WriteFile(cp, []byte("#!/bin/sh\n"), 0755), IsNil)
+	c.Assert(ioutil.WriteFile(sync, []byte(sh), 0755), IsNil)
+
+	oldPATH := os.Getenv("PATH")
+	defer os.Setenv("PATH", oldPATH)
+	os.Setenv("PATH", dir+":"+oldPATH)
+
+	src := filepath.Join(dir, "meep")
+	dst := filepath.Join(dir, "copied-meep")
+
+	err := ioutil.WriteFile(src, []byte(nil), 0644)
+	c.Assert(err, IsNil)
+
+	err = CopyFile(src, dst, CopyFlagPreserveAll|CopyFlagSync)
+	c.Assert(err, ErrorMatches, `failed to sync: "OUCH: sync failed." \(42\)`)
+}

overlord/snapstate/backend/copydata_test.go

@@ -25,6 +25,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"strconv"
 
 	. "gopkg.in/check.v1"
 
@@ -370,7 +371,11 @@ exit 3
 	defer os.Setenv("PATH", oldPATH)
 	os.Setenv("PATH", fakeBinDir+":"+oldPATH)
 
+	q := func(s string) string {
+		return regexp.QuoteMeta(strconv.Quote(s))
+	}
+
 	// copy data will fail
 	err = s.be.CopySnapData(v2, v1, &s.nullProgress)
-	c.Assert(err, ErrorMatches, regexp.QuoteMeta(fmt.Sprintf("cannot copy %s to %s: cp: boom", v1.DataDir(), v2.DataDir())))
+	c.Assert(err, ErrorMatches, fmt.Sprintf(`cannot copy %s to %s: .*: "cp: boom" \(3\)`, q(v1.DataDir()), q(v2.DataDir())))
 }

overlord/snapstate/backend/snapdata.go

@@ -20,12 +20,11 @@
 package backend
 
 import (
-	"bytes"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/snap"
 )
 
@@ -112,14 +111,9 @@ func copySnapData(oldSnap, newSnap *snap.Info) (err error) {
 func copySnapDataDirectory(oldPath, newPath string) (err error) {
 	if _, err := os.Stat(oldPath); err == nil {
 		if _, err := os.Stat(newPath); err != nil {
-			// there is no golang "CopyFile"
-			cmd := exec.Command("cp", "-a", oldPath, newPath)
-			if output, err := cmd.CombinedOutput(); err != nil {
-				output = bytes.TrimSpace(output)
-				if len(output) > 0 {
-					err = fmt.Errorf("%s", output)
-				}
-				return fmt.Errorf("cannot copy %s to %s: %v", oldPath, newPath, err)
+			err := osutil.CopyFile(oldPath, newPath, osutil.CopyFlagPreserveAll|osutil.CopyFlagSync)
+			if err != nil {
+				return fmt.Errorf("cannot copy %q to %q: %v", oldPath, newPath, err)
 			}
 		}
 	}