Re: [devel] [PATCH 1/1] base: Destructor of TraceLog causes coredump V2 [#2860]
Hi Hans, Place it on heap so destructor won't be called as part of __do_global_dtor_aux I guess, it should be equivalent to the previous of TraceLog regarding this issue. I think you can make "if (mutex_ && mutext_->good())", it can pass the check mutex_ against null but not sure after that. I will try the shared_ptr and test it to see if it is good to go. Thanks, Minh On 24/05/18 16:26, Hans Nordeback wrote: Hi Minh, yes gl_trace/gl_log can be put on the heap, you mean without any owner? But in this case when destructors for one of the threads has been run other threads should notice that some states may not be valid anymore, e.g. that mutex_->good() returns false, but if mutex_ has been released it should be if (mutex_ && mutex_->good() instead. /Thanks HansN On 05/24/2018 06:20 AM, Hans Nordebäck wrote: Hi Minh, yes you are right about the possibility for a segv, but using a std::shared_ptr instead of the naked ptr may be an option ? /Thanks Hans Från: Minh Hon ChauSkickat: den 24 maj 2018 02:34:13 Till: Hans Nordebäck; Anders Widell; Gary Lee Kopia: opensaf-devel@lists.sourceforge.net Ämne: Re: [PATCH 1/1] base: Destructor of TraceLog causes coredump V2 [#2860] Hi Hans, It is good to give an option to Mutex class not to abort. We can avoid the abort in mutex_unlock (as reported in coredump), but I feel the issue is still there. We may hit a problem (segv?) with "mutex_->good()" since the other thread is wiping out the mutex_ in destructor, it is a matter of timing to happen I guess. As we don't have (and don't want to have) any protection between two threads for the TraceLog, so the good one (I hope) is making one of those threads not to touch the TraceLog. If you don't like to remove the destructor, another way is locating the gl_trace/gl_log to the HEAP? Thanks, Minh On 23/05/18 20:50, Hans Nordeback wrote: Change Mutex class to make it possible for caller to decide if abort --- src/base/logtrace_client.cc | 5 - src/base/mutex.cc | 2 +- src/base/mutex.h | 22 +- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/base/logtrace_client.cc b/src/base/logtrace_client.cc index 0dac6d389..f597c1ae3 100644 --- a/src/base/logtrace_client.cc +++ b/src/base/logtrace_client.cc @@ -76,7 +76,7 @@ bool TraceLog::Init(const char *msg_id, WriteMode mode) { msg_id_ = base::LogMessage::MsgId{msg_id}; log_socket_ = new base::UnixClientSocket{Osaflog::kServerSocketPath, static_cast(mode)}; - mutex_ = new base::Mutex{}; + mutex_ = new base::Mutex{false}; return true; } @@ -91,6 +91,9 @@ void TraceLog::Log(base::LogMessage::Severity severity, const char *fmt, void TraceLog::LogInternal(base::LogMessage::Severity severity, const char *fmt, va_list ap) { base::Lock lock(*mutex_); + + if (!mutex_->good()) return; + uint32_t id = sequence_id_; sequence_id_ = id < kMaxSequenceId ? id + 1 : 1; buffer_.clear(); diff --git a/src/base/mutex.cc b/src/base/mutex.cc index 5fa6ac55a..1627ac20b 100644 --- a/src/base/mutex.cc +++ b/src/base/mutex.cc @@ -20,7 +20,7 @@ namespace base { -Mutex::Mutex() : mutex_{} { +Mutex::Mutex(bool abort) : abort_{abort}, mutex_{}, result_{0} { pthread_mutexattr_t attr; int result = pthread_mutexattr_init(); if (result != 0) osaf_abort(result); diff --git a/src/base/mutex.h b/src/base/mutex.h index 7b3cee187..e3c54a711 100644 --- a/src/base/mutex.h +++ b/src/base/mutex.h @@ -31,30 +31,34 @@ namespace base { class Mutex { public: using NativeHandleType = pthread_mutex_t*; - Mutex(); + Mutex(bool abort = true); ~Mutex(); void Lock() { - int result = pthread_mutex_lock(_); - if (result != 0) osaf_abort(result); + result_ = pthread_mutex_lock(_); + if (abort_ && result_ != 0) osaf_abort(result_); } bool TryLock() { - int result = pthread_mutex_trylock(_); - if (result == 0) { + result_ = pthread_mutex_trylock(_); + if (result_ == 0) { return true; - } else if (result == EBUSY) { + } else if (result_ == EBUSY) { return false; } else { - osaf_abort(result); + if (abort_) osaf_abort(result_); + return false; } } void Unlock() { - int result = pthread_mutex_unlock(_); - if (result != 0) osaf_abort(result); + result_ = pthread_mutex_unlock(_); + if (abort_ && result_ != 0) osaf_abort(result_); } NativeHandleType native_handle() { return _; } + bool good() const {return result_ == 0;}; private: + bool abort_; pthread_mutex_t mutex_; + int result_; DELETE_COPY_AND_MOVE_OPERATORS(Mutex); }; -- Check out the vibrant tech community on one of the world's most engaging tech sites,
Re: [devel] [PATCH 1/1] base: Destructor of TraceLog causes coredump V2 [#2860]
Hi Minh, yes gl_trace/gl_log can be put on the heap, you mean without any owner? But in this case when destructors for one of the threads has been run other threads should notice that some states may not be valid anymore, e.g. that mutex_->good() returns false, but if mutex_ has been released it should be if (mutex_ && mutex_->good() instead. /Thanks HansN On 05/24/2018 06:20 AM, Hans Nordebäck wrote: Hi Minh, yes you are right about the possibility for a segv, but using a std::shared_ptr instead of the naked ptr may be an option ? /Thanks Hans Från: Minh Hon ChauSkickat: den 24 maj 2018 02:34:13 Till: Hans Nordebäck; Anders Widell; Gary Lee Kopia: opensaf-devel@lists.sourceforge.net Ämne: Re: [PATCH 1/1] base: Destructor of TraceLog causes coredump V2 [#2860] Hi Hans, It is good to give an option to Mutex class not to abort. We can avoid the abort in mutex_unlock (as reported in coredump), but I feel the issue is still there. We may hit a problem (segv?) with "mutex_->good()" since the other thread is wiping out the mutex_ in destructor, it is a matter of timing to happen I guess. As we don't have (and don't want to have) any protection between two threads for the TraceLog, so the good one (I hope) is making one of those threads not to touch the TraceLog. If you don't like to remove the destructor, another way is locating the gl_trace/gl_log to the HEAP? Thanks, Minh On 23/05/18 20:50, Hans Nordeback wrote: Change Mutex class to make it possible for caller to decide if abort --- src/base/logtrace_client.cc | 5 - src/base/mutex.cc | 2 +- src/base/mutex.h| 22 +- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/base/logtrace_client.cc b/src/base/logtrace_client.cc index 0dac6d389..f597c1ae3 100644 --- a/src/base/logtrace_client.cc +++ b/src/base/logtrace_client.cc @@ -76,7 +76,7 @@ bool TraceLog::Init(const char *msg_id, WriteMode mode) { msg_id_ = base::LogMessage::MsgId{msg_id}; log_socket_ = new base::UnixClientSocket{Osaflog::kServerSocketPath, static_cast(mode)}; - mutex_ = new base::Mutex{}; + mutex_ = new base::Mutex{false}; return true; } @@ -91,6 +91,9 @@ void TraceLog::Log(base::LogMessage::Severity severity, const char *fmt, void TraceLog::LogInternal(base::LogMessage::Severity severity, const char *fmt, va_list ap) { base::Lock lock(*mutex_); + + if (!mutex_->good()) return; + uint32_t id = sequence_id_; sequence_id_ = id < kMaxSequenceId ? id + 1 : 1; buffer_.clear(); diff --git a/src/base/mutex.cc b/src/base/mutex.cc index 5fa6ac55a..1627ac20b 100644 --- a/src/base/mutex.cc +++ b/src/base/mutex.cc @@ -20,7 +20,7 @@ namespace base { -Mutex::Mutex() : mutex_{} { +Mutex::Mutex(bool abort) : abort_{abort}, mutex_{}, result_{0} { pthread_mutexattr_t attr; int result = pthread_mutexattr_init(); if (result != 0) osaf_abort(result); diff --git a/src/base/mutex.h b/src/base/mutex.h index 7b3cee187..e3c54a711 100644 --- a/src/base/mutex.h +++ b/src/base/mutex.h @@ -31,30 +31,34 @@ namespace base { class Mutex { public: using NativeHandleType = pthread_mutex_t*; - Mutex(); + Mutex(bool abort = true); ~Mutex(); void Lock() { -int result = pthread_mutex_lock(_); -if (result != 0) osaf_abort(result); +result_ = pthread_mutex_lock(_); +if (abort_ && result_ != 0) osaf_abort(result_); } bool TryLock() { -int result = pthread_mutex_trylock(_); -if (result == 0) { +result_ = pthread_mutex_trylock(_); +if (result_ == 0) { return true; -} else if (result == EBUSY) { +} else if (result_ == EBUSY) { return false; } else { - osaf_abort(result); + if (abort_) osaf_abort(result_); + return false; } } void Unlock() { -int result = pthread_mutex_unlock(_); -if (result != 0) osaf_abort(result); +result_ = pthread_mutex_unlock(_); +if (abort_ && result_ != 0) osaf_abort(result_); } NativeHandleType native_handle() { return _; } + bool good() const {return result_ == 0;}; private: + bool abort_; pthread_mutex_t mutex_; + int result_; DELETE_COPY_AND_MOVE_OPERATORS(Mutex); }; -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Re: [devel] [PATCH 1/1] base: Destructor of TraceLog causes coredump V2 [#2860]
Hi Minh, yes you are right about the possibility for a segv, but using a std::shared_ptr instead of the naked ptr may be an option ? /Thanks Hans Från: Minh Hon ChauSkickat: den 24 maj 2018 02:34:13 Till: Hans Nordebäck; Anders Widell; Gary Lee Kopia: opensaf-devel@lists.sourceforge.net Ämne: Re: [PATCH 1/1] base: Destructor of TraceLog causes coredump V2 [#2860] Hi Hans, It is good to give an option to Mutex class not to abort. We can avoid the abort in mutex_unlock (as reported in coredump), but I feel the issue is still there. We may hit a problem (segv?) with "mutex_->good()" since the other thread is wiping out the mutex_ in destructor, it is a matter of timing to happen I guess. As we don't have (and don't want to have) any protection between two threads for the TraceLog, so the good one (I hope) is making one of those threads not to touch the TraceLog. If you don't like to remove the destructor, another way is locating the gl_trace/gl_log to the HEAP? Thanks, Minh On 23/05/18 20:50, Hans Nordeback wrote: > Change Mutex class to make it possible for caller to decide if abort > --- > src/base/logtrace_client.cc | 5 - > src/base/mutex.cc | 2 +- > src/base/mutex.h| 22 +- > 3 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/src/base/logtrace_client.cc b/src/base/logtrace_client.cc > index 0dac6d389..f597c1ae3 100644 > --- a/src/base/logtrace_client.cc > +++ b/src/base/logtrace_client.cc > @@ -76,7 +76,7 @@ bool TraceLog::Init(const char *msg_id, WriteMode mode) { > msg_id_ = base::LogMessage::MsgId{msg_id}; > log_socket_ = new base::UnixClientSocket{Osaflog::kServerSocketPath, > static_cast(mode)}; > - mutex_ = new base::Mutex{}; > + mutex_ = new base::Mutex{false}; > > return true; > } > @@ -91,6 +91,9 @@ void TraceLog::Log(base::LogMessage::Severity severity, > const char *fmt, > void TraceLog::LogInternal(base::LogMessage::Severity severity, const char > *fmt, > va_list ap) { > base::Lock lock(*mutex_); > + > + if (!mutex_->good()) return; > + > uint32_t id = sequence_id_; > sequence_id_ = id < kMaxSequenceId ? id + 1 : 1; > buffer_.clear(); > diff --git a/src/base/mutex.cc b/src/base/mutex.cc > index 5fa6ac55a..1627ac20b 100644 > --- a/src/base/mutex.cc > +++ b/src/base/mutex.cc > @@ -20,7 +20,7 @@ > > namespace base { > > -Mutex::Mutex() : mutex_{} { > +Mutex::Mutex(bool abort) : abort_{abort}, mutex_{}, result_{0} { > pthread_mutexattr_t attr; > int result = pthread_mutexattr_init(); > if (result != 0) osaf_abort(result); > diff --git a/src/base/mutex.h b/src/base/mutex.h > index 7b3cee187..e3c54a711 100644 > --- a/src/base/mutex.h > +++ b/src/base/mutex.h > @@ -31,30 +31,34 @@ namespace base { > class Mutex { >public: > using NativeHandleType = pthread_mutex_t*; > - Mutex(); > + Mutex(bool abort = true); > ~Mutex(); > void Lock() { > -int result = pthread_mutex_lock(_); > -if (result != 0) osaf_abort(result); > +result_ = pthread_mutex_lock(_); > +if (abort_ && result_ != 0) osaf_abort(result_); > } > bool TryLock() { > -int result = pthread_mutex_trylock(_); > -if (result == 0) { > +result_ = pthread_mutex_trylock(_); > +if (result_ == 0) { > return true; > -} else if (result == EBUSY) { > +} else if (result_ == EBUSY) { > return false; > } else { > - osaf_abort(result); > + if (abort_) osaf_abort(result_); > + return false; > } > } > void Unlock() { > -int result = pthread_mutex_unlock(_); > -if (result != 0) osaf_abort(result); > +result_ = pthread_mutex_unlock(_); > +if (abort_ && result_ != 0) osaf_abort(result_); > } > NativeHandleType native_handle() { return _; } > > + bool good() const {return result_ == 0;}; >private: > + bool abort_; > pthread_mutex_t mutex_; > + int result_; > DELETE_COPY_AND_MOVE_OPERATORS(Mutex); > }; > -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 1/1] base: Destructor of TraceLog causes coredump V2 [#2860]
Hi Hans, It is good to give an option to Mutex class not to abort. We can avoid the abort in mutex_unlock (as reported in coredump), but I feel the issue is still there. We may hit a problem (segv?) with "mutex_->good()" since the other thread is wiping out the mutex_ in destructor, it is a matter of timing to happen I guess. As we don't have (and don't want to have) any protection between two threads for the TraceLog, so the good one (I hope) is making one of those threads not to touch the TraceLog. If you don't like to remove the destructor, another way is locating the gl_trace/gl_log to the HEAP? Thanks, Minh On 23/05/18 20:50, Hans Nordeback wrote: Change Mutex class to make it possible for caller to decide if abort --- src/base/logtrace_client.cc | 5 - src/base/mutex.cc | 2 +- src/base/mutex.h| 22 +- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/base/logtrace_client.cc b/src/base/logtrace_client.cc index 0dac6d389..f597c1ae3 100644 --- a/src/base/logtrace_client.cc +++ b/src/base/logtrace_client.cc @@ -76,7 +76,7 @@ bool TraceLog::Init(const char *msg_id, WriteMode mode) { msg_id_ = base::LogMessage::MsgId{msg_id}; log_socket_ = new base::UnixClientSocket{Osaflog::kServerSocketPath, static_cast(mode)}; - mutex_ = new base::Mutex{}; + mutex_ = new base::Mutex{false}; return true; } @@ -91,6 +91,9 @@ void TraceLog::Log(base::LogMessage::Severity severity, const char *fmt, void TraceLog::LogInternal(base::LogMessage::Severity severity, const char *fmt, va_list ap) { base::Lock lock(*mutex_); + + if (!mutex_->good()) return; + uint32_t id = sequence_id_; sequence_id_ = id < kMaxSequenceId ? id + 1 : 1; buffer_.clear(); diff --git a/src/base/mutex.cc b/src/base/mutex.cc index 5fa6ac55a..1627ac20b 100644 --- a/src/base/mutex.cc +++ b/src/base/mutex.cc @@ -20,7 +20,7 @@ namespace base { -Mutex::Mutex() : mutex_{} { +Mutex::Mutex(bool abort) : abort_{abort}, mutex_{}, result_{0} { pthread_mutexattr_t attr; int result = pthread_mutexattr_init(); if (result != 0) osaf_abort(result); diff --git a/src/base/mutex.h b/src/base/mutex.h index 7b3cee187..e3c54a711 100644 --- a/src/base/mutex.h +++ b/src/base/mutex.h @@ -31,30 +31,34 @@ namespace base { class Mutex { public: using NativeHandleType = pthread_mutex_t*; - Mutex(); + Mutex(bool abort = true); ~Mutex(); void Lock() { -int result = pthread_mutex_lock(_); -if (result != 0) osaf_abort(result); +result_ = pthread_mutex_lock(_); +if (abort_ && result_ != 0) osaf_abort(result_); } bool TryLock() { -int result = pthread_mutex_trylock(_); -if (result == 0) { +result_ = pthread_mutex_trylock(_); +if (result_ == 0) { return true; -} else if (result == EBUSY) { +} else if (result_ == EBUSY) { return false; } else { - osaf_abort(result); + if (abort_) osaf_abort(result_); + return false; } } void Unlock() { -int result = pthread_mutex_unlock(_); -if (result != 0) osaf_abort(result); +result_ = pthread_mutex_unlock(_); +if (abort_ && result_ != 0) osaf_abort(result_); } NativeHandleType native_handle() { return _; } + bool good() const {return result_ == 0;}; private: + bool abort_; pthread_mutex_t mutex_; + int result_; DELETE_COPY_AND_MOVE_OPERATORS(Mutex); }; -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel