Hi, It must be possible to find a solution that does not need to iterate twice over the ccb contents for ALL ccbs Including ccb 1 (the loading ccb).
I would prefer to see some new member/members added to CcbInfo and/or Objectmutation for dealing with this special case and avoiding impact on all ccb processing. This case (mutations affecting no-dangling on top of a create in same ccb) is likely to be quite rare. /AndersBj From: Hung Nguyen [mailto:[email protected]] Sent: den 2 juni 2015 05:46 To: Zoran Milinkovic; Neelakanta Reddy; Anders Björnerstedt Cc: [email protected] Subject: Re: [devel] [PATCH 1 of 1] imm: Ensure IMM_MODIFY mutations are always committed first [#1377] Hi again, I tried the solution that changes the if condition in addNewNoDanglingRefs(). The code is now like this: ObjectNameSet::iterator si; ObjectMutationMap::iterator omuti; ObjectMap::iterator omi; CcbVector::iterator i = std::find_if(sCcbVector.begin(), sCcbVector.end(), CcbIdIs(ccbId)); osafassert(i != sCcbVector.end()); CcbInfo* ccb = (*i); for(si=dnSet.begin(); si!=dnSet.end(); ++si) { omi = sObjectMap.find(*si); // After all validation, object must exist osafassert(omi != sObjectMap.end()); omuti = ccb->mMutations.find(*si); if (omuti != ccb->mMutations.end() && omuti->second->mOpType == IMM_CREATE) { sReverseRefsNoDanglingMMap.insert(std::pair<ObjectInfo *, ObjectInfo *>(omi->second, obj)); } } But we still need 2 iterations because we can't free memory (delete omut) in the loop. addNewNoDanglingRefs() still needs the ObjectMutation to check the mOpType. We have to free the memory in another loop. Attached is the patch. Best Regards, Hung Nguyen DEK Technologies ________________________________ From: Hung Nguyen Sent: Tuesday, June 02, 2015 10:03AM To: Zoran Milinkovic, Reddy Neelakanta Reddy Peddavandla, Anders Bjornerstedt Cc: opensaf-devel Subject: Re: [devel] [PATCH 1 of 1] imm: Ensure IMM_MODIFY mutations are always committed first [#1377] Hi, About this IF statement if((omi->second->mObjFlags & IMM_CREATE_LOCK)|| omit!=ccb->mMutations.end()) If the OpType of the mutation is IMM_MODIFY, the statement will return true and add duplicated NO_DANGLING reference to sReverseRefsNoDanglingMMap. Here's the bug: # immcfg > immcfg -c TestClass testClass=1 > immcfg -c TestClass testClass=2 Add no-dangling ref # immcfg > immcfg -a dep=testClass=1 testClass=2 > immcfg -a dep=testClass=2 testClass=1 Remove no-dangling ref # immcfg > immcfg -a dep= testClass=1 > immcfg -a dep= testClass=2 The duplicated references are not removed so the object can't be deleted. # immcfg -d testClass=1 error - saImmOmCcbApply FAILED: SA_AIS_ERR_FAILED_OPERATION (21) # immcfg -d testClass=2 error - saImmOmCcbApply FAILED: SA_AIS_ERR_FAILED_OPERATION (21) Best Regards, Hung Nguyen DEK Technologies ________________________________ From: Zoran Milinkovic Sent: Monday, June 01, 2015 7:53PM To: Reddy Neelakanta Reddy Peddavandla, Hung Nguyen, Anders BjörnerstedtAnders Björnerstedt Cc: opensaf-devel Subject: RE: [devel] [PATCH 1 of 1] imm: Ensure IMM_MODIFY mutations are always committed first [#1377] Hi, Anders and I have talked about the patch and had more-less same points as Neelakanta mentioned in the review. The patch has an impact on the performance, and it will affect every CCB (two iterations), specially loading data. The change could be done only for NO_DANGLING objects, changing addNewNoDanglingRefs, as Neelakanta commented in his review. Best regards, Zoran -----Original Message----- From: Neelakanta Reddy [mailto:[email protected]] Sent: Monday, June 01, 2015 2:33 PM To: Hung Nguyen D; Anders Björnerstedt Cc: [email protected]<mailto:[email protected]> Subject: Re: [devel] [PATCH 1 of 1] imm: Ensure IMM_MODIFY mutations are always committed first [#1377] Hi Hung, Reviewed and tested the patch. the patch is working good. But the alternate solution may be passing ccbId, through commitModify -->addNewNoDanglingRefs In addNewNoDanglingRefs, modify the if condition (if CCB create is committed and CREATE_LOCK is removed, then it will be present in ccbvector of same CCB) CcbInfo* ccb =(*(std::find_if(sCcbVector.begin(), sCcbVector.end(), CcbIdIs(ccbId)))); ObjectMutationMap::iterator omit; omit=omit.find(omi); if((omi->second->mObjFlags & IMM_CREATE_LOCK)|| omit!=ccb->mMutations.end()) sReverseRefsNoDanglingMMap.insert(std::pair<ObjectInfo *, ObjectInfo *>(omi->second, obj)); wait for AndersBj comments on alternate solution. /Neel. On Monday 01 June 2015 12:41 PM, Hung Nguyen wrote: osaf/services/saf/immsv/immnd/ImmModel.cc | 65 +++++++++++++++++------------- 1 files changed, 36 insertions(+), 29 deletions(-) When an IMM_CREATE mutation is committed, the IMM_CREATE_LOCK flag is also cleared in commitCreate(). If the CCB has IMM_MODIFY mutations that add references to object of committed IMM_CREATE mutations, it will fail to add NO_DANGLING references to sReverseRefsNoDanglingMMap in addNewNoDanglingRefs() due to cleared IMM_CREATE_LOCK flag. mMutations map is sorted by object DN so the order of mutations to be committed depends on the object DNs. This patch ensures that IMM_MODIFY mutations of the CCB are always committed first. The mutation list will be looped through twice. The first loop commits all IMM_MODIFY mutations, the second loop commits the rest and free the allocated memory. 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 @@ -5470,35 +5470,42 @@ ImmModel::ccbCommit(SaUint32T ccbId, Con ccb->mWaitStartTime = 0; //Do the actual commit! - ObjectMutationMap::iterator omit; - for(omit=ccb->mMutations.begin(); omit!=ccb->mMutations.end(); ++omit){ - ccbNotEmpty=true; - ObjectMutation* omut = omit->second; - osafassert(!omut->mWaitForImplAck); - switch(omut->mOpType){ - case IMM_CREATE: - if(ccbId != 1) { - TRACE_5("COMMITING CREATE of %s", omit->first.c_str()); - } - osafassert(omut->mAfterImage); - commitCreate(omut->mAfterImage); - omut->mAfterImage=NULL; - break; - case IMM_MODIFY: - osafassert(omut->mAfterImage); - pbeModeChange = commitModify(omit->first, omut->mAfterImage) || pbeModeChange; - omut->mAfterImage=NULL; - break; - case IMM_DELETE: - osafassert(omut->mAfterImage==NULL); - commitDelete(omit->first); - break; - default: - abort(); - }//switch - delete omut; - }//for - + /* Ensure that modifications are committed first [#1377] */ + for (int doIt = 0; doIt < 2; ++doIt) { + ObjectMutationMap::iterator omit; + for(omit=ccb->mMutations.begin(); omit!=ccb->mMutations.end(); ++omit){ + ObjectMutation* omut = omit->second; + if ((!doIt && omut->mOpType != IMM_MODIFY) || (doIt && omut->mOpType == IMM_MODIFY)) { + if (doIt) delete omut; //only delete in the second loop + continue; + } + ccbNotEmpty=true; + osafassert(!omut->mWaitForImplAck); + switch(omut->mOpType){ + case IMM_CREATE: + if(ccbId != 1) { + TRACE_5("COMMITING CREATE of %s", omit->first.c_str()); + } + osafassert(omut->mAfterImage); + commitCreate(omut->mAfterImage); + omut->mAfterImage=NULL; + break; + case IMM_MODIFY: + osafassert(omut->mAfterImage); + pbeModeChange = commitModify(omit->first, omut->mAfterImage) || pbeModeChange; + omut->mAfterImage=NULL; + break; + case IMM_DELETE: + osafassert(omut->mAfterImage==NULL); + commitDelete(omit->first); + break; + default: + abort(); + }//switch + if (doIt) delete omut; //only delete in the second loop + }//for omit + }//for doIt + ccb->mMutations.clear(); if(ccbIdLongDnGuard == ccbId) {ccbIdLongDnGuard= 0;} ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected]<mailto:[email protected]> https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected]<mailto:[email protected]> https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
