Hi Hans,

Thanks for reviewing the patch. I've made the necessary changes based on your comments before having the patch pushed.

Regards,
Nguyen

On 3/9/2018 4:58 PM, Hans Nordebäck wrote:
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;
    }



------------------------------------------------------------------------------
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