Iox #805 enhance posix call #807

elBoberido · 2021-05-20T20:54:15Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This PR refactors the posixCall to handle a common case were errnos are ignored just to suppress error logging.

Additionaly, the handling of returning the errno value instead of setting the errno is now handled explicit.

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
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes Enhance posixCall #805
Closes Segmentation Fault in FileLock #808

Signed-off-by: Mathias Kraus <[email protected]>

… tests Signed-off-by: Mathias Kraus <[email protected]>

…dErrnos Signed-off-by: Mathias Kraus <[email protected]>

…ntainValue Signed-off-by: Mathias Kraus <[email protected]>

…algorithm::doesContainValue Signed-off-by: Mathias Kraus <[email protected]>

…n returns the errno value instead of setting it Signed-off-by: Mathias Kraus <[email protected]>

Signed-off-by: Mathias Kraus <[email protected]>

…iate Signed-off-by: Mathias Kraus <[email protected]>

elBoberido · 2021-05-20T20:54:51Z

cmake/googletest/CMakeLists.txt

@@ -72,7 +72,7 @@ if(BUILD_TEST)
            WORKING_DIRECTORY "${SOURCE_DIR}"
            RESULT_VARIABLE result)
        if(result)
-            message(FATAL_ERROR "CMake step [patch] for googletest failed: ${result}")
+            message(WARNING "CMake step [patch] for googletest failed: ${result}! Build of gtest might fail")


had to do this since I had problems running cmake mutliple times

Mmmhhh thats not so nice. Is it possible to run this command only once when we clone the repo and when its already present we skip it?

I could not find a solution for this but in the end it doesn't matter. If this doesn't stop the build the -Werror does

iceoryx_hoofs/include/iceoryx_hoofs/cxx/algorithm.hpp

iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/posix_call.hpp

Signed-off-by: Mathias Kraus <[email protected]>

codecov · 2021-05-20T21:52:56Z

Codecov Report

Merging #807 (f8956e1) into master (2414145) will increase coverage by 0.08%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
+ Coverage   74.40%   74.49%   +0.08%     
==========================================
  Files         322      323       +1     
  Lines       11532    11521      -11     
  Branches     1956     1950       -6     
==========================================
+ Hits         8580     8582       +2     
+ Misses       2192     2180      -12     
+ Partials      760      759       -1

Flag	Coverage Δ
unittests	`73.33% <95.23%> (+0.10%)`	⬆️
unittests_timing	`31.09% <63.49%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...include/iceoryx_hoofs/posix_wrapper/posix_call.hpp	`100.00% <ø> (ø)`
iceoryx_posh/source/roudi/process_manager.cpp	`60.35% <0.00%> (-0.18%)`	⬇️
...eoryx_hoofs/source/posix_wrapper/message_queue.cpp	`71.42% <91.66%> (+1.09%)`	⬆️
...s/include/iceoryx_hoofs/internal/cxx/algorithm.inl	`100.00% <100.00%> (ø)`
...ceoryx_hoofs/internal/posix_wrapper/posix_call.inl	`100.00% <100.00%> (ø)`
iceoryx_hoofs/source/posix_wrapper/file_lock.cpp	`36.36% <100.00%> (+4.36%)`	⬆️
iceoryx_hoofs/source/posix_wrapper/mutex.cpp	`81.25% <100.00%> (ø)`
iceoryx_hoofs/source/posix_wrapper/semaphore.cpp	`63.90% <100.00%> (ø)`
...six_wrapper/shared_memory_object/shared_memory.cpp	`65.71% <100.00%> (+0.24%)`	⬆️
..._hoofs/source/posix_wrapper/unix_domain_socket.cpp	`62.28% <100.00%> (+2.79%)`	⬆️
... and 5 more

Signed-off-by: Mathias Kraus <[email protected]>

iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/posix_call.inl

iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/posix_call.hpp

FerdinandSpitzschnueffler · 2021-05-21T07:52:42Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/algorithm.hpp

+/// @param[in] remainingValueListEntries are the remaining variadic arguments of ValueList
+template <typename T, typename... ValueList>
+inline constexpr bool
+doesContainValue(const T value, const T firstValueListEntry, const ValueList... remainingValueListEntries) noexcept;


I have a feeling of déjà vu. Didn't we already implement this helper function? I couldn't find it though.

I have the same feeling but maybe I am mixing it up with doesContainType which we use to verify if the cxx::variant contains a certain type.

I couldn't find something similar in helplets or algorithm. Maybe it's buried in some class

elfenpiff

The API looks even more cleaner now. Thank you.

elfenpiff · 2021-05-21T08:21:57Z

cmake/googletest/CMakeLists.txt

@@ -72,7 +72,7 @@ if(BUILD_TEST)
            WORKING_DIRECTORY "${SOURCE_DIR}"
            RESULT_VARIABLE result)
        if(result)
-            message(FATAL_ERROR "CMake step [patch] for googletest failed: ${result}")
+            message(WARNING "CMake step [patch] for googletest failed: ${result}! Build of gtest might fail")


Mmmhhh thats not so nice. Is it possible to run this command only once when we clone the repo and when its already present we skip it?

elfenpiff · 2021-05-21T08:23:40Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/algorithm.hpp

+/// @param[in] remainingValueListEntries are the remaining variadic arguments of ValueList
+template <typename T, typename... ValueList>
+inline constexpr bool
+doesContainValue(const T value, const T firstValueListEntry, const ValueList... remainingValueListEntries) noexcept;


I have the same feeling but maybe I am mixing it up with doesContainType which we use to verify if the cxx::variant contains a certain type.

iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/posix_call.hpp

iceoryx_hoofs/test/moduletests/test_posix_posix_call.cpp

Signed-off-by: Mathias Kraus <[email protected]>

…ssErrorMessagesForErrnos Signed-off-by: Mathias Kraus <[email protected]>

…or message suppresion Signed-off-by: Mathias Kraus <[email protected]>

Signed-off-by: Mathias Kraus <[email protected]>

iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/posix_call.inl

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido added 8 commits May 20, 2021 22:25

iox-eclipse-iceoryx#805 add ignoreErrnos method to posixCall

88b8a22

Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 replace evaluateWithInoredErrnos in posixCall…

014e513

… tests Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 use ingoreErrnos and remove evaluateWithInore…

da2da25

…dErrnos Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 refactor isErrnumIgnored to algorithm::doesCo…

7e75cac

…ntainValue Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 simplify success/failureReturnValue by using …

9c9ef77

…algorithm::doesContainValue Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 add explicit method for cases where a functio…

990c47e

…n returns the errno value instead of setting it Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 add method to suppress printing of errno warning

c64e961

Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 use suppressErrorLoggingOfErrnos where apropr…

7d37274

…iate Signed-off-by: Mathias Kraus <[email protected]>

elBoberido added enhancement New feature refactoring Refactor code without adding features labels May 20, 2021

elBoberido requested review from mossmaurice, elfenpiff and FerdinandSpitzschnueffler May 20, 2021 20:54

elBoberido self-assigned this May 20, 2021

elBoberido commented May 20, 2021

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/cxx/algorithm.hpp Show resolved Hide resolved

elBoberido commented May 20, 2021

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/posix_call.hpp Outdated Show resolved Hide resolved

iox-eclipse-iceoryx#805 add tests for algorithm::doesContainValue

24aca05

Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#808 fix segfault in FileLock

7d3b381

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido force-pushed the iox-#805-enhance-posixCall branch 2 times, most recently from 061c51f to 04ceec2 Compare May 20, 2021 22:23

iox-eclipse-iceoryx#805 extend tests for ignoreErrnos

3eddd5f

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido force-pushed the iox-#805-enhance-posixCall branch from 04ceec2 to 3eddd5f Compare May 20, 2021 22:34

FerdinandSpitzschnueffler reviewed May 21, 2021

View reviewed changes

elfenpiff reviewed May 21, 2021

View reviewed changes

iox-eclipse-iceoryx#805 remove dead code

41a59a1

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido force-pushed the iox-#805-enhance-posixCall branch from 6f69fb7 to 41a59a1 Compare May 21, 2021 09:29

elBoberido added 2 commits May 21, 2021 12:06

iox-eclipse-iceoryx#805 add tests for suppressed errno messages

e4a441a

Signed-off-by: Mathias Kraus <[email protected]>

iox-eclipse-iceoryx#805 rename suppressErrorLoggingOfErrnos to suppre…

82c6740

…ssErrorMessagesForErrnos Signed-off-by: Mathias Kraus <[email protected]>

elBoberido requested review from elfenpiff and FerdinandSpitzschnueffler May 21, 2021 10:17

iox-eclipse-iceoryx#805 add test for combinded errno ignoring and err…

28fb7a0

…or message suppresion Signed-off-by: Mathias Kraus <[email protected]>

elBoberido force-pushed the iox-#805-enhance-posixCall branch from 8f020ea to 28fb7a0 Compare May 21, 2021 10:20

iox-eclipse-iceoryx#805 revert a change in MessageQueue::timedReceive

f445b3b

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido force-pushed the iox-#805-enhance-posixCall branch from 50d1890 to f445b3b Compare May 21, 2021 10:47

FerdinandSpitzschnueffler previously approved these changes May 21, 2021

View reviewed changes

elfenpiff reviewed May 21, 2021

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/posix_call.inl Outdated Show resolved Hide resolved

elfenpiff previously approved these changes May 21, 2021

View reviewed changes

iox-eclipse-iceoryx#805 add ctor to PosixCallDetails

f8956e1

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido dismissed stale reviews from elfenpiff and FerdinandSpitzschnueffler via f8956e1 May 21, 2021 12:52

elfenpiff approved these changes May 21, 2021

View reviewed changes

elBoberido requested a review from FerdinandSpitzschnueffler May 21, 2021 12:54

FerdinandSpitzschnueffler approved these changes May 21, 2021

View reviewed changes

elBoberido merged commit e6f5999 into eclipse-iceoryx:master May 21, 2021

elBoberido deleted the iox-#805-enhance-posixCall branch May 21, 2021 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #805 enhance posix call #807

Iox #805 enhance posix call #807

elBoberido commented May 20, 2021 •

edited

Loading

elBoberido May 20, 2021

elfenpiff May 21, 2021

elBoberido May 21, 2021

codecov bot commented May 20, 2021 •

edited

Loading

FerdinandSpitzschnueffler May 21, 2021

elfenpiff May 21, 2021

elBoberido May 21, 2021

elfenpiff left a comment

elfenpiff May 21, 2021

elfenpiff May 21, 2021

Iox #805 enhance posix call #807

Iox #805 enhance posix call #807

Conversation

elBoberido commented May 20, 2021 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 20, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elfenpiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido commented May 20, 2021 •

edited

Loading

codecov bot commented May 20, 2021 •

edited

Loading