From 2fc771383448e6c9ab5bfc22ff97a80944234b5f Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Thu, 12 Jan 2023 15:31:18 +0100 Subject: [PATCH] iox-#1732 Refactor allocate methods Change return type of SharedMemoryObject::allocate to expected, introduce error enums Signed-off-by: Marika Lehmann --- .../source/posix_wrapper/named_pipe.cpp | 10 ++- .../internal/posix_wrapper/ipc_channel.hpp | 3 +- .../posix_wrapper/shared_memory_object.hpp | 16 +++-- .../memory/include/iox/bump_allocator.hpp | 9 +-- .../memory/source/bump_allocator.cpp | 6 +- .../posix_wrapper/shared_memory_object.cpp | 26 +++++-- .../test_memory_bump_allocator.cpp | 9 +-- .../test_posix_shared_memory_object.cpp | 69 +++++++++++++++---- 8 files changed, 111 insertions(+), 37 deletions(-) diff --git a/iceoryx_dust/source/posix_wrapper/named_pipe.cpp b/iceoryx_dust/source/posix_wrapper/named_pipe.cpp index ca428921f8..f7a7cd8af0 100644 --- 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(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(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 index bdf38ac3b1..36b269cb42 100644 --- 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 index 3dcda59583..7339fce40e 100644 --- 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 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 index 8904bb1ca0..86fbc2d901 100644 --- 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 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 index e654069651..c651215724 100644 --- 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 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::REQUESTED_ZERO_SIZED_MEMORY); + } // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) required for low level pointer alignment uint64_t currentAddress = reinterpret_cast(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 index 72df522dab..c457cfd87e 100644 --- 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 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::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::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::NOT_ENOUGH_MEMORY); + } + return cxx::success(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 index f2151118e5..077475e62c 100644 --- 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 index 8633cde782..db758d4c5a 100644 --- 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(sut->allocate(sizeof(int), 1)); + auto result = sut->allocate(sizeof(int), 1); + ASSERT_THAT(result.has_error(), Eq(false)); + int* test = static_cast(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(shmMemory->allocate(sizeof(int), 1)); + auto result = shmMemory->allocate(sizeof(int), 1); + ASSERT_THAT(result.has_error(), Eq(false)); + int* test = static_cast(result.value()); *test = 4557; - int* test2 = static_cast(shmMemory->allocate(sizeof(int), 1)); + + result = shmMemory->allocate(sizeof(int), 1); + ASSERT_THAT(result.has_error(), Eq(false)); + int* test2 = static_cast(result.value()); *test2 = 8912; @@ -217,8 +253,13 @@ TEST_F(SharedMemoryObject_Test, OpeningSharedMemoryAndReadMultipleContents) .permissions(cxx::perms::owner_all) .create(); - int* sutValue1 = static_cast(sut->allocate(sizeof(int), 1)); - int* sutValue2 = static_cast(sut->allocate(sizeof(int), 1)); + result = sut->allocate(sizeof(int), 1); + ASSERT_THAT(sut.has_error(), Eq(false)); + int* sutValue1 = static_cast(result.value()); + + result = sut->allocate(sizeof(int), 1); + ASSERT_THAT(sut.has_error(), Eq(false)); + int* sutValue2 = static_cast(result.value()); EXPECT_THAT(*sutValue1, Eq(4557)); EXPECT_THAT(*sutValue2, Eq(8912));