Hi Hans,
Thanks for comments, I will resubmit the patch.
/Minh
On 1/9/2016 3:44 AM, Hans Nordebäck wrote:
> ack, code review only. Minor comments below/Thanks HansN
>
> On 12/14/2015 01:04 AM, Minh Hon Chau wrote:
>> osaf/services/saf/amf/amfnd/comp.cc | 71
>> +++++++++++++-----------
>> osaf/services/saf/amf/amfnd/include/avnd_comp.h | 2 +-
>> 2 files changed, 40 insertions(+), 33 deletions(-)
>>
>>
>> There's a compound *if* conditions to determine whether a component
>> can be assigned/removed for a csi. Duplication of it has been placed
>> in some amfnd functions. Also, it's not quite informative since the
>> false case is being logged in generic meanings.
>>
>> The patch makes a separated function to do this valuation, add more
>> traces for specific cases
>>
>> diff --git a/osaf/services/saf/amf/amfnd/comp.cc
>> b/osaf/services/saf/amf/amfnd/comp.cc
>> --- a/osaf/services/saf/amf/amfnd/comp.cc
>> +++ b/osaf/services/saf/amf/amfnd/comp.cc
>> @@ -944,14 +944,9 @@ uint32_t avnd_comp_csi_assign(AVND_CB *c
>> TRACE_ENTER2("comp:'%s'", comp->name.value);
>> LOG_IN("Assigning '%s' %s to '%s'", csiname,
>> ha_state[curr_csi->si->curr_state], comp->name.value);
>> - /* skip assignments to failed / unregistered comp */
>> + /* skip assignments to unqualified comp */
>> if (!m_AVND_SU_IS_RESTART(comp->su) &&
>> - (m_AVND_COMP_IS_FAILED(comp) ||
>> - (m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(comp) &&
>> - ((!m_AVND_COMP_IS_REG(comp) &&
>> !m_AVND_COMP_PRES_STATE_IS_ORPHANED (comp)) ||
>> - (!m_AVND_COMP_PRES_STATE_IS_INSTANTIATED(comp) &&
>> - (comp->su->pres == SA_AMF_PRESENCE_INSTANTIATION_FAILED) &&
>> - !m_AVND_COMP_PRES_STATE_IS_ORPHANED (comp)))))) {
>> + !is_comp_qualified_assignment(comp)) {
>> TRACE("Not suRestart context and comp is failed or
>> unregistered");
>> /* dont skip restarting components. wait till restart is
>> complete */
>> if ((comp->pres == SA_AMF_PRESENCE_RESTARTING) &&
>> m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(comp)) { /* mark the csi(s)
>> assigned */
>> @@ -1253,19 +1248,8 @@ uint32_t avnd_comp_csi_remove(AVND_CB *c
>> TRACE_ENTER2("comp: '%s' : csi: '%p'", comp->name.value, csi);
>> LOG_IN("Removing '%s' from '%s'", csiname, comp->name.value);
>> - /* skip removal from failed / unregistered comp */
>> - if (m_AVND_COMP_IS_FAILED(comp) ||
>> (m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(comp) &&
>> ((!m_AVND_COMP_IS_REG(comp)
>> - &&
>> - !m_AVND_COMP_PRES_STATE_IS_ORPHANED
>> - (comp))
>> - ||
>> - (!m_AVND_COMP_PRES_STATE_IS_INSTANTIATED
>> - (comp)
>> - && (comp->su->pres ==
>> - SA_AMF_PRESENCE_INSTANTIATION_FAILED)
>> - &&
>> - !m_AVND_COMP_PRES_STATE_IS_ORPHANED
>> - (comp))))) {
>> + /* skip removal from unqualified comp */
>> + if (!is_comp_qualified_assignment(comp)) {
>> if (csi) {
>> /* after restart we should go ahead with next csi, so
>> mark the curr_csi as assigning */
>> m_AVND_COMP_CSI_CURR_ASSIGN_STATE_SET(csi,
>> AVND_COMP_CSI_ASSIGN_STATE_REMOVING);
>> @@ -1461,6 +1445,37 @@ uint32_t avnd_comp_csi_reassign(AVND_CB
>> }
> [HansN] This new function should use google c++ style guide
>> /**
>> + * Check whether a component is qualified for assignment
>> + * @param comp
>> + *
>> + * @return bool
>> + */
>> +bool is_comp_qualified_assignment(const AVND_COMP *comp)
>> +{
>> + bool rc = true;
>> + TRACE_ENTER2();
>> + if (m_AVND_COMP_IS_FAILED(comp)) {
>> + LOG_IN("Ignoring Failed comp:'%s', comp_flag %x",
>> + comp->name.value, comp->flag);
>> + rc = false;
>> + } else if (m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(comp)) {
>> + if (!m_AVND_COMP_IS_REG(comp) &&
>> !m_AVND_COMP_PRES_STATE_IS_ORPHANED(comp)) {
> [HansN] minor comment, Ignoring is misspelled below
>> + LOG_IN("Ingoring Unregistered comp:'%s'",
>> comp->name.value);
>> + rc = false;
>> + } else if (!m_AVND_COMP_PRES_STATE_IS_INSTANTIATED (comp) &&
>> + comp->su->pres ==
>> SA_AMF_PRESENCE_INSTANTIATION_FAILED &&
>> + !m_AVND_COMP_PRES_STATE_IS_ORPHANED(comp)) {
>> + LOG_IN("Ingoring comp with invalid presence state:'%s',
>> comp_flag %x, comp_pres=%u, su_pres=%u",
>> + comp->name.value, comp->flag,
>> + comp->pres, comp->su->pres);
>> + rc = false;
>> + }
>> + }
>> + TRACE_LEAVE2("rc:%u", rc);
>> + return rc;
>> +}
>> +
>> +/**
>> * Are all CSIs at the specified rank assigned for this SI?
>> * @param si
>> * @param rank
>> @@ -1477,18 +1492,10 @@ static bool all_csis_at_rank_assigned(st
>> csi =
>> (AVND_COMP_CSI_REC*)m_NCS_DBLIST_FIND_NEXT(&csi->si_dll_node)) {
>> if ((csi->rank == rank) && (csi->curr_assign_state !=
>> AVND_COMP_CSI_ASSIGN_STATE_ASSIGNED)) {
>> - /* Ignore the case of failed component/unregistered
>> comp. */
>> - if (!m_AVND_SU_IS_RESTART(csi->comp->su) &&
>> (m_AVND_COMP_IS_FAILED(csi->comp) ||
>> - (m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(csi->comp) &&
>> - ((!m_AVND_COMP_IS_REG(csi->comp) &&
>> - !m_AVND_COMP_PRES_STATE_IS_ORPHANED
>> (csi->comp)) ||
>> - (!m_AVND_COMP_PRES_STATE_IS_INSTANTIATED (csi->comp) &&
>> - (csi->comp->su->pres ==
>> SA_AMF_PRESENCE_INSTANTIATION_FAILED) &&
>> - !m_AVND_COMP_PRES_STATE_IS_ORPHANED
>> - (csi->comp)))))) {
>> -
>> - LOG_IN("Ignoring Failed/Unreg Comp'%s' comp pres
>> state=%u, comp flag %x, su pres state %u",
>> - csi->comp->name.value, csi->comp->pres,
>> csi->comp->flag, csi->comp->su->pres);
>> + /* Ignore unqualified comp. */
>> + if (!m_AVND_SU_IS_RESTART(csi->comp->su) &&
>> !is_comp_qualified_assignment(csi->comp)) {
>> + LOG_IN("Ignoring comp'%s' which is not qualified for
>> assignment",
>> + csi->comp->name.value);
>> } else if
>> (m_AVND_COMP_TYPE_IS_PREINSTANTIABLE(csi->comp) &&
>> csi->suspending_assignment) {
>> LOG_IN("Ignoring quiescing/quiesced assignment on
>> unassigned comp'%s'",
>> diff --git a/osaf/services/saf/amf/amfnd/include/avnd_comp.h
>> b/osaf/services/saf/amf/amfnd/include/avnd_comp.h
>> --- a/osaf/services/saf/amf/amfnd/include/avnd_comp.h
>> +++ b/osaf/services/saf/amf/amfnd/include/avnd_comp.h
>> @@ -920,7 +920,7 @@ extern int avnd_comp_config_reinit(AVND_
>> extern void avnd_comp_delete(AVND_COMP *comp);
>> extern void avnd_comp_pres_state_set(AVND_COMP *comp,
>> SaAmfPresenceStateT newstate);
>> bool comp_has_quiesced_assignment(const AVND_COMP *comp);
>> -
> [HansN] use const qualifier for function below
>> +bool is_comp_qualified_assignment(const AVND_COMP *comp);
>> /**
>> * Initiate restart of a component.
>> * This means either:
>
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
