Hi Hung,
comments inline.
On Friday 13 May 2016 01:25 PM, Hung Nguyen wrote:
> Hi Neel,
>
> This only works on the originating node.
> On the remote nodes where mOriginatingConn and reqConn are always 0, it will
> pass the check.
> That will cause an inconsistency between IMMNDs.
Even the check is passed in remote node, The ERR_FAILED_OPERATION will
be the error at remote nodes.
The following code will check the ccb:
if(!ccb->isOk()) {
setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an
error state");
err = SA_AIS_ERR_FAILED_OPERATION;
}
> There's one more problem that I observed if we don't abort the ccb right away.
> immModel_getCcbIdsForOrigCon() uses mOriginatingConn to get all CCB from a
> connection.
> After purging the ccb, it will fail to get the ccb associated with the
> connection.
> So if client doesn't explicitly ImmOmCcbFinalize() (using ImmOmFinalize()
> instead) we will have resource leak.
> That ccb will never be finalized.
>
> We can keep the CCB and wait for OI until IMMA_OI_CALLBACK_TIMEOUT expires
> but that will be a complex implementation.
> I think that should be an enhancement ticket.
New enhancement is not required, the present code covers the scenario.
If the CCB is not finalized, then the OI timeout will abort the ccb and
call immModel_ccbFinalize.
/Neel.
> BR,
>
> Hung Nguyen - DEK Technologies
>
> --------------------------------------------------------------------------------
> From: Neelakanta [email protected]
> Sent: Friday, May 13, 2016 2:23PM
> To: Hung Nguyen, Zoran Milinkovic
> [email protected],[email protected]
> Cc: Opensaf-devel
> [email protected]
> Subject: Re: [PATCH 1 of 1] imm: Abort ccb when timeout on client while
> waiting for implementer [#1817]
>
>
> Hi Hung,
>
> Since the purgesyncrequest has cleared moriginationgconn, when the
> ccbApply is called ERR_BAD_HANDLE has to be returned.
> No, need to abort again as the CCB will be aborted after OI_TIEMOUT.
>
> The following error logging will solve the problem:
>
> --- a/osaf/services/saf/immsv/immnd/ImmModel.cc
> +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc
> @@ -5445,6 +5445,9 @@ ImmModel::ccbApply(SaUint32T ccbId,
> if (i == sCcbVector.end()) {
> LOG_NO("ERR_BAD_HANDLE: CCB %u does not exist", ccbId);
> err = SA_AIS_ERR_BAD_HANDLE;
> + } else if(reqConn && (ccb->mOriginatingConn != reqConn)) {
> + LOG_NO("ERR_BAD_HANDLE: Missmatch on connection for ccb id
> %u", ccbId);
> + err = SA_AIS_ERR_BAD_HANDLE;
> } else {
> CcbInfo* ccb = (*i);
> i2 = std::find_if(sOwnerVector.begin(), sOwnerVector.end(),
>
> /Neel.
>
> On Friday 13 May 2016 10:03 AM, Hung Nguyen wrote:
>> osaf/services/saf/immsv/immnd/ImmModel.cc | 18 +++++++++++++-----
>> osaf/services/saf/immsv/immnd/ImmModel.hh | 2 +-
>> osaf/services/saf/immsv/immnd/immnd_evt.c | 18 +++++++++++++++++-
>> osaf/services/saf/immsv/immnd/immnd_init.h | 2 +-
>> 4 files changed, 32 insertions(+), 8 deletions(-)
>>
>>
>> Timeout on client while waiting for implementer happens when
>> IMMA_SYNCR_TIMEOUT < IMMA_OI_CALLBACK_TIMEOUT.
>> mOriginatingConn is clear and we can't send response in
>> ::ccbObjCreate/Modify/Del/CompletedContinuation().
>> If the client invokes more ccb operations, it will get ERR_TIMEOUT.
>> We better abort the ccb in this case. This is the same behavior as
>> when there's a timeout from OI.
>>
>> 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
>> @@ -1154,9 +1154,9 @@ immModel_authorizedGroup(IMMND_CB *cb)
>> }
>> SaBoolT
>> -immModel_purgeSyncRequest(IMMND_CB *cb, SaUint32T clientId)
>> -{
>> - return
>> (ImmModel::instance(&cb->immModel)->purgeSyncRequest(clientId)) ?
>> +immModel_purgeSyncRequest(IMMND_CB *cb, SaUint32T clientId,
>> SaUint32T* ccbId)
>> +{
>> + return
>> (ImmModel::instance(&cb->immModel)->purgeSyncRequest(clientId, ccbId)) ?
>> SA_TRUE : SA_FALSE;
>> }
>> @@ -5498,7 +5498,12 @@ ImmModel::ccbApply(SaUint32T ccbId,
>> }
>> }
>> - osafassert(reqConn==0 || (ccb->mOriginatingConn == reqConn));
>> + if (reqConn && !ccb->mOriginatingConn) {
>> + /* When the ccb continuation is purged, the ccb must be
>> aborted */
>> + osafassert(ccb->mState == IMM_CCB_ABORTED);
>> + } else {
>> + osafassert(!reqConn || ccb->mOriginatingConn == reqConn);
>> + }
>> if(!ccb->isOk()) {
>> setCcbErrorString(ccb, IMM_RESOURCE_ABORT "CCB is in an
>> error state");
>> @@ -13596,7 +13601,7 @@ ImmModel::getAdminOwnerIdsForCon(SaUint3
>> }
>> bool
>> -ImmModel::purgeSyncRequest(SaUint32T clientId)
>> +ImmModel::purgeSyncRequest(SaUint32T clientId, SaUint32T* ccbId)
>> {
>> TRACE_ENTER();
>> bool purged=false;
>> @@ -13700,6 +13705,9 @@ ImmModel::purgeSyncRequest(SaUint32T cli
>> on this ccb (e.g. a late arriving OI reply.
>> */
>> (*ccbFound)->mOriginatingConn = 0;
>> + /* mOriginatingConn is cleared so we can't send response to
>> the client if
>> + they invoke more ccb operations. Abort the ccb now. */
>> + *ccbId = (*ccbFound)->mId;
>> purged = true;
>> TRACE_5("Purged Ccb continuation for ccb:%u in state %u",
>> (*ccbFound)->mId, (*ccbFound)->mState);
>> 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
>> @@ -117,7 +117,7 @@ public:
>> bool protocol47Allowed();
>> bool protocol50Allowed();
>> bool oneSafe2PBEAllowed();
>> - bool purgeSyncRequest(SaUint32T clientId);
>> + bool purgeSyncRequest(SaUint32T clientId,
>> SaUint32T* ccbId);
>> bool verifySchemaChange(const std::string&
>> className,
>> ClassInfo* oldClass,
>> ClassInfo* newClass,
>> 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
>> @@ -2118,8 +2118,9 @@ static uint32_t immnd_evt_proc_cl_imma_t
>> immnd_client_node_get(cb, evt->info.finReq.client_hdl, &cl_node);
>> if(!cl_node) {goto done;}
>> + SaUint32T ccbId = 0;
>> SaUint32T clientId =
>> m_IMMSV_UNPACK_HANDLE_HIGH(evt->info.finReq.client_hdl);
>> - if(immModel_purgeSyncRequest(cb, clientId)) {
>> + if(immModel_purgeSyncRequest(cb, clientId, &ccbId)) {
>> /* One and only one request has been purged => no reply
>> message send
>> will be attempted for *that* request. We can
>> safely clear the
>> MDS reply info. This will also clear the handle for new
>> use in
>> @@ -2141,6 +2142,21 @@ static uint32_t immnd_evt_proc_cl_imma_t
>> }
>> }
>> + /* The client is timed out when we're still waiting for
>> response from OI.
>> + This happens when IMMA_SYNCR_TIMEOUT <
>> IMMA_OI_CALLBACK_TIMEOUT. */
>> + if (ccbId) {
>> + IMMSV_EVT send_evt;
>> + memset(&send_evt, '\0', sizeof(IMMSV_EVT));
>> + send_evt.type = IMMSV_EVT_TYPE_IMMD;
>> + send_evt.info.immd.type = IMMD_EVT_ND2D_ABORT_CCB;
>> + send_evt.info.immd.info.ccbId = ccbId;
>> +
>> + LOG_WA("Timeout on client while waiting for implementer,
>> aborting ccb:%u", ccbId);
>> + if (immnd_mds_msg_send(cb, NCSMDS_SVC_ID_IMMD,
>> cb->immd_mdest_id, &send_evt) != NCSCC_RC_SUCCESS) {
>> + LOG_ER("Failure to broadcast discard Ccb for ccbId:%u",
>> ccbId);
>> + }
>> + }
>> +
>> done:
>> TRACE_LEAVE();
>> return NCSCC_RC_SUCCESS;
>> 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
>> @@ -324,7 +324,7 @@ extern "C" {
>> OsafImmAccessControlModeT immModel_accessControlMode(IMMND_CB
>> *cb);
>> const char *immModel_authorizedGroup(IMMND_CB *cb);
>> - SaBoolT immModel_purgeSyncRequest(IMMND_CB *cb, SaUint32T
>> clientId);
>> + SaBoolT immModel_purgeSyncRequest(IMMND_CB *cb, SaUint32T
>> clientId, SaUint32T* ccbId);
>> void immModel_recognizedIsolated(IMMND_CB *cb);
>
>
------------------------------------------------------------------------------
Mobile security can be enabling, not merely restricting. Employees who
bring their own devices (BYOD) to work are irked by the imposition of MDM
restrictions. Mobile Device Manager Plus allows you to control only the
apps on BYO-devices by containerizing them, leaving personal data untouched!
https://ad.doubleclick.net/ddm/clk/304595813;131938128;j
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
