Hi zoran, comments inline.
On Monday 11 January 2016 05:49 PM, Zoran Milinkovic wrote: > Hi Neelakanta, > > Ack from me. > Reviewed and tested the patch with minor inline comments. > No need to send the patch for another review (for inline comments). > > It would be good if error string is attached to aborted CCB (resource abort). Setting of resource abort error string is done ImmModel::ccbAbort. > Error string can go in another ticket, or can be fixed in this ticket. I'm > leaving for you to decide. Add, the code for error string while pushing. > > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Friday, January 08, 2016 5:56 AM > To: Hung Duc Nguyen; Zoran Milinkovic > Cc: [email protected] > Subject: [PATCH 1 of 1] imm : Aborted reply for augumentated Ccb should be > sent to clients [#1503] > > osaf/services/saf/immsv/immnd/ImmModel.cc | 46 ++++++++++--- > osaf/services/saf/immsv/immnd/ImmModel.hh | 2 +- > osaf/services/saf/immsv/immnd/immnd_evt.c | 97 > ++++++++++++++++------------- > osaf/services/saf/immsv/immnd/immnd_init.h | 4 +- > 4 files changed, 90 insertions(+), 59 deletions(-) > > > When the augumented CCB or the OI for the augumented CCB crahes, then reply > has to be sent to the all clients. > > The reply has to be sent to OM client when augumented OI crashes. > The reply has to be sent to both OM and augumneted OI, when OI, for which > augumented CCb is waiting crashes. > > 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 > @@ -1175,15 +1175,16 @@ immModel_ccbAbort(IMMND_CB *cb, > SaUint32T ccbId, > SaUint32T* arrSize, > SaUint32T** implConnArr, > - SaUint32T* client, > + SaUint32T** clientArr, > + SaUint32T* clArrSize, > SaUint32T* nodeId, > SaClmNodeIdT* pbeNodeId) > { > - ConnVector cv; > - ConnVector::iterator cvi; > + ConnVector cv, cl; > + ConnVector::iterator cvi, cli; > unsigned int ix=0; > > - bool aborted = ImmModel::instance(&cb->immModel)->ccbAbort(ccbId, cv, > client, nodeId, pbeNodeId); > + bool aborted = ImmModel::instance(&cb->immModel)->ccbAbort(ccbId, cv, > cl, nodeId, pbeNodeId); > > *arrSize = (SaUint32T) cv.size(); > if(*arrSize) { > @@ -1194,6 +1195,16 @@ immModel_ccbAbort(IMMND_CB *cb, > } > } > osafassert(ix==(*arrSize)); > + > + ix=0; > + *clArrSize = (SaUint32T) cl.size(); > + if(*clArrSize){ > + *clientArr = (SaUint32T *) malloc((*clArrSize)* sizeof(SaUint32T)); > + for(cli = cl.begin(); cli!=cl.end(); ++cli, ++ix){ > + (*clientArr)[ix] = (*cli); > + } > + } > + osafassert(ix==(*clArrSize)); > return aborted; > } > > @@ -5838,7 +5849,7 @@ ImmModel::ccbCommit(SaUint32T ccbId, Con > } > > bool > -ImmModel::ccbAbort(SaUint32T ccbId, ConnVector& connVector, SaUint32T* > client, > +ImmModel::ccbAbort(SaUint32T ccbId, ConnVector& connVector, ConnVector& > clVector, > SaUint32T* nodeId, unsigned int* pbeNodeIdPtr) > { > SaUint32T pbeConn=0; > @@ -5861,30 +5872,25 @@ ImmModel::ccbAbort(SaUint32T ccbId, Conn > case IMM_CCB_CREATE_OP: > LOG_NO("Aborting ccb %u while waiting for " > "reply from implementer on CREATE-OP", ccbId); > - *client = ccb->mOriginatingConn; > break; > > case IMM_CCB_MODIFY_OP: > LOG_NO("Aborting ccb %u while waiting for " > "reply from implementers on MODIFY-OP", ccbId); > - *client = ccb->mOriginatingConn; > break; > > case IMM_CCB_DELETE_OP: > LOG_NO("Aborting ccb %u while waiting for " > "replies from implementers on DELETE-OP", ccbId); > - *client = ccb->mOriginatingConn; > break; > > case IMM_CCB_PREPARE: > case IMM_CCB_VALIDATING: > LOG_NO("Ccb %u aborted in COMPLETED processing (validation)", > ccbId); > - *client = ccb->mOriginatingConn; > break; > > case IMM_CCB_VALIDATED: > LOG_NO("Ccb %u aborted after explicit validation", ccbId); > - *client = ccb->mOriginatingConn; > break; > > case IMM_CCB_CRITICAL: > @@ -5894,7 +5900,6 @@ ImmModel::ccbAbort(SaUint32T ccbId, Conn > > case IMM_CCB_PBE_ABORT: > TRACE_5("Aborting ccb %u because PBE decided ABORT", ccbId); > - *client = ccb->mOriginatingConn; > break; > > case IMM_CCB_COMMITTED: > @@ -5926,8 +5931,25 @@ ImmModel::ccbAbort(SaUint32T ccbId, Conn > } > } > > + > + clVector.push_back(ccb->mOriginatingConn); > *nodeId = ccb->mOriginatingNode; > - *client = ccb->mOriginatingConn; > + > + if(ccb->mAugCcbParent && ccb->mAugCcbParent->mOriginatingConn) { > + if(*nodeId && ccb->mOriginatingConn) { > > [Zoran] 4 lines above: "*nodeId = ccb->mOriginatingNode;" > IF statement can be changed to something like: > if(*nodeId) > or > if(ccb->mOriginatingConn) Add if(ccb->mOriginatingConn) > > + /* Case where augumented client and Augumented parent > + are originated from ths node. Send the client response > + to both clients. */ > + osafassert(*nodeId == ccb->mAugCcbParent->mOriginatingNode); > + } else if (!ccb->mOriginatingConn) { > > [Zoran] The first IF statement check if ccb->mOriginatingConn is not NULL, > then this IF statement can be removed and there can only be ELSE remove else if and keep only else. > + /* Case where augumented client is not orignated from this node > + and Augumented parent is originated from this node. > + Send the response to the Augumented parent. */ > + *nodeId = ccb->mAugCcbParent->mOriginatingNode; > + clVector.pop_back(); > + } > + clVector.push_back(ccb->mAugCcbParent->mOriginatingConn); > + } > > ccb->mState = IMM_CCB_ABORTED; > if(ccb->mVeto == SA_AIS_OK) { > 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 > @@ -553,7 +553,7 @@ public: > bool ccbAbort( > SaUint32T ccbId, > ConnVector& connVector, > - SaUint32T* client, > + ConnVector& clientVector, > SaUint32T* nodeId, > unsigned int* pbeNodeIdPtr); > SaUint32T cleanTheBasement( > 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 > @@ -221,7 +221,8 @@ static uint32_t immnd_evt_proc_mds_evt(I > > /*static uint32_t immnd_evt_immd_new_active(IMMND_CB *cb);*/ > > -static void immnd_evt_ccb_abort(IMMND_CB *cb, SaUint32T ccbId, SaUint32T > *client, SaUint32T *nodeId); > +static void immnd_evt_ccb_abort(IMMND_CB *cb, SaUint32T ccbId, SaUint32T > **clientArr, > + SaUint32T * clArrSize, SaUint32T *nodeId); > > static uint32_t immnd_evt_proc_reset(IMMND_CB *cb, IMMND_EVT *evt, > IMMSV_SEND_INFO *sinfo); > > @@ -4096,7 +4097,7 @@ static void immnd_evt_proc_ccb_compl_rsp > TRACE("Abort in immnd_evt_proc_ccb_compl_rsp reqConn: > %u", reqConn); > /*err != SA_AIS_OK => generate SaImmOiCcbAbortCallbackT > upcall > for all local implementers involved in the > Ccb */ > - immnd_evt_ccb_abort(cb, evt->info.ccbUpcallRsp.ccbId, > NULL, NULL); > + immnd_evt_ccb_abort(cb, evt->info.ccbUpcallRsp.ccbId, > NULL, NULL, NULL); > errStrings = immModel_ccbGrabErrStrings(cb, > evt->info.ccbUpcallRsp.ccbId); > } > /* Either commit or abort has been decided. Ccb is now done. > @@ -6726,12 +6727,13 @@ static void immnd_evt_proc_rt_object_mod > TRACE_LEAVE(); > } > > -static void immnd_evt_ccb_abort(IMMND_CB *cb, SaUint32T ccbId, SaUint32T > *client, SaUint32T *nodeId) > +static void immnd_evt_ccb_abort(IMMND_CB *cb, SaUint32T ccbId, SaUint32T > **clientArr, SaUint32T *clArrsize, SaUint32T *nodeId) > { > IMMSV_EVT send_evt; > SaUint32T *implConnArr = NULL; > - SaUint32T arrSize = 0; > - SaUint32T dummyClient = 0, dummynodeId = 0; > + SaUint32T arrSize = 0, dummyClsize = 0; > + SaUint32T dummynodeId = 0; > + SaUint32T *dummyClient = NULL; > SaImmOiHandleT implHandle = 0LL; > NCS_NODE_ID pbeNodeId = 0; > NCS_NODE_ID *pbeNodeIdPtr = NULL; > @@ -6747,14 +6749,15 @@ static void immnd_evt_ccb_abort(IMMND_CB > */ > } > > - if(!immModel_ccbAbort(cb, ccbId, &arrSize, &implConnArr, &dummyClient, > &dummynodeId, pbeNodeIdPtr)) { > + if(!immModel_ccbAbort(cb, ccbId, &arrSize, &implConnArr, &dummyClient, > &dummyClsize, &dummynodeId, pbeNodeIdPtr)) { > goto done; > } > > - if (client) { > - *client = dummyClient; > + if (clientArr) { > + *clientArr = dummyClient; > + *clArrsize = dummyClsize; > } else { > - dummyClient = 0; /* dont reply to client here*/ > + dummyClient = NULL; /* dont reply to client here*/ > > [Zoran] Memory leak. Allocated memory for dummyClient must be freed. will free the memory > } > > if (nodeId && cb->node_id == dummynodeId) { > @@ -7494,11 +7497,12 @@ static void immnd_evt_proc_ccb_finalize( > SaAisErrorT err = SA_AIS_OK; > IMMSV_EVT send_evt; > IMMND_IMM_CLIENT_NODE *cl_node = NULL; > - SaUint32T client = 0, nodeId = 0; > + SaUint32T clArrSize = 0, nodeId = 0, ix = 0; > + SaUint32T *clientArr = NULL; > TRACE_ENTER(); > > osafassert(evt); > - immnd_evt_ccb_abort(cb, evt->info.ccbId, &client, &nodeId); > + immnd_evt_ccb_abort(cb, evt->info.ccbId, &clientArr, &clArrSize, > &nodeId); > > [Zoran] Memory leak. Allocated memory for clientArr must be freed > will free the memory. /Neel. > if (nodeId && err == SA_AIS_OK && !originatedAtThisNd) { > /* nodeId will be set when CCB is originated from this node.The > CCB is aborted, > @@ -7506,41 +7510,46 @@ static void immnd_evt_proc_ccb_finalize( > */ > > originatedAtThisNd = SA_TRUE; > - clnt_hdl = m_IMMSV_PACK_HANDLE(client, nodeId); > err = SA_AIS_ERR_FAILED_OPERATION; > } else if(err == SA_AIS_OK) { > TRACE_2("ccb aborted and finalized"); > } > > if (originatedAtThisNd) { /*Send reply to client from this ND. */ > - TRACE_2("ccbFinalize originated at this node => Send reply"); > - /* Perhaps the following osafassert is a bit strong. A corrupt > client/agent > - could "accidentally" use the wrong ccbId if its heap was > thrashed. > - This wrong ccbid could accidentally be an existing used > ccbId. > - But in this case it would also have to originate at this > node. > - */ > - TRACE_2("client == m_IMMSV_UNPACK_HANDLE_HIGH(clnt_hdl))" > - "??: %u == %u", client, > (SaUint32T)m_IMMSV_UNPACK_HANDLE_HIGH(clnt_hdl)); > - osafassert(!client || client == > m_IMMSV_UNPACK_HANDLE_HIGH(clnt_hdl)); > - immnd_client_node_get(cb, clnt_hdl, &cl_node); > - if (cl_node == NULL || cl_node->mIsStale) { > - LOG_WA("IMMND - Client went down so no response"); > - goto done; > - } > - > - memset(&send_evt, '\0', sizeof(IMMSV_EVT)); > - > - send_evt.type = IMMSV_EVT_TYPE_IMMA; > - send_evt.info.imma.type = IMMA_EVT_ND2A_IMM_ERROR; > - send_evt.info.imma.info.errRsp.error = err; > - > - TRACE_2("SENDRSP %u", err); > - > - if (immnd_mds_send_rsp(cb, &(cl_node->tmpSinfo), &send_evt) != > NCSCC_RC_SUCCESS) { > - LOG_WA("Failed to send response to agent/client over > MDS"); > - } > - } > - done: > + do { > + TRACE_2("ccbFinalize originated at this node => Send > reply"); > + /* Perhaps the following osafassert is a bit strong. A > corrupt client/agent > + could "accidentally" use the wrong ccbId if its heap > was thrashed. > + This wrong ccbid could accidentally be an existing > used ccbId. > + But in this case it would also have to originate at > this node. > + */ > + if(clArrSize) { > + clnt_hdl = m_IMMSV_PACK_HANDLE(clientArr[ix], > nodeId); > + TRACE_2("client == > m_IMMSV_UNPACK_HANDLE_HIGH(clnt_hdl))" > + "??: %u == %u", clientArr[ix], > (SaUint32T)m_IMMSV_UNPACK_HANDLE_HIGH(clnt_hdl)); > + osafassert(!clArrSize || clientArr[ix] == > m_IMMSV_UNPACK_HANDLE_HIGH(clnt_hdl)); > + } > + immnd_client_node_get(cb, clnt_hdl, &cl_node); > + if (cl_node == NULL || cl_node->mIsStale) { > + LOG_WA("IMMND - Client went down so no > response"); > + } else { > + > + memset(&send_evt, '\0', sizeof(IMMSV_EVT)); > + > + send_evt.type = IMMSV_EVT_TYPE_IMMA; > + send_evt.info.imma.type = > IMMA_EVT_ND2A_IMM_ERROR; > + send_evt.info.imma.info.errRsp.error = err; > + > + TRACE_2("SENDRSP %u", err); > + > + if (immnd_mds_send_rsp(cb, > &(cl_node->tmpSinfo), &send_evt) != NCSCC_RC_SUCCESS) { > + LOG_WA("Failed to send response to > agent/client over MDS"); > + } > + } > + ix++; > + } while(clArrSize && (ix < clArrSize)); > + } > + > err = immModel_ccbFinalize(cb, evt->info.ccbId); > TRACE_LEAVE(); > } > @@ -7866,7 +7875,7 @@ static void immnd_evt_proc_ccb_apply(IMM > applConnArr = NULL; > } > } else { > - SaUint32T client = 0; > + SaUint32T clArrSize = 0, *clArr = NULL; > if(err == SA_AIS_ERR_ACCESS_DENIED) { > LOG_WA("Spurious and redundant ccb-apply > request ignored ccbId:%u", > evt->info.ccbId); > @@ -7875,8 +7884,8 @@ static void immnd_evt_proc_ccb_apply(IMM > } > /*err != SA_AIS_OK => generate SaImmOiCcbAbortCallbackT > upcalls > */ > - immnd_evt_ccb_abort(cb, evt->info.ccbId, &client, NULL); > - osafassert(!client || originatedAtThisNd); > + immnd_evt_ccb_abort(cb, evt->info.ccbId, &clArr, > &clArrSize, NULL); > > [Zoran] Memory leak. Allocated memory for clArr must be freed. > > Best regards,, > Zoran > > + osafassert(!clArrSize || originatedAtThisNd); > } > TRACE_2("CCB APPLY TERMINATING CCB: %u", evt->info.ccbId); > bCcbFinalize = 1; > @@ -9021,7 +9030,7 @@ static void immnd_evt_proc_discard_node( > SaUint32T ix; > for (ix = 0; ix < arrSize; ++ix) { > LOG_WA("Detected crash at node %x, abort ccbId %u", > evt->info.ctrl.nodeId, idArr[ix]); > - immnd_evt_ccb_abort(cb, idArr[ix], NULL, NULL); > + immnd_evt_ccb_abort(cb, idArr[ix], NULL, NULL, NULL); > err = immModel_ccbFinalize(cb, idArr[ix]); > if (err != SA_AIS_OK) { > LOG_WA("Failed to remove Ccb %u - ignoring", > idArr[ix]); > diff --git a/osaf/services/saf/immsv/immnd/immnd_init.h > b/osaf/services/saf/immsv/immnd/immnd_init.h > --- a/osaf/services/saf/immsv/immnd/immnd_init.h > +++ b/osaf/services/saf/immsv/immnd/immnd_init.h > @@ -145,8 +145,8 @@ extern "C" { > SaUint32T **implIdArr, SaUint32T **ctnArr, bool > validateOnly); > > bool immModel_ccbAbort(IMMND_CB *cb, > - SaUint32T ccbId, SaUint32T *arrSize, SaUint32T **implConnArr, > SaUint32T *client, > - SaUint32T* nodeId, SaClmNodeIdT *pbeNodeId); > + SaUint32T ccbId, SaUint32T *arrSize, SaUint32T **implConnArr, > SaUint32T **clientArr, > + SaUint32T *clArrSize, SaUint32T* nodeId, SaClmNodeIdT > *pbeNodeId); > > void immModel_getCcbIdsForOrigCon(IMMND_CB *cb, SaUint32T origConn, > SaUint32T *arrSize, SaUint32T **ccbIdArr); > ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140 _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
