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. 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. /* 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( ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohodev2dev _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
