Not sure about changes in avnd_evt_clc_resp_evh() since amcmd flow may get impacted other changes will be incorporated.
Please suggest. Thanks, Praveen On 07-Aug-13 2:51 PM, Hans Feldt wrote: > You could apply the attached patch on top of this one (merged to one before > push). > Thanks, > Hans > ________________________________________ > From: Hans Feldt [[email protected]] > Sent: Wednesday, 07 August 2013 9:58 AM > To: praveen malviya > Cc: Hans Feldt; [email protected]; Hans Nordebäck; Mathivanan Naickan > Palanivelu; [email protected] > Subject: Re: [devel] [PATCH 1 of 1] amfnd: correlate clc scripts response > event based on comp name [#514] > > 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
