This patch only affects the ccb-operation callbacks for ccb- create/delete/modify. It does not append anything to a user error string since the callback never reached the user. It adds an error string in the imm server where there previously could be no string, now clarifying what the cause of the ERR_NOT_EXIST was.
The only issue I have with the fix (that I realized after acking) is that for the create callback it is really the ERR_EXIST case that mainly needs clarification. The only reason for this patch is to increase comprehension of the cause by some human user. This string is not as such intended to be caught and processed by a program. It is backwards compatible with what exists. I think the point you are addressing is the need to support program processing of the error string to discriminate (process differently) based on the different causes of the same error code. There is a need for that also, particularly for handling the result of an aborted CCB. Here we are talking some "taging" either as prefix or postfix. We can and will add that also, at least to the cases where it is important. The cardinal example being SA_AIS_ERR_FAILED_OPERATION for a CCB (abort of a CCB). We may even add this also to ccb-create/delete/modify if there is need to programmetricaly process diferrently, e.g. The cases of an object not esisting from the implementer being detached. In summary, this patch solves a human factors problem and does not introiduce any new problem Or make it more difficult to solve the other problem, i.e. I dont see it as controversial. /AndersBj -----Original Message----- From: Hans Feldt [mailto:hans.fe...@ericsson.com] Sent: den 3 april 2014 12:33 To: Zoran Milinkovic; reddy.neelaka...@oracle.com Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [devel] [PATCH 1 of 2] IMM: add error strings for CCB operations for ERR_NOT_EXIST error [#459] I think this patch is a bit controversial. We need to define a "protocol" for the text carried in the error strings to allow for future changes. For example we should consider source identification, maybe we should have key/value pairs in the string? Remember that the string might end up in front of an operator... Thanks, Hans On 04/02/2014 03:36 PM, Zoran Milinkovic wrote: > osaf/services/saf/immsv/immnd/ImmModel.cc | 60 > +++++++++++++++++++++++++++++++ > osaf/services/saf/immsv/immnd/ImmModel.hh | 5 ++ > osaf/services/saf/immsv/immnd/immnd_evt.c | 27 ++++++++++++- > 3 files changed, 89 insertions(+), 3 deletions(-) > > > When an implementer is detached from an object/class, CCB operations return > ERR_NOT_EXIST error code. > The patch should give more information regarding the error code in the error > string. > > 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 > @@ -6950,6 +6950,10 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im > TRACE_7("ERR_NOT_EXIST: object '%s' does not have an " > "implementer and flag SA_IMM_CCB_REGISTERED_OI is > set", > objectName.c_str()); > + setCcbErrorString(ccb, > + "ERR_NOT_EXIST: object '%s' does not have an " > + "implementer and flag SA_IMM_CCB_REGISTERED_OI > is set", > + objectName.c_str()); > err = SA_AIS_ERR_NOT_EXIST; > } > } else { /* SA_IMM_CCB_REGISTERED_OI NOT set */ @@ > -7790,6 +7794,10 @@ ImmModel::ccbObjectModify(const ImmsvOmC > TRACE_7("ERR_NOT_EXIST: object '%s' does not have an " > "implementer and flag SA_IMM_CCB_REGISTERED_OI is set", > objectName.c_str()); > + setCcbErrorString(ccb, > + "ERR_NOT_EXIST: object '%s' does not have an " > + "implementer and flag SA_IMM_CCB_REGISTERED_OI is > set", > + objectName.c_str()); > err = SA_AIS_ERR_NOT_EXIST; > } > } else { /* SA_IMM_CCB_REGISTERED_OI NOT set */ @@ -8201,6 > +8209,10 @@ ImmModel::deleteObject(ObjectMap::iterat > TRACE_7("ERR_NOT_EXIST: object '%s' does not have an > implementer " > "and flag SA_IMM_CCB_REGISTERED_OI is set", > oi->first.c_str()); > + setCcbErrorString(ccb, > + "ERR_NOT_EXIST: object '%s' does not have an > implementer " > + "and flag SA_IMM_CCB_REGISTERED_OI is set", > + oi->first.c_str()); > return SA_AIS_ERR_NOT_EXIST; > } > } else { /* SA_IMM_CCB_REGISTERED_OI NOT set */ @@ -8370,6 > +8382,54 @@ ImmModel::deleteObject(ObjectMap::iterat > return SA_AIS_OK; > } > > +void > +ImmModel::setCcbErrorString(CcbInfo *ccb, const char *errorString, > +...) { > + int errLen = strlen(errorString) + 1; > + char *fmtError = (char *)malloc(errLen); > + int len; > + va_list vl; > + > + va_start(vl, errorString); > + len = vsnprintf(fmtError, errLen, errorString, vl); > + va_end(vl); > + > + osafassert(len >= 0); > + len++; /* Reserve one byte for null-terminated sign '\0' */ > + if(len > errLen) { > + fmtError = (char *)realloc(fmtError, len); > + va_start(vl, errorString); > + osafassert(vsnprintf(fmtError, len, errorString, vl) >= 0); > + va_end(vl); > + } > + > + unsigned int ix=0; > + ImmsvAttrNameList* errStr = ccb->mErrorStrings; > + ImmsvAttrNameList** errStrTail = &(ccb->mErrorStrings); > + while(errStr) { > + if(!strncmp(fmtError, errStr->name.buf, len)) { > + TRACE_5("Discarding duplicate error string '%s' for ccb id %u", > + fmtError, ccb->mId); > + free(fmtError); > + return; > + } > + ++ix; > + errStrTail = &(errStr->next); > + errStr = errStr->next; > + } > + > + if(ix >= IMMSV_MAX_ATTRIBUTES) { > + TRACE_5("Discarding error string '%s' for ccb id %u (too many)", > + fmtError, ccb->mId); > + free(fmtError); > + } else { > + (*errStrTail) = (ImmsvAttrNameList *) > malloc(sizeof(ImmsvAttrNameList)); > + (*errStrTail)->next = NULL; > + (*errStrTail)->name.size = len; > + (*errStrTail)->name.buf = fmtError; > + } > +} > + > bool > ImmModel::hasLocalClassAppliers(ClassInfo* classInfo) > { > diff --git a/osaf/services/saf/immsv/immnd/ImmModel.hh > b/osaf/services/saf/immsv/immnd/ImmModel.hh > --- a/osaf/services/saf/immsv/immnd/ImmModel.hh > +++ b/osaf/services/saf/immsv/immnd/ImmModel.hh > @@ -259,6 +259,11 @@ public: > IdVector& continuations, > unsigned int pbeIsLocal); > > + void setCcbErrorString( > + CcbInfo *ccb, > + const char *errorString, > + ...); > + > bool hasLocalClassAppliers(ClassInfo* classInfo); > bool hasLocalObjAppliers(const std::string& objName); > > 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 > @@ -5732,11 +5732,18 @@ static void immnd_evt_proc_object_create > memset(&send_evt, '\0', sizeof(IMMSV_EVT)); > send_evt.type = IMMSV_EVT_TYPE_IMMA; > send_evt.info.imma.info.errRsp.error = err; > - send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR; > + send_evt.info.imma.info.errRsp.errStrings = > +immModel_ccbGrabErrStrings(cb, evt->info.objCreate.ccbId); > + > + if(send_evt.info.imma.info.errRsp.errStrings) { > + send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR_2; > + } else { > + send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR; > + } > > if (immnd_mds_send_rsp(cb, &(cl_node->tmpSinfo), &send_evt) != > NCSCC_RC_SUCCESS) { > LOG_WA("Failed to send result to Agent over MDS"); > } > + > +immsv_evt_free_attrNames(send_evt.info.imma.info.errRsp.errStrings); > } > TRACE_LEAVE(); > } > @@ -5957,11 +5964,18 @@ static void immnd_evt_proc_object_modify > memset(&send_evt, '\0', sizeof(IMMSV_EVT)); > send_evt.type = IMMSV_EVT_TYPE_IMMA; > send_evt.info.imma.info.errRsp.error = err; > - send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR; > + send_evt.info.imma.info.errRsp.errStrings = > +immModel_ccbGrabErrStrings(cb, evt->info.objModify.ccbId); > + > + if(send_evt.info.imma.info.errRsp.errStrings) { > + send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR_2; > + } else { > + send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR; > + } > > if (immnd_mds_send_rsp(cb, &(cl_node->tmpSinfo), &send_evt) != > NCSCC_RC_SUCCESS) { > LOG_WA("Failed to send result to Agent over MDS"); > } > + > +immsv_evt_free_attrNames(send_evt.info.imma.info.errRsp.errStrings); > } > > done: > @@ -6715,11 +6729,18 @@ static void immnd_evt_proc_object_delete > memset(&send_evt, '\0', sizeof(IMMSV_EVT)); > send_evt.type = IMMSV_EVT_TYPE_IMMA; > send_evt.info.imma.info.errRsp.error = err; > - send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR; > + send_evt.info.imma.info.errRsp.errStrings = > +immModel_ccbGrabErrStrings(cb, evt->info.objDelete.ccbId); > + > + if(send_evt.info.imma.info.errRsp.errStrings) { > + send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR_2; > + } else { > + send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR; > + } > > if (immnd_mds_send_rsp(cb, &(cl_node->tmpSinfo), &send_evt) != > NCSCC_RC_SUCCESS) { > LOG_WA("Failed to send result to Agent over MDS"); > } > + > +immsv_evt_free_attrNames(send_evt.info.imma.info.errRsp.errStrings); > } > } > > > ---------------------------------------------------------------------- > -------- _______________________________________________ > 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 ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
