This is a tricky question. There is an enhancement ticket to "canonicalize" update callbacks. https://sourceforge.net/p/opensaf/tickets/801/
In lack of that (i.e. currently) it is assumed that the OI can interpret the modify in the same way as the immsv does. The OI is here assumed of course to have full knowledge of the class for the objects that it is the OI for. But we should perhaps be nice to the OI here by way of being restrictive to the OM client if this flag is set. That is we could just reject that form of modify if this flag is set for the time being. The best would be to fix #801 also for 4.7. But given the short time left until FC fro 4.7 I doubt we could manage it. Or ? /AndersBj On 09/23/2015 03:26 PM, Hung Nguyen wrote: > Hi Anders, > > I found a problem with this ticket. > What should IMM sent to OI when the value is replaced with default value. > > IMM should send the NULL value or the default value? > > In case of SA_IMM_ATTR_VALUES_DELETE, if IMM send the default value to > OI, the attrModType will have to be changes to > SA_IMM_ATTR_VALUES_REPLACE. > That might cause some confusion. > > Best Regards, > > Hung Nguyen - DEK Technologies > > > -------------------------------------------------------------------------------- > > > From: Anders Bjornerstedt [email protected] > Sent: Wednesday, September 23, 2015 8:01PM > To: Hung Nguyen, Zoran Milinkovic, Neelakanta Reddy > [email protected], [email protected], > [email protected] > Cc: Opensaf-devel > [email protected] > Subject: Re: [PATCH 2 of 4] imm: Support SA_IMM_ATTR_STRONG_DEFAULT > flag [#1425] > > > Hi Hung, > Good work. > I am ok with almost everything. > > Three comments below. > > > On 09/22/2015 07:05 PM, Hung Nguyen wrote: >> osaf/services/saf/immsv/immloadd/imm_loader.cc | 7 +- >> osaf/services/saf/immsv/immnd/ImmModel.cc | 81 >> +++++++++++++- >> osaf/services/saf/immsv/immnd/immnd_evt.c | 8 +- >> osaf/services/saf/immsv/schema/SAI-AIS-IMM-XSD-A.02.16.xsd | 1 + >> 4 files changed, 94 insertions(+), 3 deletions(-) >> >> >> This flag is only allowed to be set on attributes that have a default >> value. >> Attributes with this flag will never be NULL. >> >> Adding this flag to an existing attribute (schema change) will be >> allowed after checking for ccb interference and existing objects of >> the class. >> Do not allow to add a new attribute with this flag when doing schema >> change. >> >> When there's an attempt to set NULL value (or to delete all values >> with ATTR_VALUES_DELETE) to attributes with this flag, default value >> will be set. >> >> ImmAttrValue::operator= is used to assign value to ImmAttrValue. >> This will also work with ImmAttrMultiValue (head value will be >> assigned). >> >> diff --git a/osaf/services/saf/immsv/immloadd/imm_loader.cc >> b/osaf/services/saf/immsv/immloadd/imm_loader.cc >> --- a/osaf/services/saf/immsv/immloadd/imm_loader.cc >> +++ b/osaf/services/saf/immsv/immloadd/imm_loader.cc >> @@ -1720,7 +1720,8 @@ static bool loadXsd(const char *xsdFile) >> strcmp(value, "SA_INITIALIZED") && >> strcmp(value, "SA_PERSISTENT") && >> strcmp(value, "SA_CACHED") && strcmp(value, >> "SA_NOTIFY") && >> strcmp(value, "SA_NO_DUPLICATES") && >> strcmp(value, "SA_NO_DANGLING") && >> - strcmp(value, "SA_DN") && strcmp(value, >> "SA_DEFAULT_REMOVED")) { >> + strcmp(value, "SA_DN") && strcmp(value, >> "SA_DEFAULT_REMOVED") && >> + strcmp(value, "SA_STRONG_DEFAULT")) { >> attrFlagSet.insert(value); >> } >> } >> @@ -1797,6 +1798,10 @@ static SaImmAttrFlagsT charsToFlagsHelpe >> { >> return SA_IMM_ATTR_DEFAULT_REMOVED; >> } >> + else if (len == strlen("SA_STRONG_DEFAULT") && strncmp((const >> char*)str, "SA_STRONG_DEFAULT", len) == 0) >> + { >> + return SA_IMM_ATTR_STRONG_DEFAULT; >> + } >> std::string unflag((char *)str, len); >> if(!isXsdLoaded) { >> 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 >> @@ -3243,6 +3243,14 @@ ImmModel::classCreate(const ImmsvOmClass >> illegal = 1; >> } >> + if (attr->attrFlags & SA_IMM_ATTR_STRONG_DEFAULT) { >> + if (!attr->attrDefaultValue) { >> + LOG_NO("ERR_INVALID_PARAM: Attribute '%s' can not >> have SA_IMM_ATTR_STRONG_DEFAULT flag " >> + "without having a default value", attNm); >> + illegal = 1; >> + } >> + } >> + >> if(attr->attrDefaultValue) { >> if(attr->attrFlags & SA_IMM_ATTR_RDN) { >> LOG_NO("ERR_INVALID_PARAM: RDN '%s' can not have a >> default", attNm); >> @@ -3866,6 +3874,7 @@ ImmModel::notCompatibleAtt(const std::st >> bool checkCcb=false; >> bool checkNoDup=false; >> bool checkNoDanglingRefs=false; >> + bool checkStrongDefault=false; >> osafassert(changedAttrs); >> if(oldAttr->mValueType != newAttr->mValueType) { >> LOG_NO("Impossible upgrade, attribute %s:%s changes >> value type", >> @@ -4048,6 +4057,22 @@ ImmModel::notCompatibleAtt(const std::st >> change = true; >> } >> } >> + >> + if(!(oldAttr->mFlags & SA_IMM_ATTR_STRONG_DEFAULT) && >> + (newAttr->mFlags & SA_IMM_ATTR_STRONG_DEFAULT)) { >> + LOG_NO("Allowed upgrade, attribute %s:%s adds flag " >> + "SA_IMM_ATTR_STRONG_DEFAULT", className.c_str(), >> attName.c_str()); >> + checkCcb = true; >> + checkStrongDefault = true; >> + change = true; >> + } >> + >> + if((oldAttr->mFlags & SA_IMM_ATTR_STRONG_DEFAULT) && >> + !(newAttr->mFlags & SA_IMM_ATTR_STRONG_DEFAULT)) { >> + LOG_NO("Allowed upgrade, attribute %s:%s removes flag " >> + "SA_IMM_ATTR_STRONG_DEFAULT", className.c_str(), >> attName.c_str()); >> + change = true; >> + } >> } >> osafassert(!checkNoDup || checkCcb); //Duplicate-check >> implies ccb-check >> @@ -4117,6 +4142,25 @@ ImmModel::notCompatibleAtt(const std::st >> } >> } >> } >> + >> + if (checkStrongDefault) { >> + /* Screen all instances of the class. >> + * If there's an instance with the attribute being >> NULL, abort the schema change. */ >> + ObjectSet::iterator osi = >> oldClassInfo->mExtent.begin(); >> + for(;osi!=oldClassInfo->mExtent.end();++osi) { >> + obj = *osi; >> + ImmAttrValueMap::iterator oavi = >> obj->mAttrValueMap.find(attName); >> + osafassert(oavi!= obj->mAttrValueMap.end()); >> + if(oavi->second->empty()) { >> + std::string objName; >> + getObjectName(obj, objName); >> + LOG_NO("Impossible upgrade, attribute %s:%s >> adds SA_IMM_ATTR_STRONG_DEFAULT flag, " >> + "but that attribute of object '%s' has >> NULL value", >> + className.c_str(), attName.c_str(), >> objName.c_str()); >> + return true; >> + } >> + } >> + } >> } >> /* "changedAttrs != NULL" ensures that this check is only >> for the schema update */ >> @@ -4241,6 +4285,12 @@ ImmModel::notCompatibleAtt(const std::st >> "flag set", className.c_str(), attName.c_str()); >> return true; >> } >> + >> + if (newAttr->mFlags & SA_IMM_ATTR_STRONG_DEFAULT) { >> + LOG_NO("Impossible upgrade, new attribute %s:%s has >> SA_IMM_ATTR_STRONG_DEFAULT " >> + "flag set", className.c_str(), attName.c_str()); >> + return true; >> + } > A new attribute added in a schema change should be allowed to have > SA_IMM_ATTR_STRONG_DEFAULT > if and only if it has a default. If it does have a default then that > default would be assigned in migrateObj(..). > > You can compare with the if statement two steps above the if statement > I am commenting here, concerning the addition of a new cached runtime > attribute. > adding a new acached RTA is only allowed if it has a default define > and instance will get the new > attribute added with the default value set. > Note: I expect migrateObj possibly needs to have a little coded added > for this new case. > > The case of adding a new attribute differs from the case of changing > an existing one. > It is not allowed to add STRONG_DEFAULT to an existing attribute that > currently has the null value > even if it has a default (currently weak!). > >> } >> return false; >> @@ -4373,7 +4423,8 @@ ImmModel::attrCreate(ClassInfo* classInf >> SA_IMM_ATTR_NOTIFY | >> SA_IMM_ATTR_NO_DANGLING | >> SA_IMM_ATTR_DN | >> - SA_IMM_ATTR_DEFAULT_REMOVED); >> + SA_IMM_ATTR_DEFAULT_REMOVED | >> + SA_IMM_ATTR_STRONG_DEFAULT); >> if(unknownFlags) { >> /* This error means that at least one attribute flag is >> not supported by this >> @@ -8467,6 +8518,11 @@ ImmModel::ccbObjectModify(const ImmsvOmC >> err = SA_AIS_ERR_INVALID_PARAM; >> break; //out of switch >> } >> + >> + if (attr->mFlags & SA_IMM_ATTR_STRONG_DEFAULT) { >> + osafassert(!attr->mDefaultValue.empty()); >> + (*attrValue) = attr->mDefaultValue; >> + } >> continue; //Ok to replace with nothing. >> } >> //else intentional fall-through >> @@ -8633,6 +8689,11 @@ ImmModel::ccbObjectModify(const ImmsvOmC >> err = SA_AIS_ERR_INVALID_PARAM; >> break; //out of switch >> } >> + >> + if (attrValue->empty() && (attr->mFlags & >> SA_IMM_ATTR_STRONG_DEFAULT)) { >> + osafassert(!attr->mDefaultValue.empty()); >> + (*attrValue) = attr->mDefaultValue; >> + } >> } >> break; //out of switch >> @@ -15991,6 +16052,11 @@ ImmModel::rtObjectUpdate(const ImmsvOmCc >> attrValue->discardValues(); >> } >> if(p->attrValue.attrValuesNumber == 0) { >> + if (attr->mFlags & >> SA_IMM_ATTR_STRONG_DEFAULT) { >> + osafassert(!attr->mDefaultValue.empty()); >> + (*attrValue) = attr->mDefaultValue; >> + } >> + >> p = p->next; >> continue; //Ok to replace with nothing. >> } >> @@ -16114,6 +16180,11 @@ ImmModel::rtObjectUpdate(const ImmsvOmCc >> al = al->next; >> } >> } >> + >> + if (attrValue->empty() && (attr->mFlags & >> SA_IMM_ATTR_STRONG_DEFAULT)) { >> + osafassert(!attr->mDefaultValue.empty()); >> + (*attrValue) = attr->mDefaultValue; >> + } >> } >> break; //out of switch >> @@ -16866,6 +16937,7 @@ >> ImmModel::objectSync(const ImmsvOmObject >> } //while(p) >> //Check that all attributes with INITIALIZED flag >> have been set. >> + //Check that all attributes with STRONG_DEFAULT flag have >> been set. >> ImmAttrValueMap::iterator i6; >> for(i6=object->mAttrValueMap.begin(); >> i6!=object->mAttrValueMap.end() && err==SA_AIS_OK; >> @@ -16883,6 +16955,13 @@ ImmModel::objectSync(const ImmsvOmObject >> attrName.c_str()); >> err = SA_AIS_ERR_INVALID_PARAM; >> } >> + >> + if ((attr->mFlags & SA_IMM_ATTR_STRONG_DEFAULT) && >> attrValue->empty()) { >> + LOG_NO("ERR_INVALID_PARAM: attr '%s' has >> STRONG_DEFAULT flag " >> + "yet no value provided in the object create call", > The log message is a bit misleading since this is objectSync and not > objectCreate. > It follows the pattern of a sim,ilar message above it that I guess you > copy pasted. > That message should also be corrected. The ironic thing is also that > the original weak. > default value is only asigned if the value is missing in objectCreate. > So having > a message saying the value is missing in objectCreate is particularly > wrong. > Again, this is an old text that is missleading. Strange that I have > not noticed it before. > The text seems to be copied from ccbObjectCreate for the flag > SA_IMM_ATTR_INITIALIZED. > > Besides changing the wording, the severity should be altered from > NOtice to at least WArning > here in objectSync because clearly there is an inconsistency and sync > will surely fail. > > >> + attrName.c_str()); >> + err = SA_AIS_ERR_INVALID_PARAM; >> + } >> } >> if(err == SA_AIS_OK) { >> diff --git a/osaf/services/saf/immsv/immnd/immnd_evt.c >> b/osaf/services/saf/immsv/immnd/immnd_evt.c >> --- a/osaf/services/saf/immsv/immnd/immnd_evt.c >> +++ b/osaf/services/saf/immsv/immnd/immnd_evt.c >> @@ -3261,7 +3261,7 @@ static SaAisErrorT immnd_fevs_local_chec >> } >> if (!immModel_protocol47Allowed(cb)) { >> /* IMM supports creating classes with unknown flags. >> - * When the upgrade process is not completed, a >> class-create request (with DEFAULT_REMOVED flag) >> + * When the upgrade process is not completed, a >> class-create request (with a new flag) >> * may be accepted on nodes with old version and >> rejected on nodes with new version. >> * That will cause an inconsistency between nodes. */ > The comment is a bit misleading (same comment is misleading in the > added code for DEFAULT_REMOVED) > because it suggests that an inconsistency is possible somehow. But > this check is preventing the > described inconsistency from occurring. So at least the comment should > say that. > > /AndersBj >> IMMSV_ATTR_DEF_LIST* list = >> frwrd_evt.info.immnd.info.classDescr.attrDefinitions; >> @@ -3272,6 +3272,12 @@ static SaAisErrorT immnd_fevs_local_chec >> error = SA_AIS_ERR_TRY_AGAIN; >> break; /* while */ >> } >> + if (list->d.attrFlags & SA_IMM_ATTR_STRONG_DEFAULT) { >> + LOG_WA("ERR_TRY_AGAIN: Can not create class with >> SA_IMM_ATTR_STRONG_DEFAULT " >> + "when proto47 is not enabled"); >> + error = SA_AIS_ERR_TRY_AGAIN; >> + break; /* while */ >> + } >> list = list->next; >> } >> } >> diff --git >> a/osaf/services/saf/immsv/schema/SAI-AIS-IMM-XSD-A.02.16.xsd >> b/osaf/services/saf/immsv/schema/SAI-AIS-IMM-XSD-A.02.16.xsd >> --- a/osaf/services/saf/immsv/schema/SAI-AIS-IMM-XSD-A.02.16.xsd >> +++ b/osaf/services/saf/immsv/schema/SAI-AIS-IMM-XSD-A.02.16.xsd >> @@ -69,6 +69,7 @@ >> <xs:enumeration value="SA_NO_DANGLING"/> >> <xs:enumeration value="SA_DN"/> >> <xs:enumeration value="SA_DEFAULT_REMOVED"/> >> + <xs:enumeration value="SA_STRONG_DEFAULT"/> >> </xs:restriction> >> </xs:simpleType> >> <!-- > > ------------------------------------------------------------------------------ Monitor Your Dynamic Infrastructure at Any Scale With Datadog! Get real-time metrics from all of your servers, apps and tools in one place. SourceForge users - Click here to start your Free Trial of Datadog now! http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140 _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
