From a56d5ef4152a0ae903e0a881c97d378e7dc375cc Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sun, 8 May 2016 01:58:05 -0700 Subject: [PATCH] xl/fs: isFunctions should only return boolean. (#1525) log the unrecognize errors. --- fs-objects-multipart.go | 8 ++-- fs-objects.go | 14 +++++- object-api-getobjectinfo_test.go | 4 +- object-api-multipart_test.go | 2 +- object-common-multipart.go | 73 +++++++++++++++++--------------- object-common.go | 34 +++++++-------- posix.go | 42 +++++++++--------- xl-objects-multipart.go | 8 ++-- xl-objects.go | 16 ++++--- 9 files changed, 112 insertions(+), 89 deletions(-) diff --git a/fs-objects-multipart.go b/fs-objects-multipart.go index 289c4538e..9db9175aa 100644 --- a/fs-objects-multipart.go +++ b/fs-objects-multipart.go @@ -46,15 +46,17 @@ func (fs fsObjects) CompleteMultipartUpload(bucket string, object string, upload if !IsValidBucketName(bucket) { return "", BucketNameInvalid{Bucket: bucket} } + // Verify whether the bucket exists. + if !isBucketExist(fs.storage, bucket) { + return "", BucketNotFound{Bucket: bucket} + } if !IsValidObjectName(object) { return "", ObjectNameInvalid{ Bucket: bucket, Object: object, } } - if status, err := isUploadIDExists(fs.storage, bucket, object, uploadID); err != nil { - return "", err - } else if !status { + if !isUploadIDExists(fs.storage, bucket, object, uploadID) { return "", InvalidUploadID{UploadID: uploadID} } diff --git a/fs-objects.go b/fs-objects.go index 1b553b9b5..1bf28bb3f 100644 --- a/fs-objects.go +++ b/fs-objects.go @@ -90,11 +90,14 @@ func (fs fsObjects) DeleteBucket(bucket string) error { func (fs fsObjects) GetObject(bucket, object string, startOffset int64) (io.ReadCloser, error) { // Verify if bucket is valid. if !IsValidBucketName(bucket) { - return nil, (BucketNameInvalid{Bucket: bucket}) + return nil, BucketNameInvalid{Bucket: bucket} + } + if !isBucketExist(fs.storage, bucket) { + return nil, BucketNotFound{Bucket: bucket} } // Verify if object is valid. if !IsValidObjectName(object) { - return nil, (ObjectNameInvalid{Bucket: bucket, Object: object}) + return nil, ObjectNameInvalid{Bucket: bucket, Object: object} } fileReader, err := fs.storage.ReadFile(bucket, object, startOffset) if err != nil { @@ -109,6 +112,9 @@ func (fs fsObjects) GetObjectInfo(bucket, object string) (ObjectInfo, error) { if !IsValidBucketName(bucket) { return ObjectInfo{}, (BucketNameInvalid{Bucket: bucket}) } + if !isBucketExist(fs.storage, bucket) { + return ObjectInfo{}, BucketNotFound{Bucket: bucket} + } // Verify if object is valid. if !IsValidObjectName(object) { return ObjectInfo{}, (ObjectNameInvalid{Bucket: bucket, Object: object}) @@ -145,6 +151,9 @@ func (fs fsObjects) DeleteObject(bucket, object string) error { if !IsValidBucketName(bucket) { return BucketNameInvalid{Bucket: bucket} } + if !isBucketExist(fs.storage, bucket) { + return BucketNotFound{Bucket: bucket} + } if !IsValidObjectName(object) { return ObjectNameInvalid{Bucket: bucket, Object: object} } @@ -154,6 +163,7 @@ func (fs fsObjects) DeleteObject(bucket, object string) error { return nil } +// ListObjects - list all objects. func (fs fsObjects) ListObjects(bucket, prefix, marker, delimiter string, maxKeys int) (ListObjectsInfo, error) { return listObjectsCommon(fs, bucket, prefix, marker, delimiter, maxKeys) } diff --git a/object-api-getobjectinfo_test.go b/object-api-getobjectinfo_test.go index da432c692..df1109371 100644 --- a/object-api-getobjectinfo_test.go +++ b/object-api-getobjectinfo_test.go @@ -67,8 +67,8 @@ func testGetObjectInfo(obj ObjectLayer, instanceType string, t *testing.T) { {"abcdefgh", "abc", ObjectInfo{}, BucketNotFound{Bucket: "abcdefgh"}, false}, {"ijklmnop", "efg", ObjectInfo{}, BucketNotFound{Bucket: "ijklmnop"}, false}, // Test cases with valid but non-existing bucket names and invalid object name (Test number 8-9). - {"abcdefgh", "", ObjectInfo{}, ObjectNameInvalid{Bucket: "abcdefgh", Object: ""}, false}, - {"ijklmnop", "", ObjectInfo{}, ObjectNameInvalid{Bucket: "ijklmnop", Object: ""}, false}, + {"test-getobjectinfo", "", ObjectInfo{}, ObjectNameInvalid{Bucket: "test-getobjectinfo", Object: ""}, false}, + {"test-getobjectinfo", "", ObjectInfo{}, ObjectNameInvalid{Bucket: "test-getobjectinfo", Object: ""}, false}, // Test cases with non-existing object name with existing bucket (Test number 10-12). {"test-getobjectinfo", "Africa", ObjectInfo{}, ObjectNotFound{Bucket: "test-getobjectinfo", Object: "Africa"}, false}, {"test-getobjectinfo", "Antartica", ObjectInfo{}, ObjectNotFound{Bucket: "test-getobjectinfo", Object: "Antartica"}, false}, diff --git a/object-api-multipart_test.go b/object-api-multipart_test.go index db11a6221..7722dd776 100644 --- a/object-api-multipart_test.go +++ b/object-api-multipart_test.go @@ -149,7 +149,7 @@ func testObjectAPIPutObjectPart(obj ObjectLayer, instanceType string, t *testing {"a", "obj", "", 1, "", "", 0, false, "", fmt.Errorf("%s", "Bucket name invalid: a")}, // Test case - 5. // Case with invalid object names. - {"abc", "", "", 1, "", "", 0, false, "", fmt.Errorf("%s", "Object name invalid: abc#")}, + {bucket, "", "", 1, "", "", 0, false, "", fmt.Errorf("%s", "Object name invalid: minio-bucket#")}, // Test case - 6. // Valid object and bucket names but non-existent bucket. {"abc", "def", "", 1, "", "", 0, false, "", fmt.Errorf("%s", "Bucket not found: abc")}, diff --git a/object-common-multipart.go b/object-common-multipart.go index 2252778f3..e5be1143c 100644 --- a/object-common-multipart.go +++ b/object-common-multipart.go @@ -76,18 +76,16 @@ func newMultipartUploadCommon(storage StorageAPI, bucket string, object string) if !IsValidBucketName(bucket) { return "", BucketNameInvalid{Bucket: bucket} } + // Verify whether the bucket exists. + if !isBucketExist(storage, bucket) { + return "", BucketNotFound{Bucket: bucket} + } + // Verify if object name is valid. if !IsValidObjectName(object) { return "", ObjectNameInvalid{Bucket: bucket, Object: object} } - // Verify whether the bucket exists. - if isExist, err := isBucketExist(storage, bucket); err != nil { - return "", err - } else if !isExist { - return "", BucketNotFound{Bucket: bucket} - } - // Loops through until successfully generates a new unique upload id. for { uuid, err := uuid.New() @@ -138,20 +136,14 @@ func putObjectPartCommon(storage StorageAPI, bucket string, object string, uploa if !IsValidBucketName(bucket) { return "", BucketNameInvalid{Bucket: bucket} } - if !IsValidObjectName(object) { - return "", ObjectNameInvalid{Bucket: bucket, Object: object} - } - // Verify whether the bucket exists. - if isExist, err := isBucketExist(storage, bucket); err != nil { - return "", err - } else if !isExist { + if !isBucketExist(storage, bucket) { return "", BucketNotFound{Bucket: bucket} } - - if status, err := isUploadIDExists(storage, bucket, object, uploadID); err != nil { - return "", err - } else if !status { + if !IsValidObjectName(object) { + return "", ObjectNameInvalid{Bucket: bucket, Object: object} + } + if !isUploadIDExists(storage, bucket, object, uploadID) { return "", InvalidUploadID{UploadID: uploadID} } @@ -251,12 +243,14 @@ func abortMultipartUploadCommon(storage StorageAPI, bucket, object, uploadID str if !IsValidBucketName(bucket) { return BucketNameInvalid{Bucket: bucket} } + // Verify whether the bucket exists. + if !isBucketExist(storage, bucket) { + return BucketNotFound{Bucket: bucket} + } if !IsValidObjectName(object) { return ObjectNameInvalid{Bucket: bucket, Object: object} } - if status, err := isUploadIDExists(storage, bucket, object, uploadID); err != nil { - return err - } else if !status { + if !isUploadIDExists(storage, bucket, object, uploadID) { return InvalidUploadID{UploadID: uploadID} } return cleanupUploadedParts(storage, mpartMetaPrefix, bucket, object, uploadID) @@ -393,11 +387,22 @@ func listMetaBucketMultipartFiles(layer ObjectLayer, prefixPath string, markerPa // listMultipartUploadsCommon - lists all multipart uploads, common // function for both object layers. func listMultipartUploadsCommon(layer ObjectLayer, bucket, prefix, keyMarker, uploadIDMarker, delimiter string, maxUploads int) (ListMultipartsInfo, error) { + var storage StorageAPI + switch l := layer.(type) { + case xlObjects: + storage = l.storage + case fsObjects: + storage = l.storage + } + result := ListMultipartsInfo{} // Verify if bucket is valid. if !IsValidBucketName(bucket) { return ListMultipartsInfo{}, BucketNameInvalid{Bucket: bucket} } + if !isBucketExist(storage, bucket) { + return ListMultipartsInfo{}, BucketNotFound{Bucket: bucket} + } if !IsValidObjectPrefix(prefix) { return ListMultipartsInfo{}, ObjectNameInvalid{Bucket: bucket, Object: prefix} } @@ -495,15 +500,17 @@ func listMultipartUploadsCommon(layer ObjectLayer, bucket, prefix, keyMarker, up func listObjectPartsCommon(storage StorageAPI, bucket, object, uploadID string, partNumberMarker, maxParts int) (ListPartsInfo, error) { // Verify if bucket is valid. if !IsValidBucketName(bucket) { - return ListPartsInfo{}, (BucketNameInvalid{Bucket: bucket}) + return ListPartsInfo{}, BucketNameInvalid{Bucket: bucket} + } + // Verify whether the bucket exists. + if !isBucketExist(storage, bucket) { + return ListPartsInfo{}, BucketNotFound{Bucket: bucket} } if !IsValidObjectName(object) { - return ListPartsInfo{}, (ObjectNameInvalid{Bucket: bucket, Object: object}) + return ListPartsInfo{}, ObjectNameInvalid{Bucket: bucket, Object: object} } - if status, err := isUploadIDExists(storage, bucket, object, uploadID); err != nil { - return ListPartsInfo{}, err - } else if !status { - return ListPartsInfo{}, (InvalidUploadID{UploadID: uploadID}) + if !isUploadIDExists(storage, bucket, object, uploadID) { + return ListPartsInfo{}, InvalidUploadID{UploadID: uploadID} } result := ListPartsInfo{} entries, err := storage.ListDir(minioMetaBucket, path.Join(mpartMetaPrefix, bucket, object, uploadID)) @@ -547,16 +554,16 @@ func listObjectPartsCommon(storage StorageAPI, bucket, object, uploadID string, } // isUploadIDExists - verify if a given uploadID exists and is valid. -func isUploadIDExists(storage StorageAPI, bucket, object, uploadID string) (bool, error) { +func isUploadIDExists(storage StorageAPI, bucket, object, uploadID string) bool { uploadIDPath := path.Join(mpartMetaPrefix, bucket, object, uploadID, incompleteFile) st, err := storage.StatFile(minioMetaBucket, uploadIDPath) if err != nil { - // Upload id does not exist. if err == errFileNotFound { - return false, nil + return false } - return false, err + log.Errorf("StatFile failed wtih %s", err) + return false } - // Upload id exists and is a regular file. - return st.Mode.IsRegular(), nil + log.Debugf("FileInfo: %v", st) + return st.Mode.IsRegular() } diff --git a/object-common.go b/object-common.go index bad4eb548..22f83bd13 100644 --- a/object-common.go +++ b/object-common.go @@ -119,18 +119,16 @@ func putObjectCommon(storage StorageAPI, bucket string, object string, size int6 if !IsValidBucketName(bucket) { return "", BucketNameInvalid{Bucket: bucket} } + // Check whether the bucket exists. + if !isBucketExist(storage, bucket) { + return "", BucketNotFound{Bucket: bucket} + } if !IsValidObjectName(object) { return "", ObjectNameInvalid{ Bucket: bucket, Object: object, } } - // Check whether the bucket exists. - if isExist, err := isBucketExist(storage, bucket); err != nil { - return "", err - } else if !isExist { - return "", BucketNotFound{Bucket: bucket} - } tempObj := path.Join(tmpMetaPrefix, bucket, object) fileWriter, err := storage.CreateFile(minioMetaBucket, tempObj) @@ -195,12 +193,12 @@ func putObjectCommon(storage StorageAPI, bucket string, object string, size int6 } func listObjectsCommon(layer ObjectLayer, bucket, prefix, marker, delimiter string, maxKeys int) (ListObjectsInfo, error) { - var disk StorageAPI + var storage StorageAPI switch l := layer.(type) { case xlObjects: - disk = l.storage + storage = l.storage case fsObjects: - disk = l.storage + storage = l.storage } // Verify if bucket is valid. @@ -209,9 +207,7 @@ func listObjectsCommon(layer ObjectLayer, bucket, prefix, marker, delimiter stri } // Verify whether the bucket exists. - if isExist, err := isBucketExist(disk, bucket); err != nil { - return ListObjectsInfo{}, err - } else if !isExist { + if !isBucketExist(storage, bucket) { return ListObjectsInfo{}, BucketNotFound{Bucket: bucket} } @@ -319,13 +315,15 @@ func listObjectsCommon(layer ObjectLayer, bucket, prefix, marker, delimiter stri } // checks whether bucket exists. -func isBucketExist(storage StorageAPI, bucketName string) (bool, error) { +func isBucketExist(storage StorageAPI, bucketName string) bool { // Check whether bucket exists. - if _, e := storage.StatVol(bucketName); e != nil { - if e == errVolumeNotFound { - return false, nil + _, err := storage.StatVol(bucketName) + if err != nil { + if err == errVolumeNotFound { + return false } - return false, e + log.Errorf("StatVol failed with %s", err) + return false } - return true, nil + return true } diff --git a/posix.go b/posix.go index f82b08143..8dcdbb4bb 100644 --- a/posix.go +++ b/posix.go @@ -40,22 +40,32 @@ type fsStorage struct { } // isDirEmpty - returns whether given directory is empty or not. -func isDirEmpty(dirname string) (status bool, err error) { +func isDirEmpty(dirname string) bool { f, err := os.Open(dirname) - if err == nil { - defer f.Close() - if _, err = f.Readdirnames(1); err == io.EOF { - status = true - err = nil + if err != nil { + log.Errorf("Unable to access directory %s, failed with %s", dirname, err) + return false + } + defer f.Close() + // List one entry. + _, err = f.Readdirnames(1) + if err != nil { + if err == io.EOF { + // Returns true if we have reached EOF, directory is + // indeed empty. + return true } + log.Errorf("Unable to list directory %s, failed with %s", dirname, err) + return false } - return status, err + // Directory is not empty. + return false } // Initialize a new storage disk. func newPosix(diskPath string) (StorageAPI, error) { if diskPath == "" { - log.Debug("Disk cannot be empty") + log.Error("Disk cannot be empty") return nil, errInvalidArgument } st, err := os.Stat(diskPath) @@ -82,8 +92,7 @@ func newPosix(diskPath string) (StorageAPI, error) { return fs, nil } -// checkDiskFree verifies if disk path has sufficient minium free disk -// space. +// checkDiskFree verifies if disk path has sufficient minium free disk space. func checkDiskFree(diskPath string, minFreeDisk int64) (err error) { di, err := disk.GetInfo(diskPath) if err != nil { @@ -527,18 +536,9 @@ func deleteFile(basePath, deletePath string) error { } return err } - if pathSt.IsDir() { + if pathSt.IsDir() && !isDirEmpty(deletePath) { // Verify if directory is empty. - empty, err := isDirEmpty(deletePath) - if err != nil { - log.WithFields(logrus.Fields{ - "deletePath": deletePath, - }).Debugf("isDirEmpty failed with %s", err) - return err - } - if !empty { - return nil - } + return nil } // Attempt to remove path. if err := os.Remove(deletePath); err != nil { diff --git a/xl-objects-multipart.go b/xl-objects-multipart.go index ee867f155..54fe979a5 100644 --- a/xl-objects-multipart.go +++ b/xl-objects-multipart.go @@ -96,15 +96,17 @@ func (xl xlObjects) CompleteMultipartUpload(bucket string, object string, upload if !IsValidBucketName(bucket) { return "", BucketNameInvalid{Bucket: bucket} } + // Verify whether the bucket exists. + if !isBucketExist(xl.storage, bucket) { + return "", BucketNotFound{Bucket: bucket} + } if !IsValidObjectName(object) { return "", ObjectNameInvalid{ Bucket: bucket, Object: object, } } - if status, err := isUploadIDExists(xl.storage, bucket, object, uploadID); err != nil { - return "", err - } else if !status { + if !isUploadIDExists(xl.storage, bucket, object, uploadID) { return "", InvalidUploadID{UploadID: uploadID} } var metadata = MultipartObjectInfo{} diff --git a/xl-objects.go b/xl-objects.go index 5a0255bf2..30d805179 100644 --- a/xl-objects.go +++ b/xl-objects.go @@ -139,6 +139,9 @@ func (xl xlObjects) GetObject(bucket, object string, startOffset int64) (io.Read if !IsValidBucketName(bucket) { return nil, BucketNameInvalid{Bucket: bucket} } + if !isBucketExist(xl.storage, bucket) { + return nil, BucketNotFound{Bucket: bucket} + } // Verify if object is valid. if !IsValidObjectName(object) { return nil, ObjectNameInvalid{Bucket: bucket, Object: object} @@ -201,16 +204,14 @@ func (xl xlObjects) GetObjectInfo(bucket, object string) (ObjectInfo, error) { if !IsValidBucketName(bucket) { return ObjectInfo{}, BucketNameInvalid{Bucket: bucket} } + // Check whether the bucket exists. + if !isBucketExist(xl.storage, bucket) { + return ObjectInfo{}, BucketNotFound{Bucket: bucket} + } // Verify if object is valid. if !IsValidObjectName(object) { return ObjectInfo{}, ObjectNameInvalid{Bucket: bucket, Object: object} } - // Check whether the bucket exists. - if isExist, err := isBucketExist(xl.storage, bucket); err != nil { - return ObjectInfo{}, err - } else if !isExist { - return ObjectInfo{}, BucketNotFound{Bucket: bucket} - } fi, err := xl.storage.StatFile(bucket, object) if err != nil { if err != errFileNotFound { @@ -265,6 +266,9 @@ func (xl xlObjects) DeleteObject(bucket, object string) error { if !IsValidBucketName(bucket) { return BucketNameInvalid{Bucket: bucket} } + if !isBucketExist(xl.storage, bucket) { + return BucketNotFound{Bucket: bucket} + } if !IsValidObjectName(object) { return ObjectNameInvalid{Bucket: bucket, Object: object} }