Hi Nagu Thanks for your feedback. I've implemented most of your suggestions. Please see my replies with [GL].
Thanks Gary On 04/09/15 23:46, Nagendra Kumar wrote: > 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. [GL] I set i_su back to NULL as that is what the old used to do, if su_found is false (i_su was eventually set to NULL when it reaches the end of list_of_su). > >> + 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. [GL] As above > >> + 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. [GL] This shouldn't be needed. If the set is empty, then iter != sg->list_of_su.end() will fail, and we don't try to deference iter. > >> 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) */ ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
