Merge pull request #71 from benbjohnson/munmap-fix

Fix db.munmap() to return an error.

NOTES

@@ -1,17 +0,0 @@
-
-
-          ===0===
-          | d|g |
-          |1|2|3|
-          =======
-           | | |
-     ------  |  -------
-    |        |         |
-===1===   ===2===   ===3===
-|a|b|c|   |d|e|f|   |g|h|i|
-|-|-|-|   |-|-|-|   |-|-|-|
-|*|*|*|   |*|*|*|   |*|*|*|
-|*|*|*|   |*|*|*|   |*|*|*|
-|*|*|*|   |*|*|*|   |*|*|*|
- 
-

db.go

@@ -19,11 +19,9 @@ const maxMmapStep = 1 << 30 // 1GB
 // All data access is performed through transactions which can be obtained through the DB.
 // All the functions on DB will return a ErrDatabaseNotOpen if accessed before Open() is called.
 type DB struct {
-	os       _os
-	syscall  _syscall
 	path     string
-	file     file
-	metafile file
+	file     *os.File
+	metafile *os.File
 	data     []byte
 	meta0    *meta
 	meta1    *meta
@@ -60,15 +58,6 @@ func (db *DB) Open(path string, mode os.FileMode) error {
 	db.metalock.Lock()
 	defer db.metalock.Unlock()
 
-	// Initialize OS/Syscall references.
-	// These are overridden by mocks during some tests.
-	if db.os == nil {
-		db.os = &sysos{}
-	}
-	if db.syscall == nil {
-		db.syscall = &syssyscall{}
-	}
-
 	// Exit if the database is currently open.
 	if db.opened {
 		return ErrDatabaseOpen
@@ -76,11 +65,11 @@ func (db *DB) Open(path string, mode os.FileMode) error {
 
 	// Open data file and separate sync handler for metadata writes.
 	db.path = path
-	if db.file, err = db.os.OpenFile(db.path, os.O_RDWR|os.O_CREATE, mode); err != nil {
+	if db.file, err = os.OpenFile(db.path, os.O_RDWR|os.O_CREATE, mode); err != nil {
 		db.close()
 		return err
 	}
-	if db.metafile, err = db.os.OpenFile(db.path, os.O_RDWR|os.O_SYNC, mode); err != nil {
+	if db.metafile, err = os.OpenFile(db.path, os.O_RDWR|os.O_SYNC, mode); err != nil {
 		db.close()
 		return err
 	}
@@ -132,7 +121,9 @@ func (db *DB) mmap(minsz int) error {
 	}
 
 	// Unmap existing data before continuing.
-	db.munmap()
+	if err := db.munmap(); err != nil {
+		return err
+	}
 
 	info, err := db.file.Stat()
 	if err != nil {
@@ -149,7 +140,7 @@ func (db *DB) mmap(minsz int) error {
 	size = db.mmapSize(size)
 
 	// Memory-map the data file as a byte slice.
-	if db.data, err = db.syscall.Mmap(int(db.file.Fd()), 0, size, syscall.PROT_READ, syscall.MAP_SHARED); err != nil {
+	if db.data, err = syscall.Mmap(int(db.file.Fd()), 0, size, syscall.PROT_READ, syscall.MAP_SHARED); err != nil {
 		return err
 	}
 
@@ -169,13 +160,14 @@ func (db *DB) mmap(minsz int) error {
 }
 
 // munmap unmaps the data file from memory.
-func (db *DB) munmap() {
+func (db *DB) munmap() error {
 	if db.data != nil {
-		if err := db.syscall.Munmap(db.data); err != nil {
-			panic("unmap error: " + err.Error())
+		if err := syscall.Munmap(db.data); err != nil {
+			return fmt.Errorf("unmap error: " + err.Error())
 		}
 		db.data = nil
 	}
+	return nil
 }
 
 // mmapSize determines the appropriate size for the mmap given the current size
@@ -200,7 +192,7 @@ func (db *DB) mmapSize(size int) int {
 // init creates a new database file and initializes its meta pages.
 func (db *DB) init() error {
 	// Set the page size to the OS page size.
-	db.pageSize = db.os.Getpagesize()
+	db.pageSize = os.Getpagesize()
 
 	// Create two meta pages on a buffer.
 	buf := make([]byte, db.pageSize*4)
@@ -243,20 +235,38 @@ func (db *DB) init() error {
 
 // Close releases all database resources.
 // All transactions must be closed before closing the database.
-func (db *DB) Close() {
+func (db *DB) Close() error {
 	db.metalock.Lock()
 	defer db.metalock.Unlock()
-	db.close()
+	return db.close()
 }
 
-func (db *DB) close() {
+func (db *DB) close() error {
 	db.opened = false
 
-	// TODO(benbjohnson): Undo everything in Open().
 	db.freelist = nil
 	db.path = ""
 
-	db.munmap()
+	// Close the mmap.
+	if err := db.munmap(); err != nil {
+		return err
+	}
+
+	// Close file handles.
+	if db.file != nil {
+		if err := db.file.Close(); err != nil {
+			return fmt.Errorf("db file close error: %s", err)
+		}
+		db.file = nil
+	}
+	if db.metafile != nil {
+		if err := db.metafile.Close(); err != nil {
+			return fmt.Errorf("db metafile close error: %s", err)
+		}
+		db.metafile = nil
+	}
+
+	return nil
 }
 
 // Tx creates a read-only transaction.

db_test.go

@@ -1,19 +1,14 @@
 package bolt
 
 import (
-	"io"
 	"io/ioutil"
 	"math/rand"
 	"os"
 	"strconv"
 	"strings"
-	"syscall"
 	"testing"
-	"time"
-	"unsafe"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/mock"
 )
 
 // Ensure that a database can be opened without error.
@@ -55,106 +50,6 @@ func TestDBReopen(t *testing.T) {
 	})
 }
 
-// Ensure that the database returns an error if the file handle cannot be open.
-func TestDBOpenFileError(t *testing.T) {
-	withMockDB(func(db *DB, mockos *mockos, mocksyscall *mocksyscall, path string) {
-		exp := &os.PathError{}
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_CREATE, os.FileMode(0666)).Return((*mockfile)(nil), exp)
-		err := db.Open(path, 0666)
-		assert.Equal(t, err, exp)
-	})
-}
-
-// Ensure that the database returns an error if the meta file handle cannot be open.
-func TestDBOpenMetaFileError(t *testing.T) {
-	withMockDB(func(db *DB, mockos *mockos, mocksyscall *mocksyscall, path string) {
-		exp := &os.PathError{}
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_CREATE, os.FileMode(0666)).Return(&mockfile{}, nil)
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_SYNC, os.FileMode(0666)).Return((*mockfile)(nil), exp)
-		err := db.Open(path, 0666)
-		assert.Equal(t, err, exp)
-	})
-}
-
-// Ensure that write errors to the meta file handler during initialization are returned.
-func TestDBMetaInitWriteError(t *testing.T) {
-	withMockDB(func(db *DB, mockos *mockos, mocksyscall *mocksyscall, path string) {
-		// Mock the file system.
-		file, metafile := &mockfile{}, &mockfile{}
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_CREATE, os.FileMode(0666)).Return(file, nil)
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_SYNC, os.FileMode(0666)).Return(metafile, nil)
-		mockos.On("Getpagesize").Return(0x10000)
-		file.On("Stat").Return(&mockfileinfo{"", 0, 0666, time.Now(), false, nil}, nil)
-		metafile.On("WriteAt", mock.Anything, int64(0)).Return(0, io.ErrShortWrite)
-
-		// Open the database.
-		err := db.Open(path, 0666)
-		assert.Equal(t, err, io.ErrShortWrite)
-	})
-}
-
-// Ensure that a database that is too small returns an error.
-func TestDBFileTooSmall(t *testing.T) {
-	withMockDB(func(db *DB, mockos *mockos, mocksyscall *mocksyscall, path string) {
-		file, metafile := &mockfile{}, &mockfile{}
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_CREATE, os.FileMode(0666)).Return(file, nil)
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_SYNC, os.FileMode(0666)).Return(metafile, nil)
-		mockos.On("Getpagesize").Return(0x1000)
-		file.On("Stat").Return(&mockfileinfo{"", 0, 0666, time.Now(), false, nil}, nil)
-		metafile.On("WriteAt", mock.Anything, int64(0)).Return(0, nil)
-		err := db.Open(path, 0666)
-		assert.Equal(t, err, &Error{"file size too small", nil})
-	})
-}
-
-// Ensure that stat errors during mmap get returned.
-func TestDBMmapStatError(t *testing.T) {
-	withMockDB(func(db *DB, mockos *mockos, mocksyscall *mocksyscall, path string) {
-		exp := &os.PathError{}
-		file, metafile := &mockfile{}, &mockfile{}
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_CREATE, os.FileMode(0666)).Return(file, nil)
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_SYNC, os.FileMode(0666)).Return(metafile, nil)
-		mockos.On("Getpagesize").Return(0x1000)
-		file.On("ReadAt", mock.Anything, int64(0)).Return(0, nil)
-		file.On("Stat").Return((*mockfileinfo)(nil), exp)
-		metafile.On("WriteAt", mock.Anything, int64(0)).Return(0, nil)
-		err := db.Open(path, 0666)
-		assert.Equal(t, err, &Error{"stat error", exp})
-	})
-}
-
-// Ensure that corrupt meta0 page errors get returned.
-func TestDBCorruptMeta0(t *testing.T) {
-	withMockDB(func(db *DB, mockos *mockos, mocksyscall *mocksyscall, path string) {
-		var m meta
-		m.magic = magic
-		m.version = version
-		m.pageSize = 0x8000
-
-		// Create a file with bad magic.
-		b := make([]byte, 0x10000)
-		p0, p1 := (*page)(unsafe.Pointer(&b[0x0000])), (*page)(unsafe.Pointer(&b[0x8000]))
-		p0.meta().magic = 0
-		p0.meta().version = version
-		p1.meta().magic = magic
-		p1.meta().version = version
-
-		// Mock file access.
-		file, metafile := &mockfile{}, &mockfile{}
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_CREATE, os.FileMode(0666)).Return(file, nil)
-		mockos.On("OpenFile", path, os.O_RDWR|os.O_SYNC, os.FileMode(0666)).Return(metafile, nil)
-		mockos.On("Getpagesize").Return(0x10000)
-		file.On("ReadAt", mock.Anything, int64(0)).Return(0, nil)
-		file.On("Stat").Return(&mockfileinfo{"", 0x10000, 0666, time.Now(), false, nil}, nil)
-		metafile.On("WriteAt", mock.Anything, int64(0)).Return(0, nil)
-		mocksyscall.On("Mmap", 0, int64(0), 0x10000, syscall.PROT_READ, syscall.MAP_SHARED).Return(b, nil)
-
-		// Open the database.
-		err := db.Open(path, 0666)
-		assert.Equal(t, err, &Error{"meta error", ErrInvalid})
-	})
-}
-
 // Ensure that a database cannot open a transaction when it's not open.
 func TestDBTxErrDatabaseNotOpen(t *testing.T) {
 	withDB(func(db *DB, path string) {
@@ -357,15 +252,6 @@ func withDB(fn func(*DB, string)) {
 	fn(&db, path)
 }
 
-// withMockDB executes a function with a database reference and a mock filesystem.
-func withMockDB(fn func(*DB, *mockos, *mocksyscall, string)) {
-	os, syscall := &mockos{}, &mocksyscall{}
-	var db DB
-	db.os = os
-	db.syscall = syscall
-	fn(&db, os, syscall, "/mock/db")
-}
-
 // withOpenDB executes a function with an already opened database.
 func withOpenDB(fn func(*DB, string)) {
 	withDB(func(db *DB, path string) {

os.go

@@ -1,27 +0,0 @@
-package bolt
-
-import (
-	"os"
-)
-
-type _os interface {
-	OpenFile(name string, flag int, perm os.FileMode) (file file, err error)
-	Getpagesize() int
-}
-
-type file interface {
-	Fd() uintptr
-	ReadAt(b []byte, off int64) (n int, err error)
-	Stat() (fi os.FileInfo, err error)
-	WriteAt(b []byte, off int64) (n int, err error)
-}
-
-type sysos struct{}
-
-func (o *sysos) OpenFile(name string, flag int, perm os.FileMode) (file file, err error) {
-	return os.OpenFile(name, flag, perm)
-}
-
-func (o *sysos) Getpagesize() int {
-	return os.Getpagesize()
-}

os_test.go

@@ -1,84 +0,0 @@
-package bolt
-
-import (
-	"os"
-	"time"
-
-	"github.com/stretchr/testify/mock"
-)
-
-type mockos struct {
-	mock.Mock
-}
-
-func (m *mockos) OpenFile(name string, flag int, perm os.FileMode) (file file, err error) {
-	args := m.Called(name, flag, perm)
-	return args.Get(0).(*mockfile), args.Error(1)
-}
-
-func (m *mockos) Stat(name string) (fi os.FileInfo, err error) {
-	args := m.Called(name)
-	return args.Get(0).(os.FileInfo), args.Error(1)
-}
-
-func (m *mockos) Getpagesize() int {
-	args := m.Called()
-	return args.Int(0)
-}
-
-type mockfile struct {
-	mock.Mock
-	fd uintptr
-}
-
-func (m *mockfile) Fd() uintptr {
-	return m.fd
-}
-
-func (m *mockfile) ReadAt(b []byte, off int64) (n int, err error) {
-	args := m.Called(b, off)
-	return args.Int(0), args.Error(1)
-}
-
-func (m *mockfile) Stat() (os.FileInfo, error) {
-	args := m.Called()
-	return args.Get(0).(os.FileInfo), args.Error(1)
-}
-
-func (m *mockfile) WriteAt(b []byte, off int64) (n int, err error) {
-	args := m.Called(b, off)
-	return args.Int(0), args.Error(1)
-}
-
-type mockfileinfo struct {
-	name    string
-	size    int64
-	mode    os.FileMode
-	modTime time.Time
-	isDir   bool
-	sys     interface{}
-}
-
-func (m *mockfileinfo) Name() string {
-	return m.name
-}
-
-func (m *mockfileinfo) Size() int64 {
-	return m.size
-}
-
-func (m *mockfileinfo) Mode() os.FileMode {
-	return m.mode
-}
-
-func (m *mockfileinfo) ModTime() time.Time {
-	return m.modTime
-}
-
-func (m *mockfileinfo) IsDir() bool {
-	return m.isDir
-}
-
-func (m *mockfileinfo) Sys() interface{} {
-	return m.sys
-}

syscall_darwin.go

@@ -1,20 +0,0 @@
-package bolt
-
-import (
-	"syscall"
-)
-
-type _syscall interface {
-	Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error)
-	Munmap([]byte) error
-}
-
-type syssyscall struct{}
-
-func (o *syssyscall) Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error) {
-	return syscall.Mmap(fd, offset, length, prot, flags)
-}
-
-func (o *syssyscall) Munmap(b []byte) error {
-	return syscall.Munmap(b)
-}

syscall_darwin_test.go

@@ -1,19 +0,0 @@
-package bolt
-
-import (
-	"github.com/stretchr/testify/mock"
-)
-
-type mocksyscall struct {
-	mock.Mock
-}
-
-func (m *mocksyscall) Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error) {
-	args := m.Called(fd, offset, length, prot, flags)
-	return args.Get(0).([]byte), args.Error(1)
-}
-
-func (m *mocksyscall) Munmap(b []byte) error {
-	args := m.Called(b)
-	return args.Error(0)
-}

syscall_linux.go

@@ -1,21 +0,0 @@
-package bolt
-
-import (
-	"syscall"
-)
-
-type _syscall interface {
-	Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error)
-	Munmap([]byte) error
-}
-
-type syssyscall struct{}
-
-func (o *syssyscall) Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error) {
-	// err = (EACCES, EBADF, EINVAL, ENODEV, ENOMEM, ENXIO, EOVERFLOW)
-	return syscall.Mmap(fd, offset, length, prot, flags)
-}
-
-func (o *syssyscall) Munmap(b []byte) error {
-	return syscall.Munmap(b)
-}

syscall_linux_test.go

@@ -1,19 +0,0 @@
-package bolt
-
-import (
-	"github.com/stretchr/testify/mock"
-)
-
-type mocksyscall struct {
-	mock.Mock
-}
-
-func (m *mocksyscall) Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error) {
-	args := m.Called(fd, offset, length, prot, flags)
-	return args.Get(0).([]byte), args.Error(1)
-}
-
-func (m *mocksyscall) Munmap(b []byte) error {
-	args := m.Called(b)
-	return args.Error(0)
-}