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
