From a3638072985a0cb71ff561ad5e5bbc2454f81c1f Mon Sep 17 00:00:00 2001
From: DCCooper <1866858@gmail.com>
Date: Wed, 8 Dec 2021 12:51:20 +0800
Subject: [PATCH 2/2] isula-build: fix problems found by code review

Signed-off-by: DCCooper <1866858@gmail.com>
---
 daemon/load.go | 67 ++++++++++++++++++++++++-------------------------
 daemon/save.go | 79 ++++++++++++++++++++--------------------------------------
 image/image.go |  3 ++-
 util/cipher.go |  8 +++++-
 4 files changed, 69 insertions(+), 88 deletions(-)

diff --git a/daemon/load.go b/daemon/load.go
index 378325c..894159b 100644
--- a/daemon/load.go
+++ b/daemon/load.go
@@ -69,9 +69,9 @@ type separatorLoad struct {
 }
 
 type loadOptions struct {
+	logEntry *logrus.Entry
 	path     string
 	format   string
-	logEntry *logrus.Entry
 	sep      separatorLoad
 }
 
@@ -355,7 +355,7 @@ func (s *separatorLoad) getTarballInfo() error {
 		return errors.Wrap(err, "join manifest file path failed")
 	}
 
-	var t = make(map[string]tarballInfo)
+	var t = make(map[string]tarballInfo, 1)
 	if err = util.LoadJSONFile(manifest, &t); err != nil {
 		return errors.Wrap(err, "load manifest file failed")
 	}
@@ -370,7 +370,7 @@ func (s *separatorLoad) getTarballInfo() error {
 }
 
 func (s *separatorLoad) constructTarballInfo() (err error) {
-	s.log.Infof("construct image tarball info for %s", s.appName)
+	s.log.Infof("Construct image tarball info for %s", s.appName)
 	// fill up path for separator
 	// this case should not happened since client side already check this flag
 	if len(s.appName) == 0 {
@@ -408,26 +408,25 @@ func (s *separatorLoad) tarballCheckSum() error {
 		return nil
 	}
 
-	// app image tarball can not be empty
-	if len(s.appPath) == 0 {
-		return errors.New("app image tarball path can not be empty")
-	}
-	if err := util.CheckSum(s.appPath, s.info.AppHash); err != nil {
-		return errors.Wrapf(err, "check sum for file %q failed", s.appPath)
-	}
-
-	// base image tarball can not be empty
-	if len(s.basePath) == 0 {
-		return errors.New("base image tarball path can not be empty")
-	}
-	if err := util.CheckSum(s.basePath, s.info.BaseHash); err != nil {
-		return errors.Wrapf(err, "check sum for file %q failed", s.basePath)
-	}
-
-	// lib image may be empty image
-	if len(s.libPath) != 0 {
-		if err := util.CheckSum(s.libPath, s.info.LibHash); err != nil {
-			return errors.Wrapf(err, "check sum for file %q failed", s.libPath)
+	type checkInfo struct {
+		path       string
+		hash       string
+		str        string
+		canBeEmpty bool
+	}
+	checkLen := 3
+	var checkList = make([]checkInfo, 0, checkLen)
+	checkList = append(checkList, checkInfo{path: s.basePath, hash: s.info.BaseHash, canBeEmpty: false, str: "base image"})
+	checkList = append(checkList, checkInfo{path: s.libPath, hash: s.info.LibHash, canBeEmpty: true, str: "lib image"})
+	checkList = append(checkList, checkInfo{path: s.appPath, hash: s.info.AppHash, canBeEmpty: false, str: "app image"})
+	for _, p := range checkList {
+		if len(p.path) == 0 && !p.canBeEmpty {
+			return errors.Errorf("%s tarball path can not be empty", p.str)
+		}
+		if len(p.path) != 0 {
+			if err := util.CheckSum(p.path, p.hash); err != nil {
+				return errors.Wrapf(err, "check sum for file %q failed", p.path)
+			}
 		}
 	}
 
@@ -457,18 +456,18 @@ func (s *separatorLoad) unpackTarballs() error {
 		return errors.Wrap(err, "failed to make temporary directories")
 	}
 
-	// unpack base first and the later images will be moved here
-	if err := util.UnpackFile(s.basePath, s.tmpDir.base, archive.Gzip, false); err != nil {
-		return errors.Wrapf(err, "unpack base tarball %q failed", s.basePath)
-	}
-
-	if err := util.UnpackFile(s.appPath, s.tmpDir.app, archive.Gzip, false); err != nil {
-		return errors.Wrapf(err, "unpack app tarball %q failed", s.appPath)
-	}
+	type unpackInfo struct{ path, dir, str string }
+	unpackLen := 3
+	var unpackList = make([]unpackInfo, 0, unpackLen)
+	unpackList = append(unpackList, unpackInfo{path: s.basePath, dir: s.tmpDir.base, str: "base image"})
+	unpackList = append(unpackList, unpackInfo{path: s.appPath, dir: s.tmpDir.app, str: "app image"})
+	unpackList = append(unpackList, unpackInfo{path: s.libPath, dir: s.tmpDir.lib, str: "lib image"})
 
-	if len(s.libPath) != 0 {
-		if err := util.UnpackFile(s.libPath, s.tmpDir.lib, archive.Gzip, false); err != nil {
-			return errors.Wrapf(err, "unpack lib tarball %q failed", s.libPath)
+	for _, p := range unpackList {
+		if len(p.path) != 0 {
+			if err := util.UnpackFile(p.path, p.dir, archive.Gzip, false); err != nil {
+				return errors.Wrapf(err, "unpack %s tarball %q failed", p.str, p.path)
+			}
 		}
 	}
 
diff --git a/daemon/save.go b/daemon/save.go
index f14a485..7a110bd 100644
--- a/daemon/save.go
+++ b/daemon/save.go
@@ -77,9 +77,9 @@ type saveOptions struct {
 }
 
 type separatorSave struct {
+	log        *logrus.Entry
 	renameData []renames
 	tmpDir     imageTmpDir
-	log        *logrus.Entry
 	base       string
 	lib        string
 	dest       string
@@ -190,7 +190,7 @@ func (b *Backend) Save(req *pb.SaveRequest, stream pb.Control_SaveServer) (err e
 	}).Info("SaveRequest received")
 
 	opts := b.getSaveOptions(req)
-	if err = opts.check(); err != nil {
+	if err = opts.manage(); err != nil {
 		return errors.Wrap(err, "check save options failed")
 	}
 
@@ -278,17 +278,17 @@ func messageHandler(stream pb.Control_SaveServer, cliLogger *logger.Logger) func
 	}
 }
 
-func (opts *saveOptions) check() error {
+func (opts *saveOptions) manage() error {
 	if err := opts.checkImageNameIsID(); err != nil {
 		return err
 	}
-	if err := opts.checkFormat(); err != nil {
+	if err := opts.setFormat(); err != nil {
 		return err
 	}
 	if err := opts.filterImageName(); err != nil {
 		return err
 	}
-	if err := opts.checkRenameFile(); err != nil {
+	if err := opts.loadRenameFile(); err != nil {
 		return err
 	}
 
@@ -318,7 +318,7 @@ func (opts *saveOptions) checkImageNameIsID() error {
 	return nil
 }
 
-func (opts *saveOptions) checkFormat() error {
+func (opts *saveOptions) setFormat() error {
 	switch opts.format {
 	case constant.DockerTransport:
 		opts.format = constant.DockerArchiveTransport
@@ -337,7 +337,7 @@ func (opts *saveOptions) filterImageName() error {
 		return nil
 	}
 
-	visitedImage := make(map[string]bool)
+	visitedImage := make(map[string]bool, 1)
 	for _, imageName := range opts.oriImgList {
 		if _, exists := visitedImage[imageName]; exists {
 			continue
@@ -351,8 +351,7 @@ func (opts *saveOptions) filterImageName() error {
 
 		finalImage, ok := opts.finalImageSet[img.ID]
 		if !ok {
-			finalImage = &savedImage{exist: true}
-			finalImage.tags = []reference.NamedTagged{}
+			finalImage = &savedImage{exist: true, tags: []reference.NamedTagged{}}
 			opts.finalImageOrdered = append(opts.finalImageOrdered, img.ID)
 		}
 
@@ -369,7 +368,7 @@ func (opts *saveOptions) filterImageName() error {
 	return nil
 }
 
-func (opts *saveOptions) checkRenameFile() error {
+func (opts *saveOptions) loadRenameFile() error {
 	if len(opts.sep.renameFile) != 0 {
 		var reName []renames
 		if err := util.LoadJSONFile(opts.sep.renameFile, &reName); err != nil {
@@ -494,12 +493,11 @@ func (s *separatorSave) adjustLayers() ([]imageManifest, error) {
 	return man, nil
 }
 
-func separateImage(opt saveOptions) error {
+func separateImage(opt saveOptions) (err error) {
 	s := &opt.sep
 	s.log.Infof("Start saving separated images %v", opt.oriImgList)
-	var errList []error
 
-	if err := os.MkdirAll(s.dest, constant.DefaultRootDirMode); err != nil {
+	if err = os.MkdirAll(s.dest, constant.DefaultRootDirMode); err != nil {
 		return err
 	}
 
@@ -507,30 +505,26 @@ func separateImage(opt saveOptions) error {
 		if tErr := os.RemoveAll(s.tmpDir.root); tErr != nil && !os.IsNotExist(tErr) {
 			s.log.Warnf("Removing save tmp directory %q failed: %v", s.tmpDir.root, tErr)
 		}
-		if len(errList) != 0 {
+		if err != nil {
 			if rErr := os.RemoveAll(s.dest); rErr != nil && !os.IsNotExist(rErr) {
 				s.log.Warnf("Removing save dest directory %q failed: %v", s.dest, rErr)
 			}
 		}
 	}()
-	if err := util.UnpackFile(opt.outputPath, s.tmpDir.untar, archive.Gzip, true); err != nil {
-		errList = append(errList, err)
+	if err = util.UnpackFile(opt.outputPath, s.tmpDir.untar, archive.Gzip, true); err != nil {
 		return errors.Wrapf(err, "unpack %q failed", opt.outputPath)
 	}
-	manifest, err := s.adjustLayers()
-	if err != nil {
-		errList = append(errList, err)
-		return errors.Wrap(err, "adjust layers failed")
+	manifest, aErr := s.adjustLayers()
+	if aErr != nil {
+		return errors.Wrap(aErr, "adjust layers failed")
 	}
 
-	imgInfos, err := s.constructImageInfos(manifest, opt.localStore)
-	if err != nil {
-		errList = append(errList, err)
-		return errors.Wrap(err, "process image infos failed")
+	imgInfos, cErr := s.constructImageInfos(manifest, opt.localStore)
+	if cErr != nil {
+		return errors.Wrap(cErr, "process image infos failed")
 	}
 
-	if err := s.processImageLayers(imgInfos); err != nil {
-		errList = append(errList, err)
+	if err = s.processImageLayers(imgInfos); err != nil {
 		return err
 	}
 
@@ -552,7 +546,7 @@ func (s *separatorSave) processImageLayers(imgInfos map[string]imageInfo) error
 	sort.Strings(sortedKey)
 	for _, k := range sortedKey {
 		info := imgInfos[k]
-		if err := s.clearDirs(true); err != nil {
+		if err := s.clearTempDirs(); err != nil {
 			return errors.Wrap(err, "clear tmp dirs failed")
 		}
 		var t tarballInfo
@@ -584,32 +578,13 @@ func (s *separatorSave) processImageLayers(imgInfos map[string]imageInfo) error
 	return nil
 }
 
-func (s *separatorSave) clearDirs(reCreate bool) error {
-	tmpDir := s.tmpDir
-	dirs := []string{tmpDir.base, tmpDir.app, tmpDir.lib}
-	var mkTmpDirs = func(dirs []string) error {
-		for _, dir := range dirs {
-			if err := os.MkdirAll(dir, constant.DefaultRootDirMode); err != nil {
-				return err
-			}
-		}
-		return nil
-	}
-
-	var rmTmpDirs = func(dirs []string) error {
-		for _, dir := range dirs {
-			if err := os.RemoveAll(dir); err != nil {
-				return err
-			}
+func (s *separatorSave) clearTempDirs() error {
+	dirs := []string{s.tmpDir.base, s.tmpDir.app, s.tmpDir.lib}
+	for _, dir := range dirs {
+		if err := os.RemoveAll(dir); err != nil {
+			return err
 		}
-		return nil
-	}
-
-	if err := rmTmpDirs(dirs); err != nil {
-		return err
-	}
-	if reCreate {
-		if err := mkTmpDirs(dirs); err != nil {
+		if err := os.MkdirAll(dir, constant.DefaultRootDirMode); err != nil {
 			return err
 		}
 	}
diff --git a/image/image.go b/image/image.go
index b24cb41..37cd7fa 100644
--- a/image/image.go
+++ b/image/image.go
@@ -626,7 +626,8 @@ func GetNamedTaggedReference(image string) (reference.NamedTagged, string, error
 		return nil, "", nil
 	}
 
-	if slashLastIndex, sepLastIndex := strings.LastIndex(image, "/"), strings.LastIndex(image, ":"); sepLastIndex == -1 || (sepLastIndex < slashLastIndex) {
+	slashLastIndex, sepLastIndex := strings.LastIndex(image, "/"), strings.LastIndex(image, ":")
+	if sepLastIndex == -1 || (sepLastIndex < slashLastIndex) {
 		image = fmt.Sprintf("%s:%s", image, constant.DefaultTag)
 	}
 
diff --git a/util/cipher.go b/util/cipher.go
index a5e3125..67cb52b 100644
--- a/util/cipher.go
+++ b/util/cipher.go
@@ -212,6 +212,9 @@ func GenRSAPublicKeyFile(key *rsa.PrivateKey, path string) error {
 	if err := pem.Encode(file, block); err != nil {
 		return err
 	}
+	if cErr := file.Close(); cErr != nil {
+		return cErr
+	}
 
 	return nil
 }
@@ -230,7 +233,10 @@ func ReadPublicKey(path string) (rsa.PublicKey, error) {
 	if err != nil {
 		return rsa.PublicKey{}, err
 	}
-	key := pubInterface.(*rsa.PublicKey)
+	key, ok := pubInterface.(*rsa.PublicKey)
+	if !ok {
+		return rsa.PublicKey{}, errors.New("failed to find public key type")
+	}
 
 	return *key, nil
 }
-- 
1.8.3.1

