Merge pull request #2083 from mheon/shm_locking_fixes

Address lingering review comments from SHM locking PR

4 files changed, 17 insertions, 6 deletions

diff --git a/libpod/lock/lock.go b/libpod/lock/lock.go
index 73c1fdcf7..1f94171fe 100644
--- a/libpod/lock/lock.go
+++ b/libpod/lock/lock.go
@@ -43,6 +43,9 @@ type Locker interface {
 	// encounters a fatal error.
 	// All errors must be handled internally, as they are not returned. For
 	// the most part, panicking should be appropriate.
+	// Some lock implementations may require that Lock() and Unlock() occur
+	// within the same goroutine (SHM locking, for example). The usual Go
+	// Lock()/defer Unlock() pattern will still work fine in these cases.
 	Lock()
 	// Unlock unlocks the lock.
 	// All errors must be handled internally, as they are not returned. For
diff --git a/libpod/lock/shm/shm_lock.go b/libpod/lock/shm/shm_lock.go
index be5e5148f..87d28e5c1 100644
--- a/libpod/lock/shm/shm_lock.go
+++ b/libpod/lock/shm/shm_lock.go
@@ -36,7 +36,7 @@ type SHMLocks struct { // nolint
 // size used by the underlying implementation.
 func CreateSHMLock(path string, numLocks uint32) (*SHMLocks, error) {
 	if numLocks == 0 {
-		return nil, errors.Wrapf(syscall.EINVAL, "number of locks must greater than 0 0")
+		return nil, errors.Wrapf(syscall.EINVAL, "number of locks must be greater than 0")
 	}
 
 	locks := new(SHMLocks)
@@ -65,7 +65,7 @@ func CreateSHMLock(path string, numLocks uint32) (*SHMLocks, error) {
 // segment was created with.
 func OpenSHMLock(path string, numLocks uint32) (*SHMLocks, error) {
 	if numLocks == 0 {
-		return nil, errors.Wrapf(syscall.EINVAL, "number of locks must greater than 0")
+		return nil, errors.Wrapf(syscall.EINVAL, "number of locks must be greater than 0")
 	}
 
 	locks := new(SHMLocks)
diff --git a/libpod/lock/shm/shm_lock_test.go b/libpod/lock/shm/shm_lock_test.go
index 0f3a96cca..594eb5d8e 100644
--- a/libpod/lock/shm/shm_lock_test.go
+++ b/libpod/lock/shm/shm_lock_test.go
@@ -256,13 +256,13 @@ func TestLockSemaphoreActuallyLocks(t *testing.T) {
 // Ensures that runtime.LockOSThread() is doing its job
 func TestLockAndUnlockTwoSemaphore(t *testing.T) {
 	runLockTest(t, func(t *testing.T, locks *SHMLocks) {
-		err := locks.LockSemaphore(0)
+		err := locks.LockSemaphore(5)
 		assert.NoError(t, err)
 
-		err = locks.LockSemaphore(1)
+		err = locks.LockSemaphore(6)
 		assert.NoError(t, err)
 
-		err = locks.UnlockSemaphore(1)
+		err = locks.UnlockSemaphore(6)
 		assert.NoError(t, err)
 
 		// Now yield scheduling
@@ -272,7 +272,7 @@ func TestLockAndUnlockTwoSemaphore(t *testing.T) {
 		// And unlock the last semaphore
 		// If we are in a different OS thread, this should fail.
 		// However, runtime.UnlockOSThread() should guarantee we are not
-		err = locks.UnlockSemaphore(0)
+		err = locks.UnlockSemaphore(5)
 		assert.NoError(t, err)
 	})
 }
diff --git a/libpod/lock/shm_lock_manager_linux.go b/libpod/lock/shm_lock_manager_linux.go
index 3e8f4f3d2..94dfd7dd7 100644
--- a/libpod/lock/shm_lock_manager_linux.go
+++ b/libpod/lock/shm_lock_manager_linux.go
@@ -3,7 +3,10 @@
 package lock
 
 import (
+	"syscall"
+
 	"github.com/containers/libpod/libpod/lock/shm"
+	"github.com/pkg/errors"
 )
 
 // SHMLockManager manages shared memory locks.
@@ -60,6 +63,11 @@ func (m *SHMLockManager) RetrieveLock(id uint32) (Locker, error) {
 	lock.lockID = id
 	lock.manager = m
 
+	if id >= m.locks.GetMaxLocks() {
+		return nil, errors.Wrapf(syscall.EINVAL, "lock ID %d is too large - max lock size is %d",
+			id, m.locks.GetMaxLocks()-1)
+	}
+
 	return lock, nil
 }