diff --git a/cmd/fs-v1-helpers.go b/cmd/fs-v1-helpers.go index e55034f16..057da9d29 100644 --- a/cmd/fs-v1-helpers.go +++ b/cmd/fs-v1-helpers.go @@ -363,8 +363,7 @@ func fsRenameFile(sourcePath, destPath string) error { return nil } -// Delete a file and its parent if it is empty at the destination path. -// this function additionally protects the basePath from being deleted. +// fsDeleteFile is a wrapper for deleteFile(), after checking the path length. func fsDeleteFile(basePath, deletePath string) error { if err := checkPathLength(basePath); err != nil { return traceError(err) @@ -374,44 +373,7 @@ func fsDeleteFile(basePath, deletePath string) error { return traceError(err) } - if basePath == deletePath { - return nil - } - - // Verify if the path exists. - pathSt, err := osStat(preparePath(deletePath)) - if err != nil { - if os.IsNotExist(err) { - return traceError(errFileNotFound) - } else if os.IsPermission(err) { - return traceError(errFileAccessDenied) - } - return traceError(err) - } - - if pathSt.IsDir() && !isDirEmpty(deletePath) { - // Verify if directory is empty. - return nil - } - - // Attempt to remove path. - if err = os.Remove(preparePath(deletePath)); err != nil { - if os.IsNotExist(err) { - return traceError(errFileNotFound) - } else if os.IsPermission(err) { - return traceError(errFileAccessDenied) - } else if isSysErrNotEmpty(err) { - return traceError(errVolumeNotEmpty) - } - return traceError(err) - } - - // Recursively go down the next path and delete again. - if err := fsDeleteFile(basePath, pathutil.Dir(deletePath)); err != nil { - return err - } - - return nil + return deleteFile(basePath, deletePath) } // fsRemoveMeta safely removes a locked file and takes care of Windows special case diff --git a/cmd/fs-v1-helpers_test.go b/cmd/fs-v1-helpers_test.go index 2937a64b8..5e9bb27a4 100644 --- a/cmd/fs-v1-helpers_test.go +++ b/cmd/fs-v1-helpers_test.go @@ -18,6 +18,7 @@ package cmd import ( "bytes" + "io" "io/ioutil" "os" "path" @@ -263,50 +264,123 @@ func TestFSDeletes(t *testing.T) { t.Fatalf("Unable to create file, %s", err) } // Seek back. - reader.Seek(0, 0) + reader.Seek(0, io.SeekStart) + + // folder is not empty + err = fsMkdir(pathJoin(path, "success-vol", "not-empty")) + if err != nil { + t.Fatal(err) + } + err = ioutil.WriteFile(pathJoin(path, "success-vol", "not-empty", "file"), []byte("data"), 0777) + if err != nil { + t.Fatal(err) + } + + // recursive + if err = fsMkdir(pathJoin(path, "success-vol", "parent")); err != nil { + t.Fatal(err) + } + if err = fsMkdir(pathJoin(path, "success-vol", "parent", "dir")); err != nil { + t.Fatal(err) + } testCases := []struct { + basePath string srcVol string srcPath string expectedErr error }{ - // Test case - 1. // valid case with existing volume and file to delete. { + basePath: path, srcVol: "success-vol", srcPath: "success-file", expectedErr: nil, }, - // Test case - 2. // The file was deleted in the last case, so DeleteFile should fail. { + basePath: path, srcVol: "success-vol", srcPath: "success-file", expectedErr: errFileNotFound, }, - // Test case - 3. // Test case with segment of the volume name > 255. { + basePath: path, srcVol: "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001", srcPath: "success-file", expectedErr: errFileNameTooLong, }, - // Test case - 4. // Test case with src path segment > 255. { + basePath: path, srcVol: "success-vol", srcPath: "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001", expectedErr: errFileNameTooLong, }, + // Base path is way too long. + { + basePath: "path03333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333", + srcVol: "success-vol", + srcPath: "object", + expectedErr: errFileNameTooLong, + }, + // Directory is not empty. Should give nil, but won't delete. + { + basePath: path, + srcVol: "success-vol", + srcPath: "not-empty", + expectedErr: nil, + }, + // Should delete recursively. + { + basePath: path, + srcVol: "success-vol", + srcPath: pathJoin("parent", "dir"), + expectedErr: nil, + }, } for i, testCase := range testCases { - if err = fsDeleteFile(path, pathJoin(path, testCase.srcVol, testCase.srcPath)); errorCause(err) != testCase.expectedErr { + if err = fsDeleteFile(testCase.basePath, pathJoin(testCase.basePath, testCase.srcVol, testCase.srcPath)); errorCause(err) != testCase.expectedErr { t.Errorf("Test case %d: Expected: \"%s\", got: \"%s\"", i+1, testCase.expectedErr, err) } } } +func BenchmarkFSDeleteFile(b *testing.B) { + // create posix test setup + _, path, err := newPosixTestSetup() + if err != nil { + b.Fatalf("Unable to create posix test setup, %s", err) + } + defer removeAll(path) + + // Setup test environment. + if err = fsMkdir(pathJoin(path, "benchmark")); err != nil { + b.Fatalf("Unable to create directory, %s", err) + } + + benchDir := pathJoin(path, "benchmark") + filename := pathJoin(benchDir, "file.txt") + + b.ResetTimer() + // We need to create and delete the file sequentially inside the benchmark. + for i := 0; i < b.N; i++ { + b.StopTimer() + err = ioutil.WriteFile(filename, []byte("data"), 0777) + if err != nil { + b.Fatal(err) + } + b.StartTimer() + + err = fsDeleteFile(benchDir, filename) + if err != nil { + b.Fatal(err) + } + } +} + // Tests fs removes. func TestFSRemoves(t *testing.T) { // create posix test setup diff --git a/cmd/posix.go b/cmd/posix.go index d0d99e08d..4a14b166d 100644 --- a/cmd/posix.go +++ b/cmd/posix.go @@ -915,27 +915,23 @@ func (s *posix) StatFile(volume, path string) (file FileInfo, err error) { }, nil } -// deleteFile - delete file path if its empty. +// deleteFile deletes a file path if its empty. If it's successfully deleted, +// it will recursively move up the tree, deleting empty parent directories +// until it finds one with files in it. Returns nil for a non-empty directory. func deleteFile(basePath, deletePath string) error { if basePath == deletePath { return nil } - // Verify if the path exists. - pathSt, err := osStat(preparePath(deletePath)) - if err != nil { - if os.IsNotExist(err) { - return errFileNotFound - } else if os.IsPermission(err) { - return errFileAccessDenied - } - return err - } - if pathSt.IsDir() && !isDirEmpty(deletePath) { - // Verify if directory is empty. - return nil - } + // Attempt to remove path. if err := os.Remove(preparePath(deletePath)); err != nil { + // Ignore errors if the directory is not empty. The server relies on + // this functionality, and sometimes uses recursion that should not + // error on parent directories. + if isSysErrNotEmpty(err) { + return nil + } + if os.IsNotExist(err) { return errFileNotFound } else if os.IsPermission(err) { @@ -943,11 +939,9 @@ func deleteFile(basePath, deletePath string) error { } return err } + // Recursively go down the next path and delete again. - if err := deleteFile(basePath, slashpath.Dir(deletePath)); err != nil { - return err - } - return nil + return deleteFile(basePath, slashpath.Dir(deletePath)) } // DeleteFile - delete a file at path.