I think this code is more complex than needed. If cmd_type is always setup
properly I don't think any additional state (amcmd) is needed.
This is what we have at the top of avnd_evt_clc_resp_evh() after your patch:
if (clc_evt->exec_ctxt) {
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) */
comp = m_AVND_COMPDB_REC_GET(cb->compdb, clc_evt->comp_name);
clc_evt->exec_ctxt is always 0 (never written to after calloc, used to hold the
old ptr) thus this can be transformed into:
comp = m_AVND_COMPDB_REC_GET(cb->compdb, clc_evt->comp_name);
and amcmd is not needed. cmd_type is used instead. We cannot keep code around
that does not make sense...
Thanks,
Hans
________________________________________
From: praveen malviya [[email protected]]
Sent: Wednesday, 07 August 2013 12:23 PM
To: Hans Feldt
Cc: [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]
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
