Merge lp:~chipaca/snappy/copyfile into lp:~snappy-dev/snappy/snappy-moved-to-github

[John Lenton]

Moved two implementations of Copy into helpers with the linux implementation being based on sendfile.

Moved two implementations of Copy into helpers.

Because I was having fun, also implemented copy using sendfile when in linux.

Using sendfile avoids at least two copies over read/write; more if copying multiple chunks of data (currently golang's io does chunks of 32k).

On the bbb, copying a 10M file using io.Copy takes .79s/.03s/.26s (real/user/sys); doing it using sendfile uses .64s/.01s/.13s (averaged over 10 runs).

This requires a kernel newer than 2.6.33, which should not be a problem.

[John Lenton (chipaca)]

Moving to WIP, because I kinda forgot all about permissions.

[John Lenton (chipaca)]

All set now :)

[Sergio Schvezov (sergiusens)]

Just really quickly going to say THANK YOU.

The split of OS dependent implementations is also welcoming.

I'll do a proper review later today during layovers

[Michael Vogt (mvo)]

This looks very nice, thanks for doing this!

META: It seems this is a good time to branch trunk into trunk and 15.04. This branch should go to trunk and I think we want to do fixes for 15.04 as well (but this branch looks more like trunk/devel material to me).

Some comments/questions inline. I really like that this branch removes duplicated code and moves in into a single well tested (and speedy!) place.

[John Lenton (chipaca)]

And neither of you spotted that I was not returning the error from copyfile. heh.

review: Needs Fixing

[lp:~chipaca/snappy/copyfile]

439. By John Lenton on 2015-05-02

mock all the things. Then, test all the things.

[John Lenton (chipaca)]

Fixed :)

review: Abstain

[John Lenton (chipaca)]

Michael, I think this pretty much covers the test you wanted ;)

[Sergio Schvezov (sergiusens)]

one comment, looks good otherwise

review: Needs Fixing

[lp:~chipaca/snappy/copyfile]

440. By John Lenton on 2015-05-05

durr

[Sergio Schvezov (sergiusens)]

review: Approve

=== added file 'helpers/cp.go'
--- helpers/cp.go	1970-01-01 00:00:00 +0000
+++ helpers/cp.go	2015-05-05 22:17:11 +0000
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2014-2015 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package helpers
+
+import (
+	"fmt"
+	"os"
+)
+
+// CopyFlag is used to tweak the behaviour of CopyFile
+type CopyFlag uint8
+
+const (
+	// CopyFlagDefault is the default behaviour
+	CopyFlagDefault CopyFlag = 0
+	// CopyFlagSync does a sync after copying the files
+	CopyFlagSync CopyFlag = 1 << iota
+)
+
+var (
+	openfile = doOpenFile
+	copyfile = doCopyFile
+)
+
+type fileish interface {
+	Close() error
+	Sync() error
+	Fd() uintptr
+	Stat() (os.FileInfo, error)
+	Read([]byte) (int, error)
+	Write([]byte) (int, error)
+}
+
+func doOpenFile(name string, flag int, perm os.FileMode) (fileish, error) {
+	return os.OpenFile(name, flag, perm)
+}
+
+// CopyFile copies src to dst
+func CopyFile(src, dst string, flags CopyFlag) (err error) {
+	fin, err := openfile(src, os.O_RDONLY, 0)
+	if err != nil {
+		return fmt.Errorf("unable to open %s: %v", src, err)
+	}
+	defer func() {
+		if cerr := fin.Close(); cerr != nil && err == nil {
+			err = fmt.Errorf("when closing %s: %v", src, cerr)
+		}
+	}()
+
+	fi, err := fin.Stat()
+	if err != nil {
+		return fmt.Errorf("unable to stat %s: %v", src, err)
+	}
+
+	fout, err := openfile(dst, os.O_WRONLY|os.O_CREATE|os.O_EXCL, fi.Mode()) //
+	if err != nil {
+		return fmt.Errorf("unable to create %s: %v", dst, err)
+	}
+	defer func() {
+		if cerr := fout.Close(); cerr != nil && err == nil {
+			err = fmt.Errorf("when closing %s: %v", dst, cerr)
+		}
+	}()
+
+	if err := copyfile(fin, fout, fi); err != nil {
+		return fmt.Errorf("unable to copy %s to %s: %v", src, dst, err)
+	}
+
+	if flags&CopyFlagSync != 0 {
+		if err = fout.Sync(); err != nil {
+			return fmt.Errorf("unable to sync %s: %v", dst, err)
+		}
+	}
+
+	return nil
+}

=== added file 'helpers/cp_linux.go'
--- helpers/cp_linux.go	1970-01-01 00:00:00 +0000
+++ helpers/cp_linux.go	2015-05-05 22:17:11 +0000
@@ -0,0 +1,46 @@
+/*
+ * Copyright (C) 2014-2015 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package helpers
+
+import (
+	"os"
+	"syscall"
+)
+
+const maxint = int64(^uint(0) >> 1)
+
+var maxcp = maxint // overridden in testing
+
+func doCopyFile(fin, fout fileish, fi os.FileInfo) error {
+	size := fi.Size()
+	var offset int64
+	for offset < size {
+		// sendfile is funny; it only copies up to maxint
+		// bytes at a time, but takes an int64 offset.
+		count := size - offset
+		if count > maxcp {
+			count = maxcp
+		}
+
+		if _, err := syscall.Sendfile(int(fout.Fd()), int(fin.Fd()), &offset, int(count)); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

=== added file 'helpers/cp_linux_test.go'
--- helpers/cp_linux_test.go	1970-01-01 00:00:00 +0000
+++ helpers/cp_linux_test.go	2015-05-05 22:17:11 +0000
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2014-2015 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package helpers
+
+import (
+	"io/ioutil"
+	"os"
+
+	. "launchpad.net/gocheck"
+)
+
+func (s *cpSuite) TestCpMulti(c *C) {
+	maxcp = 2
+	defer func() { maxcp = maxint }()
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagDefault), IsNil)
+	bs, err := ioutil.ReadFile(s.f2)
+	c.Check(err, IsNil)
+	c.Check(bs, DeepEquals, s.data)
+}
+
+func (s *cpSuite) TestDoCpErr(c *C) {
+	f1, err := os.Open(s.f1)
+	c.Assert(err, IsNil)
+	st, err := f1.Stat()
+	c.Assert(err, IsNil)
+	// force an error by asking it to write to a readonly stream
+	c.Check(doCopyFile(f1, os.Stdin, st), NotNil)
+}

=== added file 'helpers/cp_other.go'
--- helpers/cp_other.go	1970-01-01 00:00:00 +0000
+++ helpers/cp_other.go	2015-05-05 22:17:11 +0000
@@ -0,0 +1,30 @@
+// +build !linux
+
+/*
+ * Copyright (C) 2014-2015 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package helpers
+
+import (
+	"io"
+	"os"
+)
+
+func doCopyFile(fin, fout fileish, fi os.FileInfo) error {
+	_, err := io.Copy(fout, fin)
+	return err
+}

=== added file 'helpers/cp_test.go'
--- helpers/cp_test.go	1970-01-01 00:00:00 +0000
+++ helpers/cp_test.go	2015-05-05 22:17:11 +0000
@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) 2014-2015 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package helpers
+
+import (
+	"errors"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+	"time"
+
+	. "launchpad.net/gocheck"
+)
+
+type cpSuite struct {
+	dir  string
+	f1   string
+	f2   string
+	data []byte
+	log  []string
+	errs []error
+	idx  int
+}
+
+var _ = Suite(&cpSuite{})
+
+func (s *cpSuite) mockCopyFile(fin, fout fileish, fi os.FileInfo) error {
+	return s.µ("copyfile")
+}
+
+func (s *cpSuite) mockOpenFile(name string, flag int, perm os.FileMode) (fileish, error) {
+	return &mockfile{s}, s.µ("open")
+}
+
+func (s *cpSuite) µ(msg string) (err error) {
+	s.log = append(s.log, msg)
+	if len(s.errs) > 0 {
+		err = s.errs[0]
+		if len(s.errs) > 1 {
+			s.errs = s.errs[1:]
+		}
+	}
+
+	return
+}
+
+func (s *cpSuite) SetUpTest(c *C) {
+	s.errs = nil
+	s.log = nil
+	s.dir = c.MkDir()
+	s.f1 = filepath.Join(s.dir, "f1")
+	s.f2 = filepath.Join(s.dir, "f2")
+	s.data = []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}
+	c.Assert(ioutil.WriteFile(s.f1, s.data, 0644), IsNil)
+}
+
+func (s *cpSuite) mock() {
+	copyfile = s.mockCopyFile
+	openfile = s.mockOpenFile
+}
+
+func (s *cpSuite) TearDownTest(c *C) {
+	copyfile = doCopyFile
+	openfile = doOpenFile
+}
+
+func (s *cpSuite) TestCp(c *C) {
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagDefault), IsNil)
+	bs, err := ioutil.ReadFile(s.f2)
+	c.Check(err, IsNil)
+	c.Check(bs, DeepEquals, s.data)
+}
+
+func (s *cpSuite) TestCpSync(c *C) {
+	s.mock()
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagDefault), IsNil)
+	c.Check(strings.Join(s.log, ":"), Not(Matches), `.*:sync(:.*)?`)
+
+	s.log = nil
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), IsNil)
+	c.Check(strings.Join(s.log, ":"), Matches, `(.*:)?sync(:.*)?`)
+}
+
+func (s *cpSuite) TestCpCantOpen(c *C) {
+	s.mock()
+	s.errs = []error{errors.New("xyzzy"), nil}
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), ErrorMatches, `unable to open \S+/f1: xyzzy`)
+}
+
+func (s *cpSuite) TestCpCantStat(c *C) {
+	s.mock()
+	s.errs = []error{nil, errors.New("xyzzy"), nil}
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), ErrorMatches, `unable to stat \S+/f1: xyzzy`)
+}
+
+func (s *cpSuite) TestCpCantCreate(c *C) {
+	s.mock()
+	s.errs = []error{nil, nil, errors.New("xyzzy"), nil}
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), ErrorMatches, `unable to create \S+/f2: xyzzy`)
+}
+
+func (s *cpSuite) TestCpCantCopy(c *C) {
+	s.mock()
+	s.errs = []error{nil, nil, nil, errors.New("xyzzy"), nil}
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), ErrorMatches, `unable to copy \S+/f1 to \S+/f2: xyzzy`)
+}
+
+func (s *cpSuite) TestCpCantSync(c *C) {
+	s.mock()
+	s.errs = []error{nil, nil, nil, nil, errors.New("xyzzy"), nil}
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), ErrorMatches, `unable to sync \S+/f2: xyzzy`)
+}
+
+func (s *cpSuite) TestCpCantStop2(c *C) {
+	s.mock()
+	s.errs = []error{nil, nil, nil, nil, nil, errors.New("xyzzy"), nil}
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), ErrorMatches, `when closing \S+/f2: xyzzy`)
+}
+
+func (s *cpSuite) TestCpCantStop1(c *C) {
+	s.mock()
+	s.errs = []error{nil, nil, nil, nil, nil, nil, errors.New("xyzzy"), nil}
+
+	c.Check(CopyFile(s.f1, s.f2, CopyFlagSync), ErrorMatches, `when closing \S+/f1: xyzzy`)
+}
+
+type mockfile struct {
+	s *cpSuite
+}
+
+var mockst = mockstat{}
+
+func (f *mockfile) Close() error               { return f.s.µ("close") }
+func (f *mockfile) Sync() error                { return f.s.µ("sync") }
+func (f *mockfile) Fd() uintptr                { f.s.µ("fd"); return 42 }
+func (f *mockfile) Read([]byte) (int, error)   { return 0, f.s.µ("read") }
+func (f *mockfile) Write([]byte) (int, error)  { return 0, f.s.µ("write") }
+func (f *mockfile) Stat() (os.FileInfo, error) { return mockst, f.s.µ("stat") }
+
+type mockstat struct{}
+
+func (mockstat) Name() string       { return "mockstat" }
+func (mockstat) Size() int64        { return 42 }
+func (mockstat) Mode() os.FileMode  { return 0644 }
+func (mockstat) ModTime() time.Time { return time.Now() }
+func (mockstat) IsDir() bool        { return false }
+func (mockstat) Sys() interface{}   { return nil }

=== modified file 'policy/policy.go'
--- policy/policy.go	2015-04-19 14:02:33 +0000
+++ policy/policy.go	2015-05-05 22:17:11 +0000
@@ -21,7 +21,6 @@
 
 import (
 	"fmt"
-	"io"
 	"os"
 	"path/filepath"
 
@@ -90,33 +89,9 @@
 			}
 		case install:
 			// do the copy
-			fin, err := os.Open(file)
-			if err != nil {
-				return fmt.Errorf("unable to read %v: %v", file, err)
-			}
-			defer func() {
-				if cerr := fin.Close(); cerr != nil && err == nil {
-					err = fmt.Errorf("when closing %v: %v", file, cerr)
-				}
-			}()
-
-			fout, err := os.Create(targetFile)
-			if err != nil {
-				return fmt.Errorf("unable to create %v: %v", targetFile, err)
-			}
-			defer func() {
-				if cerr := fout.Close(); cerr != nil && err == nil {
-					err = fmt.Errorf("when closing %v: %v", targetFile, cerr)
-				}
-			}()
-
-			if _, err = io.Copy(fout, fin); err != nil {
-				return fmt.Errorf("unable to copy %v to %v: %v", file, targetFile, err)
-			}
-			if err = fout.Sync(); err != nil {
-				return fmt.Errorf("when syncing %v: %v", targetFile, err)
-			}
-
+			if err := helpers.CopyFile(file, targetFile, helpers.CopyFlagSync); err != nil {
+				return err
+			}
 		default:
 			return fmt.Errorf("unknown operation %s", op)
 		}

=== modified file 'policy/policy_test.go'
--- policy/policy_test.go	2015-04-17 06:42:44 +0000
+++ policy/policy_test.go	2015-05-05 22:17:11 +0000
@@ -118,7 +118,7 @@
 	fn := filepath.Join(s.appg, "policygroups0")
 	c.Assert(os.Chmod(fn, 0), IsNil)
 	err := iterOp(install, filepath.Join(s.appg, "*"), s.dest, "foo_")
-	c.Check(err, ErrorMatches, ".*unable to read.*")
+	c.Check(err, ErrorMatches, ".*unable to open.*")
 }
 
 func (s *policySuite) TestIterOpInstallBadTarget(c *C) {

=== modified file 'snappy/build.go'
--- snappy/build.go	2015-04-16 05:03:51 +0000
+++ snappy/build.go	2015-05-05 22:17:11 +0000
@@ -21,7 +21,6 @@
 	"bufio"
 	"encoding/json"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -394,26 +393,7 @@
 			return nil
 		}
 		// sigh. ok, copy it is.
-		in, err := os.Open(path)
-		if err != nil {
-			return err
-		}
-		defer in.Close()
-
-		out, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, info.Mode())
-		if err != nil {
-			return err
-		}
-		defer func() {
-			// XXX: write a test for this. I dare you. I double-dare you.
-			xerr := out.Close()
-			if err == nil {
-				err = xerr
-			}
-		}()
-		_, err = io.Copy(out, in)
-		// no need to sync, as it's a tempdir
-		return err
+		return helpers.CopyFile(path, dest, helpers.CopyFlagDefault)
 	})
 }