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
This commit is contained in:
Andreas Auernhammer 2017-09-25 11:32:56 -07:00 committed by Dee Koder
parent 4cadb33da2
commit 7e6b5bdbb7
13 changed files with 50 additions and 143 deletions

View File

@ -95,13 +95,7 @@ func (s ErasureStorage) HealFile(staleDisks []StorageAPI, volume, path string,
if int64(len(buffer)) != chunksize { if int64(len(buffer)) != chunksize {
buffer = make([]byte, chunksize) buffer = make([]byte, chunksize)
} }
if !verifiers[i].IsVerified() { _, err = disk.ReadFile(volume, path, chunkOffset, buffer, verifiers[i])
_, err = disk.ReadFileWithVerify(volume, path,
chunkOffset, buffer, verifiers[i])
} else {
_, err = disk.ReadFile(volume, path,
chunkOffset, buffer)
}
if err != nil { if err != nil {
// LOG FIXME: add a conditional log // LOG FIXME: add a conditional log
// for read failures, once per-disk // for read failures, once per-disk

View File

@ -157,11 +157,6 @@ func erasureReadFromFile(disk StorageAPI, volume, path string, offset int64, buf
errChan <- traceError(errDiskNotFound) errChan <- traceError(errDiskNotFound)
return return
} }
var err error _, err := disk.ReadFile(volume, path, offset, buffer, verifier)
if !verifier.IsVerified() {
_, err = disk.ReadFileWithVerify(volume, path, offset, buffer, verifier)
} else {
_, err = disk.ReadFile(volume, path, offset, buffer)
}
errChan <- err errChan <- err
} }

View File

@ -27,11 +27,7 @@ import (
"github.com/minio/minio/pkg/bpool" "github.com/minio/minio/pkg/bpool"
) )
func (d badDisk) ReadFile(volume string, path string, offset int64, buf []byte) (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
}
func (d badDisk) ReadFileWithVerify(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error) {
return 0, errFaultyDisk return 0, errFaultyDisk
} }

View File

@ -115,18 +115,11 @@ func (d *naughtyDisk) ListDir(volume, path string) (entries []string, err error)
return d.disk.ListDir(volume, path) 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 { if err := d.calcError(); err != nil {
return 0, err return 0, err
} }
return d.disk.ReadFile(volume, path, offset, buf) return d.disk.ReadFile(volume, path, offset, buf, verifier)
}
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)
} }
func (d *naughtyDisk) PrepareFile(volume, path string, length int64) error { func (d *naughtyDisk) PrepareFile(volume, path string, length int64) error {

View File

@ -533,25 +533,14 @@ func (s *posix) ReadAll(volume, path string) (buf []byte, err error) {
// for io.EOF. // for io.EOF.
// //
// If an EOF happens after reading some but not all the bytes, // 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 // Additionally ReadFile also starts reading from an offset. ReadFile
// semantics are same as io.ReadFull. // semantics are same as io.ReadFull.
func (s *posix) ReadFile(volume, path string, offset int64, buf []byte) (n int64, err error) { func (s *posix) ReadFile(volume, path string, offset int64, buffer []byte, verifier *BitrotVerifier) (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) {
defer func() { defer func() {
if err == syscall.EIO { if err == syscall.EIO {
atomic.AddInt32(&s.ioErrCount, 1) atomic.AddInt32(&s.ioErrCount, 1)
@ -612,7 +601,7 @@ func (s *posix) ReadFileWithVerify(volume, path string, offset int64, buffer []b
return 0, errIsNotRegular return 0, errIsNotRegular
} }
if !verifier.IsVerified() { if verifier != nil && !verifier.IsVerified() {
bufp := s.pool.Get().(*[]byte) bufp := s.pool.Get().(*[]byte)
defer s.pool.Put(bufp) defer s.pool.Put(bufp)

View File

@ -1074,7 +1074,7 @@ func TestPosixReadFile(t *testing.T) {
var n int64 var n int64
// Common read buffer. // Common read buffer.
var buf = make([]byte, testCase.bufSize) 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 { if err != nil && testCase.expectedErr != nil {
// Validate if the type string of the errors are an exact match. // Validate if the type string of the errors are an exact match.
if err.Error() != testCase.expectedErr.Error() { if err.Error() != testCase.expectedErr.Error() {
@ -1135,7 +1135,7 @@ func TestPosixReadFile(t *testing.T) {
if err == nil { if err == nil {
// Common read buffer. // Common read buffer.
var buf = make([]byte, 10) 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) t.Errorf("expected: %s, got: %s", errFileAccessDenied, err)
} }
} }
@ -1149,7 +1149,7 @@ func TestPosixReadFile(t *testing.T) {
posixType.ioErrCount = int32(6) posixType.ioErrCount = int32(6)
// Common read buffer. // Common read buffer.
var buf = make([]byte, 10) var buf = make([]byte, 10)
_, err = posixType.ReadFile("abc", "yes", 0, buf) _, err = posixType.ReadFile("abc", "yes", 0, buf, nil)
if err != errFaultyDisk { if err != errFaultyDisk {
t.Fatalf("Expected \"Faulty Disk\", got: \"%s\"", err) 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 {file: "myobject", offset: 1000, length: 1001, algorithm: BLAKE2b512, expError: nil}, // 15
} }
// TestPosixReadFileWithVerify - tests the posix level // TestPosixReadFile with bitrot verification - tests the posix level
// ReadFileWithVerify API. Only tests hashing related // ReadFile API with a BitrotVerifier. Only tests hashing related
// functionality. Other functionality is tested with // functionality. Other functionality is tested with
// TestPosixReadFile. // TestPosixReadFile.
func TestPosixReadFileWithVerify(t *testing.T) { func TestPosixReadFileWithVerify(t *testing.T) {
@ -1218,7 +1218,7 @@ func TestPosixReadFileWithVerify(t *testing.T) {
} }
buffer := make([]byte, test.length) 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 { switch {
case err == nil && test.expError != nil: case err == nil && test.expError != nil:

View File

@ -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. // 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() { if f.IsOffline() {
return m, errDiskNotFound 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) { 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)
} }
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. // ListDir - a retryable implementation of listing directory entries.
func (f *retryStorage) ListDir(volume, path string) (entries []string, err error) { func (f *retryStorage) ListDir(volume, path string) (entries []string, err error) {
if f.IsOffline() { if f.IsOffline() {

View File

@ -290,7 +290,7 @@ func TestRetryStorage(t *testing.T) {
for _, disk := range storageDisks { for _, disk := range storageDisks {
var buf2 = make([]byte, 5) var buf2 = make([]byte, 5)
var n int64 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) t.Fatal(err)
} }
if err != nil { if err != nil {
@ -312,11 +312,11 @@ func TestRetryStorage(t *testing.T) {
var buf2 = make([]byte, 5) var buf2 = make([]byte, 5)
verifier := NewBitrotVerifier(SHA256, sha256Hash([]byte("Hello, World"))) verifier := NewBitrotVerifier(SHA256, sha256Hash([]byte("Hello, World")))
var n int64 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) t.Fatal(err)
} }
if err != nil { if err != nil {
t.Error("Error in ReadFileWithVerify", err) t.Error("Error in ReadFile with bitrot verification", err)
} }
if n != 5 { if n != 5 {
t.Fatalf("Expected 5, got %d", n) t.Fatalf("Expected 5, got %d", n)

View File

@ -40,8 +40,7 @@ type StorageAPI interface {
// File operations. // File operations.
ListDir(volume, dirPath string) ([]string, error) ListDir(volume, dirPath string) ([]string, error)
ReadFile(volume string, path string, offset int64, buf []byte) (n int64, err error) ReadFile(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error)
ReadFileWithVerify(volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error)
PrepareFile(volume string, path string, len int64) (err error) PrepareFile(volume string, path string, len int64) (err error)
AppendFile(volume string, path string, buf []byte) (err error) AppendFile(volume string, path string, buf []byte) (err error)
RenameFile(srcVolume, srcPath, dstVolume, dstPath string) 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) { 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 r.offset += nn
n = int(nn) n = int(nn)

View File

@ -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. // 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() { defer func() {
if r := recover(); r != nil { if r := recover(); r != nil {
// Recover any panic from allocation, and return error. // 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. }() // Do not crash the server.
var result []byte args := ReadFileArgs{
err = n.rpcClient.Call("Storage.ReadFileHandler", &ReadFileArgs{ Vol: volume,
Vol: volume, Path: path,
Path: path, Offset: offset,
Offset: offset, Buffer: buffer,
Buffer: buffer, Verified: true, // mark read as verified by default
}, &result) }
if verifier != nil {
// Copy results to buffer. args.Algo = verifier.algorithm
copy(buffer, result) args.ExpectedHash = verifier.sum
args.Verified = verifier.IsVerified()
// 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.
var result []byte var result []byte
err = n.rpcClient.Call("Storage.ReadFileWithVerifyHandler", err = n.rpcClient.Call("Storage.ReadFileHandler", &args, &result)
&ReadFileWithVerifyArgs{
Vol: volume,
Path: path,
Offset: offset,
Buffer: buffer,
Algo: verifier.algorithm,
ExpectedHash: verifier.sum,
}, &result)
// Copy results to buffer. // Copy results to buffer.
copy(buffer, result) copy(buffer, result)

View File

@ -379,7 +379,7 @@ func (s *TestRPCStorageSuite) testRPCStorageFileOps(t *testing.T) {
t.Errorf("Expected `Hello, world`, got %s", string(buf)) t.Errorf("Expected `Hello, world`, got %s", string(buf))
} }
buf1 := make([]byte, 5) buf1 := make([]byte, 5)
n, err := storageDisk.ReadFile("myvol", "file1", 4, buf1) n, err := storageDisk.ReadFile("myvol", "file1", 4, buf1, nil)
if err != nil { if err != nil {
t.Error("Unable to initiate ReadFile", err) t.Error("Unable to initiate ReadFile", err)
} }
@ -396,9 +396,9 @@ func (s *TestRPCStorageSuite) testRPCStorageFileOps(t *testing.T) {
} }
verifier := NewBitrotVerifier(BLAKE2b512, blakeHash(buf)) verifier := NewBitrotVerifier(BLAKE2b512, blakeHash(buf))
buf2 := make([]byte, 2) 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 { if err != nil {
t.Error("Error in ReadFileWithVerify", err) t.Error("Error in ReadFile with bitrot verification", err)
} }
if n != 2 { if n != 2 {
t.Errorf("Expected `2`, got %d", n) t.Errorf("Expected `2`, got %d", n)

View File

@ -59,30 +59,15 @@ type ReadFileArgs struct {
// Data buffer read from the path at offset. // Data buffer read from the path at offset.
Buffer []byte 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. // Algorithm used in bit-rot hash computation.
Algo BitrotAlgorithm Algo BitrotAlgorithm
// Stored hash value used to compare with computed value. // Stored hash value used to compare with computed value.
ExpectedHash []byte ExpectedHash []byte
// Indicates whether the disk has already been verified
Verified bool
} }
// PrepareFileArgs represents append file RPC arguments. // PrepareFileArgs represents append file RPC arguments.

View File

@ -145,26 +145,13 @@ func (s *storageServer) ReadFileHandler(args *ReadFileArgs, reply *[]byte) (err
if err = args.IsAuthenticated(); err != nil { if err = args.IsAuthenticated(); err != nil {
return err return err
} }
var verifier *BitrotVerifier
if !args.Verified {
verifier = NewBitrotVerifier(args.Algo, args.ExpectedHash)
}
var n int64 var n int64
n, err = s.storage.ReadFile(args.Vol, args.Path, args.Offset, args.Buffer) 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.
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))
// Sending an error over the rpc layer, would cause unmarshalling to fail. In situations // 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 // when we have short read i.e `io.ErrUnexpectedEOF` treat it as good condition and copy
// the buffer properly. // the buffer properly.