Hi Canh,
I have some comments. Please see [Lennart] the attached .diff file that can be
applied on top of the review branch
Thanks
Lennart
________________________________
Från: Canh Van Truong <[email protected]>
Skickat: den 5 december 2018 07:06:38
Till: Lennart Lund; Minh Hon Chau
Kopia: [email protected]; Canh Van Truong
Ämne: [PATCH 1/1] ntf: Fix to NTFD doesn't send the response if MDS_DEST of
client is disconnected [#2973]
When the sending notification request come to NTFD , they are put in the mbx
to wait for being processed. if client of these request is down (MDS_DEST of
this client
is disconnected), MDS thread receive the event down and also put to mbx. The
priority
of event down and send request are the same. So NTF will process the send
request
before the event down.
The patch fix to NTFD will not send the response of request if the MDS_DEST of
client has
already disconnected.
---
src/ntf/ntfd/NtfAdmin.cc | 1 +
src/ntf/ntfd/NtfClient.cc | 20 +++++++++++++-------
src/ntf/ntfd/NtfClient.h | 3 ++-
src/ntf/ntfd/ntfs_mds.c | 8 ++++----
4 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/ntf/ntfd/NtfAdmin.cc b/src/ntf/ntfd/NtfAdmin.cc
index 2cb99457c..d0bb50733 100644
--- a/src/ntf/ntfd/NtfAdmin.cc
+++ b/src/ntf/ntfd/NtfAdmin.cc
@@ -1048,6 +1048,7 @@ uint32_t NtfAdmin::send_cluster_membership_msg_to_clients(
ClientMap::iterator pos;
for (pos = clientMap.begin(); pos != clientMap.end(); pos++) {
NtfClient *client = pos->second;
+ if (client->GetClientDownFlag() == true) continue;
client_id = client->getClientId();
mds_dest = client->getMdsDest();
NODE_ID tmp_node_id = m_NTFS_GET_NODE_ID_FROM_ADEST(mds_dest);
diff --git a/src/ntf/ntfd/NtfClient.cc b/src/ntf/ntfd/NtfClient.cc
index 6393a3617..57e591f04 100644
--- a/src/ntf/ntfd/NtfClient.cc
+++ b/src/ntf/ntfd/NtfClient.cc
@@ -180,7 +180,7 @@ void NtfClient::notificationReceived(unsigned int clientId,
notification->storeMatchingSubscription(
clientId_, subscription->getSubscriptionId());
// if active, send out the notification
- if (activeController()) {
+ if (activeController() && client_down_flag_ == false) {
subscription->sendNotification(notification, this);
}
} else {
@@ -279,7 +279,7 @@ void NtfClient::sendNotConfirmedNotification(
NtfSmartPtr notification, SaNtfSubscriptionIdT subscriptionId) {
TRACE_ENTER();
// if active, send out the notification
- if (activeController()) {
+ if (activeController() && client_down_flag_ == false) {
SubscriptionMap::iterator pos;
pos = subscriptionMap.find(subscriptionId);
if (pos != subscriptionMap.end()) {
@@ -305,7 +305,8 @@ void NtfClient::confirmNtfSubscribe(SaNtfSubscriptionIdT
subscriptionId,
"NtfClient::confirmNtfSubscribe subscribe_res_lib called, "
"client %u, subscription %u",
clientId_, subscriptionId);
- subscribe_res_lib(SA_AIS_OK, subscriptionId, mdsDest_, mds_ctxt);
+ if (client_down_flag_ == false)
+ subscribe_res_lib(SA_AIS_OK, subscriptionId, mdsDest_, mds_ctxt);
}
/**
@@ -321,7 +322,8 @@ void NtfClient::confirmNtfUnsubscribe(SaNtfSubscriptionIdT
subscriptionId,
"NtfClient::confirmNtfUnsubscribe unsubscribe_res_lib called,"
" client %u, subscription %u",
clientId_, subscriptionId);
- unsubscribe_res_lib(SA_AIS_OK, subscriptionId, mdsDest_, mdsCtxt);
+ if (client_down_flag_ == false)
+ unsubscribe_res_lib(SA_AIS_OK, subscriptionId, mdsDest_, mdsCtxt);
}
/**
@@ -335,17 +337,20 @@ void
NtfClient::confirmNtfUnsubscribe(SaNtfSubscriptionIdT subscriptionId,
void NtfClient::confirmNtfNotification(SaNtfIdentifierT notificationId,
MDS_SYNC_SND_CTXT* mdsCtxt,
MDS_DEST mdsDest) {
- notfication_result_lib(SA_AIS_OK, notificationId, mdsCtxt, mdsDest);
+ if (client_down_flag_ == false)
+ notfication_result_lib(SA_AIS_OK, notificationId, mdsCtxt, mdsDest);
}
void NtfClient::newReaderResponse(SaAisErrorT* error, unsigned int readerId,
MDS_SYNC_SND_CTXT* mdsCtxt) {
- new_reader_res_lib(*error, readerId, mdsDest_, mdsCtxt);
+ if (client_down_flag_ == false)
+ new_reader_res_lib(*error, readerId, mdsDest_, mdsCtxt);
}
void NtfClient::readNextResponse(SaAisErrorT* error, NtfSmartPtr& notification,
MDS_SYNC_SND_CTXT* mdsCtxt) {
TRACE_ENTER();
+ if (client_down_flag_ == true) return;
if (*error == SA_AIS_OK) {
read_next_res_lib(*error, notification->sendNotInfo_, mdsDest_, mdsCtxt);
} else {
@@ -356,7 +361,8 @@ void NtfClient::readNextResponse(SaAisErrorT* error,
NtfSmartPtr& notification,
void NtfClient::deleteReaderResponse(SaAisErrorT* error,
MDS_SYNC_SND_CTXT* mdsCtxt) {
- delete_reader_res_lib(*error, mdsDest_, mdsCtxt);
+ if (client_down_flag_ == false)
+ delete_reader_res_lib(*error, mdsDest_, mdsCtxt);
}
NtfReader* NtfClient::createReaderWithoutFilter(ntfsv_reader_init_req_t rp,
diff --git a/src/ntf/ntfd/NtfClient.h b/src/ntf/ntfd/NtfClient.h
index 87921246f..76c708d4a 100644
--- a/src/ntf/ntfd/NtfClient.h
+++ b/src/ntf/ntfd/NtfClient.h
@@ -22,6 +22,7 @@
#ifndef NTF_NTFD_NTFCLIENT_H_
#define NTF_NTFD_NTFCLIENT_H_
+#include <atomic>
#include "ntf/ntfd/NtfSubscription.h"
#include "ntf/ntfd/NtfNotification.h"
#include "ntf/ntfd/NtfReader.h"
@@ -86,7 +87,7 @@ class NtfClient {
SaVersionT safVersion_;
// The flag to indicate that the client down and is going to deleted
- bool client_down_flag_;
+ std::atomic<bool> client_down_flag_;
typedef std::map<SaNtfSubscriptionIdT, NtfSubscription *> SubscriptionMap;
SubscriptionMap subscriptionMap;
diff --git a/src/ntf/ntfd/ntfs_mds.c b/src/ntf/ntfd/ntfs_mds.c
index a4b4a5f09..8594db674 100644
--- a/src/ntf/ntfd/ntfs_mds.c
+++ b/src/ntf/ntfd/ntfs_mds.c
@@ -947,10 +947,10 @@ static uint32_t mds_svc_event(struct ncsmds_callback_info
*info)
// Set flag for client down at standby node then ntfd
// just removes only the downed clients. This is
helpful
// in some cases ntfd receives this event after
- // checkpoint of initializing new client
- if (ntfs_cb->ha_state == SA_AMF_HA_STANDBY) {
- SetClientsDownFlag(evt->fr_dest);
- }
+ // checkpoint of initializing new client in standby or
+ // checking if client is downed in active.
+
+ SetClientsDownFlag(evt->fr_dest);
/* Push the event and we are done */
if (m_NCS_IPC_SEND(&ntfs_cb->mbx, evt,
--
2.15.1
diff --git a/src/ntf/ntfd/NtfAdmin.cc b/src/ntf/ntfd/NtfAdmin.cc
index d0bb507..4cf7d10 100644
--- a/src/ntf/ntfd/NtfAdmin.cc
+++ b/src/ntf/ntfd/NtfAdmin.cc
@@ -531,9 +531,34 @@ void NtfAdmin::ClientsDownRemoved(MDS_DEST mds_dest) {
*
* @param mds_dest
*/
+// [Lennart] This function name is misleading. This function does not
+// only set the client down flag. I sets the flag to true for the client id
+// given as input. The only function that actually sets the flag to true is the
+// NtfClient::SetClientDownFlag() method and it should be named so it is clear
+// that the value it gives the flag is 'true'. It is possible to set a flag to
+// false as well.
+// It is als not a good habit to have several functions with the same name even
+// if in this case I can somewhat understand why. But it does not make it clear
+// how this work.
+//
+// First there is a C wrapper "SetClientsDownFlag(...)" calling
+// "NtfAdmin::theNtfAdmin->SetClientsDownFlag(mds_dest)" which is
+// this method refered using a pointer with a hard to understand/misleading name
+// Second we have this method that is not setting any flag. What is done here
+// is to search for the client given as in parameter and use the found clients
+// Third there is a method in the NtfClient::SetClientDownFlag() that is the
+// only method that sets the flag to some value, in this case 'true'
+// Please, think of a way to clarify this by for example using descriptive names.
+//
+// Why is a mutex used here? The client_flag variable in the NtfClien class
+// is atomic "std::atomic<bool> client_down_flag_"
+//
+// Where is the client_down_flag_ set to false when the client is no longer down?
void NtfAdmin::SetClientsDownFlag(MDS_DEST mds_dest) {
TRACE_ENTER();
osaf_mutex_lock_ordie(&client_down_mutex);
+ // [Lennart] it is not a very descriptive name. Should be renamed to
+ // something like "clientMaps"
for (const auto &it : clientMap) {
NtfClient *client = it.second;
if (client->getMdsDest() == mds_dest) {
diff --git a/src/ntf/ntfd/NtfClient.cc b/src/ntf/ntfd/NtfClient.cc
index 57e591f..1bdf9ae 100644
--- a/src/ntf/ntfd/NtfClient.cc
+++ b/src/ntf/ntfd/NtfClient.cc
@@ -503,7 +503,8 @@ void NtfClient::set_client_version(SaVersionT* ver) { safVersion_ = *ver; }
*/
SaVersionT* NtfClient::getSafVersion() { return &safVersion_; }
-
+// [Lennart] These two functions should be moved to the class
+// declaration file NtfClient.h
void NtfClient::SetClientDownFlag() { client_down_flag_ = true; }
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
