From eba89259a5938f047d78b74c5bbbc908f8ff4be8 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 4 Jan 2019 14:15:08 -0500 Subject: Address lingering review comments from SHM locking PR Signed-off-by: Matthew Heon --- libpod/lock/lock.go | 3 +++ libpod/lock/shm/shm_lock.go | 4 ++-- libpod/lock/shm/shm_lock_test.go | 8 ++++---- libpod/lock/shm_lock_manager_linux.go | 8 ++++++++ 4 files changed, 17 insertions(+), 6 deletions(-) (limited to 'libpod/lock') 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 } -- cgit v1.2.3-54-g00ecf