> -----Original Message----- > From: Nagendra Kumar [mailto:[email protected]] > Sent: den 4 juli 2014 13:44 > To: Hans Feldt; Hans Nordebäck; Praveen Malviya > Cc: [email protected] > Subject: RE: [PATCH 1 of 1] amfd: allow admin commands before cluster timer > expiry [#620] > > Thanks for your review. I will incorporate these changes. Is this an Ack then? [Hans] Yes
> > Thanks > -Nagu > > > -----Original Message----- > > From: Hans Feldt [mailto:[email protected]] > > Sent: 04 July 2014 16:47 > > To: Nagendra Kumar; Hans Nordebäck; Praveen Malviya > > Cc: [email protected] > > Subject: RE: [PATCH 1 of 1] amfd: allow admin commands before cluster timer > > expiry [#620] > > > > Just review, see inline > > /Hans > > > > > -----Original Message----- > > > From: [email protected] [mailto:[email protected]] > > > Sent: den 30 juni 2014 12:48 > > > To: Hans Feldt; Hans Nordebäck; [email protected] > > > Cc: [email protected] > > > Subject: [PATCH 1 of 1] amfd: allow admin commands before cluster timer > > expiry [#620] > > > > > > osaf/services/saf/amf/amfd/imm.cc | 9 ++++- > > > osaf/services/saf/amf/amfd/include/util.h | 1 + > > > osaf/services/saf/amf/amfd/node.cc | 26 +++++++++++--- > > > osaf/services/saf/amf/amfd/sg.cc | 16 +++++++++ > > > osaf/services/saf/amf/amfd/si.cc | 10 +++++- > > > osaf/services/saf/amf/amfd/su.cc | 23 +++++++++---- > > > osaf/services/saf/amf/amfd/util.cc | 52 > > +++++++++++++++++++++++++++++++ > > > 7 files changed, 122 insertions(+), 15 deletions(-) > > > > > > > > > SA_AMF_ADMIN_UNLOCK, SA_AMF_ADMIN_LOCK, > > SA_AMF_ADMIN_UNLOCK_INSTANTIATION > > > and SA_AMF_ADMIN_REPAIRED can be issued on amf entities before cluster > > > timer expiry. The other commands will be returned as try_again > > > during the period. > > > > > > 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 > > > @@ -666,7 +666,14 @@ static void admin_operation_cb(SaImmOiHa > > > admin_op_name(static_cast<SaAmfAdminOperationIdT>(op_id)), > > object_name->value, invocation); > > > > > > if (admin_op_callback[type] != NULL) { > > > - admin_op_callback[type](immoi_handle, invocation, > > object_name, op_id, params); > > > + if (false == is_admin_op_valid(op_id, type)) { > > [Hans] please revert this statement to: "is_admin_op_valid(op_id, type) == > > false" > > > > > + report_admin_op_error(immoi_handle, invocation, > > SA_AIS_ERR_TRY_AGAIN, NULL, > > > + "AMF (state %u) is not available for > > admin op'%llu' on '%s'", > > > + avd_cb->init_state, op_id, > > object_name->value); > > > + goto done; > > > + } else { > > > + admin_op_callback[type](immoi_handle, invocation, > > object_name, op_id, params); > > > + } > > > } else { > > > LOG_ER("Admin operation not supported for %s (%u)", > > object_name->value, type); > > > report_admin_op_error(immoi_handle, invocation, > > SA_AIS_ERR_INVALID_PARAM, NULL, > > > diff --git a/osaf/services/saf/amf/amfd/include/util.h > > b/osaf/services/saf/amf/amfd/include/util.h > > > --- a/osaf/services/saf/amf/amfd/include/util.h > > > +++ b/osaf/services/saf/amf/amfd/include/util.h > > > @@ -96,6 +96,7 @@ struct avd_comp_csi_rel_tag; > > > struct avd_csi_tag; > > > > > > void avd_d2n_reboot_snd(struct avd_avnd_tag *node); > > > +bool is_admin_op_valid(SaImmAdminOperationIdT opId, > > AVSV_AMF_CLASS_ID class_id); > > > void amflog(int priority, const char *format, ...); > > > void d2n_msg_free(AVD_DND_MSG *msg); > > > uint32_t avd_d2n_msg_dequeue(struct cl_cb_tag *cb); > > > diff --git a/osaf/services/saf/amf/amfd/node.cc > > b/osaf/services/saf/amf/amfd/node.cc > > > --- a/osaf/services/saf/amf/amfd/node.cc > > > +++ b/osaf/services/saf/amf/amfd/node.cc > > > @@ -1041,12 +1041,6 @@ static void node_admin_op_cb(SaImmOiHand > > > > > > TRACE_ENTER2("%llu, '%s', %llu", invocation, objectName->value, > > operationId); > > > > > > - if (avd_cb->init_state != AVD_APP_STATE) { > > > - report_admin_op_error(immOiHandle, invocation, > > SA_AIS_ERR_TRY_AGAIN, NULL, > > > - "AVD not in APP_STATE"); > > > - goto done; > > > - } > > > - > > > node = avd_node_get(objectName); > > > osafassert(node != AVD_AVND_NULL); > > > > > > @@ -1136,6 +1130,17 @@ static void node_admin_op_cb(SaImmOiHand > > > goto done; > > > } > > > > > > + if (avd_cb->init_state == AVD_INIT_DONE) { > > > + node_admin_state_set(node, > > SA_AMF_ADMIN_UNLOCKED); > > > + for(su = node->list_of_su; su != NULL; su = su- > > >avnd_list_su_next) { > > > + if (su->is_in_service() == true) { > > > + su- > > >set_readiness_state(SA_AMF_READINESS_IN_SERVICE); > > > + } > > > + } > > > + avd_saImmOiAdminOperationResult(immOiHandle, > > invocation, SA_AIS_OK); > > > + break; > > > + } > > > + > > > avd_node_admin_lock_unlock_shutdown(node, invocation, > > static_cast<SaAmfAdminOperationIdT>(operationId)); > > > break; > > > > > > @@ -1160,6 +1165,15 @@ static void node_admin_op_cb(SaImmOiHand > > > goto done; > > > } > > > > > > + if (avd_cb->init_state == AVD_INIT_DONE) { > > [Hans] white space indent problem? > > > > > + node_admin_state_set(node, SA_AMF_ADMIN_LOCKED); > > > + for(su = node->list_of_su; su != NULL; su = su- > > >avnd_list_su_next) { > > > + su- > > >set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE); > > > + } > > > + avd_saImmOiAdminOperationResult(immOiHandle, > > invocation, SA_AIS_OK); > > > + break; > > > + } > > > + > > > avd_node_admin_lock_unlock_shutdown(node, invocation, > > static_cast<SaAmfAdminOperationIdT>(operationId)); > > > break; > > > > > > diff --git a/osaf/services/saf/amf/amfd/sg.cc > > b/osaf/services/saf/amf/amfd/sg.cc > > > --- a/osaf/services/saf/amf/amfd/sg.cc > > > +++ b/osaf/services/saf/amf/amfd/sg.cc > > > @@ -1234,6 +1234,14 @@ static void sg_admin_op_cb(SaImmOiHandle > > > > > > adm_state = sg->saAmfSGAdminState; > > > avd_sg_admin_state_set(sg, SA_AMF_ADMIN_UNLOCKED); > > > + if (avd_cb->init_state == AVD_INIT_DONE) { > > > + for (su = sg->list_of_su; su != NULL; su = su- > > >sg_list_su_next) { > > > + if (su->is_in_service() == true) { > > > + su- > > >set_readiness_state(SA_AMF_READINESS_IN_SERVICE); > > > + } > > > + } > > > + break; > > > + } > > > if (avd_sg_app_sg_admin_func(avd_cb, sg) != > > NCSCC_RC_SUCCESS) { > > > avd_sg_admin_state_set(sg, adm_state); > > > report_admin_op_error(immOiHandle, invocation, > > SA_AIS_ERR_BAD_OPERATION, NULL, > > > @@ -1257,6 +1265,14 @@ static void sg_admin_op_cb(SaImmOiHandle > > > > > > adm_state = sg->saAmfSGAdminState; > > > avd_sg_admin_state_set(sg, SA_AMF_ADMIN_LOCKED); > > > + > > > + if (avd_cb->init_state == AVD_INIT_DONE) { > > > + for (su = sg->list_of_su; su != NULL; su = su- > > >sg_list_su_next) { > > > + su- > > >set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE); > > > + } > > > + break; > > > + } > > > + > > > if (avd_sg_app_sg_admin_func(avd_cb, sg) != > > NCSCC_RC_SUCCESS) { > > > avd_sg_admin_state_set(sg, adm_state); > > > report_admin_op_error(immOiHandle, invocation, > > SA_AIS_ERR_BAD_OPERATION, NULL, > > > diff --git a/osaf/services/saf/amf/amfd/si.cc > > b/osaf/services/saf/amf/amfd/si.cc > > > --- a/osaf/services/saf/amf/amfd/si.cc > > > +++ b/osaf/services/saf/amf/amfd/si.cc > > > @@ -809,6 +809,14 @@ static void si_admin_op_cb(SaImmOiHandle > > > > > > si->set_admin_state(SA_AMF_ADMIN_UNLOCKED); > > > > > > + if (avd_cb->init_state == AVD_INIT_DONE) { > > > + /* Mark the admin state to unlocked, assignments will > > be > > > + delivered once cluster timer expires. */ > > [Hans] there is no "mark" of admin state here... > > > > > + rc = SA_AIS_OK; > > > + avd_saImmOiAdminOperationResult(immOiHandle, > > invocation, rc); > > > + goto done; > > > + } > > > + > > > err = si->sg_of_si->si_assign(avd_cb, si); > > > if (si->list_of_sisu == NULL) > > > LOG_NO("'%s' could not be assigned to any SU", si- > > >name.value); > > > @@ -849,7 +857,7 @@ static void si_admin_op_cb(SaImmOiHandle > > > goto done; > > > } > > > > > > - if (si->list_of_sisu == AVD_SU_SI_REL_NULL) { > > > + if ((si->list_of_sisu == AVD_SU_SI_REL_NULL) || (avd_cb- > > >init_state == AVD_INIT_DONE)) { > > > si->set_admin_state(SA_AMF_ADMIN_LOCKED); > > > /* This may happen when SUs are locked before SI is > > locked. */ > > > LOG_WA("SI lock of %s, has no assignments", > > objectName->value); > > > diff --git a/osaf/services/saf/amf/amfd/su.cc > > b/osaf/services/saf/amf/amfd/su.cc > > > --- a/osaf/services/saf/amf/amfd/su.cc > > > +++ b/osaf/services/saf/amf/amfd/su.cc > > > @@ -840,6 +840,14 @@ void AVD_SU::unlock(SaImmOiHandleT immoi > > > TRACE_ENTER2("'%s'", name.value); > > > set_admin_state(SA_AMF_ADMIN_UNLOCKED); > > > > > > + /* Mark the admin state to unlock and return as cluster timer haven't > > expired.*/ > > [Hans] [Hans] there is no "mark" of admin state here... > > > > > + if (avd_cb->init_state == AVD_INIT_DONE) { > > > + if (is_in_service() == true) > > > + > > set_readiness_state(SA_AMF_READINESS_IN_SERVICE); > > > + avd_saImmOiAdminOperationResult(immoi_handle, > > invocation, SA_AIS_OK); > > > + goto done; > > > + } > > > + > > > if ((is_in_service() == true) || (sg_of_su->sg_ncs_spec == true)) { > > > /* Reason for checking for MW component is that node oper > > state and > > > * SU oper state are marked enabled after they gets > > assignments. > > > @@ -866,7 +874,7 @@ void AVD_SU::unlock(SaImmOiHandleT immoi > > > report_admin_op_error(immoi_handle, invocation, > > SA_AIS_ERR_FAILED_OPERATION, NULL, > > > "SG redundancy model specific handler > > failed"); > > > } > > > - > > > +done: > > > TRACE_LEAVE(); > > > } > > > > > > @@ -878,6 +886,13 @@ void AVD_SU::lock(SaImmOiHandleT immoi_h > > > bool is_oper_successful = true; > > > > > > TRACE_ENTER2("'%s'", name.value); > > > + /* Mark the admin state to lock and return as cluster timer haven't > > expired.*/ > > [Hans] mark->change ? > > > > > + if (avd_cb->init_state == AVD_INIT_DONE) { > > > + set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE); > > > + set_admin_state(SA_AMF_ADMIN_LOCKED); > > > + avd_saImmOiAdminOperationResult(immoi_handle, > > invocation, SA_AIS_OK); > > > + goto done; > > > + } > > > > > > if (list_of_susi == NULL) { > > > set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE); > > > @@ -1098,12 +1113,6 @@ static void su_admin_op_cb(SaImmOiHandle > > > goto done; > > > } > > > > > > - if (cb->init_state != AVD_APP_STATE ) { > > > - report_admin_op_error(immoi_handle, invocation, > > SA_AIS_ERR_TRY_AGAIN, NULL, > > > - "AMF (state %u) is not available for admin > > ops", cb->init_state); > > > - goto done; > > > - } > > > - > > > if (NULL == (su = su_db->find(Amf::to_string(su_name)))) { > > > LOG_CR("SU '%s' not found", su_name->value); > > > /* internal error? osafassert instead? */ > > > diff --git a/osaf/services/saf/amf/amfd/util.cc > > b/osaf/services/saf/amf/amfd/util.cc > > > --- a/osaf/services/saf/amf/amfd/util.cc > > > +++ b/osaf/services/saf/amf/amfd/util.cc > > > @@ -1704,3 +1704,55 @@ void amflog(int priority, const char *fo > > > saflog(priority, amfSvcUsrName, "%s", str); > > > } > > > } > > > + > > > +/** > > > + * Validates whether the initial state is apt to process the admin > > > command. > > [Hans] "initial state", what is that? > > > > > + * @param: admin operation id. > > > + * @param: class id related to objects on which admin command has been > > issued. > > > + */ > > > +bool is_admin_op_valid(SaImmAdminOperationIdT opId, > > AVSV_AMF_CLASS_ID class_id) > > [Hans] admin_op_is_valid makes code using this function more readable > > > > > +{ > > > + bool valid = false; > > > + TRACE_ENTER2("%llu, %u", opId, class_id); > > > + > > > + if (avd_cb->init_state == AVD_APP_STATE) > > > + return true; > > > + > > > + switch (class_id) { > > > + case AVSV_SA_AMF_SU: > > > + { > > [Hans] wrong placement of brace not even needed, same below > > > + /* Support for admin op on su before cluster > > timer expires.*/ > > > + if ((avd_cb->init_state == AVD_INIT_DONE) > > && > > > + ((opId == > > SA_AMF_ADMIN_LOCK) || (opId == SA_AMF_ADMIN_UNLOCK) > > > + || (opId == > > SA_AMF_ADMIN_REPAIRED) || > > [Hans] please put logical or at the end of a line > > > + (opId == > > SA_AMF_ADMIN_UNLOCK_INSTANTIATION))) > > > + valid = true; > > > + } > > > + break; > > > + case AVSV_SA_AMF_NODE: > > > + case AVSV_SA_AMF_SG: > > > + { > > > + /* Support for admin op on node/sg before > > cluster timer expires.*/ > > > + if ((avd_cb->init_state == AVD_INIT_DONE) > > && > > > + ((opId == > > SA_AMF_ADMIN_LOCK) || (opId == SA_AMF_ADMIN_UNLOCK) > > > + || (opId == > > SA_AMF_ADMIN_UNLOCK_INSTANTIATION))) > > [Hans] please put logical or at the end of a line > > > > > + valid = true; > > > + } > > > + break; > > > + > > > + case AVSV_SA_AMF_SI: > > > + { > > > + /* Support for admin op on si before cluster > > timer expires. */ > > > + if ((avd_cb->init_state == AVD_INIT_DONE) > > && > > > + ((opId == > > SA_AMF_ADMIN_LOCK) || (opId == SA_AMF_ADMIN_UNLOCK))) > > > + valid = true; > > > + } > > > + break; > > > + case AVSV_SA_AMF_COMP: > > > + default: > > > + break; > > > + } > > > + > > > + return valid; > > > +} > > > + ------------------------------------------------------------------------------ Open source business process management suite built on Java and Eclipse Turn processes into business applications with Bonita BPM Community Edition Quickly connect people, data, and systems into organized workflows Winner of BOSSIE, CODIE, OW2 and Gartner awards http://p.sf.net/sfu/Bonitasoft _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
