From b6eb85631c17210ab0418fc6184008691bc9172b Mon Sep 17 00:00:00 2001 From: mpl Date: Wed, 18 Feb 2015 18:30:13 +0100 Subject: [PATCH] blob SubFetcher: explicitely states with errors the testSubFetcher constraints testSubFetcher in blobserver/storagetest was already checking that we'd get specific error messages in the case of negative input parameters or an out of range offset. This change rationalizes these constraints with named errors (ErrNegativeSubFetch and ErrOutOfRangeOffsetSubFetch) specified in the SubFetcher interface. It also fixes the googlestorage and s3 implementations so that they pass the aforementioned test. Change-Id: I25b72b842855b90ee3cab44c90654581dccf4b8e --- pkg/blob/fetcher.go | 11 +++++++++-- pkg/blobserver/blobpacked/subfetch.go | 5 ++--- pkg/blobserver/interface.go | 8 +++++--- pkg/blobserver/localdisk/localdisk.go | 8 +++++--- pkg/blobserver/memory/mem.go | 5 ++--- pkg/blobserver/storagetest/storagetest.go | 4 ++-- pkg/googlestorage/googlestorage.go | 10 +++++++--- pkg/misc/amazon/s3/client.go | 4 ++++ 8 files changed, 36 insertions(+), 19 deletions(-) diff --git a/pkg/blob/fetcher.go b/pkg/blob/fetcher.go index d547ce433..a2aaada61 100644 --- a/pkg/blob/fetcher.go +++ b/pkg/blob/fetcher.go @@ -27,6 +27,11 @@ import ( "camlistore.org/pkg/types" ) +var ( + ErrNegativeSubFetch = errors.New("invalid negative subfetch parameters") + ErrOutOfRangeOffsetSubFetch = errors.New("subfetch offset greater than blob size") +) + // Fetcher is the minimal interface for retrieving a blob from storage. // The full storage interface is blobserver.Storage. type Fetcher interface { @@ -47,8 +52,10 @@ type SubFetcher interface { // SubFetch returns part of a blob. // The caller must close the returned io.ReadCloser. // The Reader may return fewer than 'length' bytes. Callers should - // check. The returned error should be os.ErrNotExist if the blob - // doesn't exist. + // check. The returned error should be: ErrNegativeSubFetch if any of + // offset or length is negative, or os.ErrNotExist if the blob + // doesn't exist, or ErrOutOfRangeOffsetSubFetch if offset goes over + // the size of the blob. SubFetch(ref Ref, offset, length int64) (io.ReadCloser, error) } diff --git a/pkg/blobserver/blobpacked/subfetch.go b/pkg/blobserver/blobpacked/subfetch.go index d457a1281..8d983f8ed 100644 --- a/pkg/blobserver/blobpacked/subfetch.go +++ b/pkg/blobserver/blobpacked/subfetch.go @@ -17,7 +17,6 @@ limitations under the License. package blobpacked import ( - "errors" "io" "io/ioutil" @@ -71,10 +70,10 @@ func (s *storage) SubFetch(ref blob.Ref, offset, length int64) (io.ReadCloser, e func capOffsetLength(size uint32, offset, length int64) (newLength int64, err error) { if offset < 0 || length < 0 { - return 0, errors.New("invalid negative subfetch parameters") + return 0, blob.ErrNegativeSubFetch } if offset > int64(size) { - return 0, errors.New("subfetch offset greater than blob size") + return 0, blob.ErrOutOfRangeOffsetSubFetch } if over := (offset + length) - int64(size); over > 0 { length -= over diff --git a/pkg/blobserver/interface.go b/pkg/blobserver/interface.go index c8de30938..8069375ce 100644 --- a/pkg/blobserver/interface.go +++ b/pkg/blobserver/interface.go @@ -31,10 +31,12 @@ import ( // MaxBlobSize is the size of a single blob in Camlistore. const MaxBlobSize = constants.MaxBlobSize -var ErrCorruptBlob = errors.New("corrupt blob; digest doesn't match") +var ( + ErrCorruptBlob = errors.New("corrupt blob; digest doesn't match") -// ErrNotImplemented should be returned in methods where the function is not implemented -var ErrNotImplemented = errors.New("not implemented") + // ErrNotImplemented should be returned in methods where the function is not implemented + ErrNotImplemented = errors.New("not implemented") +) // BlobReceiver is the interface for receiving type BlobReceiver interface { diff --git a/pkg/blobserver/localdisk/localdisk.go b/pkg/blobserver/localdisk/localdisk.go index 2268483b5..7a87bcc2b 100644 --- a/pkg/blobserver/localdisk/localdisk.go +++ b/pkg/blobserver/localdisk/localdisk.go @@ -31,7 +31,6 @@ Example low-level config: package localdisk import ( - "errors" "fmt" "io" "os" @@ -131,7 +130,7 @@ func (ds *DiskStorage) Fetch(br blob.Ref) (io.ReadCloser, uint32, error) { func (ds *DiskStorage) SubFetch(br blob.Ref, offset, length int64) (io.ReadCloser, error) { if offset < 0 || length < 0 { - return nil, errors.New("invalid offset or length") + return nil, blob.ErrNegativeSubFetch } rc, _, err := ds.fetch(br, offset, length) return rc, err @@ -158,7 +157,10 @@ func (ds *DiskStorage) fetch(br blob.Ref, offset, length int64) (rc io.ReadClose } // SubFetch: if offset < 0 || offset > stat.Size() { - return nil, 0, errors.New("invalid offset") + if offset < 0 { + return nil, 0, blob.ErrNegativeSubFetch + } + return nil, 0, blob.ErrOutOfRangeOffsetSubFetch } return struct { io.Reader diff --git a/pkg/blobserver/memory/mem.go b/pkg/blobserver/memory/mem.go index 40e292181..1de74b4f4 100644 --- a/pkg/blobserver/memory/mem.go +++ b/pkg/blobserver/memory/mem.go @@ -20,7 +20,6 @@ package memory import ( "bytes" - "errors" "fmt" "io" "io/ioutil" @@ -109,7 +108,7 @@ func (s *Storage) Fetch(ref blob.Ref) (file io.ReadCloser, size uint32, err erro func (s *Storage) SubFetch(ref blob.Ref, offset, length int64) (io.ReadCloser, error) { if offset < 0 || length < 0 { - return nil, errors.New("invalid negative subfetch parameters") + return nil, blob.ErrNegativeSubFetch } s.mu.RLock() defer s.mu.RUnlock() @@ -118,7 +117,7 @@ func (s *Storage) SubFetch(ref blob.Ref, offset, length int64) (io.ReadCloser, e return nil, os.ErrNotExist } if offset > int64(len(b)) { - return nil, errors.New("subfetch offset greater than blob size") + return nil, blob.ErrOutOfRangeOffsetSubFetch } atomic.AddInt64(&s.blobsFetched, 1) atomic.AddInt64(&s.bytesFetched, length) diff --git a/pkg/blobserver/storagetest/storagetest.go b/pkg/blobserver/storagetest/storagetest.go index f8ab84029..f00af8d8a 100644 --- a/pkg/blobserver/storagetest/storagetest.go +++ b/pkg/blobserver/storagetest/storagetest.go @@ -233,10 +233,10 @@ func (r *run) testSubFetcher() { r, err := sf.SubFetch(big.BlobRef(), tt.off, tt.limit) if err == nil { r.Close() - t.Errorf("No error fetching with off=%d limit=%d; wanted and error", tt.off, tt.limit) + t.Errorf("No error fetching with off=%d limit=%d; wanted an error", tt.off, tt.limit) continue } - if v := err.Error(); !strings.Contains(v, "negative") && !strings.Contains(v, "offset") { + if err != blob.ErrNegativeSubFetch && err != blob.ErrOutOfRangeOffsetSubFetch { t.Errorf("Unexpected error fetching with off=%d limit=%d: %v", tt.off, tt.limit, err) } } diff --git a/pkg/googlestorage/googlestorage.go b/pkg/googlestorage/googlestorage.go index 48471bfe6..a3beb4b9f 100644 --- a/pkg/googlestorage/googlestorage.go +++ b/pkg/googlestorage/googlestorage.go @@ -33,10 +33,11 @@ import ( "strings" "unicode/utf8" + "camlistore.org/pkg/blob" "camlistore.org/pkg/httputil" "camlistore.org/third_party/code.google.com/p/goauth2/oauth" - api "camlistore.org/third_party/google.golang.org/api/storage/v1" "camlistore.org/third_party/github.com/bradfitz/gce" + api "camlistore.org/third_party/google.golang.org/api/storage/v1" ) const ( @@ -186,8 +187,8 @@ func (c *Client) GetObject(obj *Object) (rc io.ReadCloser, size int64, err error // If length is negative, the rest of the object is returned. // The caller must close rc. func (c *Client) GetPartialObject(obj Object, offset, length int64) (rc io.ReadCloser, err error) { - if offset < 0 { - return nil, errors.New("invalid negative length") + if offset < 0 || length < 0 { + return nil, blob.ErrNegativeSubFetch } if err = obj.valid(); err != nil { return @@ -214,6 +215,9 @@ func (c *Client) GetPartialObject(obj Object, offset, length int64) (rc io.ReadC } if !(resp.StatusCode == http.StatusPartialContent || (offset == 0 && resp.StatusCode == http.StatusOK)) { resp.Body.Close() + if resp.StatusCode == http.StatusRequestedRangeNotSatisfiable { + return nil, blob.ErrOutOfRangeOffsetSubFetch + } return nil, fmt.Errorf("GS GET request failed status: %v\n", resp.Status) } diff --git a/pkg/misc/amazon/s3/client.go b/pkg/misc/amazon/s3/client.go index 54b107428..a7b44e95a 100644 --- a/pkg/misc/amazon/s3/client.go +++ b/pkg/misc/amazon/s3/client.go @@ -36,6 +36,7 @@ import ( "strings" "time" + "camlistore.org/pkg/blob" "camlistore.org/pkg/httputil" ) @@ -324,6 +325,9 @@ func (c *Client) GetPartial(bucket, key string, offset, length int64) (rc io.Rea case http.StatusNotFound: res.Body.Close() return nil, os.ErrNotExist + case http.StatusRequestedRangeNotSatisfiable: + res.Body.Close() + return nil, blob.ErrOutOfRangeOffsetSubFetch default: res.Body.Close() return nil, fmt.Errorf("Amazon HTTP error on GET: %d", res.StatusCode)