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:
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
