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

[#81] fix signal handler #84

Merged
merged 3 commits into from
Jan 14, 2024

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Jan 14, 2024

Notes for Reviewer

The original problem was that I defined a struct sigaction_t in rust posix types and reinterpreted it has the posix c type sigaction, this worked for all platforms, but this was pure luck since posix states nothing about the member order or hidden members. It is not possible to use the sigaction struct directly from bindgen since some libc implementation have weird C macro hacks. So there are posix libc implementations that strictly speaking violate the posix standard and correct this with a macro called sa_handler (the member they renamed to something else).
With bindgen it ends up with different struct member names on different libc implementation therefore, there was nothing else possible but to create an iox2_sigaction struct in posix.h and do the translation in the c function iox2_sigaction_func since C supports these weird macro hacks in a platform independent way.

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.

Relates to #81

… since it can have different layouts (linux)
@elfenpiff elfenpiff requested a review from elBoberido January 14, 2024 18:25
@elfenpiff elfenpiff self-assigned this Jan 14, 2024
Copy link

codecov bot commented Jan 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1607c02) 78.01% compared to head (9192cad) 78.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
- Coverage   78.01%   78.00%   -0.01%     
==========================================
  Files         172      172              
  Lines       18791    18778      -13     
==========================================
- Hits        14659    14647      -12     
+ Misses       4132     4131       -1     
Files Coverage Δ
iceoryx2-bb/posix/src/signal.rs 85.02% <100.00%> (-0.07%) ⬇️
iceoryx2-pal/posix/src/linux/dirent.rs 100.00% <ø> (ø)
iceoryx2-pal/posix/src/linux/signal.rs 100.00% <100.00%> (ø)
iceoryx2-pal/posix/src/linux/types.rs 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

@elfenpiff elfenpiff force-pushed the iox2-81-fix-signal-handler branch from c965b3b to 92f4fed Compare January 14, 2024 18:34
@elfenpiff elfenpiff changed the title Iox2 81 fix signal handler [#81] fix signal handler Jan 14, 2024
@elfenpiff elfenpiff force-pushed the iox2-81-fix-signal-handler branch 3 times, most recently from a7cff2e to dbe188a Compare January 14, 2024 19:04
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 but I think you mixed up one variable

iceoryx2-pal/posix/src/c/sigaction.c Outdated Show resolved Hide resolved
iceoryx2-pal/posix/src/c/socket_macros.c Show resolved Hide resolved
@elfenpiff elfenpiff force-pushed the iox2-81-fix-signal-handler branch from dbe188a to 9192cad Compare January 14, 2024 22:08
@elfenpiff elfenpiff requested a review from elBoberido January 14, 2024 22:10
@elfenpiff elfenpiff merged commit 891ae9b into eclipse-iceoryx:main Jan 14, 2024
22 of 23 checks passed
@elfenpiff elfenpiff deleted the iox2-81-fix-signal-handler branch January 14, 2024 22:25
@elBoberido elBoberido linked an issue Jan 14, 2024 that may be closed by this pull request
2 tasks
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.

benchmark-publish subscripe panic
2 participants