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

iox2-51 shm long name support #53

Merged

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Dec 20, 2023

Notes for Reviewer

Do not review before #52 is merged, since it is based on it.

MacOS has the restrictions that only 31 characters are allowed for the shared memory name but iceoryx2 requires much more. The trick is that a file is created to:

  • be able to list all shared memorys in mac os (already present)
  • store the actual shm name, the file name corresponds to the user given shared memory name

In the file is then the triple stored ($PID$_$TIMESTAMP_SECS$_$TIMESTAMP_NSEC$). In the worst case scenario where PID + SEC is larger than 1.000.000.000 it would hit the max name length of mac os (32 > 31), but to our luck NSEC is always smaller than 1.000.000.000 giving us the extra character we need.

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Use either 'Closes #123' or 'Relates to #123' to reference the corresponding issue.

Closes #ISSUE-NUMBER

@elfenpiff elfenpiff requested a review from elBoberido December 20, 2023 11:44
@elfenpiff elfenpiff self-assigned this Dec 20, 2023
doc/release-notes/iceoryx2-unreleased.md Show resolved Hide resolved
@@ -864,7 +864,7 @@ mod service_publish_subscribe {
const BUFFER_SIZE: usize = 1;
const HISTORY_SIZE: usize = 0;
const MAX_BORROW: usize = 1;
const MAX_SUBSCRIBERS: usize = 100;
const MAX_SUBSCRIBERS: usize = 50;
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking whether it would make sense to keep the old value on Linux as canary check that something is broken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this level I would like to have the same tests, behavior, limits etc. and say with confidence that every platform supports at least 50 subscribers.

I completely understand your rational, but when we go this route we may have a wild mixture of verified upper limits for every platform. I would rather go the path that we say 50 subscribers are supported for every platform, but an individual higher limit for a specific platform is maybe working.

Maybe we can introduce platform specific system limit tests where exactly those differences can be verified. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. These limits might be a criteria for tier 2 and for tier 1 we could have tests for additional limits.

.cirrus.yml Outdated Show resolved Hide resolved
iceoryx2-pal/posix/src/macos/mman.rs Show resolved Hide resolved
iceoryx2-pal/posix/src/macos/mman.rs Show resolved Hide resolved
iceoryx2-pal/posix/src/macos/mman.rs Show resolved Hide resolved
iceoryx2-pal/posix/src/macos/mman.rs Outdated Show resolved Hide resolved
@elfenpiff elfenpiff force-pushed the iox2-51-shm-long-name-support branch from 471a73e to 0ea33c1 Compare December 21, 2023 06:59
…nsure that null terminator is always present
@elfenpiff elfenpiff requested a review from elBoberido December 21, 2023 07:27
@elfenpiff elfenpiff force-pushed the iox2-51-shm-long-name-support branch 2 times, most recently from 8cae210 to 903597d Compare December 22, 2023 11:16
@elfenpiff elfenpiff force-pushed the iox2-51-shm-long-name-support branch from 903597d to edf0eef Compare December 22, 2023 11:58
elBoberido
elBoberido previously approved these changes Dec 22, 2023
iceoryx2-pal/posix/src/macos/mman.rs Outdated Show resolved Hide resolved
…, rename wait_while into blocking_wait_while to be consistent with other blocking functions
Copy link

codecov bot commented Dec 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5d54039) 77.90% compared to head (a8cb41f) 77.89%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
- Coverage   77.90%   77.89%   -0.02%     
==========================================
  Files         172      172              
  Lines       18664    18664              
==========================================
- Hits        14541    14539       -2     
- Misses       4123     4125       +2     
Files Coverage Δ
iceoryx2-bb/posix/src/condition_variable.rs 75.23% <100.00%> (ø)
iceoryx2-bb/posix/src/thread.rs 55.75% <ø> (ø)
iceoryx2-cal/src/event/process_local.rs 85.82% <100.00%> (ø)

... and 1 file with indirect coverage changes

@elfenpiff elfenpiff merged commit 03a4ec2 into eclipse-iceoryx:main Dec 24, 2023
20 of 21 checks passed
@elfenpiff elfenpiff deleted the iox2-51-shm-long-name-support branch December 24, 2023 01:17
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