Hi Neel, See my comments/answers inline [Lennart]
Thanks Lennart > -----Original Message----- > From: Neelakanta Reddy [mailto:reddy.neelaka...@oracle.com] > Sent: den 27 september 2016 11:04 > To: Lennart Lund <lennart.l...@ericsson.com>; Rafael Odzakow > <rafael.odza...@ericsson.com> > Cc: opensaf-devel@lists.sourceforge.net > Subject: Re: [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when > deleting node group [#2046] > > Hi Lennart, > > Reviewed the patch, I have to NACK the patch because of the comments. > > Following are the comments: > > 1. The retries can not be infinite, while(1) has to be changed to some > finite number of retries. [Lennart] Since immutil_saImmOmCcbObjectDelete() may have to be called more than once I have chosen to use a loop. The loop is not infinite but the loop is not ended because of a loop counter and the exit condition is not tested within the while parenthesis. The loop continues only if immutil_saImmOmCcbObjectDelete returns SA_AIS_ERR_BAD_HANDLE. Before the loop continues new handles are requested but if this fails the loop will end. The loop will be infinite only if it is possible to create handles (no Fail) and saImmOmCcbObjectDelete always returns SA_AIS_ERR_BAD_HANDLE even if new handles just have been successfully created. If you think this could happen I can insert a loop counter to end the loop in case? > 2. make saImmOiFinalize as part of getAllImmHandles. (for more > information look into avd_imm_reinit_bg_thread in > osaf/services/saf/amf/amfd/imm.cc) [Lennart] This is not an OI it's an object manager. In this case immutil_saImmOmFinalize() is called before the attempt to create new handles. However it is not done in the getAllImmHandles() function. This function is also used to create the handles in the constructor and if this fails none of the admin operations will be executed (campaign will fail) > 3. for each retry there must be sleep. [Lennart] Note that this is not a loop for handling SA_AIS_ERR_TRY_AGAIN this is handled within the immutil_xxx functions. A sleep is not needed here since there is no attempt made to call immutil_saImmOmCcbObjectDelete() again until after both OM Finalize and creation of new handles is done > > Regards, > Neel. > > > On 2016/09/22 07:03 PM, Lennart Lund wrote: > > osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc | 49 > +++++++++++++++++-------- > > 1 files changed, 33 insertions(+), 16 deletions(-) > > > > > > Fix the deleteNodeGroup() method must be so that if the delete operation > fails > > with bad handles new handles are created so that the node group can be > deleted > > > > diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc > b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc > > --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc > > +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc > > @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup( > > &nodeGroupName); > > > > TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str()); > > - > > - m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle, > > - &nodeGroupName); > > - if (m_errno != SA_AIS_OK) { > > - LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s", > > - __FUNCTION__, nodeGroupName_s.c_str(), > > - saf_error(m_errno)); > > - rc = false; > > - } else { > > - m_errno = saImmOmCcbApply(m_ccbHandle); > > - if (m_errno != SA_AIS_OK) { > > - LOG_NO("%s: saImmOmCcbApply() Fail '%s'", > > - __FUNCTION__, saf_error(m_errno)); > > - rc = false; > > - } > > - } > > + while (true) { > > + m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle, > > + &nodeGroupName); > > + if (m_errno == SA_AIS_ERR_BAD_HANDLE) { > > + // Handles may have been invalidated/timed out > > + (void)immutil_saImmOmFinalize(m_omHandle); > > + bool rc = getAllImmHandles(); > > + if (rc == false) { > > + LOG_NO("%s: getAllImmHandles Fail", > > + __FUNCTION__); > > + break; > > + } > > + continue; > > + } > > + if (m_errno != SA_AIS_OK) { > > + LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s", > > + __FUNCTION__, nodeGroupName_s.c_str(), > > + saf_error(m_errno)); > > + rc = false; > > + break; > > + } else { > > + m_errno = saImmOmCcbApply(m_ccbHandle); > > + if (m_errno != SA_AIS_OK) { > > + LOG_NO("%s: saImmOmCcbApply() Fail '%s'", > > + __FUNCTION__, saf_error(m_errno)); > > + rc = false; > > + } > > + break; > > + } > > + } > > > > TRACE_LEAVE(); > > return rc; > > @@ -3633,6 +3647,9 @@ bool SmfAdminOperation::nodeGroupAdminOp > > &nodeGroupName, 0, adminOp, params, > > &oi_rc, smfd_cb->adminOpTimeout); > > > > + if (m_errno == SA_AIS_OK && oi_rc == SA_AIS_OK) > > + break; > > + > > if (retry <= 0) { > > LOG_NO("Fail to invoke admin operation, too many > OI " > > "TRY_AGAIN, giving up. %s", > __FUNCTION__); ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
