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
