From 7e6b5bdbb7f7eceaa001e668bdab7c2aaad68dc6 Mon Sep 17 00:00:00 2001
From: Andreas Auernhammer <aead@mail.de>
Date: Mon, 25 Sep 2017 11:32:56 -0700
Subject: [PATCH] remove ReadFileWithVerify from StorageAPI (#4947)

This change removes the ReadFileWithVerify function from the
StorageAPI. The ReadFile was basically a redirection to ReadFileWithVerify.
This change removes the redirection and moves the logic of
ReadFileWithVerify directly into ReadFile.
This removes a lot of unnecessary code in all StorageAPI implementations.

Fixes #4946

* review: fix doc and typos
---
 cmd/erasure-healfile.go             |  8 +----
 cmd/erasure-readfile.go             |  7 +----
 cmd/erasure-readfile_test.go        |  6 +---
 cmd/naughty-disk_test.go            | 11 ++-----
 cmd/posix.go                        | 23 ++++----------
 cmd/posix_test.go                   | 12 ++++----
 cmd/retry-storage.go                | 17 ++--------
 cmd/retry-storage_test.go           |  6 ++--
 cmd/storage-interface.go            |  5 ++-
 cmd/storage-rpc-client.go           | 48 +++++++++--------------------
 cmd/storage-rpc-client_test.go      |  6 ++--
 cmd/storage-rpc-server-datatypes.go | 21 ++-----------
 cmd/storage-rpc-server.go           | 23 +++-----------
 13 files changed, 50 insertions(+), 143 deletions(-)

diff --git a/cmd/erasure-healfile.go b/cmd/erasure-healfile.go
index 79d598535..b993416fd 100644
--- a/cmd/erasure-healfile.go
+++ b/cmd/erasure-healfile.go
@@ -95,13 +95,7 @@ func (s ErasureStorage) HealFile(staleDisks []StorageAPI, volume, path string,
 			if int64(len(buffer)) != chunksize {
 				buffer = make([]byte, chunksize)
 			}
-			if !verifiers[i].IsVerified() {
-				_, err = disk.ReadFileWithVerify(volume, path,
-					chunkOffset, buffer, verifiers[i])
-			} else {
-				_, err = disk.ReadFile(volume, path,
-					chunkOffset, buffer)
-			}
+			_, err = disk.ReadFile(volume, path, chunkOffset, buffer, verifiers[i])
 			if err != nil {
 				// LOG FIXME: add a conditional log
 				// for read failures, once per-disk
diff --git a/cmd/erasure-readfile.go b/cmd/erasure-readfile.go
index b054754cb..b66c09441 100644
--- a/cmd/erasure-readfile.go
+++ b/cmd/erasure-readfile.go
@@ -157,11 +157,6 @@ func erasureReadFromFile(disk StorageAPI, volume, path string, offset int64, buf
 		errChan <- traceError(errDiskNotFound)
 		return
 	}
-	var err error
-	if !verifier.IsVerified() {
-		_, err = disk.ReadFileWithVerify(volume, path, offset, buffer, verifier)
-	} else {
-		_, err = disk.ReadFile(volume, path, offset, buffer)
-	}
+	_, err := disk.ReadFile(volume, path, offset, buffer, verifier)
 	errChan <- err
 }
diff --git a/cmd/erasure-readfile_test.go b/cmd/erasure-readfile_test.go
index be1febfbc..a17a87532 100644
--- a/cmd/erasure-readfile_test.go
+++ b/cmd/erasure-readfile_test.go
@@ -27,11 +27,7 @@ import (
 	"github.com/minio/minio/pkg/bpool"
 )
 
-func (d badDisk) ReadFile(volume string, path string, offset int64, buf []byte) (n int64, err error) {
-	return 0, errFaultyDisk
-}
-
-func (d badDisk) ReadFileWithVerify(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error) {
+func (d badDisk) ReadFile(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error) {
 	return 0, errFaultyDisk
 }
 
diff --git a/cmd/naughty-disk_test.go b/cmd/naughty-disk_test.go
index a018bad59..a8b2c36c2 100644
--- a/cmd/naughty-disk_test.go
+++ b/cmd/naughty-disk_test.go
@@ -115,18 +115,11 @@ func (d *naughtyDisk) ListDir(volume, path string) (entries []string, err error)
 	return d.disk.ListDir(volume, path)
 }
 
-func (d *naughtyDisk) ReadFile(volume string, path string, offset int64, buf []byte) (n int64, err error) {
+func (d *naughtyDisk) ReadFile(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error) {
 	if err := d.calcError(); err != nil {
 		return 0, err
 	}
-	return d.disk.ReadFile(volume, path, offset, buf)
-}
-
-func (d *naughtyDisk) ReadFileWithVerify(volume, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error) {
-	if err := d.calcError(); err != nil {
-		return 0, err
-	}
-	return d.disk.ReadFileWithVerify(volume, path, offset, buf, verifier)
+	return d.disk.ReadFile(volume, path, offset, buf, verifier)
 }
 
 func (d *naughtyDisk) PrepareFile(volume, path string, length int64) error {
diff --git a/cmd/posix.go b/cmd/posix.go
index 2e531655e..29672e13b 100644
--- a/cmd/posix.go
+++ b/cmd/posix.go
@@ -533,25 +533,14 @@ func (s *posix) ReadAll(volume, path string) (buf []byte, err error) {
 // for io.EOF.
 //
 // If an EOF happens after reading some but not all the bytes,
-// ReadFull returns ErrUnexpectedEOF.
+// ReadFile returns ErrUnexpectedEOF.
+//
+// If the BitrotVerifier is not nil or not verified ReadFile
+// tries to verify whether the disk has bitrot.
 //
 // Additionally ReadFile also starts reading from an offset. ReadFile
 // semantics are same as io.ReadFull.
-func (s *posix) ReadFile(volume, path string, offset int64, buf []byte) (n int64, err error) {
-	return s.ReadFileWithVerify(volume, path, offset, buf, &BitrotVerifier{verified: true})
-}
-
-// ReadFileWithVerify is the same as ReadFile but with hashsum
-// verification: the operation will fail if the hash verification
-// fails.
-//
-// The `expectedHash` is the expected hex-encoded hash string for
-// verification. With an empty expected hash string, hash verification
-// is skipped. An empty HashAlgo defaults to `blake2b`.
-//
-// The function takes care to minimize the number of disk read
-// operations.
-func (s *posix) ReadFileWithVerify(volume, path string, offset int64, buffer []byte, verifier *BitrotVerifier) (n int64, err error) {
+func (s *posix) ReadFile(volume, path string, offset int64, buffer []byte, verifier *BitrotVerifier) (n int64, err error) {
 	defer func() {
 		if err == syscall.EIO {
 			atomic.AddInt32(&s.ioErrCount, 1)
@@ -612,7 +601,7 @@ func (s *posix) ReadFileWithVerify(volume, path string, offset int64, buffer []b
 		return 0, errIsNotRegular
 	}
 
-	if !verifier.IsVerified() {
+	if verifier != nil && !verifier.IsVerified() {
 		bufp := s.pool.Get().(*[]byte)
 		defer s.pool.Put(bufp)
 
diff --git a/cmd/posix_test.go b/cmd/posix_test.go
index d366fce75..952705335 100644
--- a/cmd/posix_test.go
+++ b/cmd/posix_test.go
@@ -1074,7 +1074,7 @@ func TestPosixReadFile(t *testing.T) {
 		var n int64
 		// Common read buffer.
 		var buf = make([]byte, testCase.bufSize)
-		n, err = posixStorage.ReadFile(testCase.volume, testCase.fileName, testCase.offset, buf)
+		n, err = posixStorage.ReadFile(testCase.volume, testCase.fileName, testCase.offset, buf, nil)
 		if err != nil && testCase.expectedErr != nil {
 			// Validate if the type string of the errors are an exact match.
 			if err.Error() != testCase.expectedErr.Error() {
@@ -1135,7 +1135,7 @@ func TestPosixReadFile(t *testing.T) {
 		if err == nil {
 			// Common read buffer.
 			var buf = make([]byte, 10)
-			if _, err = posixStorage.ReadFile("proc", "1/fd", 0, buf); err != errFileAccessDenied {
+			if _, err = posixStorage.ReadFile("proc", "1/fd", 0, buf, nil); err != errFileAccessDenied {
 				t.Errorf("expected: %s, got: %s", errFileAccessDenied, err)
 			}
 		}
@@ -1149,7 +1149,7 @@ func TestPosixReadFile(t *testing.T) {
 		posixType.ioErrCount = int32(6)
 		// Common read buffer.
 		var buf = make([]byte, 10)
-		_, err = posixType.ReadFile("abc", "yes", 0, buf)
+		_, err = posixType.ReadFile("abc", "yes", 0, buf, nil)
 		if err != errFaultyDisk {
 			t.Fatalf("Expected \"Faulty Disk\", got: \"%s\"", err)
 		}
@@ -1183,8 +1183,8 @@ var posixReadFileWithVerifyTests = []struct {
 	{file: "myobject", offset: 1000, length: 1001, algorithm: BLAKE2b512, expError: nil},             // 15
 }
 
-// TestPosixReadFileWithVerify - tests the posix level
-// ReadFileWithVerify API. Only tests hashing related
+// TestPosixReadFile with bitrot verification - tests the posix level
+// ReadFile API with a BitrotVerifier. Only tests hashing related
 // functionality. Other functionality is tested with
 // TestPosixReadFile.
 func TestPosixReadFileWithVerify(t *testing.T) {
@@ -1218,7 +1218,7 @@ func TestPosixReadFileWithVerify(t *testing.T) {
 		}
 
 		buffer := make([]byte, test.length)
-		n, err := posixStorage.ReadFileWithVerify(volume, test.file, int64(test.offset), buffer, NewBitrotVerifier(test.algorithm, h.Sum(nil)))
+		n, err := posixStorage.ReadFile(volume, test.file, int64(test.offset), buffer, NewBitrotVerifier(test.algorithm, h.Sum(nil)))
 
 		switch {
 		case err == nil && test.expError != nil:
diff --git a/cmd/retry-storage.go b/cmd/retry-storage.go
index 14a5d0ecb..a75e02e61 100644
--- a/cmd/retry-storage.go
+++ b/cmd/retry-storage.go
@@ -207,29 +207,18 @@ func (f *retryStorage) ReadAll(volume, path string) (buf []byte, err error) {
 }
 
 // ReadFile - a retryable implementation of reading at offset from a file.
-func (f *retryStorage) ReadFile(volume, path string, offset int64, buffer []byte) (m int64, err error) {
+func (f *retryStorage) ReadFile(volume, path string, offset int64, buffer []byte, verifier *BitrotVerifier) (m int64, err error) {
 	if f.IsOffline() {
 		return m, errDiskNotFound
 	}
-	m, err = f.remoteStorage.ReadFile(volume, path, offset, buffer)
+	m, err = f.remoteStorage.ReadFile(volume, path, offset, buffer, verifier)
 	if f.reInitUponDiskNotFound(err) {
-		m, err = f.remoteStorage.ReadFile(volume, path, offset, buffer)
+		m, err = f.remoteStorage.ReadFile(volume, path, offset, buffer, verifier)
 		return m, retryToStorageErr(err)
 	}
 	return m, retryToStorageErr(err)
 }
 
-// ReadFileWithVerify - a retryable implementation of reading at
-// offset from a file with verification.
-func (f retryStorage) ReadFileWithVerify(volume, path string, offset int64, buffer []byte, verifier *BitrotVerifier) (m int64, err error) {
-
-	m, err = f.remoteStorage.ReadFileWithVerify(volume, path, offset, buffer, verifier)
-	if f.reInitUponDiskNotFound(err) {
-		m, err = f.remoteStorage.ReadFileWithVerify(volume, path, offset, buffer, verifier)
-	}
-	return m, retryToStorageErr(err)
-}
-
 // ListDir - a retryable implementation of listing directory entries.
 func (f *retryStorage) ListDir(volume, path string) (entries []string, err error) {
 	if f.IsOffline() {
diff --git a/cmd/retry-storage_test.go b/cmd/retry-storage_test.go
index 263427720..28d9af5c0 100644
--- a/cmd/retry-storage_test.go
+++ b/cmd/retry-storage_test.go
@@ -290,7 +290,7 @@ func TestRetryStorage(t *testing.T) {
 	for _, disk := range storageDisks {
 		var buf2 = make([]byte, 5)
 		var n int64
-		if n, err = disk.ReadFile("existent", "path", 7, buf2); err != nil {
+		if n, err = disk.ReadFile("existent", "path", 7, buf2, nil); err != nil {
 			t.Fatal(err)
 		}
 		if err != nil {
@@ -312,11 +312,11 @@ func TestRetryStorage(t *testing.T) {
 		var buf2 = make([]byte, 5)
 		verifier := NewBitrotVerifier(SHA256, sha256Hash([]byte("Hello, World")))
 		var n int64
-		if n, err = disk.ReadFileWithVerify("existent", "path", 7, buf2, verifier); err != nil {
+		if n, err = disk.ReadFile("existent", "path", 7, buf2, verifier); err != nil {
 			t.Fatal(err)
 		}
 		if err != nil {
-			t.Error("Error in ReadFileWithVerify", err)
+			t.Error("Error in ReadFile with bitrot verification", err)
 		}
 		if n != 5 {
 			t.Fatalf("Expected 5, got %d", n)
diff --git a/cmd/storage-interface.go b/cmd/storage-interface.go
index 52b1d1f16..597dd8cc8 100644
--- a/cmd/storage-interface.go
+++ b/cmd/storage-interface.go
@@ -40,8 +40,7 @@ type StorageAPI interface {
 
 	// File operations.
 	ListDir(volume, dirPath string) ([]string, error)
-	ReadFile(volume string, path string, offset int64, buf []byte) (n int64, err error)
-	ReadFileWithVerify(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error)
+	ReadFile(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error)
 	PrepareFile(volume string, path string, len int64) (err error)
 	AppendFile(volume string, path string, buf []byte) (err error)
 	RenameFile(srcVolume, srcPath, dstVolume, dstPath string) error
@@ -60,7 +59,7 @@ type storageReader struct {
 }
 
 func (r *storageReader) Read(p []byte) (n int, err error) {
-	nn, err := r.storage.ReadFile(r.volume, r.path, r.offset, p)
+	nn, err := r.storage.ReadFile(r.volume, r.path, r.offset, p, nil)
 	r.offset += nn
 	n = int(nn)
 
diff --git a/cmd/storage-rpc-client.go b/cmd/storage-rpc-client.go
index f5e627f2a..b39f11481 100644
--- a/cmd/storage-rpc-client.go
+++ b/cmd/storage-rpc-client.go
@@ -238,7 +238,7 @@ func (n *networkStorage) ReadAll(volume, path string) (buf []byte, err error) {
 }
 
 // ReadFile - reads a file at remote path and fills the buffer.
-func (n *networkStorage) ReadFile(volume string, path string, offset int64, buffer []byte) (m int64, err error) {
+func (n *networkStorage) ReadFile(volume string, path string, offset int64, buffer []byte, verifier *BitrotVerifier) (m int64, err error) {
 	defer func() {
 		if r := recover(); r != nil {
 			// Recover any panic from allocation, and return error.
@@ -246,41 +246,21 @@ func (n *networkStorage) ReadFile(volume string, path string, offset int64, buff
 		}
 	}() // Do not crash the server.
 
-	var result []byte
-	err = n.rpcClient.Call("Storage.ReadFileHandler", &ReadFileArgs{
-		Vol:    volume,
-		Path:   path,
-		Offset: offset,
-		Buffer: buffer,
-	}, &result)
-
-	// Copy results to buffer.
-	copy(buffer, result)
-
-	// Return length of result, err if any.
-	return int64(len(result)), toStorageErr(err)
-}
-
-// ReadFileWithVerify - reads a file at remote path and fills the buffer.
-func (n *networkStorage) ReadFileWithVerify(volume string, path string, offset int64, buffer []byte, verifier *BitrotVerifier) (m int64, err error) {
-
-	defer func() {
-		if r := recover(); r != nil {
-			// Recover any panic from allocation, and return error.
-			err = bytes.ErrTooLarge
-		}
-	}() // Do not crash the server.
+	args := ReadFileArgs{
+		Vol:      volume,
+		Path:     path,
+		Offset:   offset,
+		Buffer:   buffer,
+		Verified: true, // mark read as verified by default
+	}
+	if verifier != nil {
+		args.Algo = verifier.algorithm
+		args.ExpectedHash = verifier.sum
+		args.Verified = verifier.IsVerified()
+	}
 
 	var result []byte
-	err = n.rpcClient.Call("Storage.ReadFileWithVerifyHandler",
-		&ReadFileWithVerifyArgs{
-			Vol:          volume,
-			Path:         path,
-			Offset:       offset,
-			Buffer:       buffer,
-			Algo:         verifier.algorithm,
-			ExpectedHash: verifier.sum,
-		}, &result)
+	err = n.rpcClient.Call("Storage.ReadFileHandler", &args, &result)
 
 	// Copy results to buffer.
 	copy(buffer, result)
diff --git a/cmd/storage-rpc-client_test.go b/cmd/storage-rpc-client_test.go
index 54ba82bad..eda9b5150 100644
--- a/cmd/storage-rpc-client_test.go
+++ b/cmd/storage-rpc-client_test.go
@@ -379,7 +379,7 @@ func (s *TestRPCStorageSuite) testRPCStorageFileOps(t *testing.T) {
 			t.Errorf("Expected `Hello, world`, got %s", string(buf))
 		}
 		buf1 := make([]byte, 5)
-		n, err := storageDisk.ReadFile("myvol", "file1", 4, buf1)
+		n, err := storageDisk.ReadFile("myvol", "file1", 4, buf1, nil)
 		if err != nil {
 			t.Error("Unable to initiate ReadFile", err)
 		}
@@ -396,9 +396,9 @@ func (s *TestRPCStorageSuite) testRPCStorageFileOps(t *testing.T) {
 		}
 		verifier := NewBitrotVerifier(BLAKE2b512, blakeHash(buf))
 		buf2 := make([]byte, 2)
-		n, err = storageDisk.ReadFileWithVerify("myvol", "file1", 1, buf2, verifier)
+		n, err = storageDisk.ReadFile("myvol", "file1", 1, buf2, verifier)
 		if err != nil {
-			t.Error("Error in ReadFileWithVerify", err)
+			t.Error("Error in ReadFile with bitrot verification", err)
 		}
 		if n != 2 {
 			t.Errorf("Expected `2`, got %d", n)
diff --git a/cmd/storage-rpc-server-datatypes.go b/cmd/storage-rpc-server-datatypes.go
index 00c51254c..b417e3da1 100644
--- a/cmd/storage-rpc-server-datatypes.go
+++ b/cmd/storage-rpc-server-datatypes.go
@@ -59,30 +59,15 @@ type ReadFileArgs struct {
 
 	// Data buffer read from the path at offset.
 	Buffer []byte
-}
-
-// ReadFileWithVerifyArgs represents read file RPC arguments.
-type ReadFileWithVerifyArgs struct {
-	// Authentication token generated by Login.
-	AuthRPCArgs
-
-	// Name of the volume.
-	Vol string
-
-	// Name of the path.
-	Path string
-
-	// Starting offset to start reading into Buffer.
-	Offset int64
-
-	// Data buffer read from the path at offset.
-	Buffer []byte
 
 	// Algorithm used in bit-rot hash computation.
 	Algo BitrotAlgorithm
 
 	// Stored hash value used to compare with computed value.
 	ExpectedHash []byte
+
+	// Indicates whether the disk has already been verified
+	Verified bool
 }
 
 // PrepareFileArgs represents append file RPC arguments.
diff --git a/cmd/storage-rpc-server.go b/cmd/storage-rpc-server.go
index 8666d20ff..cc206b439 100644
--- a/cmd/storage-rpc-server.go
+++ b/cmd/storage-rpc-server.go
@@ -145,26 +145,13 @@ func (s *storageServer) ReadFileHandler(args *ReadFileArgs, reply *[]byte) (err
 	if err = args.IsAuthenticated(); err != nil {
 		return err
 	}
+	var verifier *BitrotVerifier
+	if !args.Verified {
+		verifier = NewBitrotVerifier(args.Algo, args.ExpectedHash)
+	}
 
 	var n int64
-	n, err = s.storage.ReadFile(args.Vol, args.Path, args.Offset, args.Buffer)
-	// Sending an error over the rpc layer, would cause unmarshalling to fail. In situations
-	// when we have short read i.e `io.ErrUnexpectedEOF` treat it as good condition and copy
-	// the buffer properly.
-	if err == io.ErrUnexpectedEOF {
-		// Reset to nil as good condition.
-		err = nil
-	}
-	*reply = args.Buffer[0:n]
-	return err
-}
-
-// ReadFileWithVerifyHandler - read file with verify handler is rpc wrapper to read file with verify.
-func (s *storageServer) ReadFileWithVerifyHandler(args *ReadFileWithVerifyArgs, reply *[]byte) (err error) {
-	if err = args.IsAuthenticated(); err != nil {
-		return err
-	}
-	n, err := s.storage.ReadFileWithVerify(args.Vol, args.Path, args.Offset, args.Buffer, NewBitrotVerifier(args.Algo, args.ExpectedHash))
+	n, err = s.storage.ReadFile(args.Vol, args.Path, args.Offset, args.Buffer, verifier)
 	// Sending an error over the rpc layer, would cause unmarshalling to fail. In situations
 	// when we have short read i.e `io.ErrUnexpectedEOF` treat it as good condition and copy
 	// the buffer properly.