store: address comments

Address review comments

Signed-off-by: Wei Zhang <zhangwei555@huawei.com>

virtcontainers/container.go

@@ -376,8 +376,7 @@ func (c *Container) setStateFstype(fstype string) error {
 
 	if !c.sandbox.supportNewStore() {
 		// experimental runtime use "persist.json" which doesn't need "state.json" anymore
-		err := c.storeState()
+		if err := c.storeState(); err != nil {
-		if err != nil {
 			return err
 		}
 	}
@@ -454,8 +453,7 @@ func (c *Container) setContainerState(state types.StateString) error {
 	} else {
 		// experimental runtime use "persist.json" which doesn't need "state.json" anymore
 		// update on-disk state
-		err := c.store.Store(store.State, c.state)
+		if err := c.store.Store(store.State, c.state); err != nil {
-		if err != nil {
 			return err
 		}
 	}
@@ -619,9 +617,9 @@ func (c *Container) unmountHostMounts() error {
 	return nil
 }
 
-func filterDevices(sandbox *Sandbox, c *Container, devices []ContainerDevice) (ret []ContainerDevice) {
+func filterDevices(c *Container, devices []ContainerDevice) (ret []ContainerDevice) {
 	for _, dev := range devices {
-		major, _ := sandbox.devManager.GetDeviceByID(dev.ID).GetMajorMinor()
+		major, _ := c.sandbox.devManager.GetDeviceByID(dev.ID).GetMajorMinor()
 		if _, ok := cdromMajors[major]; ok {
 			c.Logger().WithFields(logrus.Fields{
 				"device": dev.ContainerPath,
@@ -773,7 +771,7 @@ func (c *Container) createDevices(contConfig ContainerConfig) error {
 				GID:           info.GID,
 			})
 		}
-		c.devices = filterDevices(c.sandbox, c, storedDevices)
+		c.devices = filterDevices(c, storedDevices)
 	}
 	return nil
 }

virtcontainers/persist.go

@@ -94,7 +94,7 @@ func (s *Sandbox) dumpDevices(ss *persistapi.SandboxState, cs map[string]persist
 	return nil
 }
 
-// versionCallback set persist data version to current version in runtime
+// verSaveCallback set persist data version to current version in runtime
 func (s *Sandbox) verSaveCallback() {
 	s.newStore.AddSaveCallback("version", func(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error {
 		ss.PersistVersion = persistapi.CurPersistVersion
@@ -102,12 +102,12 @@ func (s *Sandbox) verSaveCallback() {
 	})
 }
 
-// PersistState register hook to set sandbox and container state to persist
+// stateSaveCallback register hook to set sandbox and container state to persist
 func (s *Sandbox) stateSaveCallback() {
 	s.newStore.AddSaveCallback("state", s.dumpState)
 }
 
-// PersistHvState register hook to save hypervisor state to persist data
+// hvStateSaveCallback register hook to save hypervisor state to persist data
 func (s *Sandbox) hvStateSaveCallback() {
 	s.newStore.AddSaveCallback("hypervisor", s.dumpHypervisor)
 }
@@ -118,7 +118,7 @@ func (s *Sandbox) devicesSaveCallback() {
 }
 
 func (s *Sandbox) getSbxAndCntStates() (*persistapi.SandboxState, map[string]persistapi.ContainerState, error) {
-	if err := s.newStore.Restore(s.id); err != nil {
+	if err := s.newStore.FromDisk(s.id); err != nil {
 		return nil, nil, err
 	}
 

virtcontainers/persist/api/interface.go

@@ -9,13 +9,13 @@ package persistapi
 type PersistDriver interface {
 	// ToDisk flushes data to disk(or other storage media such as a remote db)
 	ToDisk() error
+	// FromDisk will restore all data for sandbox with `sid` from storage.
+	// We only support get data for one whole sandbox
+	FromDisk(sid string) error
 	// AddSaveCallback addes callback function named `name` to driver storage list
 	// The callback functions will be invoked when calling `ToDisk()`, notice that
 	// callback functions are not order guaranteed,
 	AddSaveCallback(name string, f SetFunc)
-	// Restore will restore all data for sandbox with `sid` from storage.
-	// We only support get data for one whole sandbox
-	Restore(sid string) error
 	// Destroy will remove everything from storage
 	Destroy() error
 	// GetStates will return SandboxState and ContainerState(s) directly

virtcontainers/persist/api/sandbox.go

@@ -43,7 +43,6 @@ type HypervisorState struct {
 	HotpluggedMemory     int
 	UUID                 string
 	HotplugVFIOOnRootBus bool
-	// TODO: should this be map[index]bool to indicate available block id??
 	BlockIndex           int
 }
 

virtcontainers/persist/fs/fs.go

@@ -15,13 +15,14 @@ import (
 	"syscall"
 
 	persistapi "github.com/kata-containers/runtime/virtcontainers/persist/api"
+	"github.com/sirupsen/logrus"
 )
 
 // persistFile is the file name for JSON sandbox/container configuration
 const persistFile = "persist.json"
 
 // dirMode is the permission bits used for creating a directory
-const dirMode = os.FileMode(0750)
+const dirMode = os.FileMode(0700)
 
 // fileMode is the permission bits used for creating a file
 const fileMode = os.FileMode(0640)
@@ -48,6 +49,15 @@ type FS struct {
 	lockFile *os.File
 }
 
+var fsLog = logrus.WithField("source", "virtcontainers/persist/fs")
+
+// Logger returns a logrus logger appropriate for logging Store messages
+func (fs *FS) Logger() *logrus.Entry {
+	return fsLog.WithFields(logrus.Fields{
+		"subsystem": "persist",
+	})
+}
+
 // Name returns driver name
 func Name() string {
 	return "fs"
@@ -63,11 +73,12 @@ func Init() (persistapi.PersistDriver, error) {
 }
 
 func (fs *FS) sandboxDir() (string, error) {
-	if fs.sandboxState.SandboxContainer == "" {
+	id := fs.sandboxState.SandboxContainer
+	if id == "" {
 		return "", fmt.Errorf("sandbox container id required")
 	}
 
-	return filepath.Join(runStoragePath, fs.sandboxState.SandboxContainer), nil
+	return filepath.Join(runStoragePath, id), nil
 }
 
 // ToDisk sandboxState and containerState to disk
@@ -77,14 +88,6 @@ func (fs *FS) ToDisk() (retErr error) {
 		fun(fs.sandboxState, fs.containerState)
 	}
 
-	// if error happened, destroy all dirs
-	defer func() {
-		if retErr != nil {
-			// TODO: log error
-			fs.Destroy()
-		}
-	}()
-
 	sandboxDir, err := fs.sandboxDir()
 	if err != nil {
 		return err
@@ -95,13 +98,25 @@ func (fs *FS) ToDisk() (retErr error) {
 	}
 
 	if err := fs.lock(); err != nil {
+		if err1 := fs.Destroy(); err1 != nil {
+			fs.Logger().WithError(err1).Errorf("failed to destroy dirs")
+		}
 		return err
 	}
 	defer fs.unlock()
 
+	// if error happened, destroy all dirs
+	defer func() {
+		if retErr != nil {
+			if err := fs.Destroy(); err != nil {
+				fs.Logger().WithError(err).Errorf("failed to destroy dirs")
+			}
+		}
+	}()
+
 	// persist sandbox configuration data
 	sandboxFile := filepath.Join(sandboxDir, persistFile)
-	f, err := os.OpenFile(sandboxFile, os.O_RDWR|os.O_CREATE|os.O_TRUNC, fileMode)
+	f, err := os.OpenFile(sandboxFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileMode)
 	if err != nil {
 		return err
 	}
@@ -133,8 +148,8 @@ func (fs *FS) ToDisk() (retErr error) {
 	return nil
 }
 
-// Restore state for sandbox with name sid
+// FromDisk restores state for sandbox with name sid
-func (fs *FS) Restore(sid string) error {
+func (fs *FS) FromDisk(sid string) error {
 	if sid == "" {
 		return fmt.Errorf("restore requires sandbox id")
 	}

virtcontainers/persist/fs/fs_test.go

@@ -71,10 +71,10 @@ func TestFsDriver(t *testing.T) {
 	})
 
 	// try non-existent dir
-	assert.NotNil(t, fs.Restore("test-fs"))
+	assert.NotNil(t, fs.FromDisk("test-fs"))
 
 	// since we didn't call ToDisk, Callbacks are not invoked, and state is still empty in disk file
-	assert.Nil(t, fs.Restore(id))
+	assert.Nil(t, fs.FromDisk(id))
 	ss, cs, err := fs.GetStates()
 	assert.Nil(t, err)
 	assert.NotNil(t, ss)
@@ -85,7 +85,7 @@ func TestFsDriver(t *testing.T) {
 
 	// flush all to disk
 	assert.Nil(t, fs.ToDisk())
-	assert.Nil(t, fs.Restore(id))
+	assert.Nil(t, fs.FromDisk(id))
 	ss, cs, err = fs.GetStates()
 	assert.Nil(t, err)
 	assert.NotNil(t, ss)