Hi Lennart, comments inline.
On 2016/09/27 04:58 PM, Lennart Lund wrote: > 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? [Neel] BAD_HANDLE will not be returned always. But the loop should not be infinite. >> 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) [Neel] you can guard this with immhandle, if it is zero or NULL do not finalize, this is to generalize and not a mandatory change. > >> 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 [Neel] even though it is not TRY_AGAIN, for each retry there must be a sleep, for any type of error, because this will give system/openSAF to do some initializations, instead of returning the same error again.. This has been done in AMF (AMF is well tested with BAD_HANDLE from IMM perspective): if (rc == SA_AIS_ERR_BAD_HANDLE) { osaf_nanosleep(&time); Thanks, Neel. >> 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
