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 #1054 unix platform support #1055

Merged

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Jan 30, 2022

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 messages are signed (git commit -s)
    • 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. Assign PR to reviewer

Notes for Reviewer

  • The platform files are all copies of the linux platform with minor adjustments
    • acl was replaced with the empty implementation version from the windows platform since they do not work with ZFS out of the box
    • IOX_UDS_SOCKET_MAX_MESSAGE_SIZE was reduced to 1024 in platform_settings.hpp
    • Add #include <unistd.h> in grp.cpp.
    • Add #define PTHREAD_MUTEX_RECURSIVE_NP PTHREAD_MUTEX_RECURSIVE and #define PTHREAD_MUTEX_FAST_NP PTHREAD_MUTEX_DEFAULT in pthread.hpp since our implementation is not POSIX conform
  • Fix wrong handling of the mqd_t message queue descriptor. A message queue descriptor is NOT a file descriptor. Stated even as a warning in the posix manual! But we use it nevertheless as a filedescriptor for instance in combination with fstat or when defining an invalid version. This was corrected mainly in the message queue posix wrapper.
  • Fix mixup between mode_t and oflags in the semaphore wrapper. The values are somehow compatible in linux, in FreeBSD we get luckily an invalid argument error.
  • When no platform is successfully detected we always assume its Unix but with an I am feeling lucky warning.

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
    • Each unit test case has a unique UUID
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • 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

@elfenpiff elfenpiff force-pushed the iox-#1054-unix-platform-support branch from 0dccc80 to 11e262a Compare January 30, 2022 12:19
@elfenpiff elfenpiff self-assigned this Jan 30, 2022
@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #1055 (9571a90) into master (1db8749) will decrease coverage by 1.53%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1055      +/-   ##
==========================================
- Coverage   77.78%   76.24%   -1.54%     
==========================================
  Files         343      343              
  Lines       12957    12953       -4     
  Branches     1864     1863       -1     
==========================================
- Hits        10078     9876     -202     
- Misses       2262     2463     +201     
+ Partials      617      614       -3     
Flag Coverage Δ
unittests 75.47% <0.00%> (-1.53%) ⬇️
unittests_timing 12.58% <0.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...eoryx_hoofs/source/posix_wrapper/message_queue.cpp 0.00% <0.00%> (-79.04%) ⬇️
iceoryx_hoofs/platform/linux/source/mqueue.cpp 0.00% <0.00%> (-50.00%) ⬇️
...oryx_hoofs/source/posix_wrapper/access_control.cpp 37.09% <0.00%> (-29.04%) ⬇️
...hoofs/source/posix_wrapper/posix_access_rights.cpp 67.02% <0.00%> (-6.39%) ⬇️
iceoryx_hoofs/source/posix_wrapper/named_pipe.cpp 58.54% <0.00%> (-4.15%) ⬇️
...e/iceoryx_hoofs/internal/concurrent/smart_lock.inl 92.45% <0.00%> (-1.89%) ⬇️
...fs/include/iceoryx_hoofs/internal/cxx/expected.inl 96.34% <0.00%> (-1.83%) ⬇️
iceoryx_hoofs/source/posix_wrapper/timer.cpp 63.07% <0.00%> (-0.83%) ⬇️
iceoryx_posh/source/roudi/port_manager.cpp 78.70% <0.00%> (+0.32%) ⬆️
... and 1 more

@@ -204,8 +204,8 @@ cxx::expected<std::string, IpcChannelError> MessageQueue::receive() const noexce
return cxx::success<std::string>(std::string(&(message[0])));
}

cxx::expected<int32_t, IpcChannelError> MessageQueue::open(const IpcChannelName_t& name,
const IpcChannelSide channelSide) noexcept
cxx::expected<mqd_t, IpcChannelError> MessageQueue::open(const IpcChannelName_t& name,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hint: mqd_t is not a file descriptor and therefore not convertable to an int32_t this worked just by accident.

@@ -323,13 +323,7 @@ cxx::expected<IpcChannelError> MessageQueue::timedSend(const std::string& msg,

cxx::expected<bool, IpcChannelError> MessageQueue::isOutdated() noexcept
{
struct stat sb = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hint: fstat requires as first input parameter a file descriptor. According to the posix manual mqd_t is a message queue descriptor and not a file descriptor. This was just a happy little accident that in linux there was actually a file descriptor used in the implementation.
But this could change from one linux version to another and then the whole code breaks.

In the end we exploited undefined behavior.

Copy link
Member

Choose a reason for hiding this comment

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

In this case this function could be removed or we need to find another way how to check whether the MessageQueue is outdated. On the other hand, this was a workaround to fix a startup race with RouDi and the applications and since we do not use the MessageQueue anymore, it is probably no harm to remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a ticket and I added this as a task into the ticket #832

@@ -138,7 +138,7 @@ TEST_F(SemaphoreCreate_test, OpenNamedSemaphore)
{
::testing::Test::RecordProperty("TEST_ID", "349cdf0d-987e-4e2f-aa35-98a40fdf979b");
auto semaphore = iox::posix::Semaphore::create(iox::posix::CreateNamedSemaphore, "/fuuSem", S_IRUSR | S_IWUSR, 10);
auto semaphore2 = iox::posix::Semaphore::create(iox::posix::OpenNamedSemaphore, "/fuuSem", S_IRUSR | S_IWUSR);
auto semaphore2 = iox::posix::Semaphore::create(iox::posix::OpenNamedSemaphore, "/fuuSem", O_CREAT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hint: Another bug uncovered by a unix posix system. The second parameter are the oflags and not mode_t.
By accident the mode_t values were compatible to the oflag values in this test. But who knows what they meant here.

@elfenpiff elfenpiff force-pushed the iox-#1054-unix-platform-support branch from 67c9d83 to f8af3af Compare January 30, 2022 13:11
@elfenpiff elfenpiff linked an issue Jan 30, 2022 that may be closed by this pull request
@elfenpiff elfenpiff marked this pull request as ready for review January 30, 2022 18:00
@dkroenke dkroenke force-pushed the iox-#1054-unix-platform-support branch 7 times, most recently from 1c0251d to 6f19f94 Compare February 2, 2022 17:43
dkroenke
dkroenke previously approved these changes Feb 3, 2022
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Looks good. Just a minor issue. I wish we could use pragma once

@@ -323,13 +323,7 @@ cxx::expected<IpcChannelError> MessageQueue::timedSend(const std::string& msg,

cxx::expected<bool, IpcChannelError> MessageQueue::isOutdated() noexcept
{
struct stat sb = {};
Copy link
Member

Choose a reason for hiding this comment

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

In this case this function could be removed or we need to find another way how to check whether the MessageQueue is outdated. On the other hand, this was a workaround to fix a startup race with RouDi and the applications and since we do not use the MessageQueue anymore, it is probably no harm to remove this.

Signed-off-by: Christian Eltzschig <me@elchris.org>
…queue descriptor is not a file descriptor

Signed-off-by: Christian Eltzschig <me@elchris.org>
…ementation for unix, link unix with rt

Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
…g /usr/local/bin/bash to /bin/bash

Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
…o on unix

Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
…message queue from isOutdatedTest

Signed-off-by: Christian Eltzschig <me@elchris.org>
…o zero in test

Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff elfenpiff force-pushed the iox-#1054-unix-platform-support branch from 400c30f to 9571a90 Compare February 4, 2022 10:22
@elfenpiff elfenpiff merged commit 7290937 into eclipse-iceoryx:master Feb 4, 2022
@elfenpiff elfenpiff deleted the iox-#1054-unix-platform-support branch February 4, 2022 12:26
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.

Unix platform support to support FreeBSD etc.
3 participants