Hi Hans
Sorry, I just read your comment more carefully again. Yes, we should
move the declaration to inside the for statement and change to auto.
Will change before pushing.
Thanks
Gary
Quoting Gary Lee <gary....@dektech.com.au>:
> Hi Hans
>
> Please see [GL].
>
> Quoting Hans Nordebäck <hans.nordeb...@ericsson.com>:
>
>> 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
> Opensaf-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
