overlord/snapshotstate/backend: address review feedback (thanks bbooz…

…zoo)
canonical · Apr 19, 2018 · d400538 · d400538
1 parent 81ae315
commit d400538
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 17 deletions.
diff --git a/dirs/dirs.go b/dirs/dirs.go
@@ -103,7 +103,7 @@ var (
 	SystemFontconfigCacheDir string
 
 	FreezerCgroupDir string
-	SnapshotDir      string
+	SnapshotsDir     string
 )
 
 const (
@@ -260,5 +260,5 @@ func SetRootDir(rootdir string) {
 	SystemFontconfigCacheDir = filepath.Join(rootdir, "/var/cache/fontconfig")
 
 	FreezerCgroupDir = filepath.Join(rootdir, "/sys/fs/cgroup/freezer/")
-	SnapshotDir = filepath.Join(rootdir, "/var/spool/snapd/snapshots")
+	SnapshotsDir = filepath.Join(rootdir, "/var/spool/snapd/snapshots")
 }
diff --git a/overlord/snapshotstate/backend/backend.go b/overlord/snapshotstate/backend/backend.go
@@ -28,6 +28,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"time"
 
@@ -39,7 +40,6 @@ import (
 	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/strutil"
-	"regexp"
 )
 
 const (
@@ -66,7 +66,7 @@ func Iter(ctx context.Context, f func(*Reader) error) error {
 		return err
 	}
 
-	dir, err := dirOpen(dirs.SnapshotDir)
+	dir, err := dirOpen(dirs.SnapshotsDir)
 	if err != nil {
 		if osutil.IsDirNotExist(err) {
 			// no dir -> no snapshots
@@ -84,7 +84,7 @@ func Iter(ctx context.Context, f func(*Reader) error) error {
 				break
 			}
 
-			filename := filepath.Join(dirs.SnapshotDir, name)
+			filename := filepath.Join(dirs.SnapshotsDir, name)
 			rsh, openError := open(filename)
 			// rsh can be non-nil even when openError is not nil (in
 			// which case rsh.Broken will have a reason). f can
@@ -141,12 +141,12 @@ func List(ctx context.Context, setID uint64, snapNames []string) ([]client.Snaps
 // Filename of the given client.Snapshot in this backend.
 func Filename(sh *client.Snapshot) string {
 	// this _needs_ the snap name and version to be valid
-	return filepath.Join(dirs.SnapshotDir, fmt.Sprintf("%d_%s_%s.zip", sh.SetID, sh.Snap, sh.Version))
+	return filepath.Join(dirs.SnapshotsDir, fmt.Sprintf("%d_%s_%s.zip", sh.SetID, sh.Snap, sh.Version))
 }
 
 // Save a snapshot
 func Save(ctx context.Context, id uint64, si *snap.Info, cfg map[string]interface{}, usernames []string) (sh *client.Snapshot, e error) {
-	if err := os.MkdirAll(dirs.SnapshotDir, 0700); err != nil {
+	if err := os.MkdirAll(dirs.SnapshotsDir, 0700); err != nil {
 		return nil, err
 	}
 

diff --git a/overlord/snapshotstate/backend/backend_test.go b/overlord/snapshotstate/backend/backend_test.go
@@ -163,7 +163,7 @@ func (s *snapshotSuite) TestIterBailsIfContextDoneMidway(c *check.C) {
 	c.Check(triedToOpenSnapshot, check.Equals, false)
 }
 
-func (s *snapshotSuite) TestIterReturnsOkIfSnapshotDirNonexistent(c *check.C) {
+func (s *snapshotSuite) TestIterReturnsOkIfSnapshotsDirNonexistent(c *check.C) {
 	triedToOpenDir := false
 	defer backend.MockDirOpen(func(string) (*os.File, error) {
 		triedToOpenDir = true
@@ -175,7 +175,7 @@ func (s *snapshotSuite) TestIterReturnsOkIfSnapshotDirNonexistent(c *check.C) {
 	c.Check(triedToOpenDir, check.Equals, true)
 }
 
-func (s *snapshotSuite) TestIterBailsIfSnapshotDirFails(c *check.C) {
+func (s *snapshotSuite) TestIterBailsIfSnapshotsDirFails(c *check.C) {
 	triedToOpenDir := false
 	defer backend.MockDirOpen(func(string) (*os.File, error) {
 		triedToOpenDir = true
@@ -370,7 +370,7 @@ func (s *snapshotSuite) TestList(c *check.C) {
 		c.Check(logbuf.String(), check.Equals, "", comm)
 		c.Check(sets, check.HasLen, t.numSets, comm)
 		nShots := 0
-		fnTpl := filepath.Join(dirs.SnapshotDir, "%d_%s_%s.zip")
+		fnTpl := filepath.Join(dirs.SnapshotsDir, "%d_%s_%s.zip")
 		for j, ss := range sets {
 			for k, sh := range ss.Snapshots {
 				comm := check.Commentf("%d: %d/%v #%d/%d", i, t.setID, t.snapnames, j, k)
@@ -442,7 +442,7 @@ func (s *snapshotSuite) TestHappyRoundtrip(c *check.C) {
 	c.Check(shw.Version, check.Equals, info.Version)
 	c.Check(shw.Revision, check.Equals, info.Revision)
 	c.Check(shw.Conf, check.DeepEquals, cfg)
-	c.Check(backend.Filename(shw), check.Equals, filepath.Join(dirs.SnapshotDir, "12_hello-snap_v1.33.zip"))
+	c.Check(backend.Filename(shw), check.Equals, filepath.Join(dirs.SnapshotsDir, "12_hello-snap_v1.33.zip"))
 	c.Check(hashkeys(shw), check.DeepEquals, []string{"archive.tgz", "user/snapuser.tgz"})
 
 	shs, err := backend.List(context.TODO(), 0, nil)
@@ -458,7 +458,7 @@ func (s *snapshotSuite) TestHappyRoundtrip(c *check.C) {
 	c.Check(shr.Version, check.Equals, info.Version)
 	c.Check(shr.Revision, check.Equals, info.Revision)
 	c.Check(shr.Conf, check.DeepEquals, cfg)
-	c.Check(shr.Name(), check.Equals, filepath.Join(dirs.SnapshotDir, "12_hello-snap_v1.33.zip"))
+	c.Check(shr.Name(), check.Equals, filepath.Join(dirs.SnapshotsDir, "12_hello-snap_v1.33.zip"))
 	c.Check(shr.SHA3_384, check.DeepEquals, shw.SHA3_384)
 
 	c.Check(shr.Check(context.TODO(), nil), check.IsNil)

diff --git a/overlord/snapshotstate/backend/helpers.go b/overlord/snapshotstate/backend/helpers.go
@@ -68,16 +68,19 @@ func member(f *os.File, member string) (r io.ReadCloser, sz int64, err error) {
 	}
 
 	fi, err := f.Stat()
+	if err != nil {
+		return nil, -1, err
+	}
 
 	arch, err := zip.NewReader(f, fi.Size())
 	if err != nil {
 		return nil, -1, err
 	}
 
-	for _, f := range arch.File {
-		if f.Name == member {
-			rc, err := f.Open()
-			return rc, int64(f.UncompressedSize64), err
+	for _, fh := range arch.File {
+		if fh.Name == member {
+			r, err = fh.Open()
+			return r, int64(fh.UncompressedSize64), err
 		}
 	}
 

diff --git a/overlord/snapshotstate/backend/reader.go b/overlord/snapshotstate/backend/reader.go
@@ -64,7 +64,7 @@ func Open(fn string) (rsh *Reader, e error) {
 		return nil, err
 	}
 	defer func() {
-		if e != nil {
+		if e != nil && f != nil {
 			f.Close()
 		}
 	}()