iox-#1821 improve error message when creating read-only segment

[tobiasblass]

Pre-Review Checklist for the PR Author

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

Notes for Reviewer

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 Creating a read-only SharedMemoryObject  #1821

[tobiasblass]

@elfenpiff I finally got around to addressing the confusing error message when combining READ_ONLY with CREATE in shared-memory segments.

In our last discussion you suggested that we shouldn't handle this with an error; instead, you proposed the following API that makes the error impossible:

auto shmCreated = SharedMemoryObjectBuilder()
                    // all the usual settings without accessMode, is always created with ReadWrite
                    .create();
auto shmOpen = SharedMemoryObjectBuilder()
                    // now it is also definitely clear that we open the shm and the other owns it
                    .open(AccessMode::READ); 

Thinking more about it, I'm a bit concerned that this re-defines the meaning of create. Additionally, it breaks the convention of all the other builders that create refers to "execute the builder", not to "perform a create operation on the thing being built".

The second thing that feels wrong is that the open/create distinction overlaps OpenMode, so we're introducing a new argument incompatibility while closing the old one.

Do you see a good way to avoid this? Or should I just add a new return code, say, INCOMPATIBLE_ARGUMENTS to address the original problem?

[codecov]

Codecov Report

Merging #1822 (d4396a5) into master (8d8b66d) will increase coverage by 0.00%.
The diff coverage is 90.90%.

@@           Coverage Diff           @@
##           master    #1822   +/-   ##
=======================================
  Coverage   75.43%   75.43%           
=======================================
  Files         377      377           
  Lines       14625    14627    +2     
  Branches     2086     2087    +1     
=======================================
+ Hits        11032    11034    +2     
- Misses       2960     2961    +1     
+ Partials      633      632    -1     

Flag	Coverage Δ
unittests	75.10% <90.90%> (+<0.01%)	⬆️
unittests_timing	15.24% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...six_wrapper/shared_memory_object/shared_memory.hpp	100.00% <ø> (ø)
...six_wrapper/shared_memory_object/shared_memory.cpp	64.49% <90.90%> (+1.02%)	⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp	74.28% <0.00%> (-5.72%)	⬇️
iceoryx_posh/source/roudi/port_manager.cpp	84.36% <0.00%> (+0.18%)	⬆️

[elfenpiff]

I think a good solution would be to add a check right before ftrunctate in line 122.
If here the shared memory hasOwnership and the m_accessMode == AccessMode::READ_ONLY it should fail.

For this you could introduce a new SharedMemoryError enum value and return it here in combination with an error message. This could then also be verified in the unit test and wouldn't fail in windows.

Or you could do a check right in the beginning of the create() method to verify that OpenMode::**CREATE** is not combined with AccessMode::READ_ONLY.

[tobiasblass]

@elfenpiff I implemented something similar to your suggestion. Checking before the truncate would work, but it could potentially hide errors. I believe we want to complain about READ_ONLY + OPEN_OR_CREATE, even if the file happens to already exist.

So I moved the check up, before the open-if-exist attempt. Since hasOwnership now has to be in scope during the open-check, I also used the opportunity to simplify the code there (the non-owning success path can now follow the same code path as the owning success path, with hasOwnership distinguishing between the two).

Pipeline seems to run through, the invalid links are not caused by this MR.

/cc @mossmaurice

[elBoberido]

Looks good. Sorry for the long delay in review.

[elBoberido]

@tobiasblass ... oh, and it seems you have to rebase

[tobiasblass]

@elfenpiff I've rebased to latest master. Can you re-approve for the second approval?

[elBoberido]

@tobiasblass took quite some time but finally it is merged. For the next PR we have to improve our review time :)