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

Reply via email to