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 Chau <minh.c...@dektech.com.au>
Skickat: 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<base::UnixSocket::Mode>(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(&attr);
     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(&mutex_);
-    if (result != 0) osaf_abort(result);
+    result_ = pthread_mutex_lock(&mutex_);
+    if (abort_ && result_ != 0) osaf_abort(result_);
     }
     bool TryLock() {
-    int result = pthread_mutex_trylock(&mutex_);
-    if (result == 0) {
+    result_ = pthread_mutex_trylock(&mutex_);
+    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(&mutex_);
-    if (result != 0) osaf_abort(result);
+    result_ = pthread_mutex_unlock(&mutex_);
+    if (abort_ && result_ != 0) osaf_abort(result_);
     }
     NativeHandleType native_handle() { return &mutex_; }

+  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
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Reply via email to