So we need to decide quickly if we are to revert out #1425. This enhancement was actually not driven by any MR related to this release. The fix was a bonus enhancement for 4.7.
The possibilities in 4.7 are: 1) revert it out of 4.7 and repush it to the next release also then with #801. 2) Dont allow dangerous/advanced modify on attributes with STRONG_DEFAULT, added to 4.7 as part of #1425. 3) Add #801 to 4.7. /AndersBj On 09/23/2015 04:15 PM, Anders Björnerstedt wrote: > 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
