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

Reply via email to