Hi Gary,

Yes it was wrong before refactoring,  I'll correct this before pushing./Thanks 
HansN

-----Original Message-----
From: Gary Lee [mailto:gary....@dektech.com.au] 
Sent: den 15 september 2015 05:43
To: Hans Nordebäck; praveen.malv...@oracle.com; nagendr...@oracle.com
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 1 of 1] amfd: Range based for loop not correct in 
map_su_to_node [#1478]

Hi Hans

Please refer to one comment marked with [GL].

On 14/09/15 22:23, Hans Nordeback wrote:
>   osaf/services/saf/amf/amfd/ckpt_dec.cc |   4 +++-
>   osaf/services/saf/amf/amfd/node.cc     |   2 +-
>   osaf/services/saf/amf/amfd/sgproc.cc   |   4 +++-
>   osaf/services/saf/amf/amfd/su.cc       |  20 ++++++++++----------
>   4 files changed, 17 insertions(+), 13 deletions(-)
>
>
> diff --git a/osaf/services/saf/amf/amfd/ckpt_dec.cc 
> b/osaf/services/saf/amf/amfd/ckpt_dec.cc
> --- a/osaf/services/saf/amf/amfd/ckpt_dec.cc
> +++ b/osaf/services/saf/amf/amfd/ckpt_dec.cc
> @@ -3043,9 +3043,11 @@ static uint32_t dec_ng_admin_state(AVD_C
>               AVD_AVND *node = avd_node_get(*iter);
>               AVD_SU *su = NULL;
>               //If this node has any susi on it.
> -             for (const auto& su : node->list_of_su)
> +             for (const auto& tmp : node->list_of_su) {
> +                     su = tmp;
>                       if (su->list_of_susi != NULL)
>                               break;
> +             }
>               if ((ng->saAmfNGAdminState == SA_AMF_ADMIN_SHUTTING_DOWN) && 
> (su != NULL))
>                       /* Still some assignments are not removed on the node.
>                          if ng in SHUTTING_DOWN state and contoller role 
> changes then 
> diff --git a/osaf/services/saf/amf/amfd/node.cc 
> b/osaf/services/saf/amf/amfd/node.cc
> --- a/osaf/services/saf/amf/amfd/node.cc
> +++ b/osaf/services/saf/amf/amfd/node.cc
> @@ -88,7 +88,7 @@ bool AVD_AVND::is_node_lock() {
>         for (curr_susi = su->list_of_susi;
>              (curr_susi) && ((SA_AMF_HA_QUIESCING != curr_susi->state) ||
>                              ((AVD_SU_SI_STATE_UNASGN == curr_susi->fsm)));
> -           curr_susi = curr_susi->su_next); \
> +           curr_susi = curr_susi->su_next);
>         if (curr_susi)
>           return false;
>       }
> diff --git a/osaf/services/saf/amf/amfd/sgproc.cc 
> b/osaf/services/saf/amf/amfd/sgproc.cc
> --- a/osaf/services/saf/amf/amfd/sgproc.cc
> +++ b/osaf/services/saf/amf/amfd/sgproc.cc
> @@ -712,7 +712,9 @@ void avd_su_oper_state_evh(AVD_CL_CB *cb
>                                */
>                               avd_node_oper_state_set(node, 
> SA_AMF_OPERATIONAL_DISABLED);
>                               node->recvr_fail_sw = true;
> -                             for (const auto& su : node->list_of_su) {
> +                             // TODO (uabhano) is the below code correct?
> +                             for (const auto& i_su : node->list_of_su) {
> +                                     TRACE("Is this for loop correct with 
> regard to i_su and su 
> +below? %s", i_su->name.value);
[GL] As you suspect, looks like below should be i_su->set_readiness_state? 
Looks like it was wrong even before refactoring.
>                                       
> su->set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE);
>                               }
>                       }       /* if 
> (n2d_msg->msg_info.n2d_opr_state.node_oper_state == 
> SA_AMF_OPERATIONAL_DISABLED) */
> diff --git a/osaf/services/saf/amf/amfd/su.cc 
> b/osaf/services/saf/amf/amfd/su.cc
> --- a/osaf/services/saf/amf/amfd/su.cc
> +++ b/osaf/services/saf/amf/amfd/su.cc
> @@ -489,6 +489,8 @@ static AVD_AVND *map_su_to_node(AVD_SU *
>       AVD_SU *su_temp = NULL;
>       AVD_AVND *node = NULL;
>       std::set<std::string>::const_iterator node_iter;
> +     std::vector<AVD_SU*>::const_iterator su_iter;
> +     std::vector<AVD_SU*> *su_list = nullptr;
>   
>       TRACE_ENTER2("'%s'", su->name.value);
>   
> @@ -517,20 +519,18 @@ static AVD_AVND *map_su_to_node(AVD_SU *
>               osafassert(node);
>   
>               if (su->sg_of_su->sg_ncs_spec == true) {
> -                     for (const auto& su_temp : node->list_of_ncs_su) {
> -                             if (su_temp->sg_of_su == su->sg_of_su)
> -                                     break;
> -                     }
> +                     su_list = &node->list_of_ncs_su;
> +             } else {
> +                     su_list = &node->list_of_su;
>               }
>   
> -             if (su->sg_of_su->sg_ncs_spec == false) {
> -                     for (const auto& su_temp : node->list_of_su) {
> -                             if (su_temp->sg_of_su == su->sg_of_su)
> -                                     break;
> -                     }
> +             for (su_iter = su_list->begin(); su_iter != su_list->end(); 
> ++su_iter) {
> +                     su_temp = *su_iter;
> +                     if (su_temp->sg_of_su == su->sg_of_su)
> +                             break;
>               }
>   
> -             if (su_temp == NULL)
> +             if (su_iter == su_list->end())
>                       goto done;
>       }
>   


------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Reply via email to