Hi Zoran, All the attribute definition will be moved to the top of the file. The response for the remaining comments.
Thanks, Neel. On 2016/08/12 07:36 PM, Zoran Milinkovic wrote: > Hi Neelakanta, > > Find my comments inline > > -----Original Message----- > From: reddy.neelaka...@oracle.com [mailto:reddy.neelaka...@oracle.com] > Sent: den 27 juli 2016 10:32 > To: Zoran Milinkovic; Hung Duc Nguyen > Cc: opensaf-devel@lists.sourceforge.net > Subject: [PATCH 3 of 5] imm: Checking of Imm limits [#195] > > osaf/services/saf/immsv/immnd/ImmModel.cc | 322 > +++++++++++++++++++++++++++++- > osaf/services/saf/immsv/immnd/ImmModel.hh | 5 +- > 2 files changed, 324 insertions(+), 3 deletions(-) > > > 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 > @@ -1154,6 +1154,13 @@ immModel_protocol50Allowed(IMMND_CB *cb) > SA_TRUE : SA_FALSE; > } > > +SaBoolT > +immModel_protocol51Allowed(IMMND_CB *cb) > +{ > + return (ImmModel::instance(&cb->immModel)->protocol51Allowed()) ? > + SA_TRUE : SA_FALSE; > +} > + > OsafImmAccessControlModeT > immModel_accessControlMode(IMMND_CB *cb) > { > @@ -3271,6 +3278,21 @@ ImmModel::classCreate(const ImmsvOmClass > return SA_AIS_ERR_INVALID_PARAM; > } > > + ObjectMap::iterator oit = sObjectMap.find(immObjectDn); > + if(protocol51Allowed() && oit != sObjectMap.end() && !isLoading ){ > + std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES); > + ObjectInfo* immObject = oit->second; > + ImmAttrValueMap::iterator avi = > immObject->mAttrValueMap.find(immMaxClasses); > + osafassert(avi != immObject->mAttrValueMap.end()); > + osafassert(!(avi->second->isMultiValued())); > + ImmAttrValue* valuep = avi->second; > + unsigned int maxClasses = valuep->getValue_int(); > + if( sClassMap.size() >= maxClasses){ > + LOG_NO("ERR_NO_RESOURCES: maximum class limit %d has been reched > for the cluster", > + maxClasses); > + return SA_AIS_ERR_NO_RESOURCES; > + } > + } > > [Zoran] "std::string immMaxClasses(OPENSAF_IMMSV_MAX_CLASSES);" can be moved > to the top of the file with other definition of strings. > I would create a common method for finding max number of classes and use it > in the block above. The code is not wrong. I'll leave it to you if you want > to create a common method. > > ClassMap::iterator i = sClassMap.find(className); > if (i == sClassMap.end()) { > /* Class-name is unique case-sensitive. > @@ -3597,6 +3619,8 @@ ImmModel::classCreate(const ImmsvOmClass > } > } > > + > + > if(illegal) { > if(err == SA_AIS_OK) { > LOG_NO("ERR_INVALID_PARAM: Problem with new class '%s'", > className.c_str()); > @@ -3966,6 +3990,34 @@ ImmModel::protocol50Allowed() > return noStdFlags & OPENSAF_IMM_FLAG_PRT50_ALLOW; > } > > + > +bool > +ImmModel::protocol51Allowed() > +{ > + //TRACE_ENTER(); > + /* Assume that all nodes are running the same version when loading */ > + if (sImmNodeState == IMM_NODE_LOADING) { > + return true; > + } > + ObjectMap::iterator oi = sObjectMap.find(immObjectDn); > + if(oi == sObjectMap.end()) { > + TRACE_LEAVE(); > + return false; > + } > + > + ObjectInfo* immObject = oi->second; > + ImmAttrValueMap::iterator avi = > + immObject->mAttrValueMap.find(immAttrNostFlags); > + osafassert(avi != immObject->mAttrValueMap.end()); > + osafassert(!(avi->second->isMultiValued())); > + ImmAttrValue* valuep = avi->second; > + unsigned int noStdFlags = valuep->getValue_int(); > + > + //TRACE_LEAVE(); > + return noStdFlags & OPENSAF_IMM_FLAG_PRT51_ALLOW; > +} > + > + > bool > ImmModel::protocol41Allowed() > { > @@ -4114,6 +4166,44 @@ ImmModel::verifySchemaChange(const std:: > verifyFailed = notCompatibleAtt(className, newClassInfo, > attName, NULL, newAttr, NULL) || > verifyFailed; > newAttrs[inew->first] = newAttr; > + if(!verifyFailed && (className == immClassName)){ > + unsigned int val; > + if ( attName == OPENSAF_IMMSV_MAX_CLASSES) { > > [Zoran] Same as in earlier comment. Create a string for > OPENSAF_IMMSV_MAX_CLASSES in the top of the file with strings for other > attributes. > > + val = newAttr->mDefaultValue.getValue_int(); > + if( sClassMap.size() > val){ > + LOG_NO("The Number of classes in the cluster %lu > greater than the schema change" > + "value %d", sClassMap.size(), val); > + verifyFailed = true; > + } > + } > + > + if ( !verifyFailed && attName == > OPENSAF_IMMSV_MAX_IMPLEMENTERS) { > > [Zoran] Same here for OPENSAF_IMMSV_MAX_IMPLEMENTERS. > > + val = newAttr->mDefaultValue.getValue_int(); > + if( sImplementerVector.size() > val){ > + LOG_NO("The Number of Implementers in the cluster > %lu greater than the schema change" > + "value %d", sImplementerVector.size(), val); > + verifyFailed = true; > + } > + } > + > + if ( !verifyFailed && attName == > OPENSAF_IMMSV_MAX_ADMINOWNERS) { > > [Zoran] And here for OPENSAF_IMMSV_MAX_ADMINOWNERS > > + val = newAttr->mDefaultValue.getValue_int(); > + if( sOwnerVector.size() > val){ > + LOG_NO("The Number of AdminOwners in the cluster %lu > greater than the schema change" > + "value %d", sOwnerVector.size(), val); > + verifyFailed = true; > + } > + } > + > + if ( !verifyFailed && attName == OPENSAF_IMMSV_MAX_CCBS) { > > [Zoran] And here for OPENSAF_IMMSV_MAX_CCBS > > + val = newAttr->mDefaultValue.getValue_int(); > + if( sCcbVector.size() > val){ > + LOG_NO("The Number of Ccbs in the cluster %lu > greater than the schema change" > + "value %d", sCcbVector.size(), val); > + verifyFailed = true; > + } > + } > + } > > [Zoran] In the upper block, maybe it would be good to add a check that with a > schema change, new attribute values cannot be lower than IMMSV_MAX_* values. > > } else { > TRACE_5("Existing attribute %s", attName.c_str()); > verifyFailed = notCompatibleAtt(className, newClassInfo, > attName, iold->second, newAttr, > @@ -4799,6 +4889,8 @@ ImmModel::adminOwnerCreate(const ImmsvOm > unsigned int nodeId) > { > SaAisErrorT err = SA_AIS_OK; > + bool isLoading = (sImmNodeState == IMM_NODE_LOADING); > + > TRACE_ENTER(); > if(immNotWritable()) { > TRACE_LEAVE(); > @@ -4812,7 +4904,24 @@ ImmModel::adminOwnerCreate(const ImmsvOm > return SA_AIS_ERR_INVALID_PARAM; > } > } > - > + > + ObjectMap::iterator oi = sObjectMap.find(immObjectDn ); > + if(protocol51Allowed() && oi != sObjectMap.end() && !isLoading ){ > + std::string immMaxAdmOwn(OPENSAF_IMMSV_MAX_ADMINOWNERS); > > [Zoran] Same here for OPENSAF_IMMSV_MAX_ADMINOWNERS > > + ObjectInfo* immObject = oi->second; > + ImmAttrValueMap::iterator avi = > immObject->mAttrValueMap.find(immMaxAdmOwn); > + osafassert(avi != immObject->mAttrValueMap.end()); > + osafassert(!(avi->second->isMultiValued())); > + ImmAttrValue* valuep = avi->second; > + unsigned int maxAdmOwn= valuep->getValue_int(); > + if( sOwnerVector.size() >= maxAdmOwn ){ > + LOG_NO("ERR_NO_RESOURCES: maximum AdminOwners limit %d has been > reached for the cluster", > + maxAdmOwn); > + TRACE_LEAVE(); > + return SA_AIS_ERR_NO_RESOURCES; > + } > + } > + > AdminOwnerInfo* info = new AdminOwnerInfo; > > info->mId = ownerId; > @@ -4899,6 +5008,15 @@ ImmModel::adminOwnerDelete(SaUint32T own > immObject->mAttrValueMap.find(immAttrNostFlags); > osafassert(avi != immObject->mAttrValueMap.end()); > osafassert(!(avi->second->isMultiValued())); > + ImmAttrValueMap::iterator avi1 = > + > immObject->mAttrValueMap.find(OPENSAF_IMMSV_MAX_CLASSES); > + ImmAttrValueMap::iterator avi2 = > + > immObject->mAttrValueMap.find(OPENSAF_IMMSV_MAX_IMPLEMENTERS); > + ImmAttrValueMap::iterator avi3 = > + > immObject->mAttrValueMap.find(OPENSAF_IMMSV_MAX_ADMINOWNERS); > + ImmAttrValueMap::iterator avi4 = > + > immObject->mAttrValueMap.find(OPENSAF_IMMSV_MAX_CCBS); > > [Zoran] Same string issue for all 4 attribute names > > + > ImmAttrValue* valuep = (ImmAttrValue *) avi->second; > unsigned int noStdFlags = valuep->getValue_int(); > noStdFlags |= OPENSAF_IMM_FLAG_PRT41_ALLOW; > @@ -4907,6 +5025,16 @@ ImmModel::adminOwnerDelete(SaUint32T own > noStdFlags |= OPENSAF_IMM_FLAG_PRT46_ALLOW; > noStdFlags |= OPENSAF_IMM_FLAG_PRT47_ALLOW; > noStdFlags |= OPENSAF_IMM_FLAG_PRT50_ALLOW; > + if( (avi1 == immObject->mAttrValueMap.end()) || > + (avi2 == immObject->mAttrValueMap.end()) || > + (avi3 == immObject->mAttrValueMap.end()) || > + (avi4 == immObject->mAttrValueMap.end())){ > + LOG_NO("protcol51 is not set for > opensafImmNostdFlags because" > + "The new OpenSAF 5.1 attributes are > not added to OpensafImm class"); > + } else { > + > + noStdFlags |= OPENSAF_IMM_FLAG_PRT51_ALLOW; > + } > valuep->setValue_int(noStdFlags); > LOG_NO("%s changed to: 0x%x", immAttrNostFlags.c_str(), > noStdFlags); > /* END Temporary code. */ > @@ -5174,12 +5302,31 @@ ImmModel::ccbCreate(SaUint32T adminOwner > SaUint32T originatingConn) > { > SaAisErrorT err = SA_AIS_OK; > + bool isLoading = (sImmNodeState == IMM_NODE_LOADING); > + > TRACE_ENTER(); > > if(immNotWritable()) { > TRACE_LEAVE(); > return SA_AIS_ERR_TRY_AGAIN; > } > + > + ObjectMap::iterator oi = sObjectMap.find(immObjectDn); > + if(protocol51Allowed() && oi != sObjectMap.end() && !isLoading){ > + std::string immMaxCcbs(OPENSAF_IMMSV_MAX_CCBS); > > [Zoran] OPENSAF_IMMSV_MAX_CCBS > > + ObjectInfo* immObject = oi->second; > + ImmAttrValueMap::iterator avi = > immObject->mAttrValueMap.find(immMaxCcbs); > + osafassert(avi != immObject->mAttrValueMap.end()); > + osafassert(!(avi->second->isMultiValued())); > + ImmAttrValue* valuep = avi->second; > + unsigned int maxCcbs= valuep->getValue_int(); > + if (sCcbVector.size() >= maxCcbs){ > + LOG_NO("ERR_NO_RESOURCES: maximum Ccbs limit %d has been reached > for the cluster", > + maxCcbs); > + TRACE_LEAVE(); > + return SA_AIS_ERR_NO_RESOURCES; > + } > + } > > CcbInfo* info = new CcbInfo; > info->mId = ccbId; > @@ -7422,6 +7569,78 @@ ImmModel::getAllWritableAttributes(const > return result; > } > > +SaAisErrorT ImmModel::verifyImmLimits(ObjectInfo* object, > + std::string objectName){ > + > + SaAisErrorT err = SA_AIS_OK; > + SaUint32T val; > + > + TRACE_ENTER(); > + osafassert(objectName == immObjectDn); > + ImmAttrValueMap::iterator class_avmi = > object->mAttrValueMap.find(OPENSAF_IMMSV_MAX_CLASSES); > + if( class_avmi != object->mAttrValueMap.end()){ > + osafassert(class_avmi->second); > + val = (SaUint32T)class_avmi->second->getValue_int(); > + } else { > + val = IMMSV_MAX_CLASSES; > + } > + if(sClassMap.size() > val){ > + LOG_NO("ERR_NO_RESOURCES: maximum class limit %d has been reched for > the cluster", > + val); > + err = SA_AIS_ERR_NO_RESOURCES; > + TRACE_LEAVE(); > + return err; > + } > + > + ImmAttrValueMap::iterator impl_avmi = > object->mAttrValueMap.find(OPENSAF_IMMSV_MAX_IMPLEMENTERS); > + if( impl_avmi != object->mAttrValueMap.end()){ > + osafassert(impl_avmi->second); > + val = (SaUint32T)impl_avmi->second->getValue_int(); > + } else { > + val = IMMSV_MAX_IMPLEMENTERS; > + } > + if(sImplementerVector.size() > val){ > + LOG_NO("ERR_NO_RESOURCES: maximum implementers limit %d has been > reched for the cluster", > + val); > + err = SA_AIS_ERR_NO_RESOURCES; > + TRACE_LEAVE(); > + return err; > + } > + > + ImmAttrValueMap::iterator admi_avmi = > object->mAttrValueMap.find(OPENSAF_IMMSV_MAX_ADMINOWNERS); > + if( admi_avmi != object->mAttrValueMap.end()){ > + osafassert(admi_avmi->second); > + val = (SaUint32T)admi_avmi->second->getValue_int(); > + } else { > + val = IMMSV_MAX_ADMINOWNERS; > + } > + if(sOwnerVector.size() > val){ > + LOG_NO("ERR_NO_RESOURCES: maximum adminownerslimit %d has been > reched for the cluster", > + val); > + err = SA_AIS_ERR_NO_RESOURCES; > + TRACE_LEAVE(); > + return err; > + } > + > + ImmAttrValueMap::iterator ccb_avmi = > object->mAttrValueMap.find(OPENSAF_IMMSV_MAX_CCBS); > + if( ccb_avmi != object->mAttrValueMap.end()){ > + osafassert(ccb_avmi->second); > + val = (SaUint32T)ccb_avmi->second->getValue_int(); > + } else { > + val = IMMSV_MAX_CCBS; > + } > + if(sCcbVector.size() > val){ > + LOG_NO("ERR_NO_RESOURCES: maximum ccbs limit %d has been reched for > the cluster", > + val); > + err = SA_AIS_ERR_NO_RESOURCES; > + TRACE_LEAVE(); > + return err; > + } > > [Zoran] It would be good to create new methods for finding max numbers of new > attributes. All the attributes are static variables in the same file, so, i thought using them directly. > Again the same issue with new attribute names which can be defined in the top > of the file > > + > + TRACE_LEAVE(); > + return err; > +} > + > /** > * Creates an object > */ > @@ -8342,6 +8561,8 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im > */ > sIsLongDnLoaded = true; > } > + err = verifyImmLimits(object, immObjectDn); > + setCcbErrorString(ccb, IMM_RESOURCE_ABORT "Verifying of > immLimits failed"); > } else { > setCcbErrorString(ccb, > "IMM: ERR_BAD_OPERATION: Imm not allowing creates > of instances of class '%s'", > @@ -9380,6 +9601,83 @@ ImmModel::ccbObjectModify(const ImmsvOmC > } > } > > + /* From OpenSAF 5.1 immObjectDn has added with attributes > maxClasses, maxImplementers, > + maxAdminowners and maxCcbs */ > + > + > + ImmAttrValueMap::iterator class_avmi = > afim->mAttrValueMap.find(OPENSAF_IMMSV_MAX_CLASSES); > + ImmAttrValueMap::iterator impl_avmi = > afim->mAttrValueMap.find(OPENSAF_IMMSV_MAX_IMPLEMENTERS); > + ImmAttrValueMap::iterator admi_avmi = > afim->mAttrValueMap.find(OPENSAF_IMMSV_MAX_ADMINOWNERS); > + ImmAttrValueMap::iterator ccb_avmi = > afim->mAttrValueMap.find(OPENSAF_IMMSV_MAX_CCBS); > + > + if ( !protocol51Allowed() && class_avmi != > afim->mAttrValueMap.end()) { > + LOG_WA("ERR_NOT_EXIST: The attribute %s can not be modified. > The cluster is not yet upgraded" > + "to OpenSAF 5.1", OPENSAF_IMMSV_MAX_CLASSES); > + err = SA_AIS_ERR_NOT_EXIST; > + goto bypass_impl; > + } else if(class_avmi != afim->mAttrValueMap.end()) { > + osafassert(class_avmi->second); > + SaUint32T val = > (SaUint32T)class_avmi->second->getValue_int(); > + if(val < IMMSV_MAX_CLASSES){ > + LOG_WA("ERR_BAD_OPERATION: The value %d is less than the > minimum supported value %d for" > + "the attribute %s", val,IMMSV_MAX_CLASSES, > OPENSAF_IMMSV_MAX_CLASSES); > + err = SA_AIS_ERR_BAD_OPERATION; > + goto bypass_impl; > + } > + } > + > + if ( !protocol51Allowed() && impl_avmi != > afim->mAttrValueMap.end()) { > + LOG_WA("ERR_NOT_EXIST: The attribute %s can not be modified. > The cluster is not yet upgraded" > + "to OpenSAF 5.1", OPENSAF_IMMSV_MAX_IMPLEMENTERS); > + err = SA_AIS_ERR_NOT_EXIST; > + goto bypass_impl; > + > + } else if(class_avmi != afim->mAttrValueMap.end()) { > + osafassert(impl_avmi->second); > + SaUint32T val = (SaUint32T)impl_avmi->second->getValue_int(); > + if(val < IMMSV_MAX_IMPLEMENTERS){ > + LOG_WA("ERR_BAD_OPERATION: The value %d is less than the > minimum supported value %d for" > + "the attribute %s", val, IMMSV_MAX_IMPLEMENTERS, > OPENSAF_IMMSV_MAX_IMPLEMENTERS); > + err = SA_AIS_ERR_BAD_OPERATION; > + goto bypass_impl; > + } > + } > + > + if ( !protocol51Allowed() && admi_avmi != > afim->mAttrValueMap.end()) { > + LOG_WA("ERR_NOT_EXIST: The attribute %s can not be modified. > The cluster is not yet upgraded" > + "to OpenSAF 5.1", OPENSAF_IMMSV_MAX_ADMINOWNERS); > + err = SA_AIS_ERR_NOT_EXIST; > + goto bypass_impl; > + > + } else if(admi_avmi != afim->mAttrValueMap.end()) { > + osafassert(admi_avmi->second); > + SaUint32T val = (SaUint32T)admi_avmi->second->getValue_int(); > + if(val < IMMSV_MAX_ADMINOWNERS){ > + LOG_WA("ERR_BAD_OPERATION: The value %d is less than the > minimum supported value %d for" > + "the attribute %s", val, IMMSV_MAX_ADMINOWNERS, > OPENSAF_IMMSV_MAX_ADMINOWNERS); > + err = SA_AIS_ERR_BAD_OPERATION; > + goto bypass_impl; > + } > + } > + > + if ( !protocol51Allowed() && ccb_avmi != > afim->mAttrValueMap.end()) { > + LOG_WA("ERR_NOT_EXIST: The attribute %s can not be modified. > The cluster is not yet upgraded" > + "to OpenSAF 5.1", OPENSAF_IMMSV_MAX_CCBS); > + err = SA_AIS_ERR_NOT_EXIST; > + goto bypass_impl; > + > + } else if(ccb_avmi != afim->mAttrValueMap.end()) { > + osafassert(ccb_avmi->second); > + SaUint32T val = (SaUint32T)ccb_avmi->second->getValue_int(); > + if(val < IMMSV_MAX_CCBS){ > + LOG_WA("ERR_BAD_OPERATION: The value %d is less than the > minimum supported value %d for" > + "the attribute %s", val, IMMSV_MAX_CCBS, > OPENSAF_IMMSV_MAX_CCBS); > + err = SA_AIS_ERR_BAD_OPERATION; > + goto bypass_impl; > + } > + } > + > + > > [Zoran] Make an extra space when breaking LOG message in a next line. > Otherwise strings will be concatenated. will do while pushing > /* Pre validate any changes. More efficent here than in > apply/completed and > we need to guard against race on long DN creation allowed. > Such long DNs > are themselves created in CCBs or RTO creates. > @@ -13972,6 +14270,8 @@ ImmModel::implementerSet(const IMMSV_OCT > { > SaAisErrorT err = SA_AIS_OK; > CcbVector::iterator i; > + bool isLoading = (sImmNodeState == IMM_NODE_LOADING); > + > TRACE_ENTER(); > > *discardImplementer = SA_FALSE; > @@ -13997,7 +14297,25 @@ ImmModel::implementerSet(const IMMSV_OCT > return err; > //This return code not formally allowed here according to IMM > standard. > } > - > + > + ObjectMap::iterator oi = sObjectMap.find(immObjectDn); > + if(protocol51Allowed() && oi != sObjectMap.end() && !isLoading){ > + std::string immMaxImp(OPENSAF_IMMSV_MAX_IMPLEMENTERS); > > [Zoran] Same here with OPENSAF_IMMSV_MAX_IMPLEMENTERS > > Thanks, > Zoran > > + ObjectInfo* immObject = oi->second; > + ImmAttrValueMap::iterator avi = > immObject->mAttrValueMap.find(immMaxImp); > + osafassert(avi != immObject->mAttrValueMap.end()); > + osafassert(!(avi->second->isMultiValued())); > + ImmAttrValue* valuep = avi->second; > + unsigned int maxImp = valuep->getValue_int(); > + if( sImplementerVector.size() >= maxImp){ > + LOG_NO("ERR_NO_RESOURCES: maximum Implementers limit %d has been > reached for the cluster", > + maxImp); > + TRACE_LEAVE(); > + return SA_AIS_ERR_NO_RESOURCES; > + } > + } > + > + > bool isApplier = (implName.at(0) == '@'); > > ImplementerInfo* info = findImplementer(implName); > diff --git a/osaf/services/saf/immsv/immnd/ImmModel.hh > b/osaf/services/saf/immsv/immnd/ImmModel.hh > --- a/osaf/services/saf/immsv/immnd/ImmModel.hh > +++ b/osaf/services/saf/immsv/immnd/ImmModel.hh > @@ -116,6 +116,7 @@ public: > bool protocol46Allowed(); > bool protocol47Allowed(); > bool protocol50Allowed(); > + bool protocol51Allowed(); > bool oneSafe2PBEAllowed(); > bool purgeSyncRequest(SaUint32T clientId); > bool verifySchemaChange(const std::string& className, > @@ -661,7 +662,9 @@ public: > ObjectInfo* obj, > AdminOwnerInfo* adm, > bool doIt); > - > + SaAisErrorT verifyImmLimits( > + ObjectInfo* object, > + std::string objectName); > > private: > bool checkSubLevel( ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
