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
