Hi Nagu, good, yes it is an ack.
/Thanks Hans On 09/14/2015 08:41 AM, Nagendra Kumar wrote: > Hi Hans N, > Thanks for your review, I will add it in function() and commit. > Is that Ack? > > Thanks > -Nagu > >> -----Original Message----- >> From: Hans Nordebäck [mailto:hans.nordeb...@ericsson.com] >> Sent: 26 August 2015 16:56 >> To: Nagendra Kumar; Praveen Malviya; minh.c...@dektech.com.au; >> gary....@dektech.com.au >> Cc: opensaf-devel@lists.sourceforge.net; hans Nordebäck >> Subject: Re: [PATCH 1 of 1] amfd: correct logic in si dep flow [#276] >> >> Hi, >> >> instead of the complex if statements you can add member functions to >> AVD_SI, >> example, if (dep_si->is_tol_timer_running() etc. >> >> /Thanks HansN >> >> On 08/26/2015 09:05 AM, Nagendra Kumar wrote: >>> Hi Praveen, >>> Thanks for your comment. >>> I think we can have two approaches: >>> 1. Remove the code itself as suggested by you. OR >>> 2. Correct the logic in the intended way. For example, please check >> below correction: >>>> - if((dep_si->si_dep_state != >> AVD_SI_TOL_TIMER_RUNNING) || >>>> - (dep_si->si_dep_state != >> AVD_SI_READY_TO_UNASSIGN)) { >>>> + /* Don't start tol timer if dep state are either in >> running or unassigned. */ >>>> + if(!((dep_si->si_dep_state == >> AVD_SI_TOL_TIMER_RUNNING) || >>>> + (dep_si->si_dep_state == >> AVD_SI_READY_TO_UNASSIGN))) { >> avd_sidep_start_tolerance_timer_for_dependant(dep_si, si); >>> The intention looks that if si_dep_state is in either tol_running or >> ready_to_unassigned, then don't start the timer, else start the timer. >>> So, I corrected the logic in the intended way. Now, the code will have the >> same sense as thought while coding. >>> And if the logic written is wrong in the first place and since the check was >> always passing, now after >>> correction of code, the test case may fail. But then we can correct the >>> fault >> in si dep flow and add logic on top. >>> Let me know everybody opinion. >>> >>> Thanks >>> -Nagu >>> >>>> -----Original Message----- >>>> From: praveen malviya >>>> Sent: 26 August 2015 12:15 >>>> To: Nagendra Kumar; hans.nordeb...@ericsson.com; >>>> minh.c...@dektech.com.au; gary....@dektech.com.au >>>> Cc: opensaf-devel@lists.sourceforge.net >>>> Subject: Re: [PATCH 1 of 1] amfd: correct logic in si dep flow [#276] >>>> >>>> Code inside the if block was always executing, which means if condition is >> not >>>> needed. >>>> Why can't simply remove the if condition. >>>> >>>> Thanks, >>>> Praveen >>>> On 26-Aug-15 11:51 AM, nagendr...@oracle.com wrote: >>>>> osaf/services/saf/amf/amfd/si_dep.cc | 25 ++++++++++++------------- >>>>> 1 files changed, 12 insertions(+), 13 deletions(-) >>>>> >>>>> >>>>> At many places, there has been tautological errors in si dep flow. >>>>> The fix corrects them >>>>> >>>>> diff --git a/osaf/services/saf/amf/amfd/si_dep.cc >>>>> b/osaf/services/saf/amf/amfd/si_dep.cc >>>>> --- a/osaf/services/saf/amf/amfd/si_dep.cc >>>>> +++ b/osaf/services/saf/amf/amfd/si_dep.cc >>>>> @@ -1468,10 +1468,10 @@ void avd_sidep_unassign_dependents(AVD_S >>>>> if (m_NCS_MDS_DEST_EQUAL(&sisu->su->su_on_node- >>>>> adest,&su->su_on_node->adest)) { >>>>> avd_si_unassign(dep_si); >>>>> } else { >>>>> - if((dep_si->si_dep_state != >>>> AVD_SI_TOL_TIMER_RUNNING) || >>>>> - (dep_si->si_dep_state != >>>> AVD_SI_READY_TO_UNASSIGN)) { >>>>> + /* Don't start tol timer if dep state are either in >>>> running or unassigned. */ >>>>> + if(!((dep_si->si_dep_state == >>>> AVD_SI_TOL_TIMER_RUNNING) || >>>>> + (dep_si->si_dep_state == >>>> AVD_SI_READY_TO_UNASSIGN))) { >>>> avd_sidep_start_tolerance_timer_for_dependant(dep_si, si); >>>>> - >>>>> } >>>>> } >>>>> /* If this dependent SI is sponsor too, then unassign >>>>> its >>>>> dependents also */ @@ -1788,9 +1788,9 @@ void >>>>> avd_sidep_update_depstate_si_failov >>>>> >>>>> if(su->su_on_node->saAmfNodeOperState == >>>> SA_AMF_OPERATIONAL_DISABLED) { >>>>> if ((m_NCS_MDS_DEST_EQUAL(&sisu- >>>>> su->su_on_node->adest,&su->su_on_node->adest))) { >>>>> - if(((dep_si->si_dep_state != >>>> AVD_SI_TOL_TIMER_RUNNING) || >>>>> - (dep_si->si_dep_state >>>> != AVD_SI_READY_TO_UNASSIGN) || >>>>> - (dep_si->si_dep_state >>>> != AVD_SI_FAILOVER_UNDER_PROGRESS)) && >>>>> + if((!((dep_si->si_dep_state == >>>> AVD_SI_TOL_TIMER_RUNNING) || >>>>> + (dep_si->si_dep_state >>>> == AVD_SI_READY_TO_UNASSIGN) || >>>>> + (dep_si->si_dep_state >>>> == AVD_SI_FAILOVER_UNDER_PROGRESS))) && >>>> (avd_sidep_sponsors_assignment_states(dep_si))) { >>>> avd_sidep_si_dep_state_set(dep_si, >>>>> AVD_SI_FAILOVER_UNDER_PROGRESS); @@ -1801,10 +1801,9 @@ void >>>> avd_sidep_update_depstate_si_failov >>>>> } >>>>> } >>>>> } else if (dep_si->sg_of_si == >>>>> si->sg_of_si) { >>>>> - if((dep_si->si_dep_state != >>>> AVD_SI_TOL_TIMER_RUNNING) || >>>>> - (dep_si->si_dep_state != >>>> AVD_SI_READY_TO_UNASSIGN) || >>>>> - (dep_si->si_dep_state != >>>> AVD_SI_FAILOVER_UNDER_PROGRESS)) { >>>>> - >>>>> + if(!((dep_si->si_dep_state == >>>> AVD_SI_TOL_TIMER_RUNNING) || >>>>> + (dep_si->si_dep_state == >>>> AVD_SI_READY_TO_UNASSIGN) || >>>>> + (dep_si->si_dep_state == >>>> AVD_SI_FAILOVER_UNDER_PROGRESS))) { >>>> avd_sidep_si_dep_state_set(dep_si, >>>> AVD_SI_FAILOVER_UNDER_PROGRESS); >>>>> if >>>>> (dep_si->num_dependents >>>>> 0) { >>>>> /* This SI also >>>>> has >>>> dependents under it, update their state >>>>> also */ @@ -1842,9 +1841,9 @@ void >> avd_sidep_update_depstate_si_failov >>>>> } >>>>> if >>>>> (sponsor_assignments_state >>>> == true) { >>>>> - if((dep_si- >>>>> si_dep_state != AVD_SI_TOL_TIMER_RUNNING) || >>>>> - (dep_si- >>>>> si_dep_state != AVD_SI_READY_TO_UNASSIGN) || >>>>> - (dep_si- >>>>> si_dep_state != AVD_SI_FAILOVER_UNDER_PROGRESS)) { >>>>> + if(!((dep_si- >>>>> si_dep_state == AVD_SI_TOL_TIMER_RUNNING) || >>>>> + (dep_si- >>>>> si_dep_state == AVD_SI_READY_TO_UNASSIGN) || >>>>> + (dep_si- >>>>> si_dep_state == AVD_SI_FAILOVER_UNDER_PROGRESS))) >>>>> +{ >>>>> >>>>> >>>> avd_sidep_si_dep_state_set(dep_si, >>>> AVD_SI_FAILOVER_UNDER_PROGRESS); >>>>> if >>>>> (dep_si- >>>>> num_dependents > 0) { >>>>> ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel