Hi Hans Please see [GL].
Quoting Hans Nordebäck <[email protected]>: > Hi Gary, > > ack, one question, please see inline. > > /Thanks HansN > > /Thanks Hans > > On 09/17/2015 07:22 AM, Gary Lee wrote: >> osaf/services/saf/amf/amfd/sg.cc | 179 >> +++++++++++++++++++------------------- >> 1 files changed, 90 insertions(+), 89 deletions(-) >> >> >> diff --git a/osaf/services/saf/amf/amfd/sg.cc >> b/osaf/services/saf/amf/amfd/sg.cc >> --- a/osaf/services/saf/amf/amfd/sg.cc >> +++ b/osaf/services/saf/amf/amfd/sg.cc >> @@ -117,7 +117,6 @@ >> sg_fsm_state(AVD_SG_FSM_STABLE), >> admin_si(NULL), >> sg_redundancy_model(SA_AMF_NO_REDUNDANCY_MODEL), >> - list_of_su(NULL), >> list_of_si(NULL), >> sg_type(NULL), >> sg_list_app_next(NULL), >> @@ -164,7 +163,7 @@ >> void avd_sg_delete(AVD_SG *sg) >> { >> /* by now SU and SI should have been deleted */ >> - osafassert(sg->list_of_su == NULL); >> + osafassert(sg->list_of_su.empty() == true); >> osafassert(sg->list_of_si == NULL); >> sg_remove_from_model(sg); >> delete sg; >> @@ -757,7 +756,6 @@ >> */ >> static void sg_nd_attribute_update(AVD_SG *sg, uint32_t attrib_id) >> { >> - AVD_SU *su = NULL; >> AVD_AVND *su_node_ptr = NULL; >> AVSV_PARAM_INFO param; >> memset(((uint8_t *)¶m), '\0', sizeof(AVSV_PARAM_INFO)); >> @@ -809,8 +807,7 @@ >> } >> /* This value has to be updated on each SU on this SG */ >> - su = sg->list_of_su; >> - while (su) { >> + for (const auto& su : sg->list_of_su) { >> su_node_ptr = su->get_node_ptr(); >> if ((su_node_ptr) && (su_node_ptr->node_state == >> AVD_AVND_STATE_PRESENT)) { >> @@ -820,7 +817,6 @@ >> LOG_ER("%s::failed for %s",__FUNCTION__, >> su_node_ptr->name.value); >> } >> } >> - su = su->sg_list_su_next; >> } >> TRACE_LEAVE(); >> } >> @@ -1103,29 +1099,36 @@ >> /** >> * Terminate SU in reverse order >> - * @param su >> * @return NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE. >> */ >> -static uint32_t avd_sg_su_term_in_reverse(AVD_SU *su) >> +uint32_t AVD_SG::term_su_list_in_reverse() >> { >> - uint32_t rc = NCSCC_RC_SUCCESS; >> - TRACE_ENTER2("su:'%s'", su ? su->name.value : NULL); >> - if (su->sg_list_su_next != NULL) >> - rc = avd_sg_su_term_in_reverse(su->sg_list_su_next); >> - if ((su->saAmfSUPreInstantiable == true) && >> - (su->saAmfSUPresenceState != SA_AMF_PRESENCE_UNINSTANTIATED) >> && >> - (su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_INSTANTIATION_FAILED) && >> - (su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_TERMINATION_FAILED)) { >> + uint32_t rc = NCSCC_RC_SUCCESS; >> + std::vector<AVD_SU*>::const_reverse_iterator iter; >> + AVD_SU *su; >> - if (avd_snd_presence_msg(avd_cb, su, true) == NCSCC_RC_SUCCESS) { >> - su->set_term_state(true); >> - } else { >> - rc = NCSCC_RC_FAILURE; >> - LOG_WA("Failed Termination '%s'", su->name.value); >> - } >> - } >> - TRACE_LEAVE(); >> - return rc ; >> + TRACE_ENTER2("sg: %s", this->name.value); >> + for (iter = list_of_su.rbegin(); iter != list_of_su.rend(); ++iter) { > > [HansN] Is there a reason not to use, for (auto iter = .. and skip > const_reverse_iter declaration above? > [GL] I think I would need to write a reverse adaptor like boost has, in order to use a range-based for loop. So I just did it quickly with a reverse iterator. >> + su = *iter; >> + TRACE("terminate su:'%s'", su ? su->name.value : NULL); >> + >> + if ((su->saAmfSUPreInstantiable == true) && >> + (su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_UNINSTANTIATED) && >> + (su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_INSTANTIATION_FAILED) && >> + (su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_TERMINATION_FAILED)) { >> + >> + if (avd_snd_presence_msg(avd_cb, su, true) == >> NCSCC_RC_SUCCESS) { >> + su->set_term_state(true); >> + } else { >> + rc = NCSCC_RC_FAILURE; >> + LOG_WA("Failed Termination '%s'", >> su->name.value); >> + } >> + } >> + } >> + >> + TRACE_LEAVE(); >> + >> + return rc ; >> } >> /** >> * perform lock-instantiation on a given SG >> @@ -1139,8 +1142,8 @@ >> TRACE_ENTER2("%s", sg->name.value); >> /* terminate all the SUs on this Node */ >> - if (sg->list_of_su != NULL) >> - rc = avd_sg_su_term_in_reverse(sg->list_of_su); >> + if (sg->list_of_su.empty() == false) >> + rc = sg->term_su_list_in_reverse(); >> TRACE_LEAVE2("%u", rc); >> return rc; >> @@ -1154,13 +1157,14 @@ >> */ >> static void sg_app_sg_admin_unlock_inst(AVD_CL_CB *cb, AVD_SG *sg) >> { >> - AVD_SU *su; >> uint32_t su_try_inst; >> TRACE_ENTER2("%s", sg->name.value); >> /* Instantiate the SUs in this SG */ >> - for (su = sg->list_of_su, su_try_inst = 0; su != NULL; su = >> su->sg_list_su_next) { >> + su_try_inst = 0; >> + >> + for (const auto& su : sg->list_of_su) { >> if ((su->saAmfSUAdminState != >> SA_AMF_ADMIN_LOCKED_INSTANTIATION) && >> (su->su_on_node->saAmfNodeAdminState != >> SA_AMF_ADMIN_LOCKED_INSTANTIATION) && >> (su->saAmfSUOperState == >> SA_AMF_OPERATIONAL_ENABLED) && >> @@ -1174,7 +1178,7 @@ >> >> __FUNCTION__, su->name.value, >> >> su->su_on_node->node_info.nodeId); >> } else { >> - su_try_inst ++; >> + su_try_inst++; >> } >> } >> } >> @@ -1214,7 +1218,6 @@ >> { >> AVD_SG *sg; >> SaAmfAdminStateT adm_state; >> - AVD_SU *su; >> AVD_AVND *node; >> TRACE_ENTER2("'%s', %llu", object_name->value, op_id); >> @@ -1233,7 +1236,7 @@ >> } >> /* Avoid multiple admin operations on other SUs belonging to >> the same SG. */ >> - for (su = sg->list_of_su; su != NULL; su = su->sg_list_su_next) { >> + for (const auto& su : sg->list_of_su) { >> node = su->get_node_ptr(); >> if (su->pend_cbk.invocation != 0) { >> report_admin_op_error(immOiHandle, invocation, >> SA_AIS_ERR_TRY_AGAIN, NULL, >> @@ -1285,7 +1288,7 @@ >> adm_state = sg->saAmfSGAdminState; >> avd_sg_admin_state_set(sg, SA_AMF_ADMIN_UNLOCKED); >> if (avd_cb->init_state == AVD_INIT_DONE) { >> - for (su = sg->list_of_su; su != NULL; su = >> su->sg_list_su_next) { >> + for (const auto& su : sg->list_of_su) { >> if (su->is_in_service() == true) { >> >> su->set_readiness_state(SA_AMF_READINESS_IN_SERVICE); >> } >> @@ -1318,7 +1321,7 @@ >> avd_sg_admin_state_set(sg, SA_AMF_ADMIN_LOCKED); >> if (avd_cb->init_state == AVD_INIT_DONE) { >> - for (su = sg->list_of_su; su != NULL; su = >> su->sg_list_su_next) { >> + for (const auto& su : sg->list_of_su) { >> >> su->set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE); >> } >> break; >> @@ -1398,7 +1401,7 @@ >> /* If any su is in terminating state, that means lock-in op >> has not completed. Allow su to move into permanent state >> i.e. either in uninstanted or term failed state. */ >> - for (su = sg->list_of_su; su != NULL; su = su->sg_list_su_next) >> { >> + for (const auto& su : sg->list_of_su) { >> if (su->saAmfSUPresenceState == >> SA_AMF_PRESENCE_TERMINATING) { >> report_admin_op_error(immOiHandle, invocation, >> SA_AIS_ERR_TRY_AGAIN, NULL, >> @@ -1410,7 +1413,7 @@ >> avd_sg_admin_state_set(sg, SA_AMF_ADMIN_LOCKED); >> - if ((sg->list_of_su != NULL) && >> (sg->list_of_su->saAmfSUPreInstantiable == false)) { >> + if ((sg->list_of_su.empty() == false) && >> (sg->first_su()->saAmfSUPreInstantiable == false)) { >> avd_saImmOiAdminOperationResult(immOiHandle, >> invocation, SA_AIS_OK); >> goto done; >> } >> @@ -1564,30 +1567,20 @@ >> void avd_sg_remove_su(AVD_SU* su) >> { >> - AVD_SU *i_su = NULL; >> - AVD_SU *prev_su = NULL; >> AVD_SG *sg = su->sg_of_su; >> if (su->sg_of_su != NULL) { >> /* remove SU from SG */ >> - i_su = su->sg_of_su->list_of_su; >> - >> - while ((i_su != NULL) && (i_su != su)) { >> - prev_su = i_su; >> - i_su = i_su->sg_list_su_next; >> + auto su_to_delete = std::find(sg->list_of_su.begin(), >> + sg->list_of_su.end(), >> + su); >> + >> + if (su_to_delete != sg->list_of_su.end()) { >> + sg->list_of_su.erase(su_to_delete); >> + } else { >> + LOG_ER("su cannot be found"); >> } >> - if (i_su != su) { >> - /* Log a fatal error */ >> - } else { >> - if (prev_su == NULL) { >> - su->sg_of_su->list_of_su = su->sg_list_su_next; >> - } else { >> - prev_su->sg_list_su_next = su->sg_list_su_next; >> - } >> - } >> - >> - su->sg_list_su_next = NULL; >> su->sg_of_su = NULL; >> } /* if (su->sg_of_su != AVD_SG_NULL) */ >> @@ -1597,26 +1590,17 @@ >> void avd_sg_add_su(AVD_SU* su) >> { >> - AVD_SU *i_su = NULL; >> - AVD_SU *prev_su = NULL; >> - >> if ((su == NULL) || (su->sg_of_su == NULL)) >> return; >> - i_su = su->sg_of_su->list_of_su; >> + su->sg_of_su->list_of_su.push_back(su); >> - while ((i_su != NULL) && (i_su->saAmfSURank < su->saAmfSURank)) { >> - prev_su = i_su; >> - i_su = i_su->sg_list_su_next; >> - } >> + // in descending order of SU rank (lowest to >> + // highest integer). Note: the lower the integer >> + // value, the higher the rank. >> + std::sort(su->sg_of_su->list_of_su.begin(), >> su->sg_of_su->list_of_su.end(), >> + [](const AVD_SU *a, const AVD_SU *b) -> bool {return >> a->saAmfSURank < b->saAmfSURank;}); >> - if (prev_su == NULL) { >> - su->sg_list_su_next = su->sg_of_su->list_of_su; >> - su->sg_of_su->list_of_su = su; >> - } else { >> - prev_su->sg_list_su_next = su; >> - su->sg_list_su_next = i_su; >> - } >> avd_verify_equal_ranked_su(su->sg_of_su); >> } >> @@ -1668,6 +1652,8 @@ >> } >> void AVD_SG::set_fsm_state(AVD_SG_FSM_STATE state) { >> + TRACE_ENTER(); >> + >> if (sg_fsm_state != state) { >> TRACE("%s sg_fsm_state %u => %u", name.value, sg_fsm_state, >> state); >> sg_fsm_state = state; >> @@ -1689,6 +1675,8 @@ >> } >> } >> } >> + >> + TRACE_LEAVE(); >> } >> void AVD_SG::set_adjust_state(SaAdjustState state) { >> @@ -1712,8 +1700,9 @@ >> } >> void AVD_SG::for_all_su_set_readiness_state(SaAmfReadinessStateT state) { >> - for (AVD_SU *su = list_of_su; su != NULL; su = su->sg_list_su_next) >> + for (const auto& su : list_of_su) { >> su->set_readiness_state(state); >> + } >> } >> bool AVD_SG::in_su_oper_list(const AVD_SU *i_su) { >> @@ -1753,20 +1742,24 @@ >> */ >> static void avd_verify_equal_ranked_su(AVD_SG *avd_sg) >> { >> - AVD_SU *pre_temp_su = NULL, *temp_su; >> + uint32_t rank = 0; >> + bool valid_rank = false; >> TRACE_ENTER(); >> - /* Check ranks are still equal or not. Mark true in the begining*/ >> + /* Check ranks are still equal or not. Mark true in the beginning*/ >> avd_sg->equal_ranked_su = true; >> - temp_su = avd_sg->list_of_su; >> - while(temp_su) { >> - if (pre_temp_su && temp_su->saAmfSURank != >> pre_temp_su->saAmfSURank) { >> + >> + for (const auto& su : avd_sg->list_of_su) { >> + if (valid_rank && rank != su->saAmfSURank) { >> avd_sg->equal_ranked_su = false; >> break; >> } >> - pre_temp_su = temp_su; >> - temp_su = temp_su->sg_list_su_next; >> + >> + // store rank for comparison >> + rank = su->saAmfSURank; >> + valid_rank = true; >> } >> + >> TRACE_LEAVE2("avd_sg->equal_ranked_su '%u'", avd_sg->equal_ranked_su); >> return; >> @@ -1782,7 +1775,7 @@ >> void avd_sg_adjust_config(AVD_SG *sg) >> { >> // SUs in an SG are equal, only need to look at the first one >> - if ((sg->list_of_su != NULL) && >> !sg->list_of_su->saAmfSUPreInstantiable) { >> + if (sg->list_of_su.empty() == false && >> !(sg->first_su()->saAmfSUPreInstantiable)) { >> // NPI SUs can only be assigned one SI, see 3.2.1.1 >> if ((sg->saAmfSGMaxActiveSIsperSU != >> static_cast<SaUint32T>(-1)) >> && (sg->saAmfSGMaxActiveSIsperSU > 1)) { >> LOG_WA("invalid saAmfSGMaxActiveSIsperSU (%u) for NPI >> SU '%s', >> adjusting...", >> @@ -1796,12 +1789,11 @@ >> } >> sg->saAmfSGMaxStandbySIsperSU = 1; >> /* saAmfSUFailover must be true for a NPI SU sec 3.11.1.3.2 >> AMF-B.04.01 spec */ >> - for (AVD_SU *su = sg->list_of_su; su != NULL; su = >> su->sg_list_su_next) { >> + for (const auto& su : sg->list_of_su) { >> if (!su->saAmfSUFailover) { >> su->set_su_failover(true); >> } >> } >> - >> } >> /* adjust saAmfSGNumPrefAssignedSUs if not configured, only >> applicable for >> @@ -1825,11 +1817,10 @@ >> */ >> uint32_t sg_instantiated_su_count(const AVD_SG *sg) >> { >> - uint32_t inst_su_count; >> - AVD_SU *su; >> + uint32_t inst_su_count = 0; >> TRACE_ENTER(); >> - for (su = sg->list_of_su, inst_su_count = 0; su != NULL; su = >> su->sg_list_su_next) { >> + for (const auto& su : sg->list_of_su) { >> TRACE_1("su'%s', pres state'%u', in_serv'%u', PrefIn'%u'", >> su->name.value, >> su->saAmfSUPresenceState, >> su->saAmfSuReadinessState, >> sg->saAmfSGNumPrefInserviceSUs); >> if (((su->saAmfSUPresenceState == SA_AMF_PRESENCE_INSTANTIATED) >> || >> @@ -1838,6 +1829,7 @@ >> inst_su_count ++; >> } >> } >> + >> TRACE_LEAVE2("%u", inst_su_count); >> return inst_su_count; >> } >> @@ -1868,7 +1860,7 @@ >> switch (sg->adminOp) { >> case SA_AMF_ADMIN_LOCK_INSTANTIATION : >> - for (AVD_SU *su = sg->list_of_su; su; su = su->sg_list_su_next) >> { >> + for (const auto& su : sg->list_of_su) { >> if ((su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_UNINSTANTIATED) && >> (su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_INSTANTIATION_FAILED) && >> (su->saAmfSUPresenceState != >> SA_AMF_PRESENCE_TERMINATION_FAILED)) >> @@ -1877,10 +1869,10 @@ >> break; >> case SA_AMF_ADMIN_UNLOCK_INSTANTIATION : >> /* Unlock-in of SG will not instantiate any component in NPI >> SU.*/ >> - if ((sg->list_of_su != NULL) && >> (sg->list_of_su->saAmfSUPreInstantiable == false)) >> + if ((sg->list_of_su.empty() == false) && >> (sg->first_su()->saAmfSUPreInstantiable == false)) >> return true; >> - >> - for (AVD_SU *su = sg->list_of_su; su; su = su->sg_list_su_next) >> { >> + >> + for (const auto& su : sg->list_of_su) { >> node_admin_state = su->su_on_node->saAmfNodeAdminState; >> >> if ((su->saAmfSUPresenceState == >> SA_AMF_PRESENCE_INSTANTIATION_FAILED) || >> @@ -1935,7 +1927,7 @@ >> */ >> bool AVD_SG::is_sg_assigned_only_in_ng(const AVD_AMF_NG *ng) >> { >> - for (AVD_SU *su = list_of_su; su; su = su->sg_list_su_next) { >> + for (const auto& su : list_of_su) { >> if (su->list_of_susi == NULL) >> continue; >> //Return if this assigned su is not in this ng. >> @@ -1952,7 +1944,7 @@ >> */ >> bool AVD_SG::is_sg_serviceable_outside_ng(const AVD_AMF_NG *ng) >> { >> - for (AVD_SU *su = list_of_su; su; su = su->sg_list_su_next) { >> + for (const auto& su : list_of_su) { >> //Check if any su exists outside nodegroup for this sg. >> if (node_in_nodegroup(Amf::to_string(&su->su_on_node->name), >> ng) >> == false) { >> /* >> @@ -2005,3 +1997,12 @@ >> return rc; >> } >> + >> +AVD_SU* AVD_SG::first_su() >> +{ >> + if (!list_of_su.empty()) { >> + return *list_of_su.begin(); >> + } else { >> + return NULL; >> + } >> +} ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
