Ack from me.
This ticket was more cmplex to fix than I had anticipated.
Good work!
Minor comments below mostly on minor improvements on performance and
clarity.
Bur also one case where longDnsAllowed values greater than one are
interpreted as 'false' but
should be interpreted as 'true'.
That is safer and matches the definition in the README:
The default value is 0 with the intended meaning that long DNs are
not allowed.
Any other value has the meaning that long DNs are allowed. The
attribute is checked
as part of processing any IMM request that would imply the usage of
a long DN.
/AndersBj
Zoran Milinkovic wrote:
> osaf/services/saf/immsv/immnd/ImmModel.cc | 86
> ++++++++++++++++++++++--------
> 1 files changed, 63 insertions(+), 23 deletions(-)
>
>
> The patch fix loading SaNameT attributes with long DNs before
> opensafImm=opensafImm,safApp=safImmService is created.
>
> 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
> @@ -453,6 +453,7 @@ static const std::string saImmOiTimeout(
> static SaImmRepositoryInitModeT immInitMode = SA_IMM_INIT_FROM_FILE;
>
> static SaUint32T ccbIdLongDnGuard = 0; /* Disallow long DN creates if
> longDnsAllowed is being changed in ccb*/
> +static bool sIsLongDnLoaded = false; /* track long DNs before
> opensafImm=opensafImm,safApp=safImmService is created */
>
> struct AttrFlagIncludes
> {
> @@ -2633,7 +2634,7 @@ ImmModel::getLongDnsAllowed(ObjectInfo*
> oi = sObjectMap.find(immObjectDn);
> if(oi == sObjectMap.end()) {
> TRACE_LEAVE();
> - return false;
> + return sImmNodeState == IMM_NODE_LOADING;
> }
> immObject = oi->second;
> }
> @@ -6800,33 +6801,47 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im
> objectName.append((const
> char*)attrValues->n.attrValue.val.x.buf,
> strnlen((const char*)attrValues->n.attrValue.val.x.buf,
> (size_t)attrValues->n.attrValue.val.x.size));
> - } else if (attrValues->n.attrValueType == SA_IMM_ATTR_SANAMET
> - && !longDnsPermitted) {
> - AttrMap::iterator it = classInfo->mAttrMap.find(attrName);
> - if(it == classInfo->mAttrMap.end()) {
> - LOG_ER("ERR_INVALID_PARAM: Cannot find attribute '%s'",
> - attrName.c_str());
> - err = SA_AIS_ERR_INVALID_PARAM; //Should never happen!
> - goto ccbObjectCreateExit;
> - }
> - if(attrValues->n.attrValue.val.x.size >=
> SA_MAX_UNEXTENDED_NAME_LENGTH) {
> - LOG_NO("ERR_NAME_TOO_LONG: Attribute '%s' has long name. "
> - "Not allowed by IMM service or extended names are
> disabled",
> - attrName.c_str());
> - err = SA_AIS_ERR_NAME_TOO_LONG;
> - goto ccbObjectCreateExit;
> - }
> -
> - IMMSV_EDU_ATTR_VAL_LIST *value = attrValues->n.attrMoreValues;
> - while(value) {
> - if(value->n.val.x.size >= SA_MAX_UNEXTENDED_NAME_LENGTH) {
> - LOG_NO("ERR_NAME_TOO_LONG: Attribute '%s' has long DN. "
> + } else if (attrValues->n.attrValueType == SA_IMM_ATTR_SANAMET) {
> + if(!longDnsPermitted) {
> + AttrMap::iterator it = classInfo->mAttrMap.find(attrName);
> + if(it == classInfo->mAttrMap.end()) {
> + LOG_ER("ERR_INVALID_PARAM: Cannot find attribute '%s'",
> + attrName.c_str());
> + err = SA_AIS_ERR_INVALID_PARAM; //Should never
> happen!
> + goto ccbObjectCreateExit;
> + }
> + if(attrValues->n.attrValue.val.x.size >=
> SA_MAX_UNEXTENDED_NAME_LENGTH) {
> + LOG_NO("ERR_NAME_TOO_LONG: Attribute '%s' has long name.
> "
> "Not allowed by IMM service or extended names are
> disabled",
> attrName.c_str());
> err = SA_AIS_ERR_NAME_TOO_LONG;
> goto ccbObjectCreateExit;
> }
> - value = value->next;
> +
> + IMMSV_EDU_ATTR_VAL_LIST *value =
> attrValues->n.attrMoreValues;
> + while(value) {
> + if(value->n.val.x.size >= SA_MAX_UNEXTENDED_NAME_LENGTH)
> {
> + LOG_NO("ERR_NAME_TOO_LONG: Attribute '%s' has long
> DN. "
> + "Not allowed by IMM service or extended names
> are disabled",
> + attrName.c_str());
> + err = SA_AIS_ERR_NAME_TOO_LONG;
> + goto ccbObjectCreateExit;
> + }
> + value = value->next;
> + }
> + } else if (isLoading && !sIsLongDnLoaded) {
> + if(attrValues->n.attrValue.val.x.size >=
> SA_MAX_UNEXTENDED_NAME_LENGTH) {
> + sIsLongDnLoaded = true;
> + } else {
> + IMMSV_EDU_ATTR_VAL_LIST *value =
> attrValues->n.attrMoreValues;
> + while(value) {
>
Above line could be changed to: while(value && !sIsLongDnsLoaded)
> + if(value->n.val.x.size >=
> SA_MAX_UNEXTENDED_NAME_LENGTH) {
> + sIsLongDnLoaded = true;
> + break;
> + }
> + value = value->next;
> + }
> + }
> }
> }
>
> @@ -6868,6 +6883,11 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im
> goto ccbObjectCreateExit;
> }
>
> + if(isLoading && !sIsLongDnLoaded
> + && objectName.size() >= SA_MAX_UNEXTENDED_NAME_LENGTH) {
> + sIsLongDnLoaded = true;
> + }
> +
> if ((i5 = sObjectMap.find(objectName)) != sObjectMap.end()) {
> if(i5->second->mObjFlags & IMM_CREATE_LOCK) {
> if(isLoading) {
> @@ -7340,6 +7360,26 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im
> LOG_WA("Imm loading encountered bogus object '%s' of
> class '%s'",
> objectName.c_str(), immClassName.c_str());
> }
> + if(sIsLongDnLoaded) {
> + i6 = object->mAttrValueMap.find(immLongDnsAllowed);
> + if(i6 == object->mAttrValueMap.end() || !i6->second)
> {
> + LOG_ER("ERR_FAILED_OPERATION: Long DN is used
> during the loading initial data, "
> + "but longDnsAllowed is not specified");
>
Above line would be clearer as: "but attribute 'longDnsAllowed' is not
defined in class %s.", immClassName.c_str());
> + err = SA_AIS_ERR_FAILED_OPERATION;
> + } else if(i6->second->getValue_int() != 1) {
>
Above line should be coded as: } else if(i6->second->getValueInt() == 0) {
Otherwise higher values than 1 will be interpreted as longDndsAllowed is
false, which contradicts how it is defined in the immsv/README.
/AndersBj
> + LOG_ER("ERR_FAILED_OPERATION: Long DN is used
> during the loading initial data, "
> + "but longDnsAllowed is set to %d",
> + i6->second->getValue_int());
> + err = SA_AIS_ERR_FAILED_OPERATION;
> + }
> + } else {
> + /* 'else' branch is not needed. Only for small
> performance issue.
> + * sIsLongDnLoaded is set to true only to avoid
> extra checks.
> + * opensafImm=opensafImm,safApp=safImmService is
> created only once,
> + * and this code will not be executed again.
> + */
> + sIsLongDnLoaded = true;
> + }
> } else {
> setCcbErrorString(ccb,
> "ERR_BAD_OPERATION: Imm not allowing creates of
> instances of class '%s'",
>
> ------------------------------------------------------------------------------
> Slashdot TV.
> Video for Nerds. Stuff that matters.
> http://tv.slashdot.org/
> _______________________________________________
> Opensaf-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>
------------------------------------------------------------------------------
Slashdot TV.
Video for Nerds. Stuff that matters.
http://tv.slashdot.org/
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
