From 4ff1b0f34f7724909701c80091e8e19356674207 Mon Sep 17 00:00:00 2001 From: Mathias Kraus Date: Fri, 1 Sep 2023 18:04:31 +0200 Subject: [PATCH 1/4] iox-#1036 Port 'MessageQueue' to builder pattern --- .../posix_wrapper/message_queue.hpp | 69 +++++-- .../source/posix_wrapper/message_queue.cpp | 177 +++++++++--------- iceoryx_examples/iceperf/mq.cpp | 1 - 3 files changed, 146 insertions(+), 101 deletions(-) diff --git a/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp b/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp index 2b7ad26e2d..a251e650fa 100644 --- a/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp +++ b/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp @@ -1,5 +1,6 @@ // Copyright (c) 2019 - 2020 by Robert Bosch GmbH. All rights reserved. // Copyright (c) 2020 - 2021 by Apex.AI Inc. All rights reserved. +// Copyright (c) 2023 by Mathias Kraus . 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. @@ -17,12 +18,13 @@ #ifndef IOX_DUST_POSIX_WRAPPER_MESSAGE_QUEUE_HPP #define IOX_DUST_POSIX_WRAPPER_MESSAGE_QUEUE_HPP -#include "iceoryx_dust/design/creation.hpp" #include "iceoryx_hoofs/internal/posix_wrapper/ipc_channel.hpp" #include "iceoryx_platform/fcntl.hpp" #include "iceoryx_platform/mqueue.hpp" #include "iceoryx_platform/stat.hpp" +#include "iox/builder.hpp" #include "iox/duration.hpp" +#include "iox/expected.hpp" #include "iox/optional.hpp" namespace iox @@ -31,11 +33,11 @@ namespace posix { /// @brief Wrapper class for posix message queue /// -/// @tparam NON_BLOCKING specifies the type of message queue. A non-blocking message queue will immediately return from -/// a send/receive call if the queue is full/empty. A blocking message has member functions timedSend and timedReceive -/// which allow to specify a maximum timeout duration. /// @code -/// auto mq = posix::MessageQueue::CreateMessageQueue("/MqName123"); +/// auto mq = iox::posix::MessageQueueBuilder() +/// .name("/MqName123") +/// .channelSide(iox::posix::IpcChannelSide::CLIENT) +/// .create(); /// if (mq.has_value()) /// { /// mq->send("important message, bla."); @@ -44,7 +46,7 @@ namespace posix /// mq->receive(str); /// } /// @endcode -class MessageQueue : public DesignPattern::Creation +class MessageQueue { public: static constexpr mqd_t INVALID_DESCRIPTOR = std::numeric_limits::max(); @@ -52,13 +54,12 @@ class MessageQueue : public DesignPattern::Creation; - + /// @todo iox-#1036 Remove when all channels are ported to the builder pattern /// default constructor. The result is an invalid MessageQueue object which can be reassigned later by using the /// move constructor. - MessageQueue() noexcept; + MessageQueue() noexcept = default; MessageQueue(const MessageQueue& other) = delete; MessageQueue(MessageQueue&& other) noexcept; @@ -67,6 +68,18 @@ class MessageQueue : public DesignPattern::Creation create(const IpcChannelName_t& name, + const IpcChannelSide channelSide, + const size_t maxMsgSize = MAX_MESSAGE_SIZE, + const uint64_t maxMsgNumber = MAX_MESSAGE_NUMBER) noexcept; + + /// @todo iox-#1036 Remove when all channels are ported to the builder pattern + bool isInitialized() const noexcept + { + return m_mqDescriptor != INVALID_DESCRIPTOR; + } + static expected unlinkIfExists(const IpcChannelName_t& name) noexcept; /// @brief send a message to queue using std::string. @@ -90,12 +103,15 @@ class MessageQueue : public DesignPattern::Creation isOutdated() noexcept; private: - MessageQueue(const IpcChannelName_t& name, - const IpcChannelSide channelSide, - const size_t maxMsgSize = MAX_MESSAGE_SIZE, - const uint64_t maxMsgNumber = 10U) noexcept; + friend class MessageQueueBuilder; + + MessageQueue(const IpcChannelName_t&& name, + const mq_attr attributes, + mqd_t mqDescriptor, + const IpcChannelSide channelSide) noexcept; - expected open(const IpcChannelName_t& name, const IpcChannelSide channelSide) noexcept; + static expected + open(const IpcChannelName_t& name, mq_attr& attributes, const IpcChannelSide channelSide) noexcept; expected close() noexcept; expected unlink() noexcept; @@ -119,8 +135,29 @@ class MessageQueue : public DesignPattern::Creation create() const noexcept; +}; + } // namespace posix } // namespace iox diff --git a/iceoryx_dust/source/posix_wrapper/message_queue.cpp b/iceoryx_dust/source/posix_wrapper/message_queue.cpp index 6f695a0630..45fab9f369 100644 --- a/iceoryx_dust/source/posix_wrapper/message_queue.cpp +++ b/iceoryx_dust/source/posix_wrapper/message_queue.cpp @@ -1,5 +1,6 @@ // Copyright (c) 2019 - 2020 by Robert Bosch GmbH. All rights reserved. // Copyright (c) 2020 - 2021 by Apex.AI Inc. All rights reserved. +// Copyright (c) 2023 by Mathias Kraus . 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. @@ -24,77 +25,83 @@ #include #include - namespace iox { namespace posix { -MessageQueue::MessageQueue() noexcept -{ - this->m_isInitialized = false; - this->m_errorValue = IpcChannelError::NOT_INITIALIZED; -} - -// NOLINTNEXTLINE(readability-function-size) @todo iox-#832 make a struct out of arguments -MessageQueue::MessageQueue(const IpcChannelName_t& name, - const IpcChannelSide channelSide, - /// NOLINTJUSTIFICATION @todo iox-#832 should be solved when the arguments are put in a - /// struct - /// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) - const size_t maxMsgSize, - const uint64_t maxMsgNumber) noexcept - : m_channelSide(channelSide) +expected MessageQueueBuilder::create() const noexcept { - sanitizeIpcChannelName(name) - .and_then([this](IpcChannelName_t& name) { this->m_name = std::move(name); }) - .or_else([this](IpcChannelError) { - this->m_isInitialized = false; - this->m_errorValue = IpcChannelError::INVALID_CHANNEL_NAME; - }); + auto sanitzedNameResult = MessageQueue::sanitizeIpcChannelName(m_name); + if (sanitzedNameResult.has_error()) + { + return err(IpcChannelError::INVALID_CHANNEL_NAME); + } + auto& sanitizedName = sanitzedNameResult.value(); + IOX_MAYBE_UNUSED std::false_type m_name; // m_name shall not be used anymore but only sanitizedName - if (maxMsgSize > MAX_MESSAGE_SIZE) + if (m_maxMsgSize > MessageQueue::MAX_MESSAGE_SIZE) { - this->m_isInitialized = false; - this->m_errorValue = IpcChannelError::MAX_MESSAGE_SIZE_EXCEEDED; + return err(IpcChannelError::MAX_MESSAGE_SIZE_EXCEEDED); } - else + + if (m_channelSide == IpcChannelSide::SERVER) { - if (channelSide == IpcChannelSide::SERVER) - { - posixCall(mq_unlink)(m_name.c_str()) - .failureReturnValue(ERROR_CODE) - .ignoreErrnos(ENOENT) - .evaluate() - .and_then([this](auto& r) { - if (r.errnum != ENOENT) - { - std::cout << "MQ still there, doing an unlink of " << m_name << std::endl; - } - }); - } - // fields have a different order in QNX, - // so we need to initialize by name - m_attributes.mq_flags = 0; - m_attributes.mq_maxmsg = static_cast(maxMsgNumber); - m_attributes.mq_msgsize = static_cast(maxMsgSize); - m_attributes.mq_curmsgs = 0L; + posixCall(mq_unlink)(sanitizedName.c_str()) + .failureReturnValue(MessageQueue::ERROR_CODE) + .ignoreErrnos(ENOENT) + .evaluate() + .and_then([&sanitizedName](auto& r) { + if (r.errnum != ENOENT) + { + std::cout << "MQ still there, doing an unlink of " << sanitizedName << std::endl; + } + }); + } + + // fields have a different order in QNX, so we need to initialize by name + mq_attr attributes; + attributes.mq_flags = 0; + attributes.mq_maxmsg = static_cast(m_maxMsgNumber); + attributes.mq_msgsize = static_cast(m_maxMsgSize); + attributes.mq_curmsgs = 0L; #ifdef __QNX__ - m_attributes.mq_recvwait = 0L; - m_attributes.mq_sendwait = 0L; + attributes.mq_recvwait = 0L; + attributes.mq_sendwait = 0L; #endif - auto openResult = open(m_name, channelSide); - if (!openResult.has_error()) - { - this->m_isInitialized = true; - this->m_errorValue = IpcChannelError::UNDEFINED; - this->m_mqDescriptor = openResult.value(); - } - else - { - this->m_isInitialized = false; - this->m_errorValue = openResult.error(); - } + + auto openResult = MessageQueue::open(sanitizedName, attributes, m_channelSide); + if (openResult.has_error()) + { + return err(openResult.error()); } + const auto mqDescriptor = openResult.value(); + + return ok(MessageQueue{std::move(sanitizedName), attributes, mqDescriptor, m_channelSide}); +} + +MessageQueue::MessageQueue(const IpcChannelName_t&& name, + const mq_attr attributes, + mqd_t mqDescriptor, + const IpcChannelSide channelSide) noexcept + : m_name(std::move(name)) + , m_attributes(attributes) + , m_mqDescriptor(mqDescriptor) + , m_channelSide(channelSide) +{ +} + +// NOLINTNEXTLINE(readability-function-size) @todo iox-#832 make a struct out of arguments +expected MessageQueue::create(const IpcChannelName_t& name, + const IpcChannelSide channelSide, + const size_t maxMsgSize, + const uint64_t maxMsgNumber) noexcept +{ + return MessageQueueBuilder() + .name(name) + .channelSide(channelSide) + .maxMsgSize(maxMsgSize) + .maxMsgNumber(maxMsgNumber) + .create(); } MessageQueue::MessageQueue(MessageQueue&& other) noexcept @@ -119,7 +126,6 @@ MessageQueue& MessageQueue::operator=(MessageQueue&& other) noexcept std::cerr << "unable to cleanup message queue \"" << m_name << "\" during move operation - resource leaks are possible!" << std::endl; } - CreationPattern_t::operator=(std::move(other)); /// NOLINTJUSTIFICATION iox-#1036 will be fixed with the builder pattern /// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved) @@ -173,7 +179,6 @@ expected MessageQueue::destroy() noexcept } m_mqDescriptor = INVALID_DESCRIPTOR; - m_isInitialized = false; return ok(); } @@ -214,38 +219,42 @@ expected MessageQueue::receive() const noexcept return ok(std::string(&(message[0]))); } -expected MessageQueue::open(const IpcChannelName_t& name, - const IpcChannelSide channelSide) noexcept +expected +MessageQueue::open(const IpcChannelName_t& name, mq_attr& attributes, const IpcChannelSide channelSide) noexcept { - auto sanitizedIpcChannelName = sanitizeIpcChannelName(name); - if (sanitizedIpcChannelName.has_error()) + auto sanitizedNameResult = sanitizeIpcChannelName(name); + if (sanitizedNameResult.has_error()) { return err(IpcChannelError::INVALID_CHANNEL_NAME); } - - int32_t openFlags = O_RDWR; - if (channelSide == IpcChannelSide::SERVER) + const auto& sanitizedName = sanitizedNameResult.value(); { - /// NOLINTJUSTIFICATION used in internal implementation which wraps the posix functionality - /// NOLINTNEXTLINE(hicpp-signed-bitwise) - openFlags |= O_CREAT; - } + IOX_MAYBE_UNUSED std::false_type name; // name shall not be used anymore but only sanitizedName - // the mask will be applied to the permissions, therefore we need to set it to 0 - mode_t umaskSaved = umask(0); - auto mqCall = posixCall(iox_mq_open4)(sanitizedIpcChannelName->c_str(), openFlags, m_filemode, &m_attributes) - .failureReturnValue(INVALID_DESCRIPTOR) - .suppressErrorMessagesForErrnos(ENOENT) - .evaluate(); + int32_t openFlags = O_RDWR; + if (channelSide == IpcChannelSide::SERVER) + { + /// NOLINTJUSTIFICATION used in internal implementation which wraps the posix functionality + /// NOLINTNEXTLINE(hicpp-signed-bitwise) + openFlags |= O_CREAT; + } - umask(umaskSaved); + // the mask will be applied to the permissions, therefore we need to set it to 0 + mode_t umaskSaved = umask(0); + auto mqCall = posixCall(iox_mq_open4)(sanitizedName.c_str(), openFlags, MessageQueue::FILE_MODE, &attributes) + .failureReturnValue(MessageQueue::INVALID_DESCRIPTOR) + .suppressErrorMessagesForErrnos(ENOENT) + .evaluate(); - if (mqCall.has_error()) - { - return err(errnoToEnum(mqCall.error().errnum)); - } + umask(umaskSaved); - return ok(mqCall->value); + if (mqCall.has_error()) + { + return err(MessageQueue::errnoToEnum(sanitizedName, mqCall.error().errnum)); + } + + return ok(mqCall->value); + } } expected MessageQueue::close() noexcept diff --git a/iceoryx_examples/iceperf/mq.cpp b/iceoryx_examples/iceperf/mq.cpp index cda1b06772..d54c012dc7 100644 --- a/iceoryx_examples/iceperf/mq.cpp +++ b/iceoryx_examples/iceperf/mq.cpp @@ -17,7 +17,6 @@ #include "mq.hpp" #include "iceoryx_dust/cxx/std_string_support.hpp" -#include "iceoryx_dust/posix_wrapper/message_queue.hpp" #include "iceoryx_hoofs/posix_wrapper/posix_call.hpp" #include "iceoryx_platform/fcntl.hpp" #include "iceoryx_platform/platform_correction.hpp" From c6505cf15fac8dfc9c078c9786dfecae495dc125 Mon Sep 17 00:00:00 2001 From: Mathias Kraus Date: Fri, 1 Sep 2023 18:14:30 +0200 Subject: [PATCH 2/4] iox-#1036 Update release notes --- .../release-notes/iceoryx-unreleased.md | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/doc/website/release-notes/iceoryx-unreleased.md b/doc/website/release-notes/iceoryx-unreleased.md index 77cc242918..1ca33dd89e 100644 --- a/doc/website/release-notes/iceoryx-unreleased.md +++ b/doc/website/release-notes/iceoryx-unreleased.md @@ -24,8 +24,6 @@ - Extend `concatenate`, `operator+`, `unsafe_append` and `append` of `iox::string` for chars [\#208](https://github.com/eclipse-iceoryx/iceoryx/issues/208) - Extend `unsafe_append` and `append` methods of `iox::string` for `std::string` [\#208](https://github.com/eclipse-iceoryx/iceoryx/issues/208) - The iceoryx development environment supports multiple running docker containers [\#1410](https://github.com/eclipse-iceoryx/iceoryx/issues/1410) -- Use builder pattern in FileLock [\#1036](https://github.com/eclipse-iceoryx/iceoryx/issues/1036) - - Add the ability to adjust path and file permissions of the file lock - Create convenience macro for `NewType` [\#1425](https://github.com/eclipse-iceoryx/iceoryx/issues/1425) - Add posix thread wrapper [\#1365](https://github.com/eclipse-iceoryx/iceoryx/issues/1365) - Apps send only the heartbeat when monitoring is enabled in roudi [\#1436](https://github.com/eclipse-iceoryx/iceoryx/issues/1436) @@ -99,8 +97,17 @@ - Use `GTEST_FAIL` and `GTEST_SUCCEED` instead of `FAIL` and `SUCCEED` [\#1072](https://github.com/eclipse-iceoryx/iceoryx/issues/1072) - posix wrapper `SharedMemoryObject` is silent on success [\#971](https://github.com/eclipse-iceoryx/iceoryx/issues/971) - Remove creation design pattern class with in place implementation [\#1036](https://github.com/eclipse-iceoryx/iceoryx/issues/1036) - - posix wrapper `SharedMemoryObject` uses builder pattern instead of creation - - Builder pattern extracted from `helplets.hpp` into `iox/builder.hpp` + - the following classes use the builder pattern instead of creation + - `SharedMemoryObject` + - `MemoryMap` + - `SharedMemory` + - `MessageQueue` + - `FileLock` + - Add the ability to adjust path and file permissions of the file lock + - `Mutex` + - `NamedSemaphore` + - `UnnamedSemaphore` + - Builder pattern extracted from `helplets.hpp` into `iox/builder.hpp` - Uninteresting mock function calls in tests [\#1341](https://github.com/eclipse-iceoryx/iceoryx/issues/1341) - `cxx::unique_ptr` owns deleter, remove all deleter classes [\#1143](https://github.com/eclipse-iceoryx/iceoryx/issues/1143) - Remove `iox::posix::Timer` [\#337](https://github.com/eclipse-iceoryx/iceoryx/issues/337) @@ -125,7 +132,6 @@ - Rename `algorithm::max` and `algorithm::min` to `algorithm::maxVal` and `algorithm::minVal` [\#1394](https://github.com/eclipse-iceoryx/iceoryx/issues/1394) - Extract `iceoryx_hoofs/platform` into separate package `iceoryx_platform` [\#1615](https://github.com/eclipse-iceoryx/iceoryx/issues/1615) - `cxx::unique_ptr` is no longer nullable [\#1104](https://github.com/eclipse-iceoryx/iceoryx/issues/1104) -- Use builder pattern in mutex [\#1036](https://github.com/eclipse-iceoryx/iceoryx/issues/1036) - Change return type of `vector::erase` to bool [\#1662](https://github.com/eclipse-iceoryx/iceoryx/issues/1662) - `ReleativePointer::registerPtr` returns `iox::optional` [\#605](https://github.com/eclipse-iceoryx/iceoryx/issues/605) - `iox::function` is no longer nullable [\#1104](https://github.com/eclipse-iceoryx/iceoryx/issues/1104) @@ -155,26 +161,21 @@ **API Breaking Changes:** -1. Builder pattern in `SharedMemoryObject` instead of creation pattern +1. Builder pattern instead of creation pattern ```cpp // before - auto sharedMemory = iox::posix::SharedMemoryObject::create("shmAllocate", - 16, - iox::posix::AccessMode::READ_WRITE, - iox::posix::OpenMode::PURGE_AND_CREATE, - iox::posix::SharedMemoryObject::NO_ADDRESS_HINT); + auto fooObject = iox::Foo::create("Bar", 42); // after - auto sharedMemory = iox::posix::SharedMemoryObjectBuilder() - .name("shmAllocate") - .memorySizeInBytes(16) - .accessMode(iox::posix::AccessMode::READ_WRITE) - .openMode(iox::posix::OpenMode::PURGE_AND_CREATE) - .permissions(iox::perms::owner_all) + auto fooObject = iox::FooBuilder() + .name("Bar") + .memorySizeInBytes(42) .create(); ``` + The **refactoring** section has a list with all the affected classes. Have a look at the documentation of these classes for more details. + 2. Builder pattern extracted from `helplets.hpp` into `iox/builder.hpp` ```cpp From 90e1a133f4945f04e6024d3dd90f06dc644087a3 Mon Sep 17 00:00:00 2001 From: Mathias Kraus Date: Fri, 1 Sep 2023 20:05:47 +0200 Subject: [PATCH 3/4] iox-#1036 Make 'MessageQueue' and 'UnixDomainSocket' non-nullable --- .../posix_wrapper/message_queue.hpp | 6 +-- .../posix_wrapper/unix_domain_socket.hpp | 6 +-- .../moduletests/test_unix_domain_sockets.cpp | 17 +++---- .../internal/runtime/ipc_interface_base.hpp | 3 +- .../source/runtime/ipc_interface_base.cpp | 44 +++++++++++++++---- .../test_mq_interface_startup_race.cpp | 17 +++---- 6 files changed, 52 insertions(+), 41 deletions(-) diff --git a/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp b/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp index a251e650fa..eda117fa7b 100644 --- a/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp +++ b/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp @@ -56,11 +56,7 @@ class MessageQueue static constexpr uint64_t MAX_MESSAGE_SIZE = 4096; static constexpr uint64_t MAX_MESSAGE_NUMBER = 10; - /// @todo iox-#1036 Remove when all channels are ported to the builder pattern - /// default constructor. The result is an invalid MessageQueue object which can be reassigned later by using the - /// move constructor. - MessageQueue() noexcept = default; - + MessageQueue() noexcept = delete; MessageQueue(const MessageQueue& other) = delete; MessageQueue(MessageQueue&& other) noexcept; MessageQueue& operator=(const MessageQueue& other) = delete; diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/unix_domain_socket.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/unix_domain_socket.hpp index dd14f3e571..e91773c0c2 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/unix_domain_socket.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/unix_domain_socket.hpp @@ -50,11 +50,7 @@ class UnixDomainSocket using result_t = expected; using errorType_t = IpcChannelError; - /// @brief default constructor. The result is an invalid UnixDomainSocket object which can be reassigned later by - /// using the - /// move constructor. - UnixDomainSocket() noexcept = default; - + UnixDomainSocket() noexcept = delete; UnixDomainSocket(const UnixDomainSocket& other) = delete; UnixDomainSocket(UnixDomainSocket&& other) noexcept; UnixDomainSocket& operator=(const UnixDomainSocket& other) = delete; diff --git a/iceoryx_hoofs/test/moduletests/test_unix_domain_sockets.cpp b/iceoryx_hoofs/test/moduletests/test_unix_domain_sockets.cpp index 65c1e29c58..c6af92d009 100644 --- a/iceoryx_hoofs/test/moduletests/test_unix_domain_sockets.cpp +++ b/iceoryx_hoofs/test/moduletests/test_unix_domain_sockets.cpp @@ -52,15 +52,6 @@ class UnixDomainSocket_test : public Test public: void SetUp() override { - auto serverResult = UnixDomainSocket::create( - goodName, IpcChannelSide::SERVER, UnixDomainSocket::MAX_MESSAGE_SIZE, MaxMsgNumber); - ASSERT_THAT(serverResult.has_error(), Eq(false)); - server = std::move(serverResult.value()); - - auto clientResult = UnixDomainSocket::create( - goodName, IpcChannelSide::CLIENT, UnixDomainSocket::MAX_MESSAGE_SIZE, MaxMsgNumber); - ASSERT_THAT(clientResult.has_error(), Eq(false)); - client = std::move(clientResult.value()); } void TearDown() override @@ -118,8 +109,12 @@ class UnixDomainSocket_test : public Test const std::chrono::milliseconds WAIT_IN_MS{10}; std::atomic_bool doWaitForThread{true}; static constexpr uint64_t MaxMsgNumber = 10U; - UnixDomainSocket server; - UnixDomainSocket client; + UnixDomainSocket server{ + UnixDomainSocket::create(goodName, IpcChannelSide::SERVER, UnixDomainSocket::MAX_MESSAGE_SIZE, MaxMsgNumber) + .expect("Valid UnixDomainSocket")}; + UnixDomainSocket client{ + UnixDomainSocket::create(goodName, IpcChannelSide::CLIENT, UnixDomainSocket::MAX_MESSAGE_SIZE, MaxMsgNumber) + .expect("Valid UnixDomainSocket")}; }; constexpr uint64_t UnixDomainSocket_test::MaxMsgNumber; diff --git a/iceoryx_posh/include/iceoryx_posh/internal/runtime/ipc_interface_base.hpp b/iceoryx_posh/include/iceoryx_posh/internal/runtime/ipc_interface_base.hpp index f048528201..ee259a089e 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/runtime/ipc_interface_base.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/runtime/ipc_interface_base.hpp @@ -29,6 +29,7 @@ #include "iceoryx_posh/internal/runtime/ipc_message.hpp" #include "iox/deadline_timer.hpp" #include "iox/duration.hpp" +#include "iox/optional.hpp" #include "iox/relative_pointer.hpp" #include "iceoryx_dust/posix_wrapper/message_queue.hpp" @@ -265,7 +266,7 @@ class IpcInterface uint64_t m_maxMessageSize{0U}; uint64_t m_maxMessages{0U}; iox::posix::IpcChannelSide m_channelSide{posix::IpcChannelSide::CLIENT}; - IpcChannelType m_ipcChannel; + optional m_ipcChannel; }; using IpcInterfaceBase = IpcInterface; diff --git a/iceoryx_posh/source/runtime/ipc_interface_base.cpp b/iceoryx_posh/source/runtime/ipc_interface_base.cpp index 205356a76a..d9c4ec541a 100644 --- a/iceoryx_posh/source/runtime/ipc_interface_base.cpp +++ b/iceoryx_posh/source/runtime/ipc_interface_base.cpp @@ -78,7 +78,14 @@ IpcInterface::IpcInterface(const RuntimeName_t& runtimeName, template bool IpcInterface::receive(IpcMessage& answer) const noexcept { - auto message = m_ipcChannel.receive(); + if (!m_ipcChannel.has_value()) + { + IOX_LOG(WARN) << "Trying to receive data on an non-initialized IPC interface! Interface name: " + << m_runtimeName; + return false; + } + + auto message = m_ipcChannel->receive(); if (message.has_error()) { return false; @@ -90,7 +97,14 @@ bool IpcInterface::receive(IpcMessage& answer) const noexcept template bool IpcInterface::timedReceive(const units::Duration timeout, IpcMessage& answer) const noexcept { - return !m_ipcChannel.timedReceive(timeout) + if (!m_ipcChannel.has_value()) + { + IOX_LOG(WARN) << "Trying to receive data on an non-initialized IPC interface! Interface name: " + << m_runtimeName; + return false; + } + + return !m_ipcChannel->timedReceive(timeout) .and_then([&answer](auto& message) { IpcInterface::setMessageFromString(message.c_str(), answer); }) @@ -113,6 +127,12 @@ bool IpcInterface::setMessageFromString(const char* buffer, IpcM template bool IpcInterface::send(const IpcMessage& msg) const noexcept { + if (!m_ipcChannel.has_value()) + { + IOX_LOG(WARN) << "Trying to send data on an non-initialized IPC interface! Interface name: " << m_runtimeName; + return false; + } + if (!msg.isValid()) { IOX_LOG(ERROR) << "Trying to send the message " << msg.getMessage() << " which " @@ -127,12 +147,18 @@ bool IpcInterface::send(const IpcMessage& msg) const noexcept IOX_LOG(ERROR) << "msg size of " << messageSize << " bigger than configured max message size"; } }; - return !m_ipcChannel.send(msg.getMessage()).or_else(logLengthError).has_error(); + return !m_ipcChannel->send(msg.getMessage()).or_else(logLengthError).has_error(); } template bool IpcInterface::timedSend(const IpcMessage& msg, units::Duration timeout) const noexcept { + if (!m_ipcChannel.has_value()) + { + IOX_LOG(WARN) << "Trying to send data on an non-initialized IPC interface! Interface name: " << m_runtimeName; + return false; + } + if (!msg.isValid()) { IOX_LOG(ERROR) << "Trying to send the message " << msg.getMessage() << " which " @@ -147,7 +173,7 @@ bool IpcInterface::timedSend(const IpcMessage& msg, units::Durat IOX_LOG(ERROR) << "msg size of " << messageSize << " bigger than configured max message size"; } }; - return !m_ipcChannel.timedSend(msg.getMessage(), timeout).or_else(logLengthError).has_error(); + return !m_ipcChannel->timedSend(msg.getMessage(), timeout).or_else(logLengthError).has_error(); } template @@ -159,7 +185,7 @@ const RuntimeName_t& IpcInterface::getRuntimeName() const noexce template bool IpcInterface::isInitialized() const noexcept { - return m_ipcChannel.isInitialized(); + return m_ipcChannel.has_value(); } template @@ -167,12 +193,12 @@ bool IpcInterface::openIpcChannel(const posix::IpcChannelSide ch { m_channelSide = channelSide; IpcChannelType::create(m_runtimeName, m_channelSide, m_maxMessageSize, m_maxMessages) - .and_then([this](auto& ipcChannel) { this->m_ipcChannel = std::move(ipcChannel); }) + .and_then([this](auto& ipcChannel) { this->m_ipcChannel.emplace(std::move(ipcChannel)); }) .or_else([](auto& err) { IOX_LOG(ERROR) << "unable to create ipc channel with error code: " << static_cast(err); }); - return m_ipcChannel.isInitialized(); + return isInitialized(); } template @@ -184,7 +210,7 @@ bool IpcInterface::reopen() noexcept template bool IpcInterface::ipcChannelMapsToFile() noexcept { - return !m_ipcChannel.isOutdated().value_or(true); + return m_ipcChannel.has_value() && !m_ipcChannel->isOutdated().value_or(true); } template <> @@ -202,7 +228,7 @@ bool IpcInterface::ipcChannelMapsToFile() noexcept template bool IpcInterface::hasClosableIpcChannel() const noexcept { - return m_ipcChannel.isInitialized(); + return isInitialized(); } template diff --git a/iceoryx_posh/test/integrationtests/test_mq_interface_startup_race.cpp b/iceoryx_posh/test/integrationtests/test_mq_interface_startup_race.cpp index c5b5338ecf..2b813f098e 100644 --- a/iceoryx_posh/test/integrationtests/test_mq_interface_startup_race.cpp +++ b/iceoryx_posh/test/integrationtests/test_mq_interface_startup_race.cpp @@ -59,14 +59,9 @@ class StringToMessage : public IpcInterfaceBase class CMqInterfaceStartupRace_test : public Test { public: - CMqInterfaceStartupRace_test() - : m_appQueue{platform::IoxIpcChannelType::create()} - { - } - virtual void SetUp() { - ASSERT_THAT(m_roudiQueue.has_error(), false); + ASSERT_THAT(m_roudiQueue.has_value(), true); } virtual void TearDown() { @@ -102,11 +97,13 @@ class CMqInterfaceStartupRace_test : public Test regAck << IpcMessageTypeToString(IpcMessageType::REG_ACK) << DUMMY_SHM_SIZE << DUMMY_SHM_OFFSET << oldMsg.getElementAtIndex(INDEX_OF_TIMESTAMP) << DUMMY_SEGMENT_ID << SEND_KEEP_ALIVE; - if (m_appQueue.has_error()) + if (!m_appQueue.has_value()) { - m_appQueue = platform::IoxIpcChannelType::create(MqAppName, IpcChannelSide::CLIENT); + platform::IoxIpcChannelType::create(MqAppName, IpcChannelSide::CLIENT).and_then([this](auto& channel) { + this->m_appQueue.emplace(std::move(channel)); + }); } - ASSERT_THAT(m_appQueue.has_error(), false); + ASSERT_THAT(m_appQueue.has_value(), true); ASSERT_FALSE(m_appQueue->send(regAck.getMessage()).has_error()); } @@ -116,7 +113,7 @@ class CMqInterfaceStartupRace_test : public Test platform::IoxIpcChannelType::result_t m_roudiQueue{ platform::IoxIpcChannelType::create(roudi::IPC_CHANNEL_ROUDI_NAME, IpcChannelSide::SERVER)}; std::mutex m_appQueueMutex; - platform::IoxIpcChannelType::result_t m_appQueue; + optional m_appQueue; }; #if !defined(__APPLE__) From 3b67f474df4c9c0303d608727ccfc5789b9a15f3 Mon Sep 17 00:00:00 2001 From: Mathias Kraus Date: Fri, 1 Sep 2023 20:11:39 +0200 Subject: [PATCH 4/4] iox-#1036 Make types more robust --- .../include/iceoryx_dust/posix_wrapper/message_queue.hpp | 4 ++-- iceoryx_dust/source/posix_wrapper/message_queue.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp b/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp index eda117fa7b..a992410359 100644 --- a/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp +++ b/iceoryx_dust/include/iceoryx_dust/posix_wrapper/message_queue.hpp @@ -67,7 +67,7 @@ class MessageQueue /// @todo iox-#1036 Remove when all channels are ported to the builder pattern static expected create(const IpcChannelName_t& name, const IpcChannelSide channelSide, - const size_t maxMsgSize = MAX_MESSAGE_SIZE, + const uint64_t maxMsgSize = MAX_MESSAGE_SIZE, const uint64_t maxMsgNumber = MAX_MESSAGE_NUMBER) noexcept; /// @todo iox-#1036 Remove when all channels are ported to the builder pattern @@ -143,7 +143,7 @@ class MessageQueueBuilder IOX_BUILDER_PARAMETER(IpcChannelSide, channelSide, IpcChannelSide::CLIENT) /// @brief Defines the max message size of the message queue - IOX_BUILDER_PARAMETER(size_t, maxMsgSize, MessageQueue::MAX_MESSAGE_SIZE) + IOX_BUILDER_PARAMETER(uint64_t, maxMsgSize, MessageQueue::MAX_MESSAGE_SIZE) /// @brief Defines the max number of messages for the message queue. IOX_BUILDER_PARAMETER(uint64_t, maxMsgNumber, MessageQueue::MAX_MESSAGE_NUMBER) diff --git a/iceoryx_dust/source/posix_wrapper/message_queue.cpp b/iceoryx_dust/source/posix_wrapper/message_queue.cpp index 45fab9f369..e4861dfce3 100644 --- a/iceoryx_dust/source/posix_wrapper/message_queue.cpp +++ b/iceoryx_dust/source/posix_wrapper/message_queue.cpp @@ -61,8 +61,8 @@ expected MessageQueueBuilder::create() const noex // fields have a different order in QNX, so we need to initialize by name mq_attr attributes; attributes.mq_flags = 0; - attributes.mq_maxmsg = static_cast(m_maxMsgNumber); - attributes.mq_msgsize = static_cast(m_maxMsgSize); + attributes.mq_maxmsg = static_cast(m_maxMsgNumber); + attributes.mq_msgsize = static_cast(m_maxMsgSize); attributes.mq_curmsgs = 0L; #ifdef __QNX__ attributes.mq_recvwait = 0L; @@ -93,7 +93,7 @@ MessageQueue::MessageQueue(const IpcChannelName_t&& name, // NOLINTNEXTLINE(readability-function-size) @todo iox-#832 make a struct out of arguments expected MessageQueue::create(const IpcChannelName_t& name, const IpcChannelSide channelSide, - const size_t maxMsgSize, + const uint64_t maxMsgSize, const uint64_t maxMsgNumber) noexcept { return MessageQueueBuilder()