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.

Sign up for GitHub

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

Jump to bottom

Refactor OS::Mutex in CMake selection #2790

Merged
merged 24 commits into from
Aug 2, 2024
Merged

Refactor OS::Mutex in CMake selection #2790

merged 24 commits into from
Aug 2, 2024

Conversation

thomas-bc
Copy link
Collaborator

@thomas-bc thomas-bc commented Jun 27, 2024 •
edited
Loading

Related Issue(s) #2722
Has Unit Tests (y/n) yes
Documentation Included (y/n) OSAL

Change Description

Refactor the Mutex part of the OSAL layer to use the new system!

Rationale

See #2722

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 27, 2024
View reviewed changes
Os/Posix/Mutex.cpp
delete reinterpret_cast<pthread_mutex_t*>(this->m_handle);
}
PosixMutex::Status PosixMutex::release() {
PlatformIntType status = pthread_mutex_unlock(&this->m_handle.m_mutex_descriptor);

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type int rather than a typedef with size and signedness.
Os/Posix/Mutex.cpp
this->m_handle = reinterpret_cast<POINTER_CAST>(handle);
}
PosixMutex::Status PosixMutex::take() {
PlatformIntType status = pthread_mutex_lock(&this->m_handle.m_mutex_descriptor);

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type int rather than a typedef with size and signedness.
Os/Posix/Mutex.cpp
stat = pthread_mutex_init(handle,&attr);
FW_ASSERT(stat == 0,stat);
PosixMutex::~PosixMutex() {
PlatformIntType status = pthread_mutex_destroy(&this->m_handle.m_mutex_descriptor);

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type int rather than a typedef with size and signedness.
Os/Posix/Mutex.cpp Fixed Show fixed Hide fixed
Os/Posix/error.cpp
@@ -70,5 +70,23 @@
return status;
}

Mutex::Status posix_status_to_mutex_status(PlatformIntType posix_status){

Check notice

Code scanning / CodeQL

Use of basic integral type Note

posix_status uses the basic integral type int rather than a typedef with size and signedness.
Os/Posix/Mutex.hpp Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 27, 2024
View reviewed changes
Os/Mutex.hpp
Status release() override; //!< unlock the mutex and get return status
void lock() override; //!< lock the mutex
void unLock() override; //!< unlock the mutex
void unlock() { this->unLock(); } //!< alias for unLock to meet BasicLockable requirements

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 3 statements; only one is allowed.
Os/Mutex.hpp Dismissed Show dismissed Hide dismissed
Os/Posix/Mutex.hpp
namespace Posix {
namespace Mutex {

struct PosixMutexHandle : public MutexHandle {

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
Os/Posix/error.cpp
@@ -70,5 +70,23 @@
return status;
}

Mutex::Status posix_status_to_mutex_status(PlatformIntType posix_status){

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 27, 2024
View reviewed changes
Os/Posix/error.cpp
@@ -70,5 +70,23 @@
return status;
}

Mutex::Status posix_status_to_mutex_status(PlatformIntType posix_status){
Mutex::Status status = Mutex::Status::ERROR_OTHER;
switch (posix_status) {

Check warning

Code scanning / CodeQL

Unchecked function argument Warning

This use of parameter posix_status has not been checked.
Os/Posix/Mutex.cpp Fixed Show resolved Hide resolved
@thomas-bc thomas-bc marked this pull request as ready for review July 2, 2024 22:44
@thomas-bc thomas-bc requested a review from LeStarch July 2, 2024 23:15
@thomas-bc
Copy link
Collaborator Author

@LeStarch ready for review when you are - no rush

thomas-bc
thomas-bc commented Jul 31, 2024
View reviewed changes
Copy link
Collaborator Author

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

Peer review comments

Os/Posix/CMakeLists.txt Outdated
set(SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/Mutex.cpp"
# "${CMAKE_CURRENT_LIST_DIR}/DefaultMutex.cpp"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct - delete and make sure it's not in other modules as well

Os/Mutex.cpp
}

Mutex::Status Mutex::take() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

implement take/release calling the others here

Os/Posix/Mutex.cpp Fixed Show resolved Hide resolved
Os/Posix/test/ut/PosixMutexTests.cpp Outdated
Os::Test::Mutex::Tester::LockMutex lock_rule;
Os::Test::Mutex::Tester::UnlockMutex unlock_rule;
lock_rule.apply(*tester);
ASSERT_DEATH_IF_SUPPORTED(delete tester.release(), Os::Test::Mutex::Tester::ASSERT_IN_MUTEX_CPP);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why tester.release() and not just tester ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tester is a std::unique_ptr. The .release() call here is that of the unique_ptr, not Mutex::release(). I will add a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But actually, since the deletion is not expected to succeed... maybe we should get() instead so that the destructor can be called again and the unique_ptr and underlying Mutex does get destroyed successfully?

Os/test/ut/mutex/MutexRules.cpp Outdated
state.m_mutex.lock();
state.m_state = Os::Test::Mutex::Tester::MutexState::LOCKED;

state.m_value = 42;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use STest random number here instead

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 2, 2024
View reviewed changes
Os/Posix/Mutex.cpp
PosixMutex::PosixMutex() : Os::MutexInterface(), m_handle() {
// set attributes
pthread_mutexattr_t attribute;
PlatformIntType status = pthread_mutexattr_init(&attribute);

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type int rather than a typedef with size and signedness.
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 2, 2024
View reviewed changes
Os/Posix/Mutex.cpp
Mutex::Mutex() {
pthread_mutex_t* handle = new(std::nothrow) pthread_mutex_t;
FW_ASSERT(handle != nullptr);
PosixMutex::PosixMutex() : Os::MutexInterface(), m_handle() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@thomas-bc thomas-bc merged commit dd463e1 into nasa:devel Aug 2, 2024
35 checks passed
@thomas-bc thomas-bc deleted the cmake-os-mutex branch August 2, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeStarch LeStarch LeStarch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomas-bc @LeStarch