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

@@ -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
 }

api-response.go

@@ -23,10 +23,10 @@ import (
 )
 
 const (
-	// Reply date format
+	timeFormatAMZ  = "2006-01-02T15:04:05.000Z" // Reply date format
-	timeFormatAMZ = "2006-01-02T15:04:05.000Z"
+	maxObjectList  = 1000                       // Limit number of objects in a listObjectsResponse.
-	// Limit number of objects in a given response.
+	maxUploadsList = 1000                       // Limit number of uploads in a listUploadsResponse.
-	maxObjectList = 1000
+	maxPartsList   = 1000                       // Limit number of parts in a listPartsResponse.
 )
 
 // LocationResponse - format for location response.

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 {
+	// Verify if delimiter is anything other than '/', which we do not support.
-		maxkeys = maxObjectList
+	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)

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
 }

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},

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.
+	modifiedTime time.Time // On Solaris and older unix distros this is empty.
-	size         int64     // On unix 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 byDirentNames) Len() int      { return len(d) }
-func (d fsDirents) Swap(i, j int) { d[i], d[j] = d[j], d[i] }
+func (d byDirentNames) Swap(i, j int) { d[i], d[j] = d[j], d[i] }
-func (d fsDirents) Less(i, j int) bool {
+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 := ""
+	var markerBase, markerDir string
-	markerRest := ""
-
 	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)
+			markerDir += string(os.PathSeparator)
-			markerRest = markerSplit[1]
+			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 {
+			if dirent.name == markerDir {
-				// we need to pass "five.txt" as marker only if we are recursing into "four/"
+				// We need to pass "five.txt" as marker only if we are
-				markerArg = markerRest
+				// 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
 }

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_<os>.go:parseDirents()
+// parseDirents - inspired from
+// https://golang.org/src/syscall/syscall_<os>.go
 func parseDirents(buf []byte) []fsDirent {
 	bufidx := 0
 	dirents := []fsDirent{}
 	for bufidx < len(buf) {
 		dirent := (*syscall.Dirent)(unsafe.Pointer(&buf[bufidx]))
+		// 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
+		}
 		bufidx += int(dirent.Reclen)
-		if skipDirent(dirent) {
+		// Skip dirents if they are absent in directory.
+		if isEmptyDirent(dirent) {
 			continue
 		}
-		if runtime.GOOS != "linux" {
-			if dirent.Reclen == 0 {
-				break
-			}
-		}
 		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
 }

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
 }

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
 }

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
 }

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"))

object-handlers.go

@@ -34,12 +34,7 @@ import (
 	"github.com/minio/minio/pkg/probe"
 )
 
-const (
+// supportedGetReqParams - supported request parameters for GET presigned request.
-	maxPartsList = 1000
-)
-
-// supportedGetReqParams - supported request parameters for GET
-// presigned request.
 var supportedGetReqParams = map[string]string{
 	"response-expires":             "Expires",
 	"response-content-type":        "Content-Type",