Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iox-#2128 Support segment name configuration #1

Open
wants to merge 1 commit into
base: iox-#2128-segment-mapping-design
Choose a base branch
from

Conversation

gpalmer-latai
Copy link
Owner

@gpalmer-latai gpalmer-latai commented Jan 9, 2024

Pre-Review Checklist for the PR Author

  1. Add a second reviewer for complex new features or larger refactorings
  2. Code follows the coding style of CONTRIBUTING.md
  3. Tests follow the best practice for testing
  4. Changelog updated in the unreleased section including API breaking changes
  5. Branch follows the naming format (iox-123-this-is-a-branch)
  6. Commits messages are according to this guideline
  7. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  8. Relevant issues are linked
  9. Add sensible notes for the reviewer
  10. All checks have passed (except task-list-completed)
  11. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  12. Assign PR to reviewer

Notes for Reviewer

One of several Pull Requests implementing the design described in https://github.com/eclipse-iceoryx/iceoryx/pull/2140/files?short_path=b929aa6#diff-b929aa69d30dd941515f8c2d7e162051113bede6fa3b4a06e1aa4693279ce3b8 and resolving issue eclipse-iceoryx#2128

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

  • Closes TBD

@gpalmer-latai gpalmer-latai changed the base branch from iox-2128-name-mapped-segments to iox-#2128-segment-mapping-design January 9, 2024 15:21
@gpalmer-latai gpalmer-latai force-pushed the iox-#2128-support-segment-name-configuration branch from 8783545 to 6b6914a Compare January 9, 2024 15:26
@gpalmer-latai gpalmer-latai force-pushed the iox-#2128-segment-mapping-design branch from 53ad6ab to 1dcc846 Compare January 9, 2024 15:55
@gpalmer-latai gpalmer-latai force-pushed the iox-#2128-support-segment-name-configuration branch 2 times, most recently from 9879ce4 to 0018fc2 Compare January 9, 2024 21:33
@@ -1,4 +1,5 @@
// Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.
// Copyright (c) 2023 by Latitude AI. All rights reserved.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Copyright (c) 2023 by Latitude AI. All rights reserved.
// Copyright (c) 2024 by Latitude AI. All rights reserved.

PosixGroup::groupName_t(iox::TruncateToCapacity, writer.c_str(), writer.size()),
mempoolConfig});
parsedConfig.m_sharedMemorySegments.emplace_back(
ShmName_t(iox::TruncateToCapacity, name.c_str(), name.size()),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care if the name gets truncated? I'm wondering about cases where we have two or more segments with names that are too long, but whose prefixes match up to the point where they are truncated, like:

a_really_long_name_for_a_segment_that_is_idential_up_to_here
a_really_long_name_for_a_segment_that_is_idential_up_to_there

But maybe the truncation of both just yields: a_really_long_name_for_a_segment_that_is_idential_up_to_ and we have a weird name conflict which is not apparent by reading the config file.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is kind of an annoying point. FWIW in the next PR I replace the "multiple writers" error with a "duplicate segment name" error which causes the program to crash. You get an informative error message. As you will see it could probably be handled more close to the config parsing better but I'm trying to balance locality of changes vs taking on larger refactoring.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume there are also going to be weird errors if the writer/reader groups are too large.

I might poke around a bit at adding a config-time check for the length of these.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense and I don't expect we'll encounter the string size limit as a problem all that often anyway.

@gpalmer-latai gpalmer-latai force-pushed the iox-#2128-support-segment-name-configuration branch from 0018fc2 to 5410bc9 Compare January 10, 2024 17:45
@gpalmer-latai gpalmer-latai force-pushed the iox-#2128-segment-mapping-design branch from 1dcc846 to 085e8b6 Compare January 11, 2024 21:09
@gpalmer-latai gpalmer-latai force-pushed the iox-#2128-support-segment-name-configuration branch from 5410bc9 to 2d15256 Compare January 11, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants