Do what you think is best. This was minor comment on white space. But personally I don't think that we should break lines like in:
>>>>> + (su_ptr->saAmfSUPresenceState == >>>>> + SA_AMF_PRESENCE_INSTANTIATED) && that makes the logic less understandable. We should address the reason why such line is too long with refactoring. Thanks, Hans On 01/24/2014 11:18 AM, Nagendra Kumar wrote: > So, you want to do revert this change and this knid of change from other > places except the comments ? > > -Nagu > >> -----Original Message----- >> From: Hans Feldt [mailto:hans.fe...@ericsson.com] >> Sent: 24 January 2014 14:27 >> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya; >> Mathivanan Naickan Palanivelu; Hrishikesh Chenna >> Cc: opensaf-devel@lists.sourceforge.net >> Subject: Re: [PATCH 1 of 1] amf: support for automatic comp instantiation and >> deletion [#597] >> >> >> >> On 01/24/2014 05:06 AM, Nagendra Kumar wrote: >>>>> + (su_ptr->saAmfSUPresenceState == >>>>> + SA_AMF_PRESENCE_INSTANTIATED) && >>>> >>>> keep above expression on same line >>> >>> But the line is exceeding 80 lines when the line break is kept in the same >>> line. >> >> That should not be the case with proper code structure and naming. I think >> the >> above is an exemption to the rule because it makes the code very hard to read >> and understand if you break lines like that. >> /Hans >> >>> >>> Thanks >>> -Nagu >>> >>>> -----Original Message----- >>>> From: Hans Feldt [mailto:hans.fe...@ericsson.com] >>>> Sent: 23 January 2014 19:23 >>>> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya; >>>> Mathivanan Naickan Palanivelu; Hrishikesh Chenna >>>> Cc: opensaf-devel@lists.sourceforge.net >>>> Subject: Re: [PATCH 1 of 1] amf: support for automatic comp >>>> instantiation and deletion [#597] >>>> >>>> Ack with minor comments inline. >>>> Thanks, >>>> Hans >>>> >>>> >>>> On 01/23/2014 07:20 AM, nagendr...@oracle.com wrote: >>>>> osaf/services/saf/amf/amfd/comp.cc | 37 >>>>> ++++++++++++++++--- >>>>> osaf/services/saf/amf/amfd/ndproc.cc | 10 +++- >>>>> osaf/services/saf/amf/amfnd/clc.cc | 10 +++++ >>>>> osaf/services/saf/amf/amfnd/compdb.cc | 47 >>>> +++++++++++++++++++++--- >>>>> osaf/services/saf/amf/amfnd/include/avnd_comp.h | 2 + >>>>> osaf/services/saf/amf/amfnd/su.cc | 27 ++++++++++++- >>>>> 6 files changed, 115 insertions(+), 18 deletions(-) >>>>> >>>>> >>>>> When any component is added in the instantiated SU, Amf will try to >>>> instantiate the component. >>>>> When any component is deleted from instantiated SU, Amf will try to >>>> terminate the component. >>>>> >>>>> diff --git a/osaf/services/saf/amf/amfd/comp.cc >>>> b/osaf/services/saf/amf/amfd/comp.cc >>>>> --- a/osaf/services/saf/amf/amfd/comp.cc >>>>> +++ b/osaf/services/saf/amf/amfd/comp.cc >>>>> @@ -173,17 +173,18 @@ void avd_comp_delete(AVD_COMP *comp) >>>>> static void comp_add_to_model(AVD_COMP *comp) >>>>> { >>>>> SaNameT dn; >>>>> + AVD_SU *su_ptr = comp->su; >>>> >>>> better with just "su" >>>> >>>>> >>>>> TRACE_ENTER2("%s", comp->comp_info.name.value); >>>>> >>>>> /* Check parent link to see if it has been added already */ >>>>> - if (comp->su != NULL) { >>>>> + if (su_ptr != NULL) { >>>>> TRACE("already added"); >>>>> goto done; >>>>> } >>>>> >>>>> avsv_sanamet_init(&comp->comp_info.name, &dn, "safSu"); >>>>> - comp->su = avd_su_get(&dn); >>>>> + su_ptr = avd_su_get(&dn); >>>>> >>>>> avd_comp_db_add(comp); >>>>> comp->comp_type = avd_comptype_get(&comp->saAmfCompType); >>>>> @@ -195,7 +196,7 @@ static void comp_add_to_model(AVD_COMP * >>>>> * corresponding node is UP send the component information >>>>> * to the Node. >>>>> */ >>>>> - if (comp->su->su_is_external) { >>>>> + if (su_ptr->su_is_external) { >>>>> /* This is an external SU, so there is no node assigned >>>>> to it. >>>>> For some purpose of validations and sending SU/Comps >>>>> info >>>> to >>>>> hosting node (Controllers), we can take use of the >>>>> hosting >>>>> @@ -219,12 +220,36 @@ static void comp_add_to_model(AVD_COMP * >>>>> if ((comp->comp_info.category == AVSV_COMP_TYPE_SA_AWARE) || >>>>> (comp->comp_info.category == >>>> AVSV_COMP_TYPE_PROXIED_LOCAL_PRE_INSTANTIABLE) || >>>>> (comp->comp_info.category == >>>> AVSV_COMP_TYPE_EXTERNAL_PRE_INSTANTIABLE)) { >>>>> - comp->su->saAmfSUPreInstantiable = >>>> static_cast<SaBoolT>(true); >>>>> + su_ptr->saAmfSUPreInstantiable = static_cast<SaBoolT>(true); >>>>> + } >>>>> + >>>>> + /* This is a case of adding a component in SU which is instantiated >>>>> + state. This could be used in upgrade scenarios. When components >>>>> + are added, it is sent to Amfnd for instantiation and Amfnd >>>>> + instantiates it. Amfnd has capability for finding which component >>>>> + has been added newly. */ >>>>> + if ((su_ptr->saAmfSUAdminState != >>>> SA_AMF_ADMIN_LOCKED_INSTANTIATION) && >>>>> + (su_ptr->saAmfSUPresenceState == >>>>> + SA_AMF_PRESENCE_INSTANTIATED) && >>>> >>>> keep above expression on same line >>>> >>>>> + (avd_cb->avail_state_avd == SA_AMF_HA_ACTIVE)) { >>>>> + AVD_AVND *node = su_ptr->su_on_node; >>>>> + if ((node->node_state == AVD_AVND_STATE_PRESENT) || >>>>> + (node->node_state == >>>>> + AVD_AVND_STATE_NO_CONFIG) || >>>> >>>> keep above expression on same line >>>> >>>>> + (node->node_state == >>>> AVD_AVND_STATE_NCS_INIT)) { >>>>> + if (avd_snd_su_msg(avd_cb, su_ptr) != >>>>> + NCSCC_RC_SUCCESS) { >>>>> + LOG_ER("%s: avd_snd_su_msg failed %s", >>>>> + __FUNCTION__, >>>>> + su_ptr->name.value); >>>> >>>> log to whom you failed to send and comp DN, one day you will need it... >>>> >>>>> + goto done; >>>>> + } >>>>> + } >>>>> } >>>>> >>>>> /* Set runtime cached attributes. */ >>>>> - avd_saImmOiRtObjectUpdate(&comp->su->name, >>>> const_cast<SaImmAttrNameT>("saAmfSUPreInstantiable"), >>>>> - SA_IMM_ATTR_SAUINT32T, &comp->su- >>>>> saAmfSUPreInstantiable); >>>>> + avd_saImmOiRtObjectUpdate(&su_ptr->name, >>>> const_cast<SaImmAttrNameT>("saAmfSUPreInstantiable"), >>>>> + SA_IMM_ATTR_SAUINT32T, &su_ptr- >>>>> saAmfSUPreInstantiable); >>>>> >>>>> avd_saImmOiRtObjectUpdate(&comp->comp_info.name, >>>> const_cast<SaImmAttrNameT>("saAmfCompReadinessState"), >>>>> SA_IMM_ATTR_SAUINT32T, &comp- >>>>> saAmfCompReadinessState); >>>>> diff --git a/osaf/services/saf/amf/amfd/ndproc.cc >>>> b/osaf/services/saf/amf/amfd/ndproc.cc >>>>> --- a/osaf/services/saf/amf/amfd/ndproc.cc >>>>> +++ b/osaf/services/saf/amf/amfd/ndproc.cc >>>>> @@ -608,9 +608,13 @@ void avd_data_update_req_evh(AVD_CL_CB * >>>>> case AVSV_SA_AMF_COMP:{ >>>>> /* Find the component record in the database, >>>> specified in the message. */ >>>>> if ((comp = avd_comp_get(&n2d_msg- >>>>> msg_info.n2d_data_req.param_info.name)) == NULL) { >>>>> - LOG_ER("%s: Invalid Comp '%s' (%u)", >>>> __FUNCTION__, >>>>> - n2d_msg- >>>>> msg_info.n2d_data_req.param_info.name.value, >>>>> - n2d_msg- >>>>> msg_info.n2d_data_req.param_info.name.length); >>>>> + /* In case of component delete, component >>>> gets >>>>> + deleted at Amfd first and then it gets >>>>> + uninstantiated and then deleted at Amfnd. >>>>> + So, when update for presence state comes >>>> from >>>>> + Amfnd, the component doesn't exists in >>>> Amfd*/ >>>>> + LOG_IN("%s: Invalid Comp '%s'", >>>> __FUNCTION__, >>>>> + n2d_msg- >>>>> msg_info.n2d_data_req.param_info.name.value); >>>>> goto done; >>>>> } >>>>> >>>>> diff --git a/osaf/services/saf/amf/amfnd/clc.cc >>>> b/osaf/services/saf/amf/amfnd/clc.cc >>>>> --- a/osaf/services/saf/amf/amfnd/clc.cc >>>>> +++ b/osaf/services/saf/amf/amfnd/clc.cc >>>>> @@ -1306,6 +1306,16 @@ uint32_t avnd_comp_clc_st_chng_prc(AVND_ >>>>> } >>>>> } >>>>> >>>>> + /* This is a case of 'component delete' when su is in intantiated >>>>> + state. Component can go into uninstantiated state or termination >>>>> + failed state, but the component information needs to be deleted from >>>>> + data base. */ >>>>> + if ((comp->pending_delete == true) && >>>>> + ((comp->pres == >>>> SA_AMF_PRESENCE_UNINSTANTIATED) || >>>>> + (comp->pres == >>>> SA_AMF_PRESENCE_TERMINATION_FAILED)) && >>>>> + (comp->su->pres == >>>> SA_AMF_PRESENCE_INSTANTIATED)) >>>>> + rc = avnd_compdb_rec_del(cb, &comp->name); >>>>> + >>>>> done: >>>>> TRACE_LEAVE2("%u", rc); >>>>> return rc; >>>>> diff --git a/osaf/services/saf/amf/amfnd/compdb.cc >>>> b/osaf/services/saf/amf/amfnd/compdb.cc >>>>> --- a/osaf/services/saf/amf/amfnd/compdb.cc >>>>> +++ b/osaf/services/saf/amf/amfnd/compdb.cc >>>>> @@ -837,15 +837,50 @@ uint32_t avnd_comp_oper_req(AVND_CB *cb, >>>>> >>>>> case AVSV_OBJ_OPR_DEL: >>>>> { >>>> >>>> the above opening brace is on the wrong line thus causing unnecessary >>>> indentation and hard top read code, please change or remove (if >>>> possible) it >>>> >>>>> - AVND_COMP *comp = 0; >>>>> + /* This request comes when any component is being >>>>> + deleted. When this request comes from Amfd, >>>>> + component can be in instantiated or uninstantiated >>>>> + state and in that case, Amfnd will uninstantiate the >>>>> + component and delete the record from its data base. >>>>> + This is kind of automatic termination of a component >>>>> + when comp is getting deleted. */ >>>>> + AVND_COMP *comp = >>>>> + m_AVND_COMPDB_REC_GET(cb->compdb, >>>> param->name); >>>>> + if (comp == NULL) { >>>>> + LOG_ER("%s: Comp '%s' not found", >>>> __FUNCTION__, >>>>> + param->name.value); >>>>> + goto done; >>>>> + } >>>>> >>>>> - /* get the record */ >>>>> - comp = m_AVND_COMPDB_REC_GET(cb->compdb, >>>> param->name); >>>>> - if (comp) { >>>>> - /* delete the record */ >>>>> - >>>> m_AVND_SEND_CKPT_UPDT_ASYNC_RMV(cb, comp, >> AVND_CKPT_COMP_CONFIG); >>>>> + if ((comp->pres == >>>> SA_AMF_PRESENCE_UNINSTANTIATED) && >>>>> + (comp->su->pres == >>>>> + >>>> SA_AMF_PRESENCE_UNINSTANTIATED)) { >>>> >>>> no line break above >>>> >>>>> + >>>> m_AVND_SEND_CKPT_UPDT_ASYNC_RMV(cb, comp, >>>>> + >>>> AVND_CKPT_COMP_CONFIG); >>>>> rc = avnd_compdb_rec_del(cb, ¶m- >> name); >>>>> + goto done; >>>>> } >>>>> + >>>>> + /* Terminate the pi comp. It will terminate the >>>>> + component and delete the comp record. */ >>>>> + if >>>> (m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(comp)) { >>>>> + comp->pending_delete = true; >>>> >>>> isn't this setting common to both types of components? >>>> >>>>> + TRACE("Running the component clc FSM, >>>> terminate" >>>>> + " the component"); >>>> >>>> th above trace is not needed, remove >>>> >>>>> + rc = avnd_comp_clc_fsm_run(cb, comp, >>>>> + >>>> AVND_COMP_CLC_PRES_FSM_EV_TERM); >>>>> + goto done; >>>>> + } >>>>> + /* Terminate the Npi comp. After deleting the csi, comp >>>>> + will be already terminated, so we just want to delete >>>>> + the comp record.*/ >>>>> + if >>>> (!m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(comp)) { >>>>> + >>>> m_AVND_SEND_CKPT_UPDT_ASYNC_RMV(cb, comp, >>>>> + >>>> AVND_CKPT_COMP_CONFIG); >>>>> + rc = avnd_compdb_rec_del(cb, ¶m- >>>>> name); >>>>> + goto done; >>>>> + } >>>>> + >>>>> } >>>>> break; >>>>> >>>>> diff --git a/osaf/services/saf/amf/amfnd/include/avnd_comp.h >>>> b/osaf/services/saf/amf/amfnd/include/avnd_comp.h >>>>> --- a/osaf/services/saf/amf/amfnd/include/avnd_comp.h >>>>> +++ b/osaf/services/saf/amf/amfnd/include/avnd_comp.h >>>>> @@ -359,6 +359,8 @@ typedef struct avnd_comp_tag { >>>>> SaBoolT admin_oper; /*set to true if undergoing admin >>>>> operation */ >>>>> int config_is_valid; /* Used to indicate that config has to be >>>>> refreshed >>>> from IMM */ >>>>> bool assigned_flag; /* Used in finding multiple csi for a single >>>>> comp >>>> while csi mod.*/ >>>>> + bool pending_delete; /* Used in deleting component when su is in >>>>> + instantiated state.*/ >>>>> >>>>> } AVND_COMP; >>>>> >>>>> diff --git a/osaf/services/saf/amf/amfnd/su.cc >>>> b/osaf/services/saf/amf/amfnd/su.cc >>>>> --- a/osaf/services/saf/amf/amfnd/su.cc >>>>> +++ b/osaf/services/saf/amf/amfnd/su.cc >>>>> @@ -79,9 +79,13 @@ uint32_t avnd_evt_avd_reg_su_evh(AVND_CB >>>>> >>>>> /* scan the su list & add each su to su-db */ >>>>> for (su_info = info->su_list; su_info; su = 0, su_info = >>>>> su_info->next) { >>>>> - su = avnd_sudb_rec_add(cb, su_info, &rc); >>>>> - if (!su) >>>>> - break; >>>>> + su = m_AVND_SUDB_REC_GET(cb->sudb, su_info->name); >>>>> + /* This function is common >>>>> + 1. for adding new SU in the data base >>>>> + 2. for adding a new componet in the existing su. >>>>> + So, we need to check wether the SU exists or not. */ >>>> >>>> two spelling issues above, remove the "we need to" >>>> >>>>> + if (su == NULL) >>>>> + su = avnd_sudb_rec_add(cb, su_info, &rc); >>>>> >>>>> m_AVND_SEND_CKPT_UPDT_ASYNC_ADD(cb, su, >>>> AVND_CKPT_SU_CONFIG); >>>>> >>>>> @@ -93,11 +97,28 @@ uint32_t avnd_evt_avd_reg_su_evh(AVND_CB >>>>> rc = NCSCC_RC_FAILURE; >>>>> break; >>>>> } >>>>> + >>>>> + /* When NPI comp is added into PI SU(that is >>>> UNINSTANTIATED), >>>>> + we don't have to run SU FSM as anyway, we are not >>>>> + instantiating NPI component. In this case, anyway, SU will >>>>> + remain in instantiated state. NPI comp will get instantiated >>>>> + when corresponding csi is added. */ >>>>> + bool su_is_instantiated; >>>>> + m_AVND_SU_IS_INSTANTIATED(su, su_is_instantiated); >>>>> + >>>>> + if ((su->pres == SA_AMF_PRESENCE_INSTANTIATED) && >>>>> + (su_is_instantiated == false)) { >>>>> + avnd_su_pres_state_set(su, >>>>> + >>>> SA_AMF_PRESENCE_UNINSTANTIATED); >>>>> + rc = avnd_su_pres_fsm_run(cb, su, 0, >>>>> + AVND_SU_PRES_FSM_EV_INST); >>>>> + } >>>>> } >>>>> >>>>> /*** send the response to AvD ***/ >>>>> rc = avnd_di_reg_su_rsp_snd(cb, &info->su_list->name, rc); >>>>> >>>>> + >>>>> done: >>>>> TRACE_LEAVE(); >>>>> return rc; >>>>> >>>>> >>> >>> > > ------------------------------------------------------------------------------ CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
