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?
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
