Hi Hung, comments inline. will resend the patch for review again.
/Neel. On Thursday 19 November 2015 09:37 AM, Hung Nguyen wrote: > Hi Neel, > > Please see my comments inline. > > BR, > Hung Nguyen - DEK Technologies > > -------------------------------------------------------------------------------- > From: Neelakanta [email protected] > Sent: Wednesday, November 18, 2015 8:28PM > To: Hung Nguyen, Zoran Milinkovic > [email protected],[email protected] > Cc: Opensaf-devel > [email protected] > Subject: [PATCH 1 of 1] imm:Allow deletion of dangling ref object, when the > object is modified by clearing NO_DANGLING attribute in same CCB [#1599] > > > osaf/services/saf/immsv/immnd/ImmModel.cc | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > > If the NO_DANGLING reference is modified(removed) in the same CCB, > then deletion of the NO_DANGLING object can be allowed because, we > are in apply and the CCBcommit will modify sReverseRefsNoDanglingMMap. > > diff --git a/osaf/services/saf/immsv/immnd/ImmModel.cc > b/osaf/services/saf/immsv/immnd/ImmModel.cc > --- a/osaf/services/saf/immsv/immnd/ImmModel.cc > +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc > @@ -5050,6 +5050,24 @@ bool ImmModel::validateNoDanglingRefsDel > if(!(ommi->second->mObjFlags & IMM_DELETE_LOCK)) { > std::string objName; > getObjectName(ommi->second, objName); > + if(ommi->second->mObjFlags & IMM_NO_DANGLING_FLAG){ // > IMM_NO_DANGLING_FLAG will be set in CcbModify > + /* If the NO_DANGLING reference is modified(removed) in the > same CCB, > + then deletion of the NO_DANGLING object can be allowed > because, we are in apply > + and the CCBcommit will modify sReverseRefsNoDanglingMMap*/ > + ObjectMutationMap::iterator omit = > ccb->mMutations.find(objName); > + if(omit != ccb->mMutations.end()){ > + ObjectInfo * afim = omit->second->mAfterImage; > + ObjectNameSet afimRefs; > + ObjectNameSet::iterator it; > + collectNoDanglingRefs(afim, afimRefs); > + if(afimRefs.find(omit->first.c_str()) == afimRefs.end()){ > + TRACE("Delete of object %s is removed from > NO_DANGLING reference in the same Ccb%u" > + "from object %s", omit->first.c_str(), > ccb->mId, objName.c_str()); > + break; > [Hung] Here we break the while() loop but there might be more objects that > have no-dangling ref to 'omi->second'. > This would leave those objects with dangling refs, which will make the > cluster fail to reboot. > So the while() shouldn't break here and continue the check. > correct, it should be continue. > And even when 'ommi->second' passes the deletion/modification check, it must > also pass the in-the-same-ccb check. > Personally I think that ccb check is a prerequisite. We should do it first. > That check should be moved above 'if(!(ommi->second->mObjFlags & > IMM_DELETE_LOCK))'. > > And the afimRefs check can be something like this: > > if(afimRefs.find(omit->first.c_str()) == afimRefs.end()){ > TRACE("..."); > ++ommi; > continue; /* This is safe because the ccb check is moved above, we will > not miss the ccb check */ > } ccb, check will be added as suggested bu zoran. > > And if you move the ccb check above, this 'if' should be an assertion. > if(omit != ccb->mMutations.end()) > > > This is just a suggestion from me, mainly to show that we might miss the > ccb-check and other objects that also have refs to the to-be-deleted object. > > I think there's also other alternative solutions. > Like, we can even remove that 'if(ommi->second->mCcbId != ccb->mId)', and use > 'omit != ccb->mMutations.end()' as ccb check. > If it is in the same ccb, we will surely find it in the mutation map. > > > > + } > + } > + } > + > LOG_WA("ERR_FAILED_OPERATION: Delete of object %s would violate > NO_DANGLING reference " > "from object %s, not scheduled for delete by this > Ccb:%u", > omit->first.c_str(), objName.c_str(), ccb->mId); > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
