Propogate error codes from SHM lock creation and open

Also add a few more unit tests

Signed-off-by: Matthew Heon <matthew.heon@gmail.com>

4 files changed, 88 insertions, 21 deletions

diff --git a/libpod/lock/locks.go b/libpod/lock/locks.go
index 14da7d6a7..c61bd78d2 100644
--- a/libpod/lock/locks.go
+++ b/libpod/lock/locks.go
@@ -27,16 +27,17 @@ type SHMLocks struct {
 // semaphores, and returns a struct that can be used to operate on those locks.
 // numLocks must be a multiple of the lock bitmap size (by default, 32).
 func CreateSHMLock(numLocks uint32) (*SHMLocks, error) {
-	if numLocks % bitmapSize != 0 {
+	if numLocks % bitmapSize != 0 || numLocks == 0 {
 		return nil, errors.Wrapf(syscall.EINVAL, "number of locks must be a multiple of %d", C.bitmap_size_c)
 	}
 
 	locks := new(SHMLocks)
 
-	lockStruct := C.setup_lock_shm(C.uint32_t(numLocks))
+	var errCode C.int = 0
+	lockStruct := C.setup_lock_shm(C.uint32_t(numLocks), &errCode)
 	if lockStruct == nil {
 		// We got a null pointer, so something errored
-		return nil, errors.Wrapf(syscall.ENOENT, "error creating shared memory locks")
+		return nil, syscall.Errno(-1 * errCode)
 	}
 
 	locks.lockStruct = lockStruct
@@ -51,16 +52,17 @@ func CreateSHMLock(numLocks uint32) (*SHMLocks, error) {
 // segment was created with and be a multiple of the lock bitmap size (default
 // 32)
 func OpenSHMLock(numLocks uint32) (*SHMLocks, error) {
-	if numLocks % bitmapSize != 0 {
+	if numLocks % bitmapSize != 0 || numLocks == 0 {
 		return nil, errors.Wrapf(syscall.EINVAL, "number of locks must be a multiple of %d", C.bitmap_size_c)
 	}
 
 	locks := new(SHMLocks)
 
-	lockStruct := C.open_lock_shm(C.uint32_t(numLocks))
+	var errCode C.int = 0
+	lockStruct := C.open_lock_shm(C.uint32_t(numLocks), &errCode)
 	if lockStruct == nil {
 		// We got a null pointer, so something errored
-		return nil, errors.Wrapf(syscall.ENOENT, "error creating shared memory locks")
+		return nil, syscall.Errno(-1 * errCode)
 	}
 
 	locks.lockStruct = lockStruct
diff --git a/libpod/lock/locks_test.go b/libpod/lock/locks_test.go
index ce4882643..62a36c5da 100644
--- a/libpod/lock/locks_test.go
+++ b/libpod/lock/locks_test.go
@@ -80,6 +80,42 @@ func TestCreateNewSHMBadSize(t *testing.T) {
 	assert.Error(t, err)
 }
 
+// Test that creating an SHM with 0 size fails
+func TestCreateNewSHMZeroSize(t *testing.T) {
+	_, err := CreateSHMLock(0)
+	assert.Error(t, err)
+}
+
+// Test that deallocating an unallocated lock errors
+func TestDeallocateUnallocatedLockErrors(t *testing.T) {
+	runLockTest(t, func(t *testing.T, locks *SHMLocks) {
+		err := locks.DeallocateSemaphore(0)
+		assert.Error(t, err)
+	})
+}
+
+// Test that unlocking an unlocked lock fails
+func TestUnlockingUnlockedLockFails(t *testing.T) {
+	runLockTest(t, func(t *testing.T, locks *SHMLocks) {
+		err := locks.UnlockSemaphore(0)
+		assert.Error(t, err)
+	})
+}
+
+// Test that locking and double-unlocking fails
+func TestDoubleUnlockFails(t *testing.T) {
+	runLockTest(t, func(t *testing.T, locks *SHMLocks) {
+		err := locks.LockSemaphore(0)
+		assert.NoError(t, err)
+
+		err = locks.UnlockSemaphore(0)
+		assert.NoError(t, err)
+
+		err = locks.UnlockSemaphore(0)
+		assert.Error(t, err)
+	})
+}
+
 // Test allocating - lock - unlock - deallocate cycle, single lock
 func TestLockLifecycleSingleLock(t *testing.T) {
 	runLockTest(t, func(t *testing.T, locks *SHMLocks) {
diff --git a/libpod/lock/shm_lock.c b/libpod/lock/shm_lock.c
index 48fd4d4a9..3fe41f63c 100644
--- a/libpod/lock/shm_lock.c
+++ b/libpod/lock/shm_lock.c
@@ -1,6 +1,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <semaphore.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/mman.h>
@@ -15,25 +16,32 @@ size_t compute_shm_size(uint32_t num_bitmaps) {
   return sizeof(shm_struct_t) + (num_bitmaps * sizeof(lock_group_t));
 }
 
-// Set up an SHM segment holding locks for libpod
-// num_locks must be a multiple of BITMAP_SIZE (32 by default)
-// Returns a valid pointer on success or NULL on error
-shm_struct_t *setup_lock_shm(uint32_t num_locks) {
+// Set up an SHM segment holding locks for libpod.
+// num_locks must be a multiple of BITMAP_SIZE (32 by default).
+// Returns a valid pointer on success or NULL on error.
+// If an error occurs, it will be written to the int pointed to by error_code.
+shm_struct_t *setup_lock_shm(uint32_t num_locks, int *error_code) {
   int shm_fd, i, j, ret_code;
   uint32_t num_bitmaps;
   size_t shm_size;
   shm_struct_t *shm;
 
-  // TODO maybe set errno so we can get errors back to libpod?
+  // If error_code doesn't point to anything, we can't reasonably return errors
+  // So fail immediately
+  if (error_code == NULL) {
+    return NULL;
+  }
 
   // We need a nonzero number of locks
   if (num_locks == 0) {
+    *error_code = EINVAL;
     return NULL;
   }
 
   // Calculate the number of bitmaps required
   if (num_locks % BITMAP_SIZE != 0) {
     // Number of locks not a multiple of BITMAP_SIZE
+    *error_code = EINVAL;
     return NULL;
   }
   num_bitmaps = num_locks / BITMAP_SIZE;
@@ -44,18 +52,21 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) {
   // Create a new SHM segment for us
   shm_fd = shm_open(SHM_NAME, O_RDWR | O_CREAT | O_EXCL, 0600);
   if (shm_fd < 0) {
+    *error_code = errno;
     return NULL;
   }
 
   // Increase its size to what we need
   ret_code = ftruncate(shm_fd, shm_size);
   if (ret_code < 0) {
+    *error_code = errno;
     goto CLEANUP_UNLINK;
   }
 
   // Map the shared memory in
   shm = mmap(NULL, shm_size, PROT_READ | PROT_WRITE, MAP_SHARED, shm_fd, 0);
   if (shm == MAP_FAILED) {
+    *error_code = errno;
     goto CLEANUP_UNLINK;
   }
 
@@ -64,9 +75,12 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) {
   shm->num_locks = num_locks;
   shm->num_bitmaps = num_bitmaps;
 
-  // Initialize the semaphore that protects the bitmaps
-  ret_code = sem_init(&(shm->segment_lock), 1, 1);
+  // Initialize the semaphore that protects the bitmaps.
+  // Initialize to value 1, as we're a mutex, and set pshared as this will be
+  // shared between processes in an SHM.
+  ret_code = sem_init(&(shm->segment_lock), true, 1);
   if (ret_code < 0) {
+    *error_code = errno;
     goto CLEANUP_UNMAP;
   }
 
@@ -75,8 +89,11 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) {
   for (i = 0; i < num_bitmaps; i++) {
     shm->locks[i].bitmap = 0;
     for (j = 0; j < BITMAP_SIZE; j++) {
-      ret_code = sem_init(&(shm->locks[i].locks[j]), 1, 1);
+      // As above, initialize to 1 to act as a mutex, and set pshared as we'll
+      // be living in an SHM.
+      ret_code = sem_init(&(shm->locks[i].locks[j]), true, 1);
       if (ret_code < 0) {
+	*error_code = errno;
 	goto CLEANUP_UNMAP;
       }
     }
@@ -97,24 +114,31 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) {
   return NULL;
 }
 
-// Open an existing SHM segment holding libpod locks
-// num_locks is the number of locks that will be configured in the SHM segment
-// num_locks must be a multiple of BITMAP_SIZE (32 by default)
-// Returns a valid pointer on success or NULL on error
-shm_struct_t *open_lock_shm(uint32_t num_locks) {
+// Open an existing SHM segment holding libpod locks.
+// num_locks is the number of locks that will be configured in the SHM segment.
+// num_locks must be a multiple of BITMAP_SIZE (32 by default).
+// Returns a valid pointer on success or NULL on error.
+// If an error occurs, it will be written to the int pointed to by error_code.
+shm_struct_t *open_lock_shm(uint32_t num_locks, int *error_code) {
   int shm_fd;
   shm_struct_t *shm;
   size_t shm_size;
   uint32_t num_bitmaps;
 
+  if (error_code == NULL) {
+    return NULL;
+  }
+
   // We need a nonzero number of locks
   if (num_locks == 0) {
+    *error_code = EINVAL;
     return NULL;
   }
 
   // Calculate the number of bitmaps required
   if (num_locks % BITMAP_SIZE != 0) {
     // Number of locks not a multiple of BITMAP_SIZE
+    *error_code = EINVAL;
     return NULL;
   }
   num_bitmaps = num_locks / BITMAP_SIZE;
@@ -129,6 +153,9 @@ shm_struct_t *open_lock_shm(uint32_t num_locks) {
 
   // Map the shared memory in
   shm = mmap(NULL, shm_size, PROT_READ | PROT_WRITE, MAP_SHARED, shm_fd, 0);
+  if (shm == MAP_FAILED) {
+    *error_code = errno;
+  }
 
   // Ignore errors, it's ok if we leak a single FD since this only runs once
   close(shm_fd);
@@ -140,9 +167,11 @@ shm_struct_t *open_lock_shm(uint32_t num_locks) {
 
   // Need to check the SHM to see if it's actually our locks
   if (shm->magic != MAGIC) {
+    *error_code = errno;
     goto CLEANUP;
   }
   if (shm->num_locks != num_locks) {
+    *error_code = errno;
     goto CLEANUP;
   }
 
diff --git a/libpod/lock/shm_lock.h b/libpod/lock/shm_lock.h
index 6988789dd..18bea47e9 100644
--- a/libpod/lock/shm_lock.h
+++ b/libpod/lock/shm_lock.h
@@ -32,8 +32,8 @@ typedef struct shm_struct {
 } shm_struct_t;
 
 size_t compute_shm_size(uint32_t num_bitmaps);
-shm_struct_t *setup_lock_shm(uint32_t num_locks);
-shm_struct_t *open_lock_shm(uint32_t num_locks);
+shm_struct_t *setup_lock_shm(uint32_t num_locks, int *error_code);
+shm_struct_t *open_lock_shm(uint32_t num_locks, int *error_code);
 int32_t close_lock_shm(shm_struct_t *shm);
 int64_t allocate_semaphore(shm_struct_t *shm);
 int32_t deallocate_semaphore(shm_struct_t *shm, uint32_t sem_index);