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:[email protected]]
> Sent: 26 August 2015 16:56
> To: Nagendra Kumar; Praveen Malviya; [email protected];
> [email protected]
> Cc: [email protected]; 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; [email protected];
> >> [email protected]; [email protected]
> >> Cc: [email protected]
> >> 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, [email protected] 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
