Hi Hung, The way I implemented the code in setCcbErrorString is to reduce the number of calls of strstr and not to have an impact on the performance. strstr is a costly function (comparing strings) and we don't need to call the function if it's not necessary.
Best regards, Zoran -----Original Message----- From: Hung Nguyen [mailto:hung.d.ngu...@dektech.com.au] Sent: Monday, October 26, 2015 1:01 PM To: Zoran Milinkovic; reddy.neelaka...@oracle.com Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [devel] [PATCH 1 of 1] imm: add a precedence of validation abort error string over resource abort error string [#1554] Hi Zoran, In ::setCcbErrorString, the if(isValidationErrString) {...} block can be replaced with this. // Precendence of validation abort over resource abort error string if (strstr(errorString, IMM_VALIDATION_ABORT) == errorString && strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { // Validation abort error string will replace resource abort error string free(errStr->name.buf); errStr->name.buf = fmtError; errStr->name.size = len; return; } else if (strstr(errorString, IMM_RESOURCE_ABORT) == errorString && (strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf || strstr(errStr->name.buf, IMM_VALIDATION_ABORT) == errStr->name.buf)) { // If validation abort or resource abort error string exist, // new resource abort string will not be added. // It can exists more validation abort error strings // or only the first resource abort error string return; } It looks more clear this way. We can also avoid declaring 'isValidationErrString' and 'if' block inside 'if' block. BR, Hung Nguyen - DEK Technologies -------------------------------------------------------------------------------- From: Zoran Milinkovic zoran.milinko...@ericsson.com Sent: Monday, October 26, 2015 6:47PM To: Neelakanta Reddy reddy.neelaka...@oracle.com Cc: Opensaf-devel opensaf-devel@lists.sourceforge.net Subject: [devel] [PATCH 1 of 1] imm: add a precedence of validation abort error string over resource abort error string [#1554] osaf/services/saf/immsv/immnd/ImmModel.cc | 103 +++++++++-------------------- 1 files changed, 31 insertions(+), 72 deletions(-) The patch add a precedence of validation error string over resource abort error string. If resource abort error string exists, it will be overwritten by validation abort error string. If at least one validation abort error string exist, new resource abort error string will not be adda. Since the new check of abort error strings is done in setCcbErrorString, the patch removes earlier code for ignoring several abort error strings. 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 @@ -5185,22 +5185,7 @@ ImmModel::ccbApply(SaUint32T ccbId, osafassert(reqConn==0 || (ccb->mOriginatingConn == reqConn)); if(!ccb->isOk()) { - /* At this point, CCB might already have error string with CCB abort reason. - * If none CCB abort reason can be found in error strings, - * then it's a resource abort. - */ - ImmsvAttrNameList *errStr = ccb->mErrorStrings; - while(errStr) { - if(strstr(errStr->name.buf, IMM_VALIDATION_ABORT) == errStr->name.buf - || strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { - break; - } - errStr = errStr->next; - } - if(!errStr) { - setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error state"); - } - + setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an + error state"); err = SA_AIS_ERR_FAILED_OPERATION; } else if((sImmNodeState == IMM_NODE_LOADING) && !sMissingParents.empty()) { MissingParentsMap::iterator mpm; @@ -6254,20 +6239,7 @@ ImmModel::ccbAugmentInit(immsv_oi_ccb_up if(ccb->mVeto != SA_AIS_OK) { TRACE("Ccb %u is already in an error state %u, can not accept augmentation", ccbId, ccb->mVeto); - - /* ccb->mVeto != SA_AIS_OK, error string with abort reason can already be set */ - ImmsvAttrNameList *errStr = ccb->mErrorStrings; - while(errStr) { - if(strstr(errStr->name.buf, IMM_VALIDATION_ABORT) == errStr->name.buf - || strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { - break; - } - errStr = errStr->next; - } - if(!errStr) { - setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error state"); - } - + setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error + state"); err = SA_AIS_ERR_FAILED_OPERATION; /*ccb->mVeto;*/ goto done; } @@ -7020,21 +6992,8 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im if(!ccb->isOk()) { LOG_NO("ERR_FAILED_OPERATION: ccb %u is in an error state " "rejecting ccbObjectCreate operation ", ccbId); - - /* !ccb->isOk(), error string with abort reason can already be set */ - ImmsvAttrNameList *errStr = ccb->mErrorStrings; - while(errStr) { - if(strstr(errStr->name.buf, IMM_VALIDATION_ABORT) == errStr->name.buf - || strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { - break; - } - errStr = errStr->next; - } - if(!errStr) { - setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error state"); - } - err = SA_AIS_ERR_FAILED_OPERATION; + setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error + state"); goto ccbObjectCreateExit; } @@ -8192,21 +8151,8 @@ ImmModel::ccbObjectModify(const ImmsvOmC if(!ccb->isOk()) { LOG_NO("ERR_FAILED_OPERATION: ccb %u is in an error state " "rejecting ccbObjectModify operation ", ccbId); - - /* !ccb->isOk(), error string with abort reason can already be set */ - ImmsvAttrNameList *errStr = ccb->mErrorStrings; - while(errStr) { - if(strstr(errStr->name.buf, IMM_VALIDATION_ABORT) == errStr->name.buf - || strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { - break; - } - errStr = errStr->next; - } - if(!errStr) { - setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error state"); - } - err = SA_AIS_ERR_FAILED_OPERATION; + setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error + state"); goto ccbObjectModifyExit; } @@ -9154,21 +9100,8 @@ ImmModel::ccbObjectDelete(const ImmsvOmC if(!ccb->isOk()) { LOG_NO("ERR_FAILED_OPERATION: ccb %u is in an error state " "rejecting ccbObjectDelete operation ", ccbId); - - /* !ccb->isOk(), error string with abort reason can already be set */ - ImmsvAttrNameList *errStr = ccb->mErrorStrings; - while(errStr) { - if(strstr(errStr->name.buf, IMM_VALIDATION_ABORT) == errStr->name.buf - || strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { - break; - } - errStr = errStr->next; - } - if(!errStr) { - setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error state"); - } - err = SA_AIS_ERR_FAILED_OPERATION; + setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an error + state"); goto ccbObjectDeleteExit; } @@ -9648,6 +9581,7 @@ ImmModel::setCcbErrorString(CcbInfo *ccb char *fmtError = (char *)malloc(errLen); int len; va_list args; + int isValidationErrString = 0; va_copy(args, vl); len = vsnprintf(fmtError, errLen, errorString, args); @@ -9660,6 +9594,12 @@ ImmModel::setCcbErrorString(CcbInfo *ccb osafassert(vsnprintf(fmtError, len, errorString, vl) >= 0); } + if(strstr(errorString, IMM_VALIDATION_ABORT) == errorString) { + isValidationErrString = 1; + } else if(strstr(errorString, IMM_RESOURCE_ABORT) == errorString) { + isValidationErrString = 2; + } + unsigned int ix=0; ImmsvAttrNameList* errStr = ccb->mErrorStrings; ImmsvAttrNameList** errStrTail = &(ccb->mErrorStrings); @@ -9670,6 +9610,25 @@ ImmModel::setCcbErrorString(CcbInfo *ccb free(fmtError); return; } + if(isValidationErrString) { + // Precendence of validation abort over resource abort error string + if(isValidationErrString == 1) { + // Validation abort error string will replace resource abort error string + if(strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { + free(errStr->name.buf); + errStr->name.buf = fmtError; + errStr->name.size = len; + return; + } + } else if(strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf + || strstr(errStr->name.buf, IMM_VALIDATION_ABORT) == errStr->name.buf) { + // If validation abort or resource abort error string exist, + // new resource abort string will not be added. + // It can exists more validation abort error strings + // or only the first resource abort error string + return; + } + } ++ix; errStrTail = &(errStr->next); errStr = errStr->next; ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
