ack, review only. Some comments below marked with [HansN].
/Thanks HansN
On 03/06/2018 07:39 AM, Nguyen Luu wrote:
Current check for the required setting of the SA_AMF_COMPONENT_NAME
env variable in some amf api's (ComponentRegister, QuiescingComplete)
would crash the invoking process if that env variable was missed
to be set for some reason, as the agent lib tries, during cleanup,
to unlock a mutex which it has not previously locked yet.
---
src/amf/agent/amf_agent.cc | 51
++++++++++++++++++++++++++++------------------
1 file changed, 31 insertions(+), 20 deletions(-)
diff --git a/src/amf/agent/amf_agent.cc b/src/amf/agent/amf_agent.cc
index f0ed7a8..a162af1 100644
--- a/src/amf/agent/amf_agent.cc
+++ b/src/amf/agent/amf_agent.cc
@@ -2,6 +2,7 @@
*
* (C) Copyright 2008 The OpenSAF Foundation
* Copyright (C) 2017, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (C) 2018 Ericsson AB. 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
@@ -13,6 +14,7 @@
* licensing terms.
*
* Author(s): Emerson Network Power
+ * Ericsson
*
*/
@@ -272,7 +274,7 @@ SaAisErrorT AmfAgent::Dispatch(SaAmfHandleT
hdl, SaDispatchFlagsT flags) {
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
- /* acquire cb read lock */
+ /* acquire cb write lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
hdl))) {
@@ -293,7 +295,7 @@ SaAisErrorT AmfAgent::Dispatch(SaAmfHandleT hdl,
SaDispatchFlagsT flags) {
pend_fin = cb->pend_fin;
done:
- /* release cb read lock and return handles */
+ /* release cb write lock and return handles */
if (cb) {
m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
ncshm_give_hdl(gl_ava_hdl);
@@ -356,7 +358,7 @@ SaAisErrorT AmfAgent::Finalize(SaAmfHandleT hdl) {
uninstall_osafAmfSCStatusChangeCallback();
}
- /* acquire cb read lock */
+ /* acquire cb write lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
/* retrieve hdl rec */
@@ -394,7 +396,7 @@ SaAisErrorT AmfAgent::Finalize(SaAmfHandleT hdl) {
cb->pend_fin++;
done:
- /* release cb read lock and return handles */
+ /* release cb read write and return handles */
if (cb) {
m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
ncshm_give_hdl(gl_ava_hdl);
@@ -457,8 +459,7 @@ SaAisErrorT
AmfAgent::ComponentRegister(SaAmfHandleT hdl,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl))) {
[HansN] the agent is c++, so use if (!(cb = static_cast<AVA_CB
*>(ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl)))) {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
@@ -627,8 +628,7 @@ SaAisErrorT
AmfAgent::ComponentUnregister(SaAmfHandleT hdl,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl))) {
[HansN] if (!(cb = static_cast<AVA_CB
*>(ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl)))) {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
@@ -636,6 +636,13 @@ SaAisErrorT
AmfAgent::ComponentUnregister(SaAmfHandleT hdl,
/* acquire cb read lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
+
+ if (!m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ TRACE_2("The SA_AMF_COMPONENT_NAME environment variable is NULL");
+ rc = SA_AIS_ERR_LIBRARY;
+ goto done;
+ }
+
/* Version is previously set in in initialize function */
if (ava_B4_ver_used(cb)) {
TRACE_2("Invalid AMF version, B 4.1");
@@ -1373,14 +1380,20 @@ SaAisErrorT
AmfAgent::CSIQuiescingComplete(SaAmfHandleT hdl, SaInvocationT inv,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl))) {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
/* acquire cb read lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
+
+ if (!m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ TRACE_2("The SA_AMF_COMPONENT_NAME environment variable is NULL");
+ rc = SA_AIS_ERR_LIBRARY;
+ goto done;
+ }
+
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
hdl))) {
rc = SA_AIS_ERR_BAD_HANDLE;
@@ -1591,7 +1604,7 @@ SaAisErrorT AmfAgent::ProtectionGroupTrack(
goto done;
}
/* acquire cb read lock */
- m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
+ m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
[HansN] what is the reason to change to a read lock? Today the
m_NCS_LOCK maps to a "pthread_mutex_lock" but it could be changed to
"pthread_rwlock_rdlock"
and "pthread_rwlock_wrlock", i.e. several readers but only one writer
is allowed. To test we can change the mapping to use "pthread_rwlock_".
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
hdl))) {
rc = SA_AIS_ERR_BAD_HANDLE;
@@ -1702,7 +1715,7 @@ SaAisErrorT AmfAgent::ProtectionGroupTrack(
done:
/* release cb read lock and return handles */
if (cb) {
- m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
+ m_NCS_UNLOCK(&cb->lock, NCS_LOCK_READ);
[HansN] same comment as above
ncshm_give_hdl(gl_ava_hdl);
}
if (hdl_rec) ncshm_give_hdl(hdl);
@@ -2193,7 +2206,6 @@ SaAisErrorT AmfAgent::Initialize_4(SaAmfHandleT
*o_hdl,
/* Create AVA/CLA CB */
if (ncs_ava_startup() != NCSCC_RC_SUCCESS) {
- ncs_agents_shutdown();
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
@@ -2257,7 +2269,7 @@ done:
/* free the hdl rec if there's some error */
if (hdl_rec && SA_AIS_OK != rc) ava_hdl_rec_del(cb, hdl_db,
&hdl_rec);
- /* release cb read lock and return handle */
+ /* release cb write lock and return handle */
if (cb) {
m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
ncshm_give_hdl(gl_ava_hdl);
@@ -2432,7 +2444,7 @@ SaAisErrorT AmfAgent::ProtectionGroupTrack_4(
goto done;
}
/* acquire cb read lock */
- m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
+ m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
[HansN] same comment as above
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
hdl))) {
rc = SA_AIS_ERR_BAD_HANDLE;
@@ -2579,7 +2591,7 @@ SaAisErrorT AmfAgent::ProtectionGroupTrack_4(
done:
/* release cb read lock and return handles */
if (cb) {
- m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
+ m_NCS_UNLOCK(&cb->lock, NCS_LOCK_READ);
[HansN] same comment as above
ncshm_give_hdl(gl_ava_hdl);
}
if (hdl_rec) ncshm_give_hdl(hdl);
@@ -2630,8 +2642,8 @@ SaAisErrorT
AmfAgent::ProtectionGroupNotificationFree_4(
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
- /* acquire cb write lock */
- m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
+ /* acquire cb read lock */
+ m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
[HansN] same comment as above
/* Version is previously set in in initialize function */
if (!ava_B4_ver_used(cb)) {
@@ -2662,7 +2674,7 @@ SaAisErrorT
AmfAgent::ProtectionGroupNotificationFree_4(
done:
/* release cb read lock and return handler */
if (cb) {
- m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
+ m_NCS_UNLOCK(&cb->lock, NCS_LOCK_READ);
[HansN] same comment as above
ncshm_give_hdl(gl_ava_hdl);
}
TRACE_LEAVE2("rc:%u", rc);
@@ -2976,7 +2988,6 @@ SaAisErrorT saAmfInitialize_o4(SaAmfHandleT
*o_hdl,
/* Create AVA/CLA CB */
if (ncs_ava_startup() != NCSCC_RC_SUCCESS) {
- ncs_agents_shutdown();
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
