Please find comment inlined with [Nagu]. Thanks -Nagu > -----Original Message----- > From: Gary Lee [mailto:[email protected]] > Sent: 30 July 2015 11:01 > To: [email protected]; Praveen Malviya; Nagendra Kumar; > [email protected] > Cc: [email protected] > Subject: [PATCH 11 of 13] amfd: convert list_of_su to std::set [#1142] > > osaf/services/saf/amf/amfd/sg_npm_fsm.cc | 240 ++++++++++++++-------------- > -- > 1 files changed, 116 insertions(+), 124 deletions(-) > > > diff --git a/osaf/services/saf/amf/amfd/sg_npm_fsm.cc > b/osaf/services/saf/amf/amfd/sg_npm_fsm.cc > --- a/osaf/services/saf/amf/amfd/sg_npm_fsm.cc > +++ b/osaf/services/saf/amf/amfd/sg_npm_fsm.cc > @@ -55,24 +55,32 @@ > > static AVD_SU *avd_sg_npm_su_next_asgn(AVD_CL_CB *cb, AVD_SG *sg, > AVD_SU *su, SaAmfHAStateT state) > { > - AVD_SU *i_su; > + std::set<AVD_SU*, AVD_SU::comparator>::iterator iter; > + AVD_SU* next_su = NULL; > + > TRACE_ENTER(); > > if (su == NULL) { > - i_su = sg->list_of_su; > + iter = sg->list_of_su.begin();
[Nagu]: Need to check whether list_of_su is empty before calling begin.
> } else {
> - i_su = su->sg_list_su_next;
> + iter = sg->list_of_su.find(su);
> + if (iter != sg->list_of_su.end()) {
> + // get the SU after 'su'
> + ++iter;
> + }
> }
> -
> - while (i_su != NULL) {
> - if ((i_su->list_of_susi != AVD_SU_SI_REL_NULL) && (i_su-
> >list_of_susi->state == state)) {
> +
> + while (iter != sg->list_of_su.end()) {
> + const AVD_SU* su = *iter;
> + if ((su->list_of_susi != AVD_SU_SI_REL_NULL) && (su-
> >list_of_susi->state == state)) {
> + next_su = *iter;
> break;
> }
>
> - i_su = i_su->sg_list_su_next;
> + ++iter;
> }
>
> - return i_su;
> + return next_su;
> }
>
>
> /****************************************************************
> *************
> @@ -206,7 +214,6 @@
> static AVD_SU* avd_sg_npm_get_least_su(AVD_SG *sg, SaAmfHAStateT
> ha_state)
> {
> AVD_SU *pref_su = NULL;
> - AVD_SU *curr_su = NULL;
> uint32_t curr_act_sus = 0;
> uint32_t curr_std_sus = 0;
>
> @@ -226,15 +233,13 @@
> TRACE("curr_act_sus = %u", curr_act_sus);
>
> if (curr_act_sus < sg->saAmfSGNumPrefActiveSUs) {
> - curr_su = sg->list_of_su;
> - while (curr_su != NULL) {
> + for (const auto& curr_su : sg->list_of_su) {
> if ((curr_su->saAmfSuReadinessState ==
> SA_AMF_READINESS_IN_SERVICE) &&
> (curr_su->list_of_susi ==
> AVD_SU_SI_REL_NULL)) {
> /* got an in-service SU with no SI
> assignments */
> pref_su = curr_su;
> goto done;
> }
> - curr_su = curr_su->sg_list_su_next;
> }
> }
>
> @@ -242,9 +247,10 @@
> * active SI assignment so now loop through the su list of the
> sg
> * to find an SU with least active SI assignments
> */
> -
> - curr_su = sg->list_of_su;
> - while (curr_su != NULL && curr_act_sus > 0) {
> + for (const auto& curr_su : sg->list_of_su) {
> + if (curr_act_sus <= 0)
[Nagu]: curr_act_sus is unsigned int, it can't be less than zero, the check may
fail compilation.
> + break;
> +
> if ((curr_su->list_of_susi != AVD_SU_SI_REL_NULL) &&
> (curr_su->list_of_susi->state == ha_state) &&
> ((curr_su->sg_of_su-
> >saAmfSGMaxActiveSIsperSU == 0) ||
> @@ -252,7 +258,6 @@
> ((!pref_su || pref_su-
> >saAmfSUNumCurrActiveSIs > curr_su->saAmfSUNumCurrActiveSIs))) {
> pref_su = curr_su;
> }
> - curr_su = curr_su->sg_list_su_next;
> }
> }
> else if (SA_AMF_HA_STANDBY == ha_state) {
> @@ -262,22 +267,24 @@
> * return the SU with the least standby SI assignments
> * otherwise, return an in-service SU which has no
> assignments
> */
> - curr_su = sg->list_of_su;
> - while ((curr_su != NULL) && (curr_std_sus < sg-
> >saAmfSGNumPrefStandbySUs)) {
> - if ((curr_su->saAmfSuReadinessState ==
> SA_AMF_READINESS_IN_SERVICE) &&
> - (curr_su->list_of_susi ==
> AVD_SU_SI_REL_NULL)) {
> - /* got an in-service SU with no SI
> assignments */
> - pref_su = curr_su;
> - goto done;
> - }
> - else if ((curr_su->list_of_susi !=
> AVD_SU_SI_REL_NULL) &&
> - (curr_su->list_of_susi->state
> == ha_state)) {
> - /* increment the current active SUs
> count as you found
> - * a SU with atleast one active SI
> assignment
> - */
> - curr_std_sus ++;
> - }
> - curr_su = curr_su->sg_list_su_next;
> + for (const auto& curr_su : sg->list_of_su) {
> + if (curr_std_sus >= sg->saAmfSGNumPrefStandbySUs) {
> + break;
> + }
> +
> + if ((curr_su->saAmfSuReadinessState ==
> SA_AMF_READINESS_IN_SERVICE) &&
> + (curr_su->list_of_susi ==
> AVD_SU_SI_REL_NULL)) {
> + /* got an in-service SU with no SI assignments
> */
> + pref_su = curr_su;
> + goto done;
> + }
> + else if ((curr_su->list_of_susi != AVD_SU_SI_REL_NULL)
> &&
> + (curr_su->list_of_susi->state ==
> ha_state)) {
> + /* increment the current active SUs count as
> you found
> + * a SU with atleast one active SI assignment
> + */
> + curr_std_sus ++;
> + }
> }
>
> TRACE("curr_std_sus = %u", curr_std_sus);
> @@ -287,8 +294,11 @@
> * to find an SU with least standby SI assignments
> */
>
> - curr_su = sg->list_of_su;
> - while (curr_su != NULL && curr_std_sus > 0) {
> + for (const auto& curr_su : sg->list_of_su) {
> + if (curr_std_sus <= 0) {
[Nagu]: curr_std_sus is unsigned int, it can't be less than zero, the check may
fail compilation.
> + break;
> + }
> +
> if ((curr_su->list_of_susi != AVD_SU_SI_REL_NULL) &&
> (curr_su->list_of_susi->state == ha_state) &&
> ((curr_su->sg_of_su-
> >saAmfSGMaxStandbySIsperSU == 0) ||
> @@ -296,7 +306,6 @@
> ((!pref_su || pref_su-
> >saAmfSUNumCurrStandbySIs > curr_su->saAmfSUNumCurrStandbySIs))) {
> pref_su = curr_su;
> }
> - curr_su = curr_su->sg_list_su_next;
> }
>
> }
> @@ -538,20 +547,19 @@
> * in service unassigned SU in
> the SG.
> */
> new_su = true;
> - i_su = sg->list_of_su;
> - while ((i_su != NULL) &&
> (su_found == false)) {
> +
> + for (const auto& iter : sg-
> >list_of_su) {
[Nagu]: Search this if su_found is false.
> + i_su = iter;
> if ((i_su-
> >saAmfSuReadinessState == SA_AMF_READINESS_IN_SERVICE)
> && (i_su-
> >list_of_susi == AVD_SU_SI_REL_NULL)) {
> su_found =
> true;
> cnt++;
> - continue;
> + break;
> }
> -
> - /* choose the next SU
> */
> - i_su = i_su-
> >sg_list_su_next;
> -
> - } /* while (i_su !=
> AVD_SU_NULL) */
> -
> + }
> + if (su_found == false) {
[Nagu]: Use some other variable for this as it is getting modified in the above
if also in the code.
> + i_su = NULL;
> + }
> }
> /* if ((i_su == AVD_SU_NULL) &&
> (cnt < sg->pref_num_active_su)) */
> @@ -561,19 +569,18 @@
> * to be used for the SI. So identify
> the next SU.
> */
>
> - while ((i_su != NULL) && (su_found ==
> false)) {
> + for (const auto& iter : sg->list_of_su)
> {
[Nagu]: Search this if su_found is false.
> + i_su = iter;
> if ((i_su-
> >saAmfSuReadinessState == SA_AMF_READINESS_IN_SERVICE) &&
> (i_su->list_of_susi ==
> AVD_SU_SI_REL_NULL)) {
> su_found = true;
> cnt++;
> - continue;
> + break;
> }
> -
> - /* choose the next SU */
> - i_su = i_su->sg_list_su_next;
> -
> - } /* while (i_su !=
> AVD_SU_NULL) */
> -
> + }
> + if (su_found == false) {
[Nagu]: Use some other variable for this.
> + i_su = NULL;
> + }
> }
> /* else if (cnt < sg->pref_num_active_su) */
> } /* else ((i_su->si_max_active > i_su-
> >si_curr_active) &&
> @@ -610,21 +617,19 @@
> * in service unassigned SU in the SG.
> */
> new_su = true;
> - i_su = sg->list_of_su;
> - while ((i_su != NULL) && (su_found == false)) {
[Nagu]: Search this if su_found is false.
> + for (const auto& iter : sg->list_of_su) {
> + i_su = iter;
> if ((i_su->saAmfSuReadinessState ==
> SA_AMF_READINESS_IN_SERVICE) &&
> (i_su->list_of_susi ==
> AVD_SU_SI_REL_NULL)) {
> su_found = true;
> actv_su_found = true;
> cnt++;
> - continue;
> + break;
> }
> -
> - /* choose the next SU */
> - i_su = i_su->sg_list_su_next;
> -
> - } /* while (i_su != AVD_SU_NULL) */
> -
> + }
> + if (su_found == false) {
[Nagu]: Use some other variable for this.
> + i_su = NULL;
> + }
> }
> /* if ((su_found == false) &&
> (cnt < sg->pref_num_active_su)) */
> @@ -739,18 +744,19 @@
> ((i_su->sg_of_su-
> >su_max_standby == 0) ||
> (l_su->sg_of_su-
> >su_max_standby > l_su->si_curr_standby))) */
> if ((new_su == true) && (cnt < sg-
> >saAmfSGNumPrefStandbySUs)) {
> - i_su = l_su;
> - while ((i_su != NULL) &&
> (su_found == false)) {
> + std::set<AVD_SU*,
> AVD_SU::comparator>::iterator iter = sg->list_of_su.find(l_su);
> +
> + while (iter != sg-
> >list_of_su.end() && su_found == false) {
> + i_su = *iter;
> if ((i_su-
> >saAmfSuReadinessState == SA_AMF_READINESS_IN_SERVICE)
> && (i_su-
> >list_of_susi == AVD_SU_SI_REL_NULL)) {
> su_found =
> true;
> cnt++;
> - continue;
> + break;
> }
>
> /* choose the next SU
> */
> - i_su = i_su-
> >sg_list_su_next;
> -
> + ++iter;
> } /* while (i_su !=
> AVD_SU_NULL) */
>
> l_su = i_su;
> @@ -774,19 +780,22 @@
> }
>
> if ((su_found == false) && (cnt
> < sg->saAmfSGNumPrefStandbySUs)) {
> + std::set<AVD_SU*,
> AVD_SU::comparator>::iterator iter;
> +
> /* All the current
> standby SUs are full. The SG can have
> * more standby
> assignments. identify the highest ranked
> * in service
> unassigned SU in the SG.
> */
> new_su = true;
> - if (n_su == NULL)
> - i_su = sg-
> >list_of_su;
> - else
> - i_su = n_su;
> -
> - while ((i_su != NULL)
> && (su_found == false)) {
> - if ((i_su-
> >saAmfSuReadinessState ==
> -
> SA_AMF_READINESS_IN_SERVICE)
> + if (n_su == NULL) {
> + iter = sg-
> >list_of_su.begin();
> + } else {
> + iter = sg-
> >list_of_su.find(n_su);
> + }
> +
> + while ((iter != sg-
> >list_of_su.end()) && (su_found == false)) {
[Nagu]: Use the same logic as above, to break rather than continue and marking
su_found as false or follow the reverse.
Following the older way/reverse is also fine as we are not breaking any
existing logic.
> + i_su = *iter;
> + if ((i_su-
> >saAmfSuReadinessState == SA_AMF_READINESS_IN_SERVICE)
> && (i_su-
> >list_of_susi == AVD_SU_SI_REL_NULL)) {
>
> su_found = true;
> cnt++;
> @@ -794,12 +803,12 @@
> }
>
> /* choose the
> next SU */
> - i_su = i_su-
> >sg_list_su_next;
> -
> + ++iter;
> } /* while (i_su
> != AVD_SU_NULL) */
> -
> + if (su_found == false) {
> + i_su = NULL;
> + }
> n_su = i_su;
> -
> }
> /* if ((su_found == false) &&
> (cnt < sg-
> >pre_num_standby_su)) */
> @@ -831,16 +840,19 @@
> }
>
> if ((su_found == false) && (cnt < sg-
> >saAmfSGNumPrefStandbySUs)) {
> + std::set<AVD_SU*,
> AVD_SU::comparator>::iterator iter;
> +
> /* All the current standby SUs are full.
> The SG can have
> * more standby assignments. identify
> the highest ranked
> * in service unassigned SU in the SG.
> */
> new_su = true;
> if (n_su == NULL)
> - i_su = sg->list_of_su;
> + iter = sg->list_of_su.begin();
[Nagu]: Check for empty.
> else
> - i_su = n_su;
> - while ((i_su != NULL) && (su_found ==
> false)) {
> + iter = sg-
> >list_of_su.find(n_su);
> + while ((iter != sg->list_of_su.end()) &&
> (su_found == false)) {
[Nagu]: Same as above, either stick to older way or newer way. This is older
way to exit the loop and I think this is ok.
> + i_su = *iter;
> if ((i_su-
> >saAmfSuReadinessState == SA_AMF_READINESS_IN_SERVICE) &&
> (i_su->list_of_susi ==
> AVD_SU_SI_REL_NULL)) {
> su_found = true;
> @@ -849,10 +861,11 @@
> }
>
> /* choose the next SU */
> - i_su = i_su->sg_list_su_next;
> -
> + ++iter;
> } /* while (i_su !=
> AVD_SU_NULL) */
> -
> + if (su_found == false) {
[Nagu]: Don't use this variable for this logic.
> + i_su = NULL;
> + }
> n_su = i_su;
>
> }
> @@ -2067,16 +2080,14 @@
> avd_sg->min_assigned_su = NULL;
> avd_sg->si_tobe_redistributed = NULL;
>
> - i_su = avd_sg->list_of_su;
> /* Screen Active SUs */
> - while (i_su != NULL) {
> + for (const auto& iter : avd_sg->list_of_su) {
> + i_su = iter;
> if ((i_su->saAmfSuReadinessState !=
> SA_AMF_READINESS_IN_SERVICE) ||
> (NULL == i_su->list_of_susi)) {
> - i_su = i_su->sg_list_su_next;
> continue;
> }
> if(i_su->list_of_susi->state != SA_AMF_HA_ACTIVE ) {
> - i_su = i_su->sg_list_su_next;
> continue;
> }
> assigned_prefferd_SUs++;
> @@ -2090,20 +2101,17 @@
>
> if (i_su->saAmfSUNumCurrActiveSIs < avd_sg->min_assigned_su-
> >saAmfSUNumCurrActiveSIs)
> avd_sg->min_assigned_su = i_su;
> -
> - i_su = i_su->sg_list_su_next;
> } /* while (i_su != NULL) */
>
> if (NULL != avd_sg->max_assigned_su) {
> if(avd_sg->saAmfSGNumPrefActiveSUs >
> assigned_prefferd_SUs) {
> - i_su = avd_sg->list_of_su;
> - while (i_su != NULL) {
> - if ((i_su->saAmfSuReadinessState ==
> SA_AMF_READINESS_IN_SERVICE) &&
> + for (const auto& iter : avd_sg->list_of_su) {
> + i_su = iter;
> + if ((i_su->saAmfSuReadinessState ==
> SA_AMF_READINESS_IN_SERVICE) &&
> (NULL == i_su->list_of_susi)) {
> - avd_sg->min_assigned_su = i_su;
> - break;
> - }
> - i_su = i_su->sg_list_su_next;
> + avd_sg->min_assigned_su = i_su;
> + break;
> + }
> }
> }
> } else {
> @@ -2122,15 +2130,13 @@
> assigned_prefferd_SUs = 0;
> m_AVSV_SEND_CKPT_UPDT_ASYNC_RMV(avd_cb, avd_sg,
> AVSV_CKPT_AVD_SI_TRANS);
>
> - i_su = avd_sg->list_of_su;
> - while (i_su != NULL) {
> + for (const auto& iter : avd_sg->list_of_su) {
> + i_su = iter;
> if ((i_su->saAmfSuReadinessState !=
> SA_AMF_READINESS_IN_SERVICE) ||
> (NULL == i_su-
> >list_of_susi)) {
> - i_su = i_su->sg_list_su_next;
> continue;
> }
> if(i_su->list_of_susi->state != SA_AMF_HA_STANDBY )
> {
> - i_su = i_su->sg_list_su_next;
> continue;
> }
> assigned_prefferd_SUs++;
> @@ -2145,19 +2151,16 @@
>
> if (i_su->saAmfSUNumCurrStandbySIs < avd_sg-
> >min_assigned_su->saAmfSUNumCurrStandbySIs)
> avd_sg->min_assigned_su = i_su;
> -
> - i_su = i_su->sg_list_su_next;
> }
> if (NULL != avd_sg->max_assigned_su) {
> if(avd_sg->saAmfSGNumPrefStandbySUs >
> assigned_prefferd_SUs) {
> - i_su = avd_sg->list_of_su;
> - while (i_su != NULL) {
> + for (const auto& iter : avd_sg->list_of_su) {
> + i_su = iter;
> if ((i_su->saAmfSuReadinessState ==
> SA_AMF_READINESS_IN_SERVICE) &&
> (NULL == i_su-
> >list_of_susi)) {
> avd_sg->min_assigned_su = i_su;
> break;
> }
> - i_su = i_su->sg_list_su_next;
> }
> }
> } else {
> @@ -2293,20 +2296,19 @@
> */
> uint32_t avd_sg_get_curr_act_cnt(AVD_SG *sg)
> {
> - AVD_SU *i_su = sg->list_of_su;
> uint32_t curr_pref_active_sus = 0;
> TRACE_ENTER2("SG name:%s ", sg->name.value);
>
> - while (i_su != NULL) {
> + for (const auto& i_su : sg->list_of_su) {
> if ((i_su->saAmfSuReadinessState !=
> SA_AMF_READINESS_IN_SERVICE) || (NULL == i_su->list_of_susi)) {
> - i_su = i_su->sg_list_su_next;
> continue;
> }
> +
> if(SA_AMF_HA_ACTIVE == i_su->list_of_susi->state)
> curr_pref_active_sus++;
> - i_su = i_su->sg_list_su_next;
> }
> - TRACE_LEAVE2("cureent Active SUs :%u",curr_pref_active_sus);
> +
> + TRACE_LEAVE2("current Active SUs :%u",curr_pref_active_sus);
> return curr_pref_active_sus;
> }
> /*
> @@ -4340,8 +4342,6 @@
> }
>
> uint32_t SG_NPM::sg_admin_down(AVD_CL_CB *cb, AVD_SG *sg) {
> - AVD_SU *i_su;
> -
> TRACE_ENTER2("%u", sg->sg_fsm_state);
>
> if ((cb->init_state != AVD_APP_STATE) && (sg->sg_ncs_spec == false)) {
> @@ -4358,8 +4358,7 @@
> * exist, no action, stay in stable state.
> */
>
> - i_su = sg->list_of_su;
> - while (i_su != NULL) {
> + for (const auto& i_su : sg->list_of_su) {
> if (i_su->list_of_susi != AVD_SU_SI_REL_NULL)
> {
> if (i_su->list_of_susi->state ==
> SA_AMF_HA_ACTIVE)
> avd_sg_su_si_mod_snd(cb,
> i_su, SA_AMF_HA_QUIESCED);
> @@ -4369,8 +4368,6 @@
> /* add the SU to the operation list */
> avd_sg_su_oper_list_add(cb, i_su,
> false);
> }
> -
> - i_su = i_su->sg_list_su_next;
> }
>
> } /* if (sg->admin_state == NCS_ADMIN_STATE_LOCK) */
> @@ -4381,8 +4378,7 @@
> * If no active SU exist, change the SG admin state to
> LOCK, stay
> * in stable state.
> */
> - i_su = sg->list_of_su;
> - while (i_su != NULL) {
> + for (const auto& i_su : sg->list_of_su) {
> if (i_su->list_of_susi != AVD_SU_SI_REL_NULL)
> {
> if (i_su->list_of_susi->state ==
> SA_AMF_HA_ACTIVE) {
> avd_sg_su_si_mod_snd(cb,
> i_su, SA_AMF_HA_QUIESCING);
> @@ -4391,7 +4387,6 @@
> avd_sg_su_oper_list_add(cb,
> i_su, false);
> }
> }
> - i_su = i_su->sg_list_su_next;
> }
>
> } /* if (sg->admin_state == NCS_ADMIN_STATE_SHUTDOWN)
> */
> @@ -4412,8 +4407,7 @@
> * with remove all to the SUs with standby assignment
> and add the
> * standby SUs to the SU oper list.
> */
> - i_su = sg->list_of_su;
> - while (i_su != NULL) {
> + for (const auto& i_su : sg->list_of_su) {
> if (i_su->list_of_susi != AVD_SU_SI_REL_NULL)
> {
> if (i_su->list_of_susi->state ==
> SA_AMF_HA_QUIESCING) {
> avd_sg_su_si_mod_snd(cb,
> i_su, SA_AMF_HA_QUIESCED);
> @@ -4425,8 +4419,6 @@
> }
>
> }
> -
> - i_su = i_su->sg_list_su_next;
> }
>
> } /* if (sg->admin_state ==
> NCS_ADMIN_STATE_LOCK) */
<<attachment: winmail.dat>>
------------------------------------------------------------------------------
_______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
