Hi Praveen, ack for code review.
Thanks, Minh On 15/09/16 19:41, praveen.malv...@oracle.com wrote: > osaf/libs/agents/saf/amfa/ava_hdl.cc | 28 +++++++++++++++++++--------- > osaf/libs/common/amf/n2avamsg.c | 14 ++++++-------- > osaf/services/saf/amf/amfd/util.cc | 2 +- > osaf/services/saf/amf/amfnd/comp.cc | 1 - > 4 files changed, 26 insertions(+), 19 deletions(-) > > > In the reported problem, active compname is improperly as \'sa\' > in CSI SET callback for HA state. > > After setting activeCompName in standby descriptor, AMFND is wrongly calling > osaf_extended_name_clear() for active descriptor. Since > SaAmfCSIStateDescriptorT > is a union, it clears the filled value. > Along with the above problems there are other minor problems like: > -AMFND is copying activecompName using osaf_extended_name_alloc() in agent > message, > only when it is an extended name. > osaf_extended_name_alloc() should be done irrespective of short or long dn. > -Agent should perform validation check on dn based on HA state of the CSI SET > callback, > as activecompname is not populated for all the HA states. > -When above problems are fixed, one more problem pops up: > a standby component gets its own name in activeCompName in CSI set callback > for standby. > There is a minor regression at AMFD. > > Patch fixes all these problems. > > diff --git a/osaf/libs/agents/saf/amfa/ava_hdl.cc > b/osaf/libs/agents/saf/amfa/ava_hdl.cc > --- a/osaf/libs/agents/saf/amfa/ava_hdl.cc > +++ b/osaf/libs/agents/saf/amfa/ava_hdl.cc > @@ -624,21 +624,31 @@ uint32_t ava_hdl_cbk_rec_prc(AVSV_AMF_CB > AVSV_AMF_CSI_SET_PARAM *csi_set = &info->param.csi_set; > > if (!ava_sanamet_is_valid(&csi_set->csi_desc.csiName) || > - !ava_sanamet_is_valid(&csi_set->comp_name) || > - > !ava_sanamet_is_valid(&csi_set->csi_desc.csiStateDescriptor.activeDescriptor.activeCompName) > || > - > !ava_sanamet_is_valid(&csi_set->csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName)) > { > + !ava_sanamet_is_valid(&csi_set->comp_name)) { > rc = SA_AIS_ERR_NAME_TOO_LONG; > } > + bool actv_or_stdby = true; > + if ((csi_set->ha == SA_AMF_HA_ACTIVE) && > + > (csi_set->csi_desc.csiStateDescriptor.activeDescriptor.transitionDescriptor > != SA_AMF_CSI_NEW_ASSIGN)) { > + actv_or_stdby = > ava_sanamet_is_valid(&csi_set->csi_desc.csiStateDescriptor.activeDescriptor.activeCompName); > + } else if (csi_set->ha == SA_AMF_HA_STANDBY) { > + actv_or_stdby = > ava_sanamet_is_valid(&csi_set->csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName); > + } > + if (!actv_or_stdby) > + rc = SA_AIS_ERR_NAME_TOO_LONG; > > if (rc == SA_AIS_OK && reg_cbk->saAmfCSISetCallback) { > TRACE("CSISet: CSIName = %s, CSIFlags = %d, HA > state = %d", > > osaf_extended_name_borrow(&csi_set->csi_desc.csiName),csi_set->csi_desc.csiFlags,csi_set->ha); > - TRACE("CSISet: Active Transition Descriptor = > %u, Active Component Name = %s", > - > csi_set->csi_desc.csiStateDescriptor.activeDescriptor.transitionDescriptor, > - > osaf_extended_name_borrow(&csi_set->csi_desc.csiStateDescriptor.activeDescriptor.activeCompName)); > - TRACE("CSISet: ActiveCompName = %s, StandbyRank > = %u", > - > osaf_extended_name_borrow(&csi_set->csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName), > - > csi_set->csi_desc.csiStateDescriptor.standbyDescriptor.standbyRank); > + if ((csi_set->ha == SA_AMF_HA_ACTIVE) && > + > (csi_set->csi_desc.csiStateDescriptor.activeDescriptor.transitionDescriptor > != SA_AMF_CSI_NEW_ASSIGN)) > + TRACE("CSISet: Active Transition > Descriptor = %u, Active Component Name = %s", > + > csi_set->csi_desc.csiStateDescriptor.activeDescriptor.transitionDescriptor, > + > osaf_extended_name_borrow(&csi_set->csi_desc.csiStateDescriptor.activeDescriptor.activeCompName)); > + if (csi_set->ha == SA_AMF_HA_STANDBY) > + TRACE("CSISet: ActiveCompName = %s, > StandbyRank = %u", > + > osaf_extended_name_borrow(&csi_set->csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName), > + > csi_set->csi_desc.csiStateDescriptor.standbyDescriptor.standbyRank); > TRACE("Invoking component's > saAmfCSISetCallback: InvocationId = %llx, component name = %s", > > info->inv,osaf_extended_name_borrow(&csi_set->comp_name)); > reg_cbk->saAmfCSISetCallback(info->inv, > diff --git a/osaf/libs/common/amf/n2avamsg.c b/osaf/libs/common/amf/n2avamsg.c > --- a/osaf/libs/common/amf/n2avamsg.c > +++ b/osaf/libs/common/amf/n2avamsg.c > @@ -333,17 +333,15 @@ uint32_t avsv_amf_cbk_copy(AVSV_AMF_CBK_ > > /* Copy csi state description */ > if (scbk->param.csi_set.ha == SA_AMF_HA_ACTIVE) { > - if > (osaf_is_an_extended_name(&scbk->param.csi_set.csi_desc.csiStateDescriptor.activeDescriptor.activeCompName)) > { > - > osaf_extended_name_alloc(osaf_extended_name_borrow(&scbk->param.csi_set.csi_desc.csiStateDescriptor.activeDescriptor.activeCompName), > - > > &(*o_dcbk)->param.csi_set.csi_desc.csiStateDescriptor.activeDescriptor.activeCompName); > - } > + //osaf_extended_name_alloc() takes care of long and > short dn. > + > osaf_extended_name_alloc(osaf_extended_name_borrow(&scbk->param.csi_set.csi_desc.csiStateDescriptor.activeDescriptor.activeCompName), > + > &(*o_dcbk)->param.csi_set.csi_desc.csiStateDescriptor.activeDescriptor.activeCompName); > } > > if (scbk->param.csi_set.ha == SA_AMF_HA_STANDBY) { > - if > (osaf_is_an_extended_name(&scbk->param.csi_set.csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName)) > { > - > osaf_extended_name_alloc(osaf_extended_name_borrow(&scbk->param.csi_set.csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName), > - > > &(*o_dcbk)->param.csi_set.csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName); > - } > + //osaf_extended_name_alloc() takes care of long and > short dn. > + > osaf_extended_name_alloc(osaf_extended_name_borrow(&scbk->param.csi_set.csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName), > + > &(*o_dcbk)->param.csi_set.csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName); > } > > /* copy the amf csi attr list */ > diff --git a/osaf/services/saf/amf/amfd/util.cc > b/osaf/services/saf/amf/amfd/util.cc > --- a/osaf/services/saf/amf/amfd/util.cc > +++ b/osaf/services/saf/amf/amfd/util.cc > @@ -957,7 +957,7 @@ uint32_t avd_snd_susi_msg(AVD_CL_CB *cb, > } > } > } else { > - > osaf_extended_name_alloc(osaf_extended_name_borrow(&l_compcsi->csi->list_compcsi->comp->comp_info.name), > + > osaf_extended_name_alloc(osaf_extended_name_borrow(&l_compcsi->csi_csicomp_next->comp->comp_info.name), > > &compcsi_info->active_comp_name); > > if ((trans_dsc == SA_AMF_CSI_QUIESCED) > && > 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 > @@ -2027,7 +2027,6 @@ uint32_t avnd_comp_cbk_send(AVND_CB *cb, > > &csi_desc.csiStateDescriptor.standbyDescriptor.activeCompName); > > > csi_desc.csiStateDescriptor.standbyDescriptor.standbyRank = > curr_csi->standby_rank; > - > osaf_extended_name_clear(&csi_desc.csiStateDescriptor.activeDescriptor.activeCompName); > } > > /* copy the attributes */ > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel