Menu
▾
▴
Re: [devel] [PATCH 2 of 4] imm: Support SA_IMM_ATTR_STRONG_DEFAULT flag [#1425]
|
From: Hung N. <hun...@de...> - 2015-09-23 15:01:47
|
Hi Anders,
I'd go for 1)
This is just a bonus enhancement anyway.
Best Regards,
Hung Nguyen - DEK Technologies
--------------------------------------------------------------------------------
From: Anders Bjornerstedt and...@er...
Sent: Wednesday, September 23, 2015 9:58PM
To: Hung Nguyen, Zoran Milinkovic, Neelakanta Reddy
hun...@de..., zor...@er..., red...@or...
Cc: Opensaf-devel
ope...@li...
Subject: Re: [PATCH 2 of 4] imm: Support SA_IMM_ATTR_STRONG_DEFAULT flag [#1425]
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 and...@er...
>> Sent: Wednesday, September 23, 2015 8:01PM
>> To: Hung Nguyen, Zoran Milinkovic, Neelakanta Reddy
>> hun...@de..., zor...@er...,
>> red...@or...
>> Cc: Opensaf-devel
>> ope...@li...
>> 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>
>>> <!--
>>
>>
>
|
