If you get BAD_HANDLE on the attempt to reply on the admop, then assuming the handle was not corrupted/misshandled by the applicaiton, it can only mean that you need to re-init the handle and there is no point then in any new attempt at replying to this request.
So the admin-resp job should be discarded in this case. This would only happen if the local IMMND had crashed, restarted and resurrect of the handle was not possible. The imm-oi-library internally handles the resurrect attempt. /AndersBj -----Original Message----- From: Hans Feldt [mailto:osafde...@gmail.com] Sent: den 11 april 2014 09:48 To: Gary Lee Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [devel] [PATCH 1 of 2] amfd: use job queue for IMM admin response [#817] My comment was wrong. In the BAD_HANDLE case avd_imm_reinit_bg() must be triggered and this is how we do it now. My concern was if the admin resp job should remain in the queue or be discarded in this particular case. /Hans On 11 April 2014 09:14, Hans Feldt <osafde...@gmail.com> wrote: > Some minor comments inline. > If we agree on them I can push. > Thanks, > Hans > > On 31 March 2014 06:32, Gary Lee <gary....@dektech.com.au> wrote: >> osaf/services/saf/amf/amfd/imm.cc | 40 >> ++++++++++++++++++++++++++----- >> osaf/services/saf/amf/amfd/include/imm.h | 16 ++++++++++++ >> 2 files changed, 49 insertions(+), 7 deletions(-) >> >> >> Currently, admin operation responses from amfd are not synchronised >> with IMM changes that may be performed as part of the admin operation. >> That is, an admin operation may return before the corresponding IMM >> changes have been performed. IMM changes by amfd are typically placed >> into a FIFO queue to be done at a later time. >> >> This patch puts admin operation responses into the same queue. >> Therefore guaranteeing that when an admin operation returns, the >> corresponding IMM changes have already been done. >> >> diff --git a/osaf/services/saf/amf/amfd/imm.cc >> b/osaf/services/saf/amf/amfd/imm.cc >> --- a/osaf/services/saf/amf/amfd/imm.cc >> +++ b/osaf/services/saf/amf/amfd/imm.cc >> @@ -228,6 +228,35 @@ ImmObjDelete::~ImmObjDelete() } >> >> // >> +AvdJobDequeueResultT ImmAdminResponse::exec(const SaImmOiHandleT handle) { >> + SaAisErrorT rc; >> + AvdJobDequeueResultT res; >> + >> + TRACE_ENTER(); > > this should be a TRACE_ENTER2 printing "adm resp inv:%llu res:%u" > >> + >> + rc = saImmOiAdminOperationResult(handle, this->invocation_, >> + this->result_); >> + >> + if (rc == SA_AIS_OK) { >> + delete Fifo::dequeue(); >> + res = JOB_EXECUTED; >> + } else if (rc == SA_AIS_ERR_TRY_AGAIN) { >> + TRACE("TRY-AGAIN"); >> + res = JOB_ETRYAGAIN; >> + } else if (rc == SA_AIS_ERR_BAD_HANDLE) { > > I don't think we should handle this (BAD_HANDLE) case, just let it > slip through to the else branch > >> + TRACE("BADHANDLE"); >> + avd_imm_reinit_bg(); >> + res = JOB_ETRYAGAIN; >> + } else { >> + delete Fifo::dequeue(); >> + LOG_ER("Admin op failed for invocation: %llu, result %u", >> + this->invocation_, this->result_); >> + res = JOB_ERR; >> + } >> + >> + TRACE_LEAVE2("%u", res); >> + return res; >> +} >> + >> Job* Fifo::peek() >> { >> Job* tmp; >> @@ -1719,15 +1748,12 @@ void avd_saImmOiAdminOperationResult(SaI >> >> SaInvocationT invocation, >> >> SaAisErrorT result) { >> - SaAisErrorT error; >> + TRACE_ENTER2("inv:%llu, res:%u", invocation, result); >> >> - TRACE_ENTER2("inv:%llu, res:%u", invocation, result); >> - saflog(LOG_NOTICE, amfSvcUsrName, "Admin op done for invocation: >> %llu, result %u", >> - invocation, result); >> + ImmAdminResponse *job = new ImmAdminResponse(invocation, result); >> + Fifo::queue(job); >> >> - error = immutil_saImmOiAdminOperationResult(immOiHandle, invocation, >> result); >> - if (error != SA_AIS_OK) >> - LOG_NO("saImmOiAdminOperationResult for %llu failed %u", >> invocation, error); >> + TRACE_LEAVE(); >> } >> >> /** >> diff --git a/osaf/services/saf/amf/amfd/include/imm.h >> b/osaf/services/saf/amf/amfd/include/imm.h >> --- a/osaf/services/saf/amf/amfd/include/imm.h >> +++ b/osaf/services/saf/amf/amfd/include/imm.h >> @@ -91,6 +91,22 @@ public: >> ~ImmObjDelete(); >> }; >> >> +class ImmAdminResponse : public Job { >> + public: >> + ImmAdminResponse(const SaInvocationT invocation, >> + const SaAisErrorT result) { >> + this->invocation_ = invocation; >> + this->result_ = result; >> + } >> + AvdJobDequeueResultT exec(SaImmOiHandleT immOiHandle); >> + >> + ~ImmAdminResponse() {} >> + private: >> + ImmAdminResponse(); >> + SaInvocationT invocation_; >> + SaAisErrorT result_; >> +}; >> + >> // >> class Fifo { >> public: >> >> --------------------------------------------------------------------- >> --------- _______________________________________________ >> Opensaf-devel mailing list >> Opensaf-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ Put Bad Developers to Shame Dominate Development with Jenkins Continuous Integration Continuously Automate Build, Test & Deployment Start a new project now. Try Jenkins in the cloud. http://p.sf.net/sfu/13600_Cloudbees _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ Put Bad Developers to Shame Dominate Development with Jenkins Continuous Integration Continuously Automate Build, Test & Deployment Start a new project now. Try Jenkins in the cloud. http://p.sf.net/sfu/13600_Cloudbees _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
