Need to correct my own review comments again.
Just goes to show that this code is really hard to review.
See below.
Anders Bjornerstedt wrote:
> Additional comments (fourth review mail for #49).
>
>
> 18) ImmModel::ccbObjectCreate
>
> Log message:
>
> AttrFlagIncludes(SA_IMM_ATTR_PERSISTENT)) ==
> omi->second->mClassInfo->mAttrMap.end()) {
> LOG_NO("ERR_INVALID_PARAM:
> NO_DANGLING attribute (%s) "
> "cannot have a
> reference to a non PRTO (Ccb %u)",
> i4->first.c_str(), ccbId);
>
> should be:
>
>
> AttrFlagIncludes(SA_IMM_ATTR_PERSISTENT)) ==
> omi->second->mClassInfo->mAttrMap.end()) {
> LOG_NO("ERR_INVALID_PARAM:
> NO_DANGLING Creation of object %s attribute (%s) "
> "refers to a non
> persistent RTO %s (Ccb %u)", ...., ....., .....,
> i4->first.c_str(), ccbId);
>
> ---------------------------------------
> Log message:
>
> if(omi->second->mCcbId == ccbId) {
> LOG_NO("ERR_BAD_OPERATION:
> NO_DANGLING attribute %s "
> "refers to object that
> will be deleted (Ccb %u)",
> i4->first.c_str(), ccbId);
>
> Should be:
>
> if(omi->second->mCcbId == ccbId) {
> LOG_NO("ERR_BAD_OPERATION:
> NO_DANGLING Creation of object %s attribute %s "
> "refers to object %s
> that will be deleted in same ccb (Ccb %u)", ..................,
> i4->first.c_str(), ccbId);
>
> ------------------------
>
> Log message:
>
> LOG_IN("ERR_BUSY: NO_DANGLING
> attribute %s refers to "
> "flagged object for
> deletion (Ccb %u)",
> i4->first.c_str(), ccbId);
> Should be.
> LOG_IN("ERR_BUSY: NO_DANGLING
> Creation of object %s attribute %s refers to "
> "object %s flagged for
> deletion in other ccb (%u), this (Ccb %u)", .......
> i4->first.c_str(), ccbId);
>
> ---------------------------------
>
> Log message:
>
> LOG_IN("ERR_BUSY: NO_DANGLING
> attribute %s refers to "
> "flagged object for
> creation (Ccb %u)",
> i4->first.c_str(), ccbId);
>
> Should be:
>
> LOG_IN("ERR_BUSY: NO_DANGLING
> Creation of object %s attribute %s refers to "
> "object flagged for
> creation in other ccb (%u), this (Ccb %u)",....
> i4->first.c_str(), ccbId);
>
> ------------------------------
> 19) ImmModel::checkNewNoDanglingReference
>
> Comment says:
> /*
> * Check new added NO_DANGLING references from ObjectModify operation
>
> But it only checks one reference. Better comment:
>
> /*
> * Check one added NO_DANGLING reference from ObjectModify operation
> ---------------------
>
> Better function name: ImmModel::objectModifyNDTargetOk()
>
> This function is only used from ccbObjectModify()
>
> I think you should add two 'const char*' arguments taking the c_str()
> for the source object DN and
> the attribute name for the reference. These should be included in LOG
> printouts to assist debugging of
> either application or server (in case of problems with the no-dangling
> logic or application was designed incorrectly).
> ----------------------
>
> Log message:
> LOG_NO("ERR_INVALID_PARAM: NO_DANGLING attribute "
> "has a reference to a non persistent runtime
> object (Ccb %u)",
>
> Must print also source-object-dn, attribute-name and dn of RTO target.
> --------------------------------
> Log message:
> LOG_NO("ERR_BAD_OPERATION: NO_DANGLING attribute "
> "has a reference to the object that will be
> deleted (Ccb %u)",
> ccbId);
>
> Must print also source-object-dn, attribute-name and dn of RTO target.
> ------------------------------
> Log message:
> LOG_IN("ERR_BUSY: NO_DANGLING attribute has a reference
> to "
> "the object flagged for deletion (Ccb %u)",
> ccbId);
>
> Must print also source-object-dn, attribute-name dn of target and
> ccb-id of target.
> ----------------------------------------
> Log message:
>
> if(omi->second->mCcbId != ccbId) {
> LOG_IN("ERR_BUSY: NO_DANGLING attribute has a reference
> to "
> "the object flagged for creation (Ccb %u)",
> ccbId);
>
> Must print also source-object-dn, attribute-name dn of target and
> ccb-id of target.
> ---------------------------------------------------------------------------------
>
>
> 20) ImmModel::ccbObjectModify
>
> Patch has this change:
> - if(attrValue->empty() && (attr->mFlags &
> SA_IMM_ATTR_INITIALIZED)) {
> + if(err == SA_AIS_OK && attrValue->empty() &&
> (attr->mFlags & SA_IMM_ATTR_INITIALIZED)) {
>
> I dont see why the added '(err == SA_AIS_OK)' is needed.
> All code above in same block that does assign err also does a 'break;'
> out of the switch statement.
> ---------------------------------------------------------------------------------
>
>
> This comments in this needs to be improved:
> + // Collect all NO_DANGLING references from the original object
> + collectNoDanglingReferences(object, origNDRefs);
> + // Collect all NO_DANGLING references from afim after
> applying operations
> + collectNoDanglingReferences(afim, afimPostOpNDRefs);
> +
>
> Note that in the various names you are somtimes using the subterm
> 'Refs' and 'ND' (origNDRefs)
> and sometimes 'References' and 'NoDangling'
> (collectNoDanglingReferences).
> Lets try to converge on 'Refs' and ND to keep down the name lengths
> without loosing meaning.
> At least try to stick to ONE naming convention. That is use one name
> for one thing everywhere
> in the code (not necessarily comments & log printouts) and not several
> variants.
> In comments and log/trace printouts its ok to use the long names when
> not referring explicitly to a
> variable/function that has the short name.
>
> + // Exclude objects for deleting from the original object
>
> You are excluding references/DN-values. Also try to express what you
> are doing/creating and
> not just what you are removing from something.
> Should be:
> //Create the set s1 of no-dangling-references that remain after
> the modification.
No should be:
//Create the set s1 of no-dangling references ADDED by the PREVIOUS
VERSION of the afim.
Possibly name s1 to something less abstract like: preOpAddedRefs.
>
> + std::set_difference(afimPreOpNDRefs.begin(),
> afimPreOpNDRefs.end(), origNDRefs.begin(), origNDRefs.end(),
> + std::inserter(s1, s1.end()));
>
> + // Exclude objects for deleting that will be added
> Should be:
> //Create set s2 of ND-references that remain after the
> modification and where not added by the modification.
No should be:
//Create the set s2 of no-dangling-references that where added by
previous afim but REMOVED in the latest AFIM.
> + std::set_difference(s1.begin(), s1.end(),
> afimPostOpNDRefs.begin(), afimPostOpNDRefs.end(),
> + std::inserter(s2, s2.end()));
> +
> + // Delete object references from sReverseRefObjectMMap
Comment states the obvious but does not capture the intent. Suggest:
//Adjust sReverse.....MMap for the dynamic change done by this
latest modification in the CCB.
//This means removing NDrefs added by a previous create or modify in
this ccb that where
//subsequently removed by a modification in the same CCB.
Zoran, please correct me if I got it wrong.
/AndersBj
> + for(it=s2.begin(); it!=s2.end(); ++it) {
> + omi = sObjectMap.find(*it);
> + if(omi != sObjectMap.end()) {
> + ommi = sReverseRefObjectMMap.find(omi->second);
> + while((ommi != sReverseRefObjectMMap.end()) &&
> (ommi->first == omi->second)) {
> + if(ommi->second == object) {
> + sReverseRefObjectMMap.erase(ommi);
> + break;
> + }
> + }
> + }
> + }
>
> ------------------------------------------------------------------------------------------------------------------------
>
>
> (still in ccbObjectModify)
>
>
>
>
------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
