iox-eclipse-iceoryx#1732 Refactor allocate methods

Change return type of SharedMemoryObject::allocate to expected, introduce error enums Signed-off-by: Marika Lehmann <[email protected]>
ApexAI · Jan 18, 2023 · 2fc7713 · 2fc7713
1 parent 8245abb
commit 2fc7713
Show file tree

Hide file tree

Showing 8 changed files with 111 additions and 37 deletions.
diff --git a/iceoryx_dust/source/posix_wrapper/named_pipe.cpp b/iceoryx_dust/source/posix_wrapper/named_pipe.cpp
@@ -107,7 +107,15 @@ NamedPipe::NamedPipe(const IpcChannelName_t& name,
         return;
     }
 
-    m_data = static_cast<NamedPipeData*>(m_sharedMemory->allocate(sizeof(NamedPipeData), alignof(NamedPipeData)));
+    auto allocationResult = m_sharedMemory->allocate(sizeof(NamedPipeData), alignof(NamedPipeData));
+    if (allocationResult.has_error())
+    {
+        std::cerr << "Unable to allocate for named pipe \"" << name << "\"" << std::endl;
+        m_isInitialized = false;
+        m_errorValue = IpcChannelError::MEMORY_ALLOCATION_FAILED;
+        return;
+    }
+    m_data = static_cast<NamedPipeData*>(allocationResult.value());
 
     m_isInitialized = true;
     if (m_sharedMemory->hasOwnership())

diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/ipc_channel.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/ipc_channel.hpp
@@ -1,5 +1,5 @@
 // Copyright (c) 2020 by Robert Bosch GmbH. All rights reserved.
-// Copyright (c) 2021 by Apex.AI Inc. All rights reserved.
+// Copyright (c) 2021 - 2022 by Apex.AI Inc. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -53,6 +53,7 @@ enum class IpcChannelError : uint8_t
     INVALID_FILE_DESCRIPTOR,
     I_O_ERROR,
     CONNECTION_RESET_BY_PEER,
+    MEMORY_ALLOCATION_FAILED,
     UNDEFINED
 };
 

diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp
@@ -40,6 +40,14 @@ enum class SharedMemoryObjectError
     INTERNAL_LOGIC_FAILURE,
 };
 
+enum class SharedMemoryAllocationError
+{
+    REQUESTED_MEMORY_AFTER_FINALIZED_ALLOCATION,
+    NOT_ENOUGH_MEMORY,
+    REQUESTED_ZERO_SIZED_MEMORY
+
+};
+
 class SharedMemoryObjectBuilder;
 
 /// @brief Creates a shared memory segment and maps it into the process space.
@@ -60,10 +68,10 @@ class SharedMemoryObject
     ///        alignment
     /// @param[in] size the size of the memory inside the shared memory
     /// @param[in] alignment the alignment of the memory
-    /// @return pointer to a memory address with the requested size and alignment.
-    ///         if finalizeAllocation was called before or not enough memory is available
-    ///         allocate will call the errorHandler via cxx::Expects
-    void* allocate(const uint64_t size, const uint64_t alignment) noexcept;
+    /// @return an expected containing a pointer to a memory address with the requested size and alignment on success,
+    /// an expected containing SharedMemoryAllocationError if finalizeAllocation was called before or not enough memory
+    /// is available
+    cxx::expected<void*, SharedMemoryAllocationError> allocate(const uint64_t size, const uint64_t alignment) noexcept;
 
     /// @brief After this call the user cannot allocate memory inside the SharedMemoryObject
     ///        anymore. This ensures that memory is only allocated in the startup phase.

diff --git a/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp b/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp
@@ -27,7 +27,8 @@ namespace iox
 
 enum class BumpAllocatorError
 {
-    OUT_OF_MEMORY
+    OUT_OF_MEMORY,
+    REQUESTED_ZERO_SIZED_MEMORY
 };
 
 class BumpAllocator
@@ -47,8 +48,8 @@ class BumpAllocator
     /// @brief allocates on the memory supplied with the ctor
     /// @param[in] size of the memory to allocate, must be greater than 0
     /// @param[in] alignment of the memory to allocate
-    /// @return returns an expected containing a pointer to the allocated memory if memory is available, otherwise
-    /// BumpAllocatorError::OUT_OF_MEMORY
+    /// @return returns an expected containing a pointer to the memory if allocation was successful, otherwise
+    /// BumpAllocatorError
     cxx::expected<void*, BumpAllocatorError> allocate(const uint64_t size, const uint64_t alignment) noexcept;
 
     /// @brief mark the memory as unused
@@ -57,7 +58,7 @@ class BumpAllocator
   private:
     cxx::byte_t* m_startAddress{nullptr};
     uint64_t m_length{0U};
-    uint64_t m_currentPosition = 0U;
+    uint64_t m_currentPosition{0U};
 };
 } // namespace iox
 

diff --git a/iceoryx_hoofs/memory/source/bump_allocator.cpp b/iceoryx_hoofs/memory/source/bump_allocator.cpp
@@ -34,7 +34,11 @@ BumpAllocator::BumpAllocator(void* const startAddress, const uint64_t length) no
 // NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
 cxx::expected<void*, BumpAllocatorError> BumpAllocator::allocate(const uint64_t size, const uint64_t alignment) noexcept
 {
-    cxx::ExpectsWithMsg(size > 0, "Cannot allocate memory of size 0");
+    if (size == 0)
+    {
+        IOX_LOG(WARN) << "Cannot allocate memory of size 0.";
+        return cxx::error<BumpAllocatorError>(BumpAllocatorError::REQUESTED_ZERO_SIZED_MEMORY);
+    }
 
     // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) required for low level pointer alignment
     uint64_t currentAddress = reinterpret_cast<uint64_t>(m_startAddress) + m_currentPosition;

diff --git a/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp b/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp
@@ -166,15 +166,29 @@ SharedMemoryObject::SharedMemoryObject(SharedMemory&& sharedMemory,
 {
 }
 
-void* SharedMemoryObject::allocate(const uint64_t size, const uint64_t alignment) noexcept
+cxx::expected<void*, SharedMemoryAllocationError> SharedMemoryObject::allocate(const uint64_t size,
+                                                                               const uint64_t alignment) noexcept
 {
-    cxx::ExpectsWithMsg(
-        !m_allocationFinalized,
-        "allocate() call after finalizeAllocation()! You are not allowed to acquire shared memory chunks anymore");
+    if (size == 0)
+    {
+        IOX_LOG(WARN) << "Cannot allocate memory of size 0.";
+        return cxx::error<SharedMemoryAllocationError>(SharedMemoryAllocationError::REQUESTED_ZERO_SIZED_MEMORY);
+    }
+    if (m_allocationFinalized)
+    {
+        IOX_LOG(WARN) << "allocate() call after finalizeAllocation()! You are not allowed to acquire shared memory "
+                         "chunks anymore.";
+        return cxx::error<SharedMemoryAllocationError>(
+            SharedMemoryAllocationError::REQUESTED_MEMORY_AFTER_FINALIZED_ALLOCATION);
+    }
 
     auto allocationResult = m_allocator.allocate(size, alignment);
-    cxx::ExpectsWithMsg(!allocationResult.has_error(), "Not enough space left in shared memory");
-    return allocationResult.value();
+    if (allocationResult.has_error())
+    {
+        IOX_LOG(WARN) << "Not enough space left in shared memory.";
+        return cxx::error<SharedMemoryAllocationError>(SharedMemoryAllocationError::NOT_ENOUGH_MEMORY);
+    }
+    return cxx::success<void*>(allocationResult.value());
 }
 
 void SharedMemoryObject::finalizeAllocation() noexcept

diff --git a/iceoryx_hoofs/test/moduletests/test_memory_bump_allocator.cpp b/iceoryx_hoofs/test/moduletests/test_memory_bump_allocator.cpp
@@ -121,12 +121,9 @@ TEST_F(BumpAllocator_Test, allocateElementOfSizeZero)
     ::testing::Test::RecordProperty("TEST_ID", "17caa50c-94bf-4a1d-a1ec-dfda563caa0b");
     iox::BumpAllocator sut(memory, memorySize);
 
-    // @todo iox-#1613 remove EXPECT_DEATH
-    // NOLINTBEGIN(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg,
-    // hiccpp-vararg)
-    EXPECT_DEATH(IOX_DISCARD_RESULT(sut.allocate(0, MEMORY_ALIGNMENT)), ".*");
-    // NOLINTEND(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg,
-    // hiccpp-vararg)
+    auto allocationResult = sut.allocate(0, MEMORY_ALIGNMENT);
+    ASSERT_TRUE(allocationResult.has_error());
+    EXPECT_THAT(allocationResult.get_error(), Eq(iox::BumpAllocatorError::REQUESTED_ZERO_SIZED_MEMORY));
 }
 
 TEST_F(BumpAllocator_Test, allocateAfterDeallocateWorks)

diff --git a/iceoryx_hoofs/test/moduletests/test_posix_shared_memory_object.cpp b/iceoryx_hoofs/test/moduletests/test_posix_shared_memory_object.cpp
@@ -89,7 +89,9 @@ TEST_F(SharedMemoryObject_Test, AllocateMemoryInSharedMemoryAndReadIt)
                    .create();
 
     ASSERT_THAT(sut.has_error(), Eq(false));
-    int* test = static_cast<int*>(sut->allocate(sizeof(int), 1));
+    auto result = sut->allocate(sizeof(int), 1);
+    ASSERT_THAT(result.has_error(), Eq(false));
+    int* test = static_cast<int*>(result.value());
     ASSERT_THAT(test, Ne(nullptr));
     *test = 123;
     EXPECT_THAT(*test, Eq(123));
@@ -107,8 +109,9 @@ TEST_F(SharedMemoryObject_Test, AllocateWholeSharedMemoryWithOneChunk)
                    .create();
 
     ASSERT_THAT(sut.has_error(), Eq(false));
-    void* test = sut->allocate(8, 1);
-    ASSERT_THAT(test, Ne(nullptr));
+    auto result = sut->allocate(8, 1);
+    ASSERT_THAT(result.has_error(), Eq(false));
+    ASSERT_THAT(result.value(), Ne(nullptr));
 }
 
 TEST_F(SharedMemoryObject_Test, AllocateWholeSharedMemoryWithMultipleChunks)
@@ -126,8 +129,9 @@ TEST_F(SharedMemoryObject_Test, AllocateWholeSharedMemoryWithMultipleChunks)
 
     for (uint64_t i = 0; i < 8; ++i)
     {
-        void* test = sut->allocate(1, 1);
-        ASSERT_THAT(test, Ne(nullptr));
+        auto result = sut->allocate(1, 1);
+        ASSERT_THAT(result.has_error(), Eq(false));
+        ASSERT_THAT(result.value(), Ne(nullptr));
     }
 }
 
@@ -146,7 +150,9 @@ TEST_F(SharedMemoryObject_Test, AllocateTooMuchMemoryInSharedMemoryWithOneChunk)
 
     ASSERT_THAT(sut.has_error(), Eq(false));
 
-    PerformDeathTest([&] { sut->allocate(cxx::align(memorySize, MEMORY_ALIGNMENT) + 1, 1); });
+    auto result = sut->allocate(cxx::align(memorySize, MEMORY_ALIGNMENT) + 1, 1);
+    ASSERT_TRUE(result.has_error());
+    EXPECT_THAT(result.get_error(), Eq(posix::SharedMemoryAllocationError::NOT_ENOUGH_MEMORY));
 }
 
 TEST_F(SharedMemoryObject_Test, AllocateTooMuchSharedMemoryWithMultipleChunks)
@@ -165,11 +171,14 @@ TEST_F(SharedMemoryObject_Test, AllocateTooMuchSharedMemoryWithMultipleChunks)
 
     for (uint64_t i = 0; i < cxx::align(memorySize, MEMORY_ALIGNMENT); ++i)
     {
-        void* test = sut->allocate(1, 1);
-        ASSERT_THAT(test, Ne(nullptr));
+        auto result = sut->allocate(1, 1);
+        ASSERT_THAT(result.has_error(), Eq(false));
+        ASSERT_THAT(result.value(), Ne(nullptr));
     }
 
-    PerformDeathTest([&] { sut->allocate(1, 1); });
+    auto result = sut->allocate(1, 1);
+    ASSERT_TRUE(result.has_error());
+    EXPECT_THAT(result.get_error(), Eq(posix::SharedMemoryAllocationError::NOT_ENOUGH_MEMORY));
 }
 
 TEST_F(SharedMemoryObject_Test, AllocateAfterFinalizeAllocation)
@@ -185,7 +194,29 @@ TEST_F(SharedMemoryObject_Test, AllocateAfterFinalizeAllocation)
 
     ASSERT_THAT(sut.has_error(), Eq(false));
     sut->finalizeAllocation();
-    PerformDeathTest([&] { sut->allocate(2, 1); });
+
+    auto result = sut->allocate(2, 1);
+    ASSERT_TRUE(result.has_error());
+    EXPECT_THAT(result.get_error(),
+                Eq(posix::SharedMemoryAllocationError::REQUESTED_MEMORY_AFTER_FINALIZED_ALLOCATION));
+}
+
+TEST_F(SharedMemoryObject_Test, AllocateFailsWithZeroSize)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "cf7f6692-1b64-4926-8326-0628ec483231");
+    auto sut = iox::posix::SharedMemoryObjectBuilder()
+                   .name("shmAllocate")
+                   .memorySizeInBytes(8)
+                   .accessMode(iox::posix::AccessMode::READ_WRITE)
+                   .openMode(iox::posix::OpenMode::PURGE_AND_CREATE)
+                   .permissions(cxx::perms::owner_all)
+                   .create();
+
+    ASSERT_THAT(sut.has_error(), Eq(false));
+
+    auto result = sut->allocate(0, 1);
+    ASSERT_TRUE(result.has_error());
+    EXPECT_THAT(result.get_error(), Eq(posix::SharedMemoryAllocationError::REQUESTED_ZERO_SIZED_MEMORY));
 }
 
 TEST_F(SharedMemoryObject_Test, OpeningSharedMemoryAndReadMultipleContents)
@@ -203,9 +234,14 @@ TEST_F(SharedMemoryObject_Test, OpeningSharedMemoryAndReadMultipleContents)
 
     ASSERT_THAT(shmMemory.has_error(), Eq(false));
 
-    int* test = static_cast<int*>(shmMemory->allocate(sizeof(int), 1));
+    auto result = shmMemory->allocate(sizeof(int), 1);
+    ASSERT_THAT(result.has_error(), Eq(false));
+    int* test = static_cast<int*>(result.value());
     *test = 4557;
-    int* test2 = static_cast<int*>(shmMemory->allocate(sizeof(int), 1));
+
+    result = shmMemory->allocate(sizeof(int), 1);
+    ASSERT_THAT(result.has_error(), Eq(false));
+    int* test2 = static_cast<int*>(result.value());
     *test2 = 8912;
 
 
@@ -217,8 +253,13 @@ TEST_F(SharedMemoryObject_Test, OpeningSharedMemoryAndReadMultipleContents)
                    .permissions(cxx::perms::owner_all)
                    .create();
 
-    int* sutValue1 = static_cast<int*>(sut->allocate(sizeof(int), 1));
-    int* sutValue2 = static_cast<int*>(sut->allocate(sizeof(int), 1));
+    result = sut->allocate(sizeof(int), 1);
+    ASSERT_THAT(sut.has_error(), Eq(false));
+    int* sutValue1 = static_cast<int*>(result.value());
+
+    result = sut->allocate(sizeof(int), 1);
+    ASSERT_THAT(sut.has_error(), Eq(false));
+    int* sutValue2 = static_cast<int*>(result.value());
 
     EXPECT_THAT(*sutValue1, Eq(4557));
     EXPECT_THAT(*sutValue2, Eq(8912));