Hi,
See my comment/reply inline [Lennart]
Thanks
Lennart
> -----Original Message-----
> From: Rafael Odzakow
> Sent: den 23 september 2016 10:34
> To: Lennart Lund <lennart.l...@ericsson.com>;
> reddy.neelaka...@oracle.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]
>
> comments one comment under [rafael] tag.
>
>
> On 09/22/2016 03:33 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) {
> [rafael] The while loop is here to retry CcbObjectDelete only one time.
> A much clearer way is to call CcbObjectDelete twice. As it looks now I
> needed to reread this section before I understood the logic. By trying
> to avoid writing extra code you have complicated a simple function. Call
> it twice instead of having a while loop that will retry after BAD_HANDLE
> endless amounts of times. A possibility for an infinite loop is not a
> good idea. After the second CcbObjectDelete you can fail normally.
> Please explain in comments or remove this while loop.
[Lennart] Unfortunately we use C API for IMM that requires tests of return
codes and in some cases actions and retries of the request. In order to
encapsulate this handling and to avoid return and goto statements in several
places a while loop is used. In this simple function where for the moment
nothing is done after this handling it would have been possible to use a
sequence containing a check where CcbObjectDelete() where called a second time
in case of BAD HANDLE error. I have chosen to keep the original solution but
has done some minor changes that hopefully will make the code easier to
understand. I have also written some comments explaining why a retry mechanism
is needed
> > + 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
