From c69fdf0cf266bd79387264505d716a806ba00122 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 4 Apr 2016 19:55:07 -0700 Subject: [PATCH] listObjects: Cleanup and naming conventions. - Marker should be escaped outside in handlers. - Delimiter should be handled outside in handlers. - Add missing comments and change the function names. - Handle case of 'maxKeys' when its set to '0', its a valid case and should be treated as such. --- api-resources.go | 19 +++++++-- api-response.go | 8 ++-- bucket-handlers.go | 22 +++++++++- fs-bucket-listobjects.go | 54 +++++++++++++++++++----- fs-bucket-listobjects_test.go | 5 --- fs-dir-common.go | 78 ++++++++++++++++++++++------------- fs-dir-nix.go | 28 +++++++------ fs-dir-others.go | 3 +- fs-dirent-fileno.go | 3 +- fs-dirent-ino.go | 3 +- fs.go | 35 ---------------- object-handlers.go | 7 +--- 12 files changed, 154 insertions(+), 111 deletions(-) diff --git a/api-resources.go b/api-resources.go index edddf832b..93d16c004 100644 --- a/api-resources.go +++ b/api-resources.go @@ -26,19 +26,26 @@ func getBucketResources(values url.Values) (prefix, marker, delimiter string, ma prefix = values.Get("prefix") marker = values.Get("marker") delimiter = values.Get("delimiter") - maxkeys, _ = strconv.Atoi(values.Get("max-keys")) + if values.Get("max-keys") != "" { + maxkeys, _ = strconv.Atoi(values.Get("max-keys")) + } else { + maxkeys = maxObjectList + } encodingType = values.Get("encoding-type") return } // Parse bucket url queries for ?uploads func getBucketMultipartResources(values url.Values) (prefix, keyMarker, uploadIDMarker, delimiter string, maxUploads int, encodingType string) { - prefix = values.Get("prefix") keyMarker = values.Get("key-marker") uploadIDMarker = values.Get("upload-id-marker") delimiter = values.Get("delimiter") - maxUploads, _ = strconv.Atoi(values.Get("max-uploads")) + if values.Get("max-uploads") != "" { + maxUploads, _ = strconv.Atoi(values.Get("max-uploads")) + } else { + maxUploads = maxUploadsList + } encodingType = values.Get("encoding-type") return } @@ -47,7 +54,11 @@ func getBucketMultipartResources(values url.Values) (prefix, keyMarker, uploadID func getObjectResources(values url.Values) (uploadID string, partNumberMarker, maxParts int, encodingType string) { uploadID = values.Get("uploadId") partNumberMarker, _ = strconv.Atoi(values.Get("part-number-marker")) - maxParts, _ = strconv.Atoi(values.Get("max-parts")) + if values.Get("max-parts") != "" { + maxParts, _ = strconv.Atoi(values.Get("max-parts")) + } else { + maxParts = maxPartsList + } encodingType = values.Get("encoding-type") return } diff --git a/api-response.go b/api-response.go index 1b1fcc086..e9c047673 100644 --- a/api-response.go +++ b/api-response.go @@ -23,10 +23,10 @@ import ( ) const ( - // Reply date format - timeFormatAMZ = "2006-01-02T15:04:05.000Z" - // Limit number of objects in a given response. - maxObjectList = 1000 + timeFormatAMZ = "2006-01-02T15:04:05.000Z" // Reply date format + maxObjectList = 1000 // Limit number of objects in a listObjectsResponse. + maxUploadsList = 1000 // Limit number of uploads in a listUploadsResponse. + maxPartsList = 1000 // Limit number of parts in a listPartsResponse. ) // LocationResponse - format for location response. diff --git a/bucket-handlers.go b/bucket-handlers.go index f5e8e37c0..644be8c90 100644 --- a/bucket-handlers.go +++ b/bucket-handlers.go @@ -237,8 +237,26 @@ func (api objectStorageAPI) ListObjectsHandler(w http.ResponseWriter, r *http.Re writeErrorResponse(w, r, ErrInvalidMaxKeys, r.URL.Path) return } - if maxkeys == 0 { - maxkeys = maxObjectList + // Verify if delimiter is anything other than '/', which we do not support. + if delimiter != "" && delimiter != "/" { + writeErrorResponse(w, r, ErrNotImplemented, r.URL.Path) + return + } + // If marker is set unescape. + if marker != "" { + // Try to unescape marker. + markerUnescaped, e := url.QueryUnescape(marker) + if e != nil { + // Return 'NoSuchKey' to indicate invalid marker key. + writeErrorResponse(w, r, ErrNoSuchKey, r.URL.Path) + return + } + marker = markerUnescaped + // Marker not common with prefix is not implemented. + if !strings.HasPrefix(marker, prefix) { + writeErrorResponse(w, r, ErrNotImplemented, r.URL.Path) + return + } } listObjectsInfo, err := api.ObjectAPI.ListObjects(bucket, prefix, marker, delimiter, maxkeys) diff --git a/fs-bucket-listobjects.go b/fs-bucket-listobjects.go index 22c7aa216..dfa6d40de 100644 --- a/fs-bucket-listobjects.go +++ b/fs-bucket-listobjects.go @@ -18,7 +18,6 @@ package main import ( "fmt" - "net/url" "os" "path/filepath" "strings" @@ -40,6 +39,38 @@ func isDirExist(dirname string) (status bool, err error) { return } +func (fs *Filesystem) saveTreeWalk(params listObjectParams, walker *treeWalker) { + fs.listObjectMapMutex.Lock() + defer fs.listObjectMapMutex.Unlock() + + walkers, _ := fs.listObjectMap[params] + walkers = append(walkers, walker) + + fs.listObjectMap[params] = walkers +} + +func (fs *Filesystem) lookupTreeWalk(params listObjectParams) *treeWalker { + fs.listObjectMapMutex.Lock() + defer fs.listObjectMapMutex.Unlock() + + if walkChs, ok := fs.listObjectMap[params]; ok { + for i, walkCh := range walkChs { + if !walkCh.timedOut { + newWalkChs := walkChs[i+1:] + if len(newWalkChs) > 0 { + fs.listObjectMap[params] = newWalkChs + } else { + delete(fs.listObjectMap, params) + } + return walkCh + } + } + // As all channels are timed out, delete the map entry + delete(fs.listObjectMap, params) + } + return nil +} + // ListObjects - lists all objects for a given prefix, returns up to // maxKeys number of objects per call. func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKeys int) (ListObjectsInfo, *probe.Error) { @@ -73,14 +104,8 @@ func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKe return result, probe.NewError(fmt.Errorf("delimiter '%s' is not supported. Only '/' is supported", delimiter)) } - // Marker is set unescape. + // Verify if marker has prefix. if marker != "" { - if markerUnescaped, err := url.QueryUnescape(marker); err == nil { - marker = markerUnescaped - } else { - return result, probe.NewError(err) - } - if !strings.HasPrefix(marker, prefix) { return result, probe.NewError(fmt.Errorf("Invalid combination of marker '%s' and prefix '%s'", marker, prefix)) } @@ -120,9 +145,9 @@ func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKe // popTreeWalker returns nil if the call to ListObject is done for the first time. // On further calls to ListObjects to retrive more objects within the timeout period, // popTreeWalker returns the channel from which rest of the objects can be retrieved. - walker := fs.popTreeWalker(listObjectParams{bucket, delimiter, marker, prefix}) + walker := fs.lookupTreeWalk(listObjectParams{bucket, delimiter, marker, prefix}) if walker == nil { - walker = startTreeWalker(fs.path, bucket, filepath.FromSlash(prefix), filepath.FromSlash(marker), recursive) + walker = startTreeWalk(fs.path, bucket, filepath.FromSlash(prefix), filepath.FromSlash(marker), recursive) } nextMarker := "" @@ -132,29 +157,36 @@ func (fs Filesystem) ListObjects(bucket, prefix, marker, delimiter string, maxKe // Closed channel. return result, nil } + // For any walk error return right away. if walkResult.err != nil { return ListObjectsInfo{}, probe.NewError(walkResult.err) } objInfo := walkResult.objectInfo objInfo.Name = filepath.ToSlash(objInfo.Name) + + // Skip temporary files. if strings.Contains(objInfo.Name, "$multiparts") || strings.Contains(objInfo.Name, "$tmpobject") { continue } + // For objects being directory and delimited we set Prefixes. if objInfo.IsDir { result.Prefixes = append(result.Prefixes, objInfo.Name) } else { result.Objects = append(result.Objects, objInfo) } + // We have listed everything return. if walkResult.end { return result, nil } nextMarker = objInfo.Name i++ } + // We haven't exhaused the list yet, set IsTruncated to 'true' so + // that the client can send another request. result.IsTruncated = true result.NextMarker = nextMarker - fs.pushTreeWalker(listObjectParams{bucket, delimiter, nextMarker, prefix}, walker) + fs.saveTreeWalk(listObjectParams{bucket, delimiter, nextMarker, prefix}, walker) return result, nil } diff --git a/fs-bucket-listobjects_test.go b/fs-bucket-listobjects_test.go index 6c7f9ce04..8ebda79fa 100644 --- a/fs-bucket-listobjects_test.go +++ b/fs-bucket-listobjects_test.go @@ -448,11 +448,6 @@ func TestListObjects(t *testing.T) { // Empty string < "" > and forward slash < / > are the ony two valid arguments for delimeter. {"test-bucket-list-object", "", "", "*", 0, ListObjectsInfo{}, fmt.Errorf("delimiter '%s' is not supported", "*"), false}, {"test-bucket-list-object", "", "", "-", 0, ListObjectsInfo{}, fmt.Errorf("delimiter '%s' is not supported", "-"), false}, - // Marker goes through url QueryUnescape, sending inputs for which QueryUnescape would fail (11-12). - // Here is how QueryUnescape behaves https://golang.org/pkg/net/url/#QueryUnescape. - // QueryUnescape is necessasry since marker is provided as URL query parameter. - {"test-bucket-list-object", "", "test%", "", 0, ListObjectsInfo{}, fmt.Errorf("invalid URL escape"), false}, - {"test-bucket-list-object", "", "test%A", "", 0, ListObjectsInfo{}, fmt.Errorf("invalid URL escape"), false}, // Testing for failure cases with both perfix and marker (13). // The prefix and marker combination to be valid it should satisy strings.HasPrefix(marker, prefix). {"test-bucket-list-object", "asia", "europe-object", "", 0, ListObjectsInfo{}, fmt.Errorf("Invalid combination of marker '%s' and prefix '%s'", "europe-object", "asia"), false}, diff --git a/fs-dir-common.go b/fs-dir-common.go index 0ac54d96c..eedaf6897 100644 --- a/fs-dir-common.go +++ b/fs-dir-common.go @@ -1,3 +1,19 @@ +/* + * Minio Cloud Storage, (C) 2016 Minio, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package main import ( @@ -8,18 +24,20 @@ import ( "time" ) +// fsDirent carries directory entries. type fsDirent struct { name string - modifiedTime time.Time // On unix this is empty. - size int64 // On unix this is empty. + modifiedTime time.Time // On Solaris and older unix distros this is empty. + size int64 // On Solaris and older unix distros this is empty. isDir bool } -type fsDirents []fsDirent +// byDirentNames is a collection satisfying sort.Interface. +type byDirentNames []fsDirent -func (d fsDirents) Len() int { return len(d) } -func (d fsDirents) Swap(i, j int) { d[i], d[j] = d[j], d[i] } -func (d fsDirents) Less(i, j int) bool { +func (d byDirentNames) Len() int { return len(d) } +func (d byDirentNames) Swap(i, j int) { d[i], d[j] = d[j], d[i] } +func (d byDirentNames) Less(i, j int) bool { n1 := d[i].name if d[i].isDir { n1 = n1 + string(os.PathSeparator) @@ -41,12 +59,16 @@ func searchDirents(dirents []fsDirent, x string) int { return sort.Search(len(dirents), processFunc) } +// Tree walk result carries results of tree walking. type treeWalkResult struct { objectInfo ObjectInfo err error end bool } +// Tree walk notify carries a channel which notifies tree walk +// results, additionally it also carries information if treeWalk +// should be timedOut. type treeWalker struct { ch <-chan treeWalkResult timedOut bool @@ -58,24 +80,23 @@ func treeWalk(bucketDir, prefixDir, entryPrefixMatch, marker string, recursive b // if prefixDir="one/two/three/" and marker="four/five.txt" treeWalk is recursively // called with prefixDir="one/two/three/four/" and marker="five.txt" - // convert dirent to ObjectInfo + // Convert dirent to ObjectInfo direntToObjectInfo := func(dirent fsDirent) (ObjectInfo, error) { objectInfo := ObjectInfo{} - // objectInfo.Name has the full object name + // Convert to full object name. objectInfo.Name = filepath.Join(prefixDir, dirent.name) if dirent.modifiedTime.IsZero() && dirent.size == 0 { - // On linux/darwin/*bsd. Refer dir_nix.go:parseDirents() for details. // ModifiedTime and Size are zero, Stat() and figure out // the actual values that need to be set. fi, err := os.Stat(filepath.Join(bucketDir, prefixDir, dirent.name)) if err != nil { return ObjectInfo{}, err } + // Fill size and modtime. objectInfo.ModifiedTime = fi.ModTime() objectInfo.Size = fi.Size() objectInfo.IsDir = fi.IsDir() } else { - // On windows. Refer dir_others.go:parseDirents() for details. // If ModifiedTime or Size are set then use them // without attempting another Stat operation. objectInfo.ModifiedTime = dirent.modifiedTime @@ -83,23 +104,22 @@ func treeWalk(bucketDir, prefixDir, entryPrefixMatch, marker string, recursive b objectInfo.IsDir = dirent.isDir } if objectInfo.IsDir { - // Add os.PathSeparator suffix again as filepath would have removed it + // Add os.PathSeparator suffix again for directories as + // filepath.Join would have removed it. objectInfo.Size = 0 objectInfo.Name += string(os.PathSeparator) } return objectInfo, nil } - markerPart := "" - markerRest := "" - + var markerBase, markerDir string if marker != "" { - // ex: if marker="four/five.txt", markerPart="four/" markerRest="five.txt" + // Ex: if marker="four/five.txt", markerDir="four/" markerBase="five.txt" markerSplit := strings.SplitN(marker, string(os.PathSeparator), 2) - markerPart = markerSplit[0] + markerDir = markerSplit[0] if len(markerSplit) == 2 { - markerPart += string(os.PathSeparator) - markerRest = markerSplit[1] + markerDir += string(os.PathSeparator) + markerBase = markerSplit[1] } } @@ -110,12 +130,12 @@ func treeWalk(bucketDir, prefixDir, entryPrefixMatch, marker string, recursive b return false } // example: - // If markerPart="four/" searchDirents() returns the index of "four/" in the sorted + // If markerDir="four/" searchDirents() returns the index of "four/" in the sorted // dirents list. We skip all the dirent entries till "four/" - dirents = dirents[searchDirents(dirents, markerPart):] + dirents = dirents[searchDirents(dirents, markerDir):] *count += len(dirents) for i, dirent := range dirents { - if i == 0 && markerPart == dirent.name && !dirent.isDir { + if i == 0 && markerDir == dirent.name && !dirent.isDir { // If the first entry is not a directory // we need to skip this entry. *count-- @@ -124,9 +144,10 @@ func treeWalk(bucketDir, prefixDir, entryPrefixMatch, marker string, recursive b if dirent.isDir && recursive { // If the entry is a directory, we will need recurse into it. markerArg := "" - if dirent.name == markerPart { - // we need to pass "five.txt" as marker only if we are recursing into "four/" - markerArg = markerRest + if dirent.name == markerDir { + // We need to pass "five.txt" as marker only if we are + // recursing into "four/" + markerArg = markerBase } *count-- if !treeWalk(bucketDir, filepath.Join(prefixDir, dirent.name), "", markerArg, recursive, send, count) { @@ -148,7 +169,7 @@ func treeWalk(bucketDir, prefixDir, entryPrefixMatch, marker string, recursive b } // Initiate a new treeWalk in a goroutine. -func startTreeWalker(fsPath, bucket, prefix, marker string, recursive bool) *treeWalker { +func startTreeWalk(fsPath, bucket, prefix, marker string, recursive bool) *treeWalker { // Example 1 // If prefix is "one/two/three/" and marker is "one/two/three/four/five.txt" // treeWalk is called with prefixDir="one/two/three/" and marker="four/five.txt" @@ -158,9 +179,8 @@ func startTreeWalker(fsPath, bucket, prefix, marker string, recursive bool) *tre // if prefix is "one/two/th" and marker is "one/two/three/four/five.txt" // treeWalk is called with prefixDir="one/two/" and marker="three/four/five.txt" // and entryPrefixMatch="th" - ch := make(chan treeWalkResult, listObjectsLimit) - walker := treeWalker{ch: ch} + walkNotify := treeWalker{ch: ch} entryPrefixMatch := prefix prefixDir := "" lastIndex := strings.LastIndex(prefix, string(os.PathSeparator)) @@ -183,11 +203,11 @@ func startTreeWalker(fsPath, bucket, prefix, marker string, recursive bool) *tre case ch <- walkResult: return true case <-timer: - walker.timedOut = true + walkNotify.timedOut = true return false } } treeWalk(filepath.Join(fsPath, bucket), prefixDir, entryPrefixMatch, marker, recursive, send, &count) }() - return &walker + return &walkNotify } diff --git a/fs-dir-nix.go b/fs-dir-nix.go index 8e853f46c..1a4890efd 100644 --- a/fs-dir-nix.go +++ b/fs-dir-nix.go @@ -28,7 +28,7 @@ import ( ) const ( - // large enough buffer size for ReadDirent() syscall + // Large enough buffer size for ReadDirent() syscall readDirentBufSize = 4096 * 25 ) @@ -42,34 +42,38 @@ func clen(n []byte) int { return len(n) } -// parseDirents - inspired from syscall_.go:parseDirents() +// parseDirents - inspired from +// https://golang.org/src/syscall/syscall_.go func parseDirents(buf []byte) []fsDirent { bufidx := 0 dirents := []fsDirent{} for bufidx < len(buf) { dirent := (*syscall.Dirent)(unsafe.Pointer(&buf[bufidx])) - bufidx += int(dirent.Reclen) - if skipDirent(dirent) { - continue + // On non-Linux operating systems for rec length of zero means + // we have reached EOF break out. + if runtime.GOOS != "linux" && dirent.Reclen == 0 { + break } - if runtime.GOOS != "linux" { - if dirent.Reclen == 0 { - break - } + bufidx += int(dirent.Reclen) + // Skip dirents if they are absent in directory. + if isEmptyDirent(dirent) { + continue } bytes := (*[10000]byte)(unsafe.Pointer(&dirent.Name[0])) var name = string(bytes[0:clen(bytes[:])]) - if name == "." || name == ".." { // Useless names + // Reserved names skip them. + if name == "." || name == ".." { continue } dirents = append(dirents, fsDirent{ name: name, - isDir: dirent.Type == syscall.DT_DIR, + isDir: (dirent.Type == syscall.DT_DIR), }) } return dirents } +// Read all directory entries, returns a list of lexically sorted entries. func readDirAll(readDirPath, entryPrefixMatch string) ([]fsDirent, error) { buf := make([]byte, readDirentBufSize) f, err := os.Open(readDirPath) @@ -96,6 +100,6 @@ func readDirAll(readDirPath, entryPrefixMatch string) ([]fsDirent, error) { } } } - sort.Sort(fsDirents(dirents)) + sort.Sort(byDirentNames(dirents)) return dirents, nil } diff --git a/fs-dir-others.go b/fs-dir-others.go index 163529bb8..9e987a717 100644 --- a/fs-dir-others.go +++ b/fs-dir-others.go @@ -25,6 +25,7 @@ import ( "strings" ) +// Read all directory entries, returns a list of lexically sorted entries. func readDirAll(readDirPath, entryPrefixMatch string) ([]fsDirent, error) { f, err := os.Open(readDirPath) if err != nil { @@ -57,6 +58,6 @@ func readDirAll(readDirPath, entryPrefixMatch string) ([]fsDirent, error) { } } // Sort dirents. - sort.Sort(fsDirents(dirents)) + sort.Sort(byDirentNames(dirents)) return dirents, nil } diff --git a/fs-dirent-fileno.go b/fs-dirent-fileno.go index 8b665ac75..e13c522f9 100644 --- a/fs-dirent-fileno.go +++ b/fs-dirent-fileno.go @@ -20,6 +20,7 @@ package main import "syscall" -func skipDirent(dirent *syscall.Dirent) bool { +// True if dirent is absent in directory. +func isEmptyDirent(dirent *syscall.Dirent) bool { return dirent.Fileno == 0 } diff --git a/fs-dirent-ino.go b/fs-dirent-ino.go index f8560e5a6..3e6145131 100644 --- a/fs-dirent-ino.go +++ b/fs-dirent-ino.go @@ -20,6 +20,7 @@ package main import "syscall" -func skipDirent(dirent *syscall.Dirent) bool { +// True if dirent is absent in directory. +func isEmptyDirent(dirent *syscall.Dirent) bool { return dirent.Ino == 0 } diff --git a/fs.go b/fs.go index 1165fc369..93ffcc2f5 100644 --- a/fs.go +++ b/fs.go @@ -58,41 +58,6 @@ type multiparts struct { ActiveSession map[string]*multipartSession `json:"activeSessions"` } -func (fs *Filesystem) pushTreeWalker(params listObjectParams, walker *treeWalker) { - fs.listObjectMapMutex.Lock() - defer fs.listObjectMapMutex.Unlock() - - walkers, _ := fs.listObjectMap[params] - walkers = append(walkers, walker) - - fs.listObjectMap[params] = walkers -} - -func (fs *Filesystem) popTreeWalker(params listObjectParams) *treeWalker { - fs.listObjectMapMutex.Lock() - defer fs.listObjectMapMutex.Unlock() - - if walkers, ok := fs.listObjectMap[params]; ok { - for i, walker := range walkers { - if !walker.timedOut { - newWalkers := walkers[i+1:] - if len(newWalkers) > 0 { - fs.listObjectMap[params] = newWalkers - } else { - delete(fs.listObjectMap, params) - } - - return walker - } - } - - // As all channels are timed out, delete the map entry - delete(fs.listObjectMap, params) - } - - return nil -} - // newFS instantiate a new filesystem. func newFS(rootPath string) (ObjectAPI, *probe.Error) { setFSMultipartsMetadataPath(filepath.Join(rootPath, "$multiparts-session.json")) diff --git a/object-handlers.go b/object-handlers.go index a19cd50c3..6f127b13a 100644 --- a/object-handlers.go +++ b/object-handlers.go @@ -34,12 +34,7 @@ import ( "github.com/minio/minio/pkg/probe" ) -const ( - maxPartsList = 1000 -) - -// supportedGetReqParams - supported request parameters for GET -// presigned request. +// supportedGetReqParams - supported request parameters for GET presigned request. var supportedGetReqParams = map[string]string{ "response-expires": "Expires", "response-content-type": "Content-Type",