I will add changes from comments when I push the code.
Find my comment in the code.
-----Original Message-----
From: Anders Björnerstedt
Sent: den 2 september 2014 14:54
To: Zoran Milinkovic
Cc: [email protected]; [email protected]
Subject: Re: [devel] [PATCH 1 of 1] imm: fix loading long DNs from imm.xml and
PBE [#974]
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)
[Zoran] Actually, this will be extra check, and it's not needed. The check is
already done in "} else if (isLoading && !sIsLongDnLoaded) {"
Best regards,
Zoran
> + 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
