Skip to content

Commit

Permalink
iox-eclipse-iceoryx#2128 Update producer segment selection logic to t…
Browse files Browse the repository at this point in the history 
…ake the segment name into account
  • Loading branch information
Graham Palmer committed Jan 22, 2024
1 parent 47a49c3 commit f2fe240
Show file tree
Hide file tree
Showing 6 changed files with 127 additions and 39 deletions.
28 changes: 26 additions & 2 deletions iceoryx_posh/include/iceoryx_posh/internal/mepoo/segment_manager.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "iox/posix_user.hpp"
#include "iox/string.hpp"
#include "iox/vector.hpp"
#include "iox/expected.hpp"

namespace iox
{
Expand Down Expand Up @@ -78,14 +79,37 @@ class SegmentManager

struct SegmentUserInformation
{
optional<std::reference_wrapper<MemoryManager>> m_memoryManager;
MemoryManager& m_memoryManager;
uint64_t m_segmentID;
};

using SegmentMappingContainer = vector<SegmentMapping, MAX_SHM_SEGMENTS>;

SegmentMappingContainer getSegmentMappings(const PosixUser& user) noexcept;
SegmentUserInformation getSegmentInformationWithWriteAccessForUser(const PosixUser& user) noexcept;

enum class SegmentLookupError
{
NoSegmentFound,
NoWriteAccess,
};

/// @brief Get the information for the requested segment to which the user has write access.
/// @param[in] name Name of the segment.
/// If empty, each user group name will be tried instead until a match is found.
/// @param[in] user Posix user information.
/// Used to determine write access and as a fallback for selecting a segment if no name is provided.
/// @return Information about the requested shared memory segment or a SegmentLookupError.
expected<SegmentUserInformation, SegmentLookupError> getSegmentInformationWithWriteAccessForUser(const ShmName_t& name, const PosixUser& user) noexcept;

/// @brief Find the segment whose name implicitly matches the user's write-access permissions.
/// @details This reflects the legacy behavior where producers could only use the unique segment
/// to which they have write access. These segments had no name specified and would be matched
/// solely based on their access rights.
/// Now, segments with no names are given a default name of the RouDi process user group and this is
/// used instead to perform a match, producing the same semantic behavior.
/// @param[in] user Posix user information.
/// @return Information about the requested shared memory segment if one is found or a SegmentLookupError.
expected<SegmentUserInformation, SegmentLookupError> getSegmentInformationWithWriteAccessForUser(const PosixUser& user) noexcept;

static uint64_t requiredManagementMemorySize(const SegmentConfig& config) noexcept;
static uint64_t requiredChunkMemorySize(const SegmentConfig& config) noexcept;
Expand Down
39 changes: 29 additions & 10 deletions iceoryx_posh/include/iceoryx_posh/internal/mepoo/segment_manager.inl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,28 +84,47 @@ SegmentManager<SegmentType>::getSegmentMappings(const PosixUser& user) noexcept
}

template <typename SegmentType>
inline typename SegmentManager<SegmentType>::SegmentUserInformation
SegmentManager<SegmentType>::getSegmentInformationWithWriteAccessForUser(const PosixUser& user) noexcept
inline expected<typename SegmentManager<SegmentType>::SegmentUserInformation,
typename SegmentManager<SegmentType>::SegmentLookupError>
SegmentManager<SegmentType>::getSegmentInformationWithWriteAccessForUser(const ShmName_t& name, const PosixUser& user) noexcept
{
auto groupContainer = user.getGroups();
for (auto& segment : m_segmentContainer)
{
if (segment.getSegmentName() == name)
{
// Verify that the user has write access to this segment.
for (const auto& groupID : user.getGroups())
{
if (segment.getWriterGroup() == groupID)
{
return ok(SegmentUserInformation{segment.getMemoryManager(), segment.getSegmentId()});
}
}
return err(SegmentLookupError::NoWriteAccess);
}
}

SegmentUserInformation segmentInfo{nullopt_t(), 0u};
return getSegmentInformationWithWriteAccessForUser(user);
}

template <typename SegmentType>
inline expected<typename SegmentManager<SegmentType>::SegmentUserInformation,
typename SegmentManager<SegmentType>::SegmentLookupError>
SegmentManager<SegmentType>::getSegmentInformationWithWriteAccessForUser(const PosixUser& user) noexcept
{
// with the groups we can search for the writable segment of this user
for (const auto& groupID : groupContainer)
for (const auto& groupID : user.getGroups())
{
for (auto& segment : m_segmentContainer)
{
if (segment.getWriterGroup() == groupID)
if (segment.getSegmentName() == groupID.getName() && segment.getWriterGroup() == groupID)
{
segmentInfo.m_memoryManager = segment.getMemoryManager();
segmentInfo.m_segmentID = segment.getSegmentId();
return segmentInfo;
return ok(SegmentUserInformation{segment.getMemoryManager(), segment.getSegmentId()});
}
}
}

return segmentInfo;
return err(SegmentLookupError::NoSegmentFound);
}

template <typename SegmentType>
Expand Down
19 changes: 10 additions & 9 deletions iceoryx_posh/source/roudi/process_manager.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) 2019 - 2021 by Robert Bosch GmbH. All rights reserved.
// Copyright (c) 2021 - 2022 by Apex.AI Inc. All rights reserved.
// Copyright (c) 2023 by Mathias Kraus <[email protected]>. All rights reserved.
// Copyright (c) 2024 by Latitude AI. 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.
Expand Down Expand Up @@ -490,9 +491,9 @@ void ProcessManager::addPublisherForProcess(const RuntimeName_t& name,
{
findProcess(name)
.and_then([&](auto& process) { // create a PublisherPort
auto segmentInfo = m_segmentManager->getSegmentInformationWithWriteAccessForUser(process->getUser());
auto maybeSegmentInfo = m_segmentManager->getSegmentInformationWithWriteAccessForUser(publisherOptions.segmentName, process->getUser());

if (!segmentInfo.m_memoryManager.has_value())
if (!maybeSegmentInfo.has_value())
{
// Tell the app no writable shared memory segment was found
runtime::IpcMessage sendBuffer;
Expand All @@ -504,7 +505,7 @@ void ProcessManager::addPublisherForProcess(const RuntimeName_t& name,
}

auto maybePublisher = m_portManager.acquirePublisherPortData(
service, publisherOptions, name, &segmentInfo.m_memoryManager.value().get(), portConfigInfo);
service, publisherOptions, name, &maybeSegmentInfo->m_memoryManager, portConfigInfo);

if (maybePublisher.has_value())
{
Expand Down Expand Up @@ -567,9 +568,9 @@ void ProcessManager::addClientForProcess(const RuntimeName_t& name,
{
findProcess(name)
.and_then([&](auto& process) { // create a ClientPort
auto segmentInfo = m_segmentManager->getSegmentInformationWithWriteAccessForUser(process->getUser());
auto maybeSegmentInfo = m_segmentManager->getSegmentInformationWithWriteAccessForUser(clientOptions.requestSegmentName, process->getUser());

if (!segmentInfo.m_memoryManager.has_value())
if (!maybeSegmentInfo.has_value())
{
// Tell the app no writable shared memory segment was found
runtime::IpcMessage sendBuffer;
Expand All @@ -582,7 +583,7 @@ void ProcessManager::addClientForProcess(const RuntimeName_t& name,

m_portManager
.acquireClientPortData(
service, clientOptions, name, &segmentInfo.m_memoryManager.value().get(), portConfigInfo)
service, clientOptions, name, &maybeSegmentInfo->m_memoryManager, portConfigInfo)
.and_then([&](auto& clientPort) {
auto relativePtrToClientPort =
UntypedRelativePointer::getOffset(segment_id_t{m_mgmtSegmentId}, clientPort);
Expand Down Expand Up @@ -621,9 +622,9 @@ void ProcessManager::addServerForProcess(const RuntimeName_t& name,
{
findProcess(name)
.and_then([&](auto& process) { // create a ServerPort
auto segmentInfo = m_segmentManager->getSegmentInformationWithWriteAccessForUser(process->getUser());
auto maybeSegmentInfo = m_segmentManager->getSegmentInformationWithWriteAccessForUser(serverOptions.responseSegmentName, process->getUser());

if (!segmentInfo.m_memoryManager.has_value())
if (!maybeSegmentInfo.has_value())
{
// Tell the app no writable shared memory segment was found
runtime::IpcMessage sendBuffer;
Expand All @@ -636,7 +637,7 @@ void ProcessManager::addServerForProcess(const RuntimeName_t& name,

m_portManager
.acquireServerPortData(
service, serverOptions, name, &segmentInfo.m_memoryManager.value().get(), portConfigInfo)
service, serverOptions, name, &maybeSegmentInfo->m_memoryManager, portConfigInfo)
.and_then([&](auto& serverPort) {
auto relativePtrToServerPort =
UntypedRelativePointer::getOffset(segment_id_t{m_mgmtSegmentId}, serverPort);
Expand Down
65 changes: 55 additions & 10 deletions iceoryx_posh/test/moduletests/test_mepoo_segment_management.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ class SegmentManager_test : public Test
SegmentConfig getSegmentConfig()
{
SegmentConfig config;
config.m_sharedMemorySegments.emplace_back("segment_name1", "iox_roudi_test1", "iox_roudi_test2", mepooConfig);
config.m_sharedMemorySegments.emplace_back("segment_name2", "iox_roudi_test3", "iox_roudi_test2", mepooConfig);
config.m_sharedMemorySegments.emplace_back("iox_roudi_test2", "iox_roudi_test1", "iox_roudi_test2", mepooConfig);
config.m_sharedMemorySegments.emplace_back("other_segment", "iox_roudi_test3", "iox_roudi_test2", mepooConfig);
return config;
}

Expand Down Expand Up @@ -165,24 +165,58 @@ TEST_F(SegmentManager_test, getMemoryManagerForUserWithWriteUser)
GTEST_SKIP_FOR_ADDITIONAL_USER() << "This test requires the -DTEST_WITH_ADDITIONAL_USER=ON cmake argument";

auto sut = createSut();
auto memoryManager = sut->getSegmentInformationWithWriteAccessForUser(PosixUser{"iox_roudi_test2"}).m_memoryManager;
ASSERT_TRUE(memoryManager.has_value());
ASSERT_THAT(memoryManager.value().get().getNumberOfMemPools(), Eq(2u));
auto maybeSegment = sut->getSegmentInformationWithWriteAccessForUser(PosixUser{"iox_roudi_test2"});
ASSERT_TRUE(maybeSegment.has_value());
auto& memoryManager = maybeSegment->m_memoryManager;
ASSERT_THAT(memoryManager.getNumberOfMemPools(), Eq(2u));

auto poolInfo1 = memoryManager.value().get().getMemPoolInfo(0);
auto poolInfo2 = memoryManager.value().get().getMemPoolInfo(1);
auto poolInfo1 = memoryManager.getMemPoolInfo(0);
auto poolInfo2 = memoryManager.getMemPoolInfo(1);
EXPECT_THAT(poolInfo1.m_numChunks, Eq(5u));
EXPECT_THAT(poolInfo2.m_numChunks, Eq(7u));
}

TEST_F(SegmentManager_test, getMemoryManagerForNamedSegmentWithWriteAccess)
{
::testing::Test::RecordProperty("TEST_ID", "f9bf7e03-8c76-4a62-9f01-b408997e29b9");
GTEST_SKIP_FOR_ADDITIONAL_USER() << "This test requires the -DTEST_WITH_ADDITIONAL_USER=ON cmake argument";

auto sut = createSut();
auto maybeSegment = sut->getSegmentInformationWithWriteAccessForUser("other_segment", PosixUser{"iox_roudi_test2"});
ASSERT_TRUE(maybeSegment.has_value());
auto& memoryManager = maybeSegment->m_memoryManager;
ASSERT_THAT(memoryManager.getNumberOfMemPools(), Eq(2u));

auto poolInfo1 = memoryManager.getMemPoolInfo(0);
auto poolInfo2 = memoryManager.getMemPoolInfo(1);
EXPECT_THAT(poolInfo1.m_numChunks, Eq(5u));
EXPECT_THAT(poolInfo2.m_numChunks, Eq(7u));
}

TEST_F(SegmentManager_test, namedAndDefaultWriteAccessSegmentAreUnique)
{
::testing::Test::RecordProperty("TEST_ID", "3a7182ed-a64f-4ffe-ba7f-ba5968431eea");
GTEST_SKIP_FOR_ADDITIONAL_USER() << "This test requires the -DTEST_WITH_ADDITIONAL_USER=ON cmake argument";

auto sut = createSut();

auto maybeNamedSegment = sut->getSegmentInformationWithWriteAccessForUser("other_segment", PosixUser{"iox_roudi_test2"});
ASSERT_TRUE(maybeNamedSegment.has_value());

auto maybeDefaultSegment = sut->getSegmentInformationWithWriteAccessForUser(PosixUser{"iox_roudi_test2"});
ASSERT_TRUE(maybeDefaultSegment.has_value());

EXPECT_NE(maybeNamedSegment->m_segmentID, maybeDefaultSegment->m_segmentID);
}

TEST_F(SegmentManager_test, getMemoryManagerForUserFailWithReadOnlyUser)
{
::testing::Test::RecordProperty("TEST_ID", "9d7c18fd-b8db-425a-830d-22c781091244");
GTEST_SKIP_FOR_ADDITIONAL_USER() << "This test requires the -DTEST_WITH_ADDITIONAL_USER=ON cmake argument";

auto sut = createSut();
EXPECT_FALSE(
sut->getSegmentInformationWithWriteAccessForUser(PosixUser{"iox_roudi_test1"}).m_memoryManager.has_value());
EXPECT_TRUE(
sut->getSegmentInformationWithWriteAccessForUser(ShmName_t{"other_segment"}, PosixUser{"iox_roudi_test1"}).error() == SUT::SegmentLookupError::NoWriteAccess);
}

TEST_F(SegmentManager_test, getMemoryManagerForUserFailWithNonExistingUser)
Expand All @@ -191,7 +225,18 @@ TEST_F(SegmentManager_test, getMemoryManagerForUserFailWithNonExistingUser)
GTEST_SKIP_FOR_ADDITIONAL_USER() << "This test requires the -DTEST_WITH_ADDITIONAL_USER=ON cmake argument";

auto sut = createSut();
EXPECT_FALSE(sut->getSegmentInformationWithWriteAccessForUser(PosixUser{"no_user"}).m_memoryManager.has_value());
// The user does not exist so no groups are checked for a matching segment.
EXPECT_TRUE(sut->getSegmentInformationWithWriteAccessForUser(PosixUser{"no_user"}).error() == SUT::SegmentLookupError::NoSegmentFound);
}

TEST_F(SegmentManager_test, getMemoryManagerForUserFailWithNoMatchingSegment)
{
::testing::Test::RecordProperty("TEST_ID", "b44a49e9-6e3f-44ac-94d4-cce7e5198fca");
GTEST_SKIP_FOR_ADDITIONAL_USER() << "This test requires the -DTEST_WITH_ADDITIONAL_USER=ON cmake argument";

auto sut = createSut();
// The user exists but none of the groups has a matching name.
EXPECT_TRUE(sut->getSegmentInformationWithWriteAccessForUser(PosixUser{"iox_roudi_test1"}).error() == SUT::SegmentLookupError::NoSegmentFound);
}

TEST_F(SegmentManager_test, addingMoreThanOneEntryWithSameNameFails)
Expand Down
4 changes: 2 additions & 2 deletions iceoryx_posh/test/moduletests/test_roudi_portmanager_fixture.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ class PortManager_test : public Test
auto user = PosixUser::getUserOfCurrentProcess();
auto segmentInfo =
m_roudiMemoryManager->segmentManager().value()->getSegmentInformationWithWriteAccessForUser(user);
ASSERT_TRUE(segmentInfo.m_memoryManager.has_value());
ASSERT_TRUE(segmentInfo.has_value());

m_payloadDataSegmentMemoryManager = &segmentInfo.m_memoryManager.value().get();
m_payloadDataSegmentMemoryManager = &segmentInfo->m_memoryManager;

// clearing the introspection, is not in d'tor -> SEGFAULT in delete sporadically
m_portManager->stopPortIntrospection();
Expand Down
11 changes: 5 additions & 6 deletions iceoryx_posh/test/moduletests/test_roudi_process_manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,11 @@ TEST_F(ProcessManager_test, HandleProcessShutdownPreparationRequestWorks)
m_sut->registerProcess(m_processname, m_pid, m_user, m_isMonitored, 1U, 1U, m_versionInfo);

auto user = PosixUser::getUserOfCurrentProcess();
auto payloadDataSegmentMemoryManager = m_roudiMemoryManager->segmentManager()
auto payloadSegment = m_roudiMemoryManager->segmentManager()
.value()
->getSegmentInformationWithWriteAccessForUser(user)
.m_memoryManager;

ASSERT_TRUE(payloadDataSegmentMemoryManager.has_value());
->getSegmentInformationWithWriteAccessForUser(user);
ASSERT_TRUE(payloadSegment.has_value());
auto& payloadDataSegmentMemoryManager = payloadSegment->m_memoryManager;

// get publisher and subscriber
PublisherOptions publisherOptions{
Expand All @@ -156,7 +155,7 @@ TEST_F(ProcessManager_test, HandleProcessShutdownPreparationRequestWorks)
->acquirePublisherPortData({"1", "1", "1"},
publisherOptions,
m_processname,
&payloadDataSegmentMemoryManager.value().get(),
&payloadDataSegmentMemoryManager,
PortConfigInfo())
.value());

Expand Down

0 comments on commit f2fe240

Please sign in to comment.