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.
+ 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.
+ std::set_difference(s1.begin(), s1.end(),
afimPostOpNDRefs.begin(), afimPostOpNDRefs.end(),
+ std::inserter(s2, s2.end()));
+
+ // Delete object references from sReverseRefObjectMMap
+ 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
