I am refloating the patch again. I am leaving your below comment as it looks ok for me as of now:
> * avd_saImmOiAdminOperationResult() was calling > immutil_saImmOiAdminOperationResult() thus handling TRYAGAIN. I think it is > OK to skip that. Thanks -Nagu -----Original Message----- From: Hans Feldt [mailto:hans.fe...@ericsson.com] Sent: 30 October 2013 16:03 To: Nagendra Kumar; Praveen Malviya Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [PATCH 1 of 1] amfd : Prototype for ticket #85 On 10/30/2013 10:59 AM, Nagendra Kumar wrote: > Hi Hans, Thanks for your review. >>> * In avd_saImmOiAdminOperationResult() there is a call to saflog() which >>> needs to be kept in the new function. >>> Also the TRACE in avd_saImmOiAdminOperationResult() should be kept in order >>> to associate request with response. > > I can keep saflog() in report_ccb_validation_error(). Already trace is > there in immAdminOperationResult_2() and when I change the name from > immAdminOperationResult_2 to report_ccb_validation_error, TRACE will exists. > Is there anything I misunderstood? the important part was to trace the invocation id for correlation purposes as in: TRACE_ENTER2("inv:%llu, res:%u", invocation, result); you might as well keep that trace as it is Thanks, hans > > Thanks -Nagu > > -----Original Message----- From: Hans Feldt > [mailto:hans.fe...@ericsson.com] Sent: 30 October 2013 14:59 To: > Nagendra Kumar; Praveen Malviya Cc: > opensaf-devel@lists.sourceforge.net Subject: Re: [PATCH 1 of 1] amfd : > Prototype for ticket #85 > > Hi, > > The approach seems fine. Just some minor comments: > > * You recently introduced report_ccb_validation_error(), I think this > new function should be named similar. I suggest report_admin_op_error > > * In avd_saImmOiAdminOperationResult() there is a call to saflog() > which needs to be kept in the new function. Also the TRACE in > avd_saImmOiAdminOperationResult() should be kept in order to associate > request with response. > > * In e.g. su_admin_op_cb() there is no longer any point to assign rc a > value, it can be removed and just put the constant directly in the > function call. Will save some statements (maybe not lines) > > * avd_saImmOiAdminOperationResult() was calling > immutil_saImmOiAdminOperationResult() thus handling TRYAGAIN. I think it is > OK to skip that. > > Thanks, Hans > > On 10/30/2013 05:48 AM, nagendr...@oracle.com wrote: >> osaf/services/saf/amf/amfd/imm.cc | 39 ++++++++++++++++++++ >> osaf/services/saf/amf/amfd/include/imm.h | 3 >> + osaf/services/saf/amf/amfd/su.cc | 61 >> ++++++++++++++++++++----------- 3 files changed, 81 insertions(+), >> 22 deletions(-) >> >> >> 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 @@ -1794,4 +1794,43 @@ void >> report_ccb_validation_error(const C else LOG_WA("%s", err_str); } >> +/** + * Respond admin op to IMM + * @param immOiHandle + * @param >> invocation + * @param result + * @param pend_cbk + * @param format + >> * @param ... + */ >> +void immAdminOperationResult_2(SaImmOiHandleT immOiHandle, >> +SaInvocationT invocation, SaAisErrorT result, + >> AVD_ADMIN_OPER_CBK *pend_cbk, const char *format, ...) { + char >> ao_err_string[256]; + SaAisErrorT error; + >> SaStringT p_ao_err_string = ao_err_string; + SaImmAdminOperationParamsT_2 >> ao_err_param = { + >> const_cast<SaStringT>(SA_IMM_PARAM_ADMOP_ERROR), + >> SA_IMM_ATTR_SASTRINGT, + const_cast<SaStringT *> >> (&p_ao_err_string)}; + const SaImmAdminOperationParamsT_2 >> *ao_err_params[2] = { + &ao_err_param, + NULL }; >> + >> ao_err_string[sizeof(ao_err_string) - 1] = 0; >> >> + va_list ap; + va_start(ap, format); + (void) vsnprintf(ao_err_string, >> sizeof(ao_err_string), format, ap); + >> va_end(ap); + TRACE("%s",ao_err_string); + + error= >> saImmOiAdminOperationResult_o2(immOiHandle, invocation, result, >> ao_err_params); + if (error != SA_AIS_OK) + >> LOG_ER("saImmOiAdminOperationResult_o2 for %llu failed %u", >> +invocation, error); + + if (pend_cbk) { + >> pend_cbk->admin_oper = static_cast<SaAmfAdminOperationIdT>(0); + >> pend_cbk->invocation = 0; + } + +} 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 @@ -88,5 +88,8 @@ extern >> void avd_saImmOiAdminOperationRes SaAisErrorT result); void >> report_ccb_validation_error(const CcbUtilOperationData_t *opdata, const char >> *format, ...) __attribute__ ((format(printf, 2, 3))); +void >> immAdminOperationResult_2(SaImmOiHandleT immOiHandle, SaInvocationT >> invocation, SaAisErrorT result, + struct admin_oper_cbk >> *pend_cbk, + const char *format, ...) __attribute__ >> ((format(printf, 5, 6))); >> >> #endif 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 @@ -881,13 +881,15 @@ static void >> su_admin_op_cb(SaImmOiHandle >> >> if ( op_id > SA_AMF_ADMIN_SHUTDOWN && op_id != SA_AMF_ADMIN_REPAIRED) { rc = >> SA_AIS_ERR_NOT_SUPPORTED; - >> LOG_WA("Unsupported admin op for SU: %llu", op_id); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, >> NULL, + "Unsupported admin op for SU: %llu", >> op_id); goto done; } >> >> if (cb->init_state != AVD_APP_STATE ) { rc = SA_AIS_ERR_TRY_AGAIN; - >> LOG_WA("AMF (state %u) is not available for >> admin ops", cb->init_state); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, NULL, + >> "AMF (state %u) >> is not available for admin ops", cb->init_state); goto done; } >> >> @@ -900,7 +902,8 @@ static void su_admin_op_cb(SaImmOiHandle if >> ((su->sg_of_su->sg_ncs_spec == SA_TRUE) && >> (cb->node_id_avd == su->su_on_node->node_info.nodeId)) { rc = >> SA_AIS_ERR_NOT_SUPPORTED; - LOG_WA("Admin operation >> on Active middleware SU is not allowed"); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, >> &su->pend_cbk, + "Admin operation on Active >> middleware SU is not allowed"); goto done; } >> >> @@ -908,20 +911,24 @@ static void su_admin_op_cb(SaImmOiHandle for >> (su_ptr = su->sg_of_su->list_of_su; su_ptr != NULL; su_ptr = >> su_ptr->sg_list_su_next) { if (su_ptr->pend_cbk.invocation != 0) { rc = >> SA_AIS_ERR_TRY_AGAIN; - LOG_WA("Admin operation is already going on >> (su'%s')", su_ptr->name.value); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "Admin operation is already going on >> (su'%s')", +su_ptr->name.value); goto done; } } >> >> if (su->sg_of_su->sg_fsm_state != AVD_SG_FSM_STABLE) { rc = >> SA_AIS_ERR_TRY_AGAIN; - LOG_WA("SG state is not >> stable"); /* whatever that means... */ + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "SG state is not stable"); /* whatever that means... */ goto done; } >> /* if Tolerance timer is running for any SI's withing this SG, then return >> SA_AIS_ERR_TRY_AGAIN */ if >> (sg_is_tolerance_timer_running_for_any_si(su->sg_of_su)) { >> rc = SA_AIS_ERR_TRY_AGAIN; - LOG_WA("Tolerance timer is running for >> some of the SI's in the SG '%s', so differing >> admin opr",su->sg_of_su->name.value); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "Tolerance timer is running for some of the SI's in the SG '%s', " + >> "so differing admin >> opr",su->sg_of_su->name.value); goto done; } >> >> @@ -932,7 +939,8 @@ static void su_admin_op_cb(SaImmOiHandle >> ((su->saAmfSUAdminState == SA_AMF_ADMIN_LOCKED) && >> (op_id == SA_AMF_ADMIN_UNLOCK_INSTANTIATION))) { >> >> rc = SA_AIS_ERR_NO_OP; - LOG_WA("Admin operation (%llu) has no >> effect on current state (%u)", op_id, >> su->saAmfSUAdminState); + immAdminOperationResult_2(immoi_handle, >> invocation, rc, &su->pend_cbk, + "Admin >> operation (%llu) has no effect on current state (%u)", +op_id, >> su->saAmfSUAdminState); goto done; } >> >> @@ -951,14 +959,16 @@ static void su_admin_op_cb(SaImmOiHandle (op_id >> == SA_AMF_ADMIN_SHUTDOWN))) { >> >> rc = SA_AIS_ERR_BAD_OPERATION; - LOG_WA("State transition >> invalid, state %u, op %llu", su->saAmfSUAdminState, >> op_id); + immAdminOperationResult_2(immoi_handle, invocation, rc, >> &su->pend_cbk, + "State transition invalid, >> state %u, op %llu", +su->saAmfSUAdminState, op_id); goto done; } >> >> m_AVD_GET_SU_NODE_PTR(cb, su, node); if >> (node->admin_node_pend_cbk.admin_oper != 0) { rc = SA_AIS_ERR_TRY_AGAIN; - >> LOG_WA("Node'%s' hosting SU'%s', undergoing admin operation'%u'", >> node->name.value, su->name.value, + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "Node'%s' hosting SU'%s', undergoing >> admin operation'%u'", +node->name.value, su->name.value, >> node->admin_node_pend_cbk.admin_oper); goto done; } @@ >> -1001,7 +1011,8 @@ static void su_admin_op_cb(SaImmOiHandle >> avd_su_readiness_state_set(su, SA_AMF_READINESS_OUT_OF_SERVICE); >> avd_su_admin_state_set(su, SA_AMF_ADMIN_LOCKED); rc = >> SA_AIS_ERR_FAILED_OPERATION; - LOG_WA("SG redundancy >> model specific handler failed"); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "SG redundancy model specific handler >> failed"); goto done; } >> >> @@ -1043,7 +1054,8 @@ static void su_admin_op_cb(SaImmOiHandle >> avd_su_readiness_state_set(su, back_red_state); >> avd_su_admin_state_set(su, back_admin_state); rc = >> SA_AIS_ERR_FAILED_OPERATION; - LOG_WA("SG redundancy model >> specific handler failed"); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "SG >> redundancy model specific handler failed"); goto done; } >> >> @@ -1060,7 +1072,8 @@ static void su_admin_op_cb(SaImmOiHandle >> >> if ( su->list_of_susi != NULL ) { rc = SA_AIS_ERR_TRY_AGAIN; - >> LOG_WA("SIs still assigned to this SU"); + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "SIs still assigned to this SU"); goto >> done; } >> >> @@ -1090,7 +1103,8 @@ static void su_admin_op_cb(SaImmOiHandle goto done; } >> rc = SA_AIS_ERR_TRY_AGAIN; - >> LOG_ER("Internal error, could not send message to avnd"); + >> immAdminOperationResult_2(immoi_handle, invocation, >> rc, &su->pend_cbk, + "Internal error, could >> not send message to avnd"); goto done; } else { >> avd_su_admin_state_set(su, SA_AMF_ADMIN_LOCKED_INSTANTIATION); @@ >> -1103,8 +1117,9 @@ static void su_admin_op_cb(SaImmOiHandle case >> SA_AMF_ADMIN_UNLOCK_INSTANTIATION: >> >> if (NULL == su->list_of_comp) { - LOG_WA("There is no >> component configured for SU '%s'.", su->name.value); rc = >> SA_AIS_ERR_BAD_OPERATION; + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "There >> is no component configured for SU '%s'.", +su->name.value); goto >> done; } >> >> @@ -1119,9 +1134,10 @@ static void su_admin_op_cb(SaImmOiHandle } >> >> if (su->saAmfSUPresenceState != SA_AMF_PRESENCE_UNINSTANTIATED) { - >> LOG_WA("Can't instantiate '%s', whose >> presense state is '%u'", su_name->value, + rc = >> SA_AIS_ERR_BAD_OPERATION; + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, + >> "Can't instantiate '%s', whose >> presense state is '%u'", +su_name->value, su->saAmfSUPresenceState); - >> rc = SA_AIS_ERR_BAD_OPERATION; goto done; >> } >> >> @@ -1142,7 +1158,8 @@ static void su_admin_op_cb(SaImmOiHandle goto done; } >> rc = SA_AIS_ERR_TRY_AGAIN; - >> LOG_ER("Internal error, could not send message to avnd"); + >> immAdminOperationResult_2(immoi_handle, invocation, >> rc, &su->pend_cbk, + "Internal error, could >> not send message to avnd"); } else { avd_su_admin_state_set(su, >> SA_AMF_ADMIN_LOCKED); avd_saImmOiAdminOperationResult(immoi_handle, >> invocation, SA_AIS_OK); @@ -1152,8 +1169,9 @@ static void >> su_admin_op_cb(SaImmOiHandle break; case SA_AMF_ADMIN_REPAIRED: if >> (su->saAmfSUOperState == >> SA_AMF_OPERATIONAL_ENABLED) { - LOG_NO("Admin repair >> request for '%s', op state already enabled", >> su_name->value); rc = SA_AIS_ERR_NO_OP; + >> immAdminOperationResult_2(immoi_handle, invocation, rc, &su->pend_cbk, >> + "Admin repair request for '%s', op >> state already enabled", >> ++su_name->value); goto done; } >> >> @@ -1165,19 +1183,18 @@ static void su_admin_op_cb(SaImmOiHandle rc = >> SA_AIS_OK; } else { - LOG_WA("Admin op >> request send failed '%s'", su_name->value); rc = SA_AIS_ERR_TIMEOUT; + >> immAdminOperationResult_2(immoi_handle, >> invocation, rc, &su->pend_cbk, + "Admin >> op request send failed '%s'", su_name->value); } break; default: - >> LOG_ER("Unsupported admin op"); rc = SA_AIS_ERR_INVALID_PARAM; + >> immAdminOperationResult_2(immoi_handle, >> invocation, rc, +&su->pend_cbk, "Unsupported admin op"); break; } >> >> done: - if (rc != SA_AIS_OK) - >> avd_saImmOiAdminOperationResult(immoi_handle, invocation, rc); >> >> TRACE_LEAVE2("%u", rc); } >> >> > > ------------------------------------------------------------------------------ Android is increasing in popularity, but the open development platform that developers love is also attractive to malware creators. Download this white paper to learn more about secure code signing practices that can help keep Android apps secure. http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
