From 6866f2e7f80ac9d8decf0e34a34de31df17c25aa Mon Sep 17 00:00:00 2001 From: DCCooper <1866858@gmail.com> Date: Tue, 2 Nov 2021 20:59:35 +0800 Subject: [PATCH 2/2] bugfix: optimize function IsExist reason: IsExist should return two value: 1. err: if err is not nil, which means the input path is not valid, so the caller should just return 2. true/false: this boolean value indicate the path is exist or not, the value only valid when no err occured also add testcase for filepath.go file Signed-off-by: DCCooper <1866858@gmail.com> --- cmd/cli/build.go | 4 +- cmd/cli/load.go | 4 +- cmd/cli/save.go | 8 +- cmd/daemon/main.go | 20 +++- util/cipher.go | 4 +- util/filepath.go | 18 ++-- util/filepath_test.go | 248 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 289 insertions(+), 17 deletions(-) create mode 100644 util/filepath_test.go diff --git a/cmd/cli/build.go b/cmd/cli/build.go index 3d9f549..b0f7765 100644 --- a/cmd/cli/build.go +++ b/cmd/cli/build.go @@ -235,7 +235,9 @@ func checkAbsPath(path string) (string, error) { } path = util.MakeAbsolute(path, pwd) } - if util.IsExist(path) { + if exist, err := util.IsExist(path); err != nil { + return "", err + } else if exist { return "", errors.Errorf("output file already exist: %q, try to remove existing tarball or rename output file", path) } diff --git a/cmd/cli/load.go b/cmd/cli/load.go index 44fefdd..90d189a 100644 --- a/cmd/cli/load.go +++ b/cmd/cli/load.go @@ -202,7 +202,9 @@ func (sep *separatorLoadOption) check(pwd string) error { } sep.dir = util.MakeAbsolute(sep.dir, pwd) - if !util.IsExist(sep.dir) { + if exist, err := util.IsExist(sep.dir); err != nil { + return errors.Wrap(err, "resolve dir path failed") + } else if !exist { return errors.Errorf("image tarball directory %q is not exist", sep.dir) } diff --git a/cmd/cli/save.go b/cmd/cli/save.go index 599d394..5a63e02 100644 --- a/cmd/cli/save.go +++ b/cmd/cli/save.go @@ -112,7 +112,9 @@ func (sep *separatorSaveOption) check(pwd string) error { sep.destPath = "Images" } sep.destPath = util.MakeAbsolute(sep.destPath, pwd) - if util.IsExist(sep.destPath) { + if exist, err := util.IsExist(sep.destPath); err != nil { + return errors.Wrap(err, "check dest path failed") + } else if exist { return errors.Errorf("dest path already exist: %q, try to remove or rename it", sep.destPath) } if len(sep.renameFile) != 0 { @@ -162,7 +164,9 @@ func (opt *saveOptions) checkSaveOpts(args []string) error { return err } opt.path = util.MakeAbsolute(opt.path, pwd) - if util.IsExist(opt.path) { + if exist, err := util.IsExist(opt.path); err != nil { + return errors.Wrap(err, "check output path failed") + } else if exist { return errors.Errorf("output file already exist: %q, try to remove existing tarball or rename output file", opt.path) } return nil diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go index 4fd5356..3665f6b 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -341,7 +341,9 @@ func setupWorkingDirectories() error { return errors.Errorf("%q not an absolute dir, the \"dataroot\" and \"runroot\" must be an absolute path", dir) } - if !util.IsExist(dir) { + if exist, err := util.IsExist(dir); err != nil { + return err + } else if !exist { if err := os.MkdirAll(dir, constant.DefaultRootDirMode); err != nil { return errors.Wrapf(err, "create directory for %q failed", dir) } @@ -363,7 +365,9 @@ func setupWorkingDirectories() error { func checkAndValidateConfig(cmd *cobra.Command) error { // check if configuration.toml file exists, merge config if exists - if !util.IsExist(constant.ConfigurationPath) { + if exist, err := util.IsExist(constant.ConfigurationPath); err != nil { + return err + } else if !exist { logrus.Warnf("Main config file missing, the default configuration is used") } else { conf, err := loadConfig(constant.ConfigurationPath) @@ -378,14 +382,18 @@ func checkAndValidateConfig(cmd *cobra.Command) error { } // file policy.json must be exist - if !util.IsExist(constant.SignaturePolicyPath) { + if exist, err := util.IsExist(constant.SignaturePolicyPath); err != nil { + return err + } else if !exist { return errors.Errorf("policy config file %v is not exist", constant.SignaturePolicyPath) } // check all config files confFiles := []string{constant.RegistryConfigPath, constant.SignaturePolicyPath, constant.StorageConfigPath} for _, file := range confFiles { - if util.IsExist(file) { + if exist, err := util.IsExist(file); err != nil { + return err + } else if exist { fi, err := os.Stat(file) if err != nil { return errors.Wrapf(err, "stat file %q failed", file) @@ -402,7 +410,9 @@ func checkAndValidateConfig(cmd *cobra.Command) error { } // if storage config file exists, merge storage config - if util.IsExist(constant.StorageConfigPath) { + if exist, err := util.IsExist(constant.StorageConfigPath); err != nil { + return err + } else if exist { return mergeStorageConfig(cmd) } diff --git a/util/cipher.go b/util/cipher.go index ce47b71..ecbbc47 100644 --- a/util/cipher.go +++ b/util/cipher.go @@ -185,7 +185,9 @@ func DecryptRSA(data string, key *rsa.PrivateKey, h crypto.Hash) (string, error) // GenRSAPublicKeyFile store public key from rsa key pair into local file func GenRSAPublicKeyFile(key *rsa.PrivateKey, path string) error { - if IsExist(path) { + if exist, err := IsExist(path); err != nil { + return err + } else if exist { if err := os.Remove(path); err != nil { return errors.Errorf("failed to delete the residual key file: %v", err) } diff --git a/util/filepath.go b/util/filepath.go index 59b22da..a10ed85 100644 --- a/util/filepath.go +++ b/util/filepath.go @@ -56,14 +56,18 @@ func IsDirectory(path string) bool { return fi.IsDir() } -// IsExist returns true if the path exists -func IsExist(path string) bool { - if _, err := os.Lstat(path); err != nil { - if os.IsNotExist(err) { - return false - } +// IsExist returns true if the path exists when err is nil +// and return false if path not exists when err is nil +// Caller should focus on whether the err is nil or not +func IsExist(path string) (bool, error) { + _, err := os.Lstat(path) + if err == nil { + return true, nil + } + if os.IsNotExist(err) { + return false, nil } - return true + return false, err } // IsSymbolFile returns true if the path file is a symbol file diff --git a/util/filepath_test.go b/util/filepath_test.go new file mode 100644 index 0000000..add4545 --- /dev/null +++ b/util/filepath_test.go @@ -0,0 +1,248 @@ +// Copyright (c) Huawei Technologies Co., Ltd. 2020. All rights reserved. +// isula-build licensed under the Mulan PSL v2. +// You can use this software according to the terms and conditions of the Mulan PSL v2. +// You may obtain a copy of Mulan PSL v2 at: +// http://license.coscl.org.cn/MulanPSL2 +// THIS SOFTWARE IS PROVIDED ON AN "AS IS" BASIS, WITHOUT WARRANTIES OF ANY KIND, EITHER EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO NON-INFRINGEMENT, MERCHANTABILITY OR FIT FOR A PARTICULAR +// PURPOSE. +// See the Mulan PSL v2 for more details. +// Author: Xiang Li +// Create: 2021-11-02 +// Description: testcase for filepath related common functions + +package util + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "gotest.tools/v3/assert" + constant "isula.org/isula-build" +) + +func TestIsExist(t *testing.T) { + type args struct { + path string + workingDir string + } + tests := []struct { + name string + args args + want string + isExist bool + wantErr bool + preHook func(t *testing.T, path string) + postHook func(t *testing.T) + }{ + { + name: "TC-filename too long", + args: args{ + path: strings.Repeat("a", 256), + workingDir: "/tmp", + }, + want: filepath.Join("/tmp", strings.Repeat("a", 256)), + isExist: false, + wantErr: true, + }, + { + name: "TC-filename valid", + args: args{ + path: strings.Repeat("a", 255), + workingDir: "/tmp", + }, + want: filepath.Join("/tmp", strings.Repeat("a", 255)), + isExist: false, + wantErr: false, + }, + { + name: "TC-path too long", + args: args{ + path: strings.Repeat(strings.Repeat("a", 256)+"/", 16), + workingDir: "/tmp", + }, + want: filepath.Join("/tmp", strings.Repeat(strings.Repeat("a", 256)+"/", 16)) + "/", + isExist: false, + wantErr: true, + }, + { + name: "TC-path exist", + args: args{ + path: strings.Repeat(strings.Repeat("a", 255)+"/", 15), + workingDir: "/tmp", + }, + want: filepath.Join("/tmp", strings.Repeat(strings.Repeat("a", 255)+"/", 15)) + "/", + isExist: true, + wantErr: false, + preHook: func(t *testing.T, path string) { + err := os.MkdirAll(path, constant.DefaultRootDirMode) + assert.NilError(t, err) + }, + postHook: func(t *testing.T) { + err := os.RemoveAll(filepath.Join("/tmp", strings.Repeat("a", 255)+"/")) + assert.NilError(t, err) + }, + }, + { + name: "TC-path with dot exist", + args: args{ + path: ".", + workingDir: filepath.Join("/tmp", strings.Repeat("./"+strings.Repeat("a", 255)+"/", 15)), + }, + want: filepath.Join("/tmp", strings.Repeat(strings.Repeat("a", 255)+"/", 15)) + "/", + isExist: true, + wantErr: false, + preHook: func(t *testing.T, path string) { + err := os.MkdirAll(path, constant.DefaultRootDirMode) + assert.NilError(t, err) + }, + postHook: func(t *testing.T) { + err := os.RemoveAll(filepath.Join("/tmp", strings.Repeat("a", 255)+"/")) + assert.NilError(t, err) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := MakeAbsolute(tt.args.path, tt.args.workingDir) + if got != tt.want { + t.Errorf("MakeAbsolute() = %v, want %v", got, tt.want) + t.Skip() + } + + if tt.preHook != nil { + tt.preHook(t, got) + } + exist, err := IsExist(got) + if exist != tt.isExist { + t.Errorf("IsExist() = %v, want %v", exist, tt.isExist) + } + if (err != nil) != tt.wantErr { + t.Errorf("IsExist() = %v, want %v", err, tt.wantErr) + } + if tt.postHook != nil { + tt.postHook(t) + } + }) + } +} + +func TestIsSymbolFile(t *testing.T) { + originFile := "/tmp/originFile" + symbolFile := "/tmp/symbolFile" + noneExistFile := "/tmp/none_exist_file" + type args struct { + path string + } + tests := []struct { + name string + args args + want bool + preHook func(t *testing.T) + postHook func(t *testing.T) + }{ + { + name: "TC-is symbol file", + args: args{path: "/tmp/symbolFile"}, + want: true, + preHook: func(t *testing.T) { + _, err := os.Create(originFile) + assert.NilError(t, err) + assert.NilError(t, os.Symlink(originFile, symbolFile)) + }, + postHook: func(t *testing.T) { + assert.NilError(t, os.RemoveAll(originFile)) + assert.NilError(t, os.RemoveAll(symbolFile)) + }, + }, + { + name: "TC-is normal file", + args: args{path: originFile}, + want: false, + preHook: func(t *testing.T) { + _, err := os.Create(originFile) + assert.NilError(t, err) + }, + postHook: func(t *testing.T) { + assert.NilError(t, os.RemoveAll(originFile)) + }, + }, + { + name: "TC-file not exist", + args: args{path: noneExistFile}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.preHook != nil { + tt.preHook(t) + } + if got := IsSymbolFile(tt.args.path); got != tt.want { + t.Errorf("IsSymbolFile() = %v, want %v", got, tt.want) + } + if tt.postHook != nil { + tt.postHook(t) + } + }) + } +} + +func TestIsDirectory(t *testing.T) { + dirPath := filepath.Join("/tmp", t.Name()) + filePath := filepath.Join("/tmp", t.Name()) + noneExistFile := "/tmp/none_exist_file" + + type args struct { + path string + } + tests := []struct { + name string + args args + want bool + preHook func(t *testing.T) + postHook func(t *testing.T) + }{ + { + name: "TC-is directory", + args: args{path: dirPath}, + preHook: func(t *testing.T) { + assert.NilError(t, os.MkdirAll(dirPath, constant.DefaultRootDirMode)) + }, + postHook: func(t *testing.T) { + assert.NilError(t, os.RemoveAll(dirPath)) + }, + want: true, + }, + { + name: "TC-is file", + args: args{path: dirPath}, + preHook: func(t *testing.T) { + _, err := os.Create(filePath) + assert.NilError(t, err) + }, + postHook: func(t *testing.T) { + assert.NilError(t, os.RemoveAll(filePath)) + }, + }, + { + name: "TC-path not exist", + args: args{path: noneExistFile}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.preHook != nil { + tt.preHook(t) + } + if got := IsDirectory(tt.args.path); got != tt.want { + t.Errorf("IsDirectory() = %v, want %v", got, tt.want) + } + if tt.postHook != nil { + tt.postHook(t) + } + }) + } +} -- 1.8.3.1