Hi Vu Ack Sorry for the long delay
This makes the code cleaner. However it does not make it easier to understand why this is needed and what it is doing. You could take better advantage of this new class for describing why this is needed, what it is doing and how it is used: - Improve English grammar and general usage of the language e.g. ways of expressing and phrasing - Describe what this is protecting - Naming of functions, variables etc. is a bit confusing I suggest that you take help from someone who maybe has English as mother tongue. Try to explain what you really mean and ask for a better way to express it. Thanks Lennart > -----Original Message----- > From: Vu Minh Nguyen [mailto:[email protected]] > Sent: den 1 september 2017 07:19 > To: [email protected]; Lennart Lund <[email protected]>; > Canh Van Truong <[email protected]> > Cc: [email protected]; Vu Minh Nguyen > <[email protected]> > Subject: [PATCH 1/1] log: duplicated code in lga_client and lga_stream > [#2567] > > Introduce `RefCounter` class to remove duplicated code in LogClient > and LogStreamInfo. > > Introduce also one new parameter `caller`, the purpose is mainly > for debug - know who is the caller to `RefCounter` methods. > --- > src/log/Makefile.am | 5 +- > src/log/agent/lga_agent.cc | 82 +++++++++++++++------------ > src/log/agent/lga_client.cc | 62 ++++----------------- > src/log/agent/lga_client.h | 45 ++++++++------- > src/log/agent/lga_common.h | 8 --- > src/log/agent/lga_mds.cc | 7 +-- > src/log/agent/lga_ref_counter.cc | 73 ++++++++++++++++++++++++ > src/log/agent/lga_ref_counter.h | 117 > +++++++++++++++++++++++++++++++++++++++ > src/log/agent/lga_stream.cc | 46 +-------------- > src/log/agent/lga_stream.h | 40 ++++++------- > 10 files changed, 300 insertions(+), 185 deletions(-) > create mode 100644 src/log/agent/lga_ref_counter.cc > create mode 100644 src/log/agent/lga_ref_counter.h > > diff --git a/src/log/Makefile.am b/src/log/Makefile.am > index b66b8a7..3d951eb 100644 > --- a/src/log/Makefile.am > +++ b/src/log/Makefile.am > @@ -31,7 +31,9 @@ lib_libSaLog_la_SOURCES = \ > src/log/agent/lga_util.cc \ > src/log/agent/lga_mds.cc \ > src/log/agent/lga_state.cc \ > - src/log/agent/lga_agent.cc > + src/log/agent/lga_agent.cc \ > + src/log/agent/lga_ref_counter.cc > + > > nodist_EXTRA_lib_libSaLog_la_SOURCES = dummy.cc > > @@ -66,6 +68,7 @@ noinst_HEADERS += \ > src/log/agent/lga_agent.h \ > src/log/agent/lga_stream.h \ > src/log/agent/lga_state.h \ > + src/log/agent/lga_ref_counter.h \ > src/log/common/lgsv_defs.h \ > src/log/common/lgsv_msg.h \ > src/log/logd/lgs.h \ > diff --git a/src/log/agent/lga_agent.cc b/src/log/agent/lga_agent.cc > index 55ee916..042bde3 100644 > --- a/src/log/agent/lga_agent.cc > +++ b/src/log/agent/lga_agent.cc > @@ -56,7 +56,9 @@ class ScopeData { > bool* is_updated; > // The increased value if @is_updated set. The aim of using enum instead > // of `int` type is to force the caller pass either value (1) or value > (-1). > - RefCounterDegree value; > + RefCounter::Degree value; > + // Who is the caller of `RestoreRefCounter` > + const char* caller; > }; > > struct LogStreamInfoData { > @@ -66,7 +68,9 @@ class ScopeData { > bool* is_updated; > // The increased value if @is_updated set. The aim of using enum instead > // of `int` type is to force the caller pass either value (1) or value > (-1). > - RefCounterDegree value; > + RefCounter::Degree value; > + // Who is the caller of `RestoreRefCounter` > + const char* caller; > }; > > explicit ScopeData(LogClientData*, bool* lock = nullptr); > @@ -107,20 +111,23 @@ ScopeData::~ScopeData() { > LogAgent::instance().EnterCriticalSection(); > LogClient* client = client_data_->client; > bool* is_updated = client_data_->is_updated; > - RefCounterDegree client_degree = client_data_->value; > + RefCounter::Degree client_degree = client_data_->value; > + const char* caller = client_data_->caller; > if (client != nullptr) { > // Do restore the reference counter if the client exists. > - client->RestoreRefCounter(client_degree, *is_updated); > - if (stream_data_ != nullptr) { > - LogStreamInfo* stream = stream_data_->stream; > - bool* stream_is_updated = stream_data_->is_updated; > - RefCounterDegree stream_degree = stream_data_->value; > - if (stream != nullptr) { > - // Do restore the reference counter if the stream exists. > - stream->RestoreRefCounter(stream_degree, *stream_is_updated); > - } > - } // stream_data_ > - } // client > + client->RestoreRefCounter(caller, client_degree, *is_updated); > + } > + > + if (stream_data_ != nullptr) { > + LogStreamInfo* stream = stream_data_->stream; > + bool* stream_is_updated = stream_data_->is_updated; > + RefCounter::Degree stream_degree = stream_data_->value; > + const char* caller = stream_data_->caller; > + if (stream != nullptr) { > + // Do restore the reference counter if the stream exists. > + stream->RestoreRefCounter(caller, stream_degree, > *stream_is_updated); > + } > + } > LogAgent::instance().LeaveCriticalSection(); > } > > @@ -137,13 +144,20 @@ LogAgent::LogAgent() { > // even they are in the same thread context. > // To avoid such risk, use RECURSIVE MUTEX for @LogClient > pthread_mutexattr_t attr; > - pthread_mutexattr_init(&attr); > - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); > - pthread_mutex_init(&mutex_, nullptr); > + int result = pthread_mutexattr_init(&attr); > + assert(result == 0 && "Failed to init mutex attribute"); > + > + result = pthread_mutexattr_settype(&attr, > PTHREAD_MUTEX_RECURSIVE); > + assert(result == 0 && "Failed to set mutex type"); > + > + result = pthread_mutex_init(&mutex_, nullptr); > + assert(result == 0 && "Failed to init `mutex_`"); > + > pthread_mutexattr_destroy(&attr); > > // Initialize @get_delete_obj_sync_mutex_ > - pthread_mutex_init(&get_delete_obj_sync_mutex_, nullptr); > + result = pthread_mutex_init(&get_delete_obj_sync_mutex_, nullptr); > + assert(result == 0 && "Failed to init `get_delete_obj_sync_mutex_`"); > } > > void LogAgent::PopulateOpenParams( > @@ -419,7 +433,7 @@ SaAisErrorT LogAgent::saLogSelectionObjectGet( > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data}; > > if (selectionObject == nullptr) { > @@ -439,7 +453,7 @@ SaAisErrorT LogAgent::saLogSelectionObjectGet( > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > // @client is being deleted. Don't touch @this client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > @@ -470,7 +484,7 @@ SaAisErrorT LogAgent::saLogDispatch(SaLogHandleT > logHandle, > // such as Restore the reference counter fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data}; > > if (is_dispatch_flag_valid(dispatchFlags) == false) { > @@ -490,7 +504,7 @@ SaAisErrorT LogAgent::saLogDispatch(SaLogHandleT > logHandle, > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > // @client is being deleted. DO NOT touch this @client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > @@ -564,7 +578,7 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT > logHandle) { > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kDecOne}; > + RefCounter::Degree::kDecOne, __func__}; > ScopeData data{&client_data, &is_locked}; > > if (true) { > @@ -578,7 +592,7 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT > logHandle) { > return ais_rc; > } > > - if (client->FetchAndDecreaseRefCounter(&updated) != 0) { > + if (client->FetchAndDecreaseRefCounter(__func__, &updated) != 0) { > // DO NOT delete this @client as it is being used by somewhere (>0) > // Or it is being deleted by other thread (=-1) > ais_rc = SA_AIS_ERR_TRY_AGAIN; > @@ -812,7 +826,7 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data, &is_locked}; > > if (lga_is_extended_name_valid(logStreamName) == false) { > @@ -838,7 +852,7 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > // @client is being deleted. DO NOT touch this @client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > @@ -1122,9 +1136,9 @@ SaAisErrorT > LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &cUpdated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData::LogStreamInfoData stream_data{stream, &sUpdated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data, &stream_data, &is_locked}; > > if (true) { > @@ -1146,13 +1160,13 @@ SaAisErrorT > LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&cUpdated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &cUpdated) == -1) { > // @client is being deleted. DO NOT touch this @client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > } > > - if (stream->FetchAndIncreaseRefCounter(&sUpdated) == -1) { > + if (stream->FetchAndIncreaseRefCounter(__func__, &sUpdated) == -1) { > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > } > @@ -1292,9 +1306,9 @@ SaAisErrorT > LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &cUpdated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData::LogStreamInfoData stream_data{stream, &sUpdated, > - RefCounterDegree::kDecOne}; > + RefCounter::Degree::kDecOne, __func__}; > ScopeData data{&client_data, &stream_data, &is_locked}; > > if (true) { > @@ -1307,7 +1321,7 @@ SaAisErrorT > LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { > return ais_rc; > } > > - if (stream->FetchAndDecreaseRefCounter(&sUpdated) != 0) { > + if (stream->FetchAndDecreaseRefCounter(__func__, &sUpdated) != 0) { > // @stream is being used somewhere (>0), DO NOT delete this @stream. > // or @stream is being deleted on other thread (=-1) > ais_rc = SA_AIS_ERR_TRY_AGAIN; > @@ -1322,7 +1336,7 @@ SaAisErrorT > LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&cUpdated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &cUpdated) == -1) { > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > } > diff --git a/src/log/agent/lga_client.cc b/src/log/agent/lga_client.cc > index 2d7c623..386c849 100644 > --- a/src/log/agent/lga_client.cc > +++ b/src/log/agent/lga_client.cc > @@ -32,7 +32,7 @@ > // LogClient > > //------------------------------------------------------------------------------ > LogClient::LogClient(const SaLogCallbacksT* cb, uint32_t id, SaVersionT ver) > - : client_id_{id}, ref_counter_{0} { > + : client_id_{id}, ref_counter_object_{} { > TRACE_ENTER(); > // Reset registered callback info > memset(&callbacks_, 0, sizeof(callbacks_)); > @@ -65,13 +65,16 @@ LogClient::LogClient(const SaLogCallbacksT* cb, > uint32_t id, SaVersionT ver) > // even they are in the same thread context. > // To avoid such risk, use RECURSIVE MUTEX for @LogClient > pthread_mutexattr_t attr; > - pthread_mutexattr_init(&attr); > - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); > - pthread_mutex_init(&mutex_, nullptr); > - pthread_mutexattr_destroy(&attr); > + int result = pthread_mutexattr_init(&attr); > + assert(result == 0 && "Failed to init mutex attribute"); > + > + result = pthread_mutexattr_settype(&attr, > PTHREAD_MUTEX_RECURSIVE); > + assert(result == 0 && "Failed to set mutex type"); > > - // Initialize @ref_counter_mutex_ > - pthread_mutex_init(&ref_counter_mutex_, nullptr); > + result = pthread_mutex_init(&mutex_, nullptr); > + assert(result == 0 && "Failed to init mutex"); > + > + pthread_mutexattr_destroy(&attr); > } > > LogClient::~LogClient() { > @@ -94,51 +97,9 @@ LogClient::~LogClient() { > // Free the allocated mailbox for this client > m_NCS_IPC_RELEASE(&mailbox_, nullptr); > > - pthread_mutex_destroy(&ref_counter_mutex_); > pthread_mutex_destroy(&mutex_); > } > > -// Return (-1) if @this object is being deleted. > -// Otherwise, increase the reference counter. > -int32_t LogClient::FetchAndIncreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - ScopeLock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being deleted. > - if (ref_counter_ == -1) return backup; > - ref_counter_ += 1; > - *updated = true; > - return backup; > -} > - > -// Return (0) if @this object is allowed to deleted > -// Otherwise, @this object is being used or being deleted by > -// other thread. > -int32_t LogClient::FetchAndDecreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - ScopeLock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being used or being deleted. > - if (ref_counter_ != 0) return backup; > - ref_counter_ = -1; > - *updated = true; > - return backup; > -} > - > -void LogClient::RestoreRefCounter(RefCounterDegree value, bool updated) > { > - TRACE_ENTER(); > - if (updated == false) return; > - ScopeLock scopeLock(ref_counter_mutex_); > - ref_counter_ -= value; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // Don't expect the @ref_counter_ is less than (-1) > - assert(ref_counter_ >= -1); > -} > - > bool LogClient::ClearMailBox(NCSCONTEXT arg, NCSCONTEXT msg) { > TRACE_ENTER(); > lgsv_msg_t *cbk, *pnext; > @@ -413,7 +374,8 @@ bool LogClient::HaveLogStreamInUse() { > TRACE_ENTER(); > ScopeLock scopeLock(mutex_); > for (const auto& s : stream_list_) { > - if ((s != nullptr) && (s->ref_counter_ > 0)) return true; > + if ((s != nullptr) && (s->ref_counter_object_.ref_counter() > 0)) > + return true; > } > return false; > } > diff --git a/src/log/agent/lga_client.h b/src/log/agent/lga_client.h > index eb1efd4..b86869c 100644 > --- a/src/log/agent/lga_client.h > +++ b/src/log/agent/lga_client.h > @@ -21,13 +21,14 @@ > #include <stdint.h> > #include <vector> > #include <atomic> > +#include <saLog.h> > #include "base/mutex.h" > #include "mds/mds_papi.h" > -#include <saLog.h> > #include "log/agent/lga_stream.h" > #include "log/common/lgsv_msg.h" > #include "log/common/lgsv_defs.h" > #include "log/agent/lga_state.h" > +#include "log/agent/lga_ref_counter.h" > > //> > // @LogClient class > @@ -44,7 +45,7 @@ > // After adding @this LogClient object to the database of @LogAgent, > // The proper way to retrieve @this object is via SearchClientById() > // or SeachClientByHandle(), and the next call MUST be > -// FetchAndUpdateObjectState() on that object to update its ref counter. > +// FetchAndUpdateRefCounter() on that object to update its ref counter. > // > // This object contains lot of important information about the log client, > // such as the mailbox @mailbox_ for putting all MDS msg which belong to > @@ -57,10 +58,10 @@ > // @saLogStreamOpen successfully, and removed from the database > // when @saLogStreamClose is called successfully. > // > -// And each @LogClient object has one special attribute, called > @ref_counter_, > -// that attribute should be updated when it is refered, modify or deleting. > -// FetchAndUpdateObjectState()/RestoreObjectState() are methods to > -// increase/restore the @ref_counter_ > +// And each @LogClient object owns one special object, > ref_counter_object_, > +// that object should be updated when it is referred, modified or deleted. > +// FetchAnd<xxx>RefCounter()/RestoreRefCounter() are methods to > update > +// ref_counter_object_ object. > // > // @LogClient object can be deleted ONLY if no log stream which it owns > // are in "being use" state (@ref_counter > 0). > @@ -144,18 +145,23 @@ class LogClient { > bool is_stale_client() const; > > // Fetch and increase reference counter. > - // Increase one if @this object is not being deleted by other thread. > - // @updated will be set to true if there is an increase, false otherwise. > - int32_t FetchAndIncreaseRefCounter(bool* updated); > + // Refer to `RefCounter` class for more info. > + int32_t FetchAndIncreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndIncreaseRefCounter(caller, > updated); > + } > > // Fetch and decrease reference counter. > - // Decrease one if @this object is not being used or deleted by other > thread. > - // @updated will be set to true if there is an decrease, false otherwise. > - int32_t FetchAndDecreaseRefCounter(bool* updated); > + // Refer to `RefCounter` class for more info. > + int32_t FetchAndDecreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndDecreaseRefCounter(caller, > updated); > + } > > // Restore the reference counter back. > - // Passing @value is either (1) [increase] or (-1) [decrease] > - void RestoreRefCounter(RefCounterDegree value, bool updated); > + // Refer to `RefCounter` class for more info. > + void RestoreRefCounter(const char* caller, RefCounter::Degree value, > + bool updated) { > + return ref_counter_object_.RestoreRefCounter(caller, value, updated); > + } > > // true if @this client does NOT recovery succcessfully. > // false, otherwise. > @@ -249,15 +255,8 @@ class LogClient { > pthread_mutex_t mutex_; > > // Hold information how many thread are referring to this object > - // If the object is being deleted, the value is (-1). > - // If the object is not being deleted/used, the value is (0) > - // If there are N thread referring to this object, the counter will be N. > - int32_t ref_counter_; > - > - // Dedicate an own mutex to protect @ref_counter_. > - // The reason not sharing the @mutex_ is to avoid deadlock with MDS > thread > - // E.g: lock(mutex_) --> send MDS sync --> receiving ack from MDS --> > deadlock > - pthread_mutex_t ref_counter_mutex_; > + // Refer to `RefCounter` class for more info. > + RefCounter ref_counter_object_; > > // Hold all log streams belong to @this client > std::vector<LogStreamInfo*> stream_list_; > diff --git a/src/log/agent/lga_common.h b/src/log/agent/lga_common.h > index ef63cbb..87bd9e7 100644 > --- a/src/log/agent/lga_common.h > +++ b/src/log/agent/lga_common.h > @@ -22,14 +22,6 @@ > #include "base/osaf_utility.h" > #include "base/macros.h" > > -// Degree of reference counter decrease/increase > -enum RefCounterDegree { > - // Say, I want referring to object. Count me in. > - kIncOne = 1, > - // Say, I want to delete the object. > - kDecOne = -1 > -}; > - > // LOG server state. The state changes according to getting MDS events. > enum class LogServerState { > // No Active LOG server. Could happen a short time during > failover/switchover. > diff --git a/src/log/agent/lga_mds.cc b/src/log/agent/lga_mds.cc > index 81791c0..ca94957 100644 > --- a/src/log/agent/lga_mds.cc > +++ b/src/log/agent/lga_mds.cc > @@ -111,8 +111,7 @@ static uint32_t lga_enc_finalize_msg(NCS_UBAID > *uba, lgsv_msg_t *msg) { > return total_bytes; > } > > -static uint32_t encode_sanamet(NCS_UBAID *uba, uint8_t *p8, SaNameT > *name) > -{ > +static uint32_t encode_sanamet(NCS_UBAID *uba, uint8_t *p8, SaNameT > *name) { > uint32_t total_bytes = 0; > p8 = ncs_enc_reserve_space(uba, 2); > if (!p8) { > @@ -527,7 +526,7 @@ static uint32_t lga_lgs_msg_proc(lgsv_msg_t > *lgsv_msg, > > // @client is being deleted in other thread. DO NOT touch this. > bool updated = false; > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > LogAgent::instance().LeaveCriticalSection(); > lga_msg_destroy(lgsv_msg); > TRACE_LEAVE(); > @@ -619,7 +618,7 @@ static uint32_t lga_lgs_msg_proc(lgsv_msg_t > *lgsv_msg, > break; > } > > - client->RestoreRefCounter(RefCounterDegree::kIncOne, updated); > + client->RestoreRefCounter(__func__, RefCounter::Degree::kIncOne, > updated); > TRACE_LEAVE(); > return rc; > } > diff --git a/src/log/agent/lga_ref_counter.cc > b/src/log/agent/lga_ref_counter.cc > new file mode 100644 > index 0000000..25574f7 > --- /dev/null > +++ b/src/log/agent/lga_ref_counter.cc > @@ -0,0 +1,73 @@ > +/* -*- OpenSAF -*- > + * > + * Copyright Ericsson AB 2017 - All Rights Reserved. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are > licensed > + * under the GNU Lesser General Public License Version 2.1, February 1999. > + * The complete license can be accessed from the following location: > + * http://opensource.org/licenses/lgpl-license.php > + * See the Copying file included with the OpenSAF distribution for full > + * licensing terms. > + * > + * Author(s): Ericsson AB > + * > + */ > + > +#include "log/agent/lga_ref_counter.h" > +#include <assert.h> > +#include "base/logtrace.h" > + > +//------------------------------------------------------------------------------ > +// RefCounter > +//------------------------------------------------------------------------------ > +RefCounter::RefCounter() : ref_counter_{0}, ref_counter_mutex_{} {} > + > +// Return (-1) if @this object is being deleted. > +// Otherwise, increase the reference counter. > +int32_t RefCounter::FetchAndIncreaseRefCounter(const char* caller, > + bool* updated) { > + TRACE_ENTER(); > + assert(caller != nullptr && updated != nullptr); > + base::Lock scopeLock(ref_counter_mutex_); > + int32_t backup = ref_counter_; > + *updated = false; > + TRACE("%s(%s): counter(%d) = %d", __func__, caller, *updated, > ref_counter_); > + // The @this object is being deleted. > + if (ref_counter_ == -1) return backup; > + ref_counter_ += 1; > + *updated = true; > + return backup; > +} > + > +// Return (0) if @this object is allowed to deleted > +// Otherwise, @this object is being used or being deleted by > +// other thread. > +int32_t RefCounter::FetchAndDecreaseRefCounter(const char* caller, > + bool* updated) { > + TRACE_ENTER(); > + assert(caller != nullptr && updated != nullptr); > + base::Lock scopeLock(ref_counter_mutex_); > + int32_t backup = ref_counter_; > + *updated = false; > + TRACE("%s(%s): counter(%d) = %d", __func__, caller, *updated, > ref_counter_); > + // @this object is being used or being deleted. > + if (ref_counter_ != 0) return backup; > + ref_counter_ = -1; > + *updated = true; > + return backup; > +} > + > +void RefCounter::RestoreRefCounter(const char* caller, > + Degree value, bool updated) { > + TRACE_ENTER(); > + assert(caller != nullptr && "No caller"); > + if (updated == false) return; > + base::Lock scopeLock(ref_counter_mutex_); > + int degree = (value == Degree::kIncOne ? 1 : -1); > + ref_counter_ -= degree; > + TRACE("%s(%s): counter(%d) = %d", __func__, caller, degree, > ref_counter_); > + // Don't expect the @ref_counter_ is less than (-1) > + assert(ref_counter_ >= -1); > +} > diff --git a/src/log/agent/lga_ref_counter.h > b/src/log/agent/lga_ref_counter.h > new file mode 100644 > index 0000000..4fbd28f > --- /dev/null > +++ b/src/log/agent/lga_ref_counter.h > @@ -0,0 +1,117 @@ > +/* -*- OpenSAF -*- > + * > + * Copyright Ericsson AB 2017 - All Rights Reserved. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are > licensed > + * under the GNU Lesser General Public License Version 2.1, February 1999. > + * The complete license can be accessed from the following location: > + * http://opensource.org/licenses/lgpl-license.php > + * See the Copying file included with the OpenSAF distribution for full > + * licensing terms. > + * > + * Author(s): Ericsson AB > + * > + */ > + > +#ifndef SRC_LOG_AGENT_LGA_REF_COUNTER_H_ > +#define SRC_LOG_AGENT_LGA_REF_COUNTER_H_ > + > +#include <stdint.h> > +#include "log/agent/lga_common.h" > +#include "base/mutex.h" > + > +//> > +// The reference counter class is used to synchronize access > +// to its object owner. The class provides helper methods > +// to let the owner object know how many thread are referring > +// to owner object, is owner object deleting. > +// > +// So, by using this RefCounter object, the owner can avoid data race. > +// > +// When the object owner is refering for reading/modifying, the caller > +// firstly should check if the object is being deleted or not by > +// using method `FetchAndIncreaseRefCounter()`. If there is deletion going, > +// means the return value is (-1), the caller should not touch this object > +// by returning try again, otherwise could get coredump. > +// > +// And if the object owner is going to be deleted, the caller firstly check > +// if if the object is being used by other thread or not by calling method > +// `FetchAndDecreaseRefCounter`. If there is thread referring to this > object, > +// the method will return non-zero (!=0). In that case, the caller > +// should not delete the object, otherwise could get coredump. > +// > +// When calling any methods of this class, the first parameter, `caller`, > +// is used mainly for trace purpose. It will be helpful to detect who > +// is the caller to these methods. > +// > +// Usage example: > +// class Owner { > +// public: > +// RefCounter ref_counter_object_; > +//} > +// > +// <! Case #1: Using Owner object !> > +// if (ref_counter_object_.FetchAndIncreaseRefCounter() == -1) { > +// // Return try again and come back later > +// } > +// > +// // It is ok to refer to Owner object. > +// > +// > +// <! Case #2: Delete Owner object !> > +// if (ref_counter_object_.FetchAndDecreaseRefCounter() != 0) { > +// // The owner is using or deleting by other thread. Return try again. > +// } > +// > +// // It is ok to delete the Owner object. > +//< > +class RefCounter { > + public: > + // Degree of reference counter. Introduce to force caller either using > + // kIncOne (1) or kDecOne(-1). Other value is not allowed. > + enum Degree { > + // Say, I want referring to object. Count me in. > + kIncOne = 1, > + // Say, I want to delete the object. > + kDecOne = -1 > + }; > + > + RefCounter(); > + ~RefCounter() {}; > + > + // Fetch and increase reference counter. > + // Increase one if @this object is not being deleted by other thread. > + // @updated will be set to true if there is an increase, false otherwise. > + // @caller shows who is the caller, the main purpose is for debugging. > + int32_t FetchAndIncreaseRefCounter(const char* caller, bool* updated); > + > + // Fetch and decrease reference counter. > + // Decrease one if @this object is not being used or deleted by other > thread. > + // @updated will be set to true if there is an decrease, false otherwise. > + // @caller shows who is the caller, the main purpose is for debugging. > + int32_t FetchAndDecreaseRefCounter(const char* caller, bool* updated); > + > + // Restore the reference counter back if @updated is true. > + // Passed @value is either (1) [increase] or (-1) [decrease] > + // @caller shows who is the caller, the main purpose is for debugging. > + void RestoreRefCounter(const char* caller, Degree value, bool updated); > + > + // Return current ref_counter_ value > + int32_t ref_counter() const { return ref_counter_; } > + > + private: > + // Hold information how many thread are referring to this object > + // If the object is being deleted, the value is (-1). > + // If the object is not being deleted/used, the value is (0) > + // If there are N thread referring to this object, the counter will be N. > + int32_t ref_counter_; > + > + // The mutex to protect @ref_counter_. > + base::Mutex ref_counter_mutex_; > + > + DELETE_COPY_AND_MOVE_OPERATORS(RefCounter); > +}; > + > +#endif // SRC_LOG_AGENT_LGA_REF_COUNTER_H_ > diff --git a/src/log/agent/lga_stream.cc b/src/log/agent/lga_stream.cc > index d64ee13..8481f42 100644 > --- a/src/log/agent/lga_stream.cc > +++ b/src/log/agent/lga_stream.cc > @@ -28,7 +28,7 @@ > #include "mds/mds_papi.h" > > LogStreamInfo::LogStreamInfo(const std::string& name, uint32_t id) > - : stream_name_{name}, ref_counter_mutex_{}, ref_counter_{0} { > + : stream_name_{name} { > TRACE_ENTER(); > stream_id_ = id; > recovered_flag_ = true; > @@ -49,50 +49,6 @@ LogStreamInfo::~LogStreamInfo() { > } > } > > -// Return (-1) if @this object is being deleted. > -// Otherwise, increase the reference counter. > -int32_t LogStreamInfo::FetchAndIncreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - base::Lock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being deleted. > - if (ref_counter_ == -1) return backup; > - ref_counter_ += 1; > - *updated = true; > - return backup; > -} > - > -// Return (0) if @this object is allowed to deleted > -// Otherwise, @this object is being used or being deleted by > -// other thread. > -int32_t LogStreamInfo::FetchAndDecreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - base::Lock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being used or being deleted. > - if (ref_counter_ != 0) return backup; > - ref_counter_ = -1; > - *updated = true; > - return backup; > -} > - > -void LogStreamInfo::RestoreRefCounter(RefCounterDegree value, bool > updated) { > - TRACE_ENTER(); > - if (updated == false) return; > - base::Lock scopeLock(ref_counter_mutex_); > - // @value is negative number in case restoring the counter > - // for @FetchAndDecreaseRefCounter(). > - // Should be a positive number, otherwise. > - ref_counter_ -= value; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // Don't expect the @ref_counter_ is less than (-1) > - assert(ref_counter_ >= -1); > -} > - > bool LogStreamInfo::SendOpenStreamMsg(uint32_t client_id) { > TRACE_ENTER(); > SaAisErrorT ais_rc = SA_AIS_OK; > diff --git a/src/log/agent/lga_stream.h b/src/log/agent/lga_stream.h > index 5b9f6f9..abacbe3 100644 > --- a/src/log/agent/lga_stream.h > +++ b/src/log/agent/lga_stream.h > @@ -23,6 +23,7 @@ > #include <saLog.h> > #include "base/mutex.h" > #include "log/agent/lga_common.h" > +#include "log/agent/lga_ref_counter.h" > > //< > // @LogStreamInfo class > @@ -53,9 +54,13 @@ > // @LogClient. > // > // To avoid @this LogStreamInfo object deleted while it is being used by > other > -// thread, such as one client thread is performing writing log record, > -// in the meantime, other client thread is closing that that log stream > -// or closing the owning @LogClient. (! refer to @LogAgent description for > more) > +// thread, such as one client thread is performing writing log record, in the > +// meantime, other client thread is closing that that log stream or closing > the > +// owning @LogClient. (! refer to @LogAgent description for more) , one > special > +// object `RefCounter` is included in. Each @LogStreamInfo object owns > one > +// ref_counter_object_, that object should be updated when this object is > +// using, modified or deleted. > FetchAnd<xxx>RefCounter()/RestoreRefCounter() > +// are methods to update ref_counter_object_ object. > // > // When SC restarts from headless, @this object will be notified to do > // recover itself for specific @LogClient Id. > @@ -83,18 +88,20 @@ class LogStreamInfo { > const std::string& GetStreamName() const { return stream_name_; } > > // Fetch and increase reference counter. > - // Increase one if @this object is not being deleted by other thread. > - // @updated will be set to true if there is an increase, false otherwise. > - int32_t FetchAndIncreaseRefCounter(bool* updated); > + int32_t FetchAndIncreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndIncreaseRefCounter(caller, > updated); > + } > > // Fetch and decrease reference counter. > - // Decrease one if @this object is not being used or deleted by other > thread. > - // @updated will be set to true if there is an decrease, false otherwise. > - int32_t FetchAndDecreaseRefCounter(bool* updated); > + int32_t FetchAndDecreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndDecreaseRefCounter(caller, > updated); > + } > > // Restore the reference counter back. > - // Passing @value is either (1) [increase] or (-1) [decrease] > - void RestoreRefCounter(RefCounterDegree value, bool updated); > + void RestoreRefCounter(const char* caller, RefCounter::Degree value, > + bool updated) { > + return ref_counter_object_.RestoreRefCounter(caller, value, updated); > + } > > private: > // Set stream open flags @open_flags_ > @@ -119,16 +126,9 @@ class LogStreamInfo { > // Log stream name mentioned during open log stream > std::string stream_name_; > > - // To protect @ref_counter_ > - // E.g: if @this object is being used in @saLogStreamWrite > - // and other thread trying to delete it, will get TRY_AGAIN. > - base::Mutex ref_counter_mutex_; > - > // Hold information how many thread are referring to this object > - // If the object is being deleted, the value is (-1). > - // If the object is not being deleted/used, the value is (0) > - // If there are N thread referring to this object, the counter will be N. > - int32_t ref_counter_; > + // Refer to `RefCounter` class for more info. > + RefCounter ref_counter_object_; > > // Log stream open flags as defined in AIS.02.01 > SaLogStreamOpenFlagsT open_flags_; > -- > 1.9.1 ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
