See inline, I think more polishing is needed.
this comment in avnd_evt.h:
/* valid only when cmd launch (fork, exec etc) fails */
should be removed
In avnd_evt_clc_resp_evh comp lookup can silently return. I think you
should add a log entry there:
LOG_WA("%s: could not find comp '%s'", clc_evt->comp_name.value);
Thanks,
Hans
On 6 August 2013 15:21, <[email protected]> wrote:
> osaf/services/saf/avsv/avnd/avnd_clc.c | 63
> ++++++++++++++++-----------------
> 1 files changed, 31 insertions(+), 32 deletions(-)
>
>
> At present when clc response event comes AMFND searches whole component
> database and matches the context.
> This context does not include component name. Thus AMFND can associate clc
> response with wrong component.
> The patch ensures that AMFND gets component name in the clc response event
> and thus solves the problem
> by associating clc response event with the component.
The changelog can be improved. Should state that AMF can e.g. think
that cleanup has failed as described in the ticket. And why (because
correlation done with a pointer to dynamic memory)
>
> diff --git a/osaf/services/saf/avsv/avnd/avnd_clc.c
> b/osaf/services/saf/avsv/avnd/avnd_clc.c
> --- a/osaf/services/saf/avsv/avnd/avnd_clc.c
> +++ b/osaf/services/saf/avsv/avnd/avnd_clc.c
> @@ -316,17 +316,9 @@ uint32_t avnd_evt_clc_resp_evh(AVND_CB *
>
> /* get the comp */
> if (clc_evt->exec_ctxt) {
> - /* => cmd successfully launched (scan the entire
> comp-database) */
> - for (comp = (AVND_COMP
> *)ncs_patricia_tree_getnext(&cb->compdb,
> - (uint8_t
> *)0); comp;
> - comp = (AVND_COMP
> *)ncs_patricia_tree_getnext(&cb->compdb, (uint8_t *)&comp->name)) {
> - if (comp->clc_info.cmd_exec_ctxt ==
> clc_evt->exec_ctxt)
> - break;
> - if (comp->clc_info.am_cmd_exec_ctxt ==
> clc_evt->exec_ctxt) {
> - amcmd = 1;
> - break;
> - }
> - }
> + comp = m_AVND_COMPDB_REC_GET(cb->compdb, clc_evt->comp_name);
> + if (comp->clc_info.am_cmd_exec_ctxt == clc_evt->exec_ctxt)
> + amcmd = 1;
> } else
> /* => cmd did not launch successfully (comp is available) */
The above comment is wrong and should be removed. comp name is now
always available.
But I think the remaining if/else can be removed too.
clc_evt->exec_ctxt is never set to a value thus
if (comp->clc_info.am_cmd_exec_ctxt == clc_evt->exec_ctxt)
amcmd = 1;
is always false which means it can be removed together with the amcmd variable.
> comp = m_AVND_COMPDB_REC_GET(cb->compdb, clc_evt->comp_name);
> @@ -637,7 +629,7 @@ done:
>
> ******************************************************************************/
> uint32_t avnd_comp_clc_resp(NCS_OS_PROC_EXECUTE_TIMED_CB_INFO *info)
> {
> - AVND_CLC_EVT clc_evt;
> + AVND_CLC_EVT *clc_evt = (AVND_CLC_EVT *)info->i_usr_hdl;
> AVND_EVT *evt = 0;
> uint32_t rc = NCSCC_RC_SUCCESS;
> TRACE_ENTER();
> @@ -645,14 +637,11 @@ uint32_t avnd_comp_clc_resp(NCS_OS_PROC_
> if (!info)
> goto done;
the above check does not make sense since you have already dereferenced info
so move initialization of clc_evt past the check
The check should probably be an assert instead
assert(info != NULL);
>
> - memset(&clc_evt, 0, sizeof(AVND_CLC_EVT));
> -
> /* fill the clc-evt param */
> - clc_evt.exec_ctxt = info->i_exec_hdl;
> - clc_evt.exec_stat = info->exec_stat;
> + clc_evt->exec_stat = info->exec_stat;
>
> /* create the event */
> - evt = avnd_evt_create(avnd_cb, AVND_EVT_CLC_RESP, 0, 0, 0, (void
> *)&clc_evt, 0);
> + evt = avnd_evt_create(avnd_cb, AVND_EVT_CLC_RESP, 0, 0, 0, (void
> *)clc_evt, 0);
void * cast should not be needed, can be removed
> if (!evt) {
> rc = NCSCC_RC_FAILURE;
> goto done;
> @@ -662,6 +651,7 @@ uint32_t avnd_comp_clc_resp(NCS_OS_PROC_
> rc = avnd_evt_send(avnd_cb, evt);
>
> done:
> + free(clc_evt);
> /* free the event */
> if (NCSCC_RC_SUCCESS != rc && evt)
> avnd_evt_destroy(evt);
> @@ -2508,7 +2498,7 @@ uint32_t avnd_comp_clc_cmd_execute(AVND_
> char env_var_nodeid[] = "NCS_ENV_NODE_ID";
> char env_var_comp_err[] = "NCS_ENV_COMPONENT_ERROR_SRC";
> char *env_attr_val = 0;
> - AVND_CLC_EVT clc_evt;
> + AVND_CLC_EVT *clc_evt;
> AVND_EVT *evt = 0;
> AVND_COMP_CLC_INFO *clc_info = &comp->clc_info;
> char scr[SAAMF_CLC_LEN];
> @@ -2528,15 +2518,16 @@ uint32_t avnd_comp_clc_cmd_execute(AVND_
> goto err;
> }
>
> + clc_evt = (AVND_CLC_EVT *)calloc(1, sizeof(AVND_CLC_EVT));
Since the clc_evt memory is not freed in this function, add comment
where it is freed.
> + memcpy(&clc_evt->comp_name, &comp->name, sizeof(SaNameT));
intialize clc_evt->cmd_type here with cmd_type once
add new line here.
> /* For external component, there is no cleanup command. So, we will
> send a
> SUCCESS message to the mail box for external components. There
> wouldn't
> be any other command for external component comming. */
> if (true == comp->su->su_is_external) {
> if (AVND_COMP_CLC_CMD_TYPE_CLEANUP == cmd_type) {
> - memset(&clc_evt, 0, sizeof(AVND_CLC_EVT));
> - memcpy(&clc_evt.comp_name, &comp->name,
> sizeof(SaNameT));
> - clc_evt.cmd_type = cmd_type;
> - clc_evt.exec_stat.value = NCS_OS_PROC_EXIT_NORMAL;
> + memcpy(&clc_evt->comp_name, &comp->name,
> sizeof(SaNameT));
> + clc_evt->cmd_type = cmd_type;
remove 2 above lines, redundant
> + clc_evt->exec_stat.value = NCS_OS_PROC_EXIT_NORMAL;
>
> clc_info->exec_cmd = cmd_type;
> m_AVND_SEND_CKPT_UPDT_ASYNC_UPDT(cb, comp,
> AVND_CKPT_COMP_EXEC_CMD);
> @@ -2545,17 +2536,21 @@ uint32_t avnd_comp_clc_cmd_execute(AVND_
> m_AVND_SEND_CKPT_UPDT_ASYNC_UPDT(cb, comp,
> AVND_CKPT_COMP_CMD_EXEC_CTXT);
>
> /* create the event */
> - evt = avnd_evt_create(cb, AVND_EVT_CLC_RESP, 0, 0, 0,
> (void *)&clc_evt, 0);
> + evt = avnd_evt_create(cb, AVND_EVT_CLC_RESP, 0, 0, 0,
> (void *)clc_evt, 0);
> if (!evt) {
> + free(clc_evt);
> rc = NCSCC_RC_FAILURE;
> goto err;
> }
>
> /* send the event */
> rc = avnd_evt_send(cb, evt);
> - if (NCSCC_RC_SUCCESS != rc)
> + if (NCSCC_RC_SUCCESS != rc) {
> + free(clc_evt);
> goto err;
> -
> + }
> +
> + free(clc_evt);
> return rc;
> } else {
> LOG_ER("Command other than cleanup recvd for ext
> comp: Comp and cmd_type are '%s': %u",\
> @@ -2682,6 +2677,7 @@ uint32_t avnd_comp_clc_cmd_execute(AVND_
> cmd_info.i_timeout_in_ms = (uint32_t)((clc_info->cmds[cmd_type -
> 1].timeout) / 1000000);
> cmd_info.i_cb = avnd_comp_clc_resp;
> cmd_info.i_set_env_args = &arg;
> + cmd_info.i_usr_hdl = (NCS_EXEC_USR_HDL) clc_evt;
>
> TRACE_1("CLC CLI script:'%s'",cmd_info.i_script);
> for(count=1;count<argc;count++)
> @@ -2707,29 +2703,32 @@ uint32_t avnd_comp_clc_cmd_execute(AVND_
> if (NCSCC_RC_SUCCESS != rc) {
> TRACE_2("The CLC CLI command execution failed");
> /* generate a cmd failure event; it'll be executed
> asynchronously */
> - memset(&clc_evt, 0, sizeof(AVND_CLC_EVT));
>
> /* fill the clc-evt param */
> - memcpy(&clc_evt.comp_name, &comp->name, sizeof(SaNameT));
> - clc_evt.cmd_type = cmd_type;
> + memcpy(&clc_evt->comp_name, &comp->name, sizeof(SaNameT));
> + clc_evt->cmd_type = cmd_type;
remove 3 above lines, redundant
>
> /* create the event */
> - evt = avnd_evt_create(cb, AVND_EVT_CLC_RESP, 0, 0, 0, (void
> *)&clc_evt, 0);
> + evt = avnd_evt_create(cb, AVND_EVT_CLC_RESP, 0, 0, 0, (void
> *)clc_evt, 0);
void * cast can be removed
> if (!evt) {
> + free(clc_evt);
> rc = NCSCC_RC_FAILURE;
> goto err;
> }
>
> /* send the event */
> rc = avnd_evt_send(cb, evt);
> - if (NCSCC_RC_SUCCESS != rc)
> + if (NCSCC_RC_SUCCESS != rc) {
> + free(clc_evt);
> goto err;
> + }
> + free(clc_evt);
> } else {
> TRACE_2("The CLC CLI command execution success");
> if (cmd_type == AVND_COMP_CLC_CMD_TYPE_AMSTART || cmd_type ==
> AVND_COMP_CLC_CMD_TYPE_AMSTOP)
> - clc_info->am_cmd_exec_ctxt = cmd_info.o_exec_hdl;
> + clc_info->am_cmd_exec_ctxt =
> (NCS_EXEC_HDL)cmd_info.i_usr_hdl;
> else {
> - clc_info->cmd_exec_ctxt = cmd_info.o_exec_hdl;
> + clc_info->cmd_exec_ctxt =
> (NCS_EXEC_HDL)cmd_info.i_usr_hdl;
> m_AVND_SEND_CKPT_UPDT_ASYNC_UPDT(cb, comp,
> AVND_CKPT_COMP_CMD_EXEC_CTXT);
> }
> }
>
> ------------------------------------------------------------------------------
> Get your SQL database under version control now!
> Version control is standard for application code, but databases havent
> caught up. So what steps can you take to put your SQL databases under
> version control? Why should you start doing it? Read more to find out.
> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
> _______________________________________________
> Opensaf-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead.
Download for free and get started troubleshooting in minutes.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
