Almost ack from me,
Code review only, not tested.
Hung, can you please repeat the same test using this patch, that you did
to expose the problem?
The only issue I have is that there is some code duplication.
The applier checks added to immModel_implIsFree() are almost the same as
the pre-existing checks done in
ImmModel::ImplementerSet().
That raises two questions:
1) A difference is that only ccb checks for applier is done in the code
added in _implIsFree,
whereas in implementerSet there are also ccb checks for regular OI
implementerSet.
I think we could just as well also do the ccb checks for regular OI in
implIsFree.
It would follow the same reasoning as for having the local pre-check for
appliers.
2) The code duplication is all inside ImmModel.cc which suggests that we
could factor the shared code
into an instance method in ImmModel called from both _implIsFree
andImplementerSet.
Point (2) could be regarded as an enhancement, if it is seen as too much
work now.
But if we dont factor the code then at the very least, the code in
implIsFree should have a comment
pointing to the the similar code in implSet (and vice versa). This so
that anyone that does bugfixing in
one will fix both.
/AndersBj
On 10/06/2015 04:18 PM, Zoran Milinkovic wrote:
> osaf/services/saf/immsv/immnd/ImmModel.cc | 44
> +++++++++++++++++++++++++++++++
> osaf/services/saf/immsv/immnd/immnd_evt.c | 7 +++-
> 2 files changed, 49 insertions(+), 2 deletions(-)
>
>
> In the first request of OiImplementerSet, from library to IMMND, a pre-check
> is added for checking if the applier can be set on a class or an object.
> The pre-check will improve response and skip sending the message over fevs,
> if the applier cannot be set to the class or the object.
>
> If the pre-check is successful, then it might be a short time frame that
> applier set can still fail until the message sent over fevs reach all nodes.
> If applier set fails at this point, discard implementer message will be sent
> to all nodes.
>
> 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
> @@ -1617,6 +1617,50 @@ immModel_implIsFree(IMMND_CB *cb, const
> if(impl->mId == 0) {return SA_AIS_OK;}
> if(impl->mDying) {return SA_AIS_ERR_TRY_AGAIN;}
>
> + /* Check if applier can be attached to a class or an object */
> + if(impl->mApplier) {
> + CcbVector::iterator i;
> + CcbInfo *ccb;
> + ObjectMutationMap::iterator omit;
> + ObjectMap::iterator oi;
> + ObjectInfo *obj;
> + ImplementerSetMap::iterator ismIter;
> +
> + for(i=sCcbVector.begin(); i!=sCcbVector.end(); ++i) {
> + ccb = (*i);
> + if(ccb->isActive()) {
> + for(omit=ccb->mMutations.begin();
> omit!=ccb->mMutations.end(); ++omit) {
> + oi = sObjectMap.find(omit->first);
> + osafassert(oi != sObjectMap.end());
> + obj = oi->second;
> +
> + if( ! obj->mClassInfo->mAppliers.empty()) {
> + ImplementerSet::iterator ii =
> obj->mClassInfo->mAppliers.begin();
> + for(; ii != obj->mClassInfo->mAppliers.end(); ++ii) {
> + if((*ii) == impl) {
> + TRACE("TRY_AGAIN: ccb %u is active on object
> '%s' "
> + "bound to class applier '%s'. Can not
> re-attach applier",
> + ccb->mId, omit->first.c_str(),
> implName.c_str());
> + return SA_AIS_ERR_EXIST;
> + }
> + }
> + }
> +
> + ismIter = sObjAppliersMap.find(omit->first);
> + if(ismIter != sObjAppliersMap.end()) {
> + ImplementerSet *implSet = ismIter->second;
> + if(implSet->find(impl) != implSet->end()) {
> + TRACE("TRY_AGAIN: ccb %u is active on object
> '%s' "
> + "bound to object applier '%s'. Can not
> re-attach applier",
> + ccb->mId, omit->first.c_str(),
> implName.c_str());
> + return SA_AIS_ERR_EXIST;
> + }
> + }
> + }
> + }
> + }
> + }
> +
> return SA_AIS_ERR_EXIST;
> }
>
> 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
> @@ -9543,7 +9543,7 @@ static void immnd_evt_proc_impl_set_rsp(
>
> if (originatedAtThisNd) { /*Send reply to client from this ND. */
> immnd_client_node_get(cb, clnt_hdl, &cl_node);
> - if (cl_node == NULL) {
> + if (cl_node == NULL || (err == SA_AIS_ERR_TRY_AGAIN &&
> !cl_node->mIsStale)) {
> /* Client was down */
> TRACE_5("Failed to get client node, discarding
> implementer id:%u for connection: %u", implId, conn);
> memset(&send_evt, '\0', sizeof(IMMSV_EVT));
> @@ -9556,7 +9556,10 @@ static void immnd_evt_proc_impl_set_rsp(
> /* Mark the implementer as dying to make sure no upcall
> is sent to the client.
> The implementer will be really discarded when
> global-discard message comes */
> immModel_discardImplementer(cb, implId, SA_FALSE, NULL,
> NULL);
> - return;
> +
> + if(cl_node == NULL) {
> + return;
> + }
> } else if (cl_node->mIsStale) {
> LOG_WA("IMMND - Client went down so no response");
> return;
>
> ------------------------------------------------------------------------------
> _______________________________________________
> 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
