See inline, minor comments on naming and code layout. Ack with these comments fixed. /Hans
> -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: den 7 maj 2014 08:39 > To: Hans Feldt; Hans Nordebäck; [email protected] > Cc: [email protected] > Subject: [PATCH 1 of 1] v3 amfd: Allow multiple csi addition in single ccb > [#750] > > osaf/services/saf/avsv/avd/avd_csi.c | 77 > ++++++++++++++------------- > osaf/services/saf/avsv/avd/avd_sgproc.c | 14 +++++ > osaf/services/saf/avsv/avd/include/avd_csi.h | 1 + > 3 files changed, 55 insertions(+), 37 deletions(-) > > > Problem: > Amf rejects csi addition if previous csi addition is in progress. > > Analysis: > Amfd can allow csi addition in sequence but can delay applying > subsequent csi addition. > > Fix: > Next csi addition is delayed until the previous csi addition is finished. > > diff --git a/osaf/services/saf/avsv/avd/avd_csi.c > b/osaf/services/saf/avsv/avd/avd_csi.c > --- a/osaf/services/saf/avsv/avd/avd_csi.c > +++ b/osaf/services/saf/avsv/avd/avd_csi.c > @@ -441,12 +441,8 @@ static SaAisErrorT csi_ccb_completed_cre > t_sisu = avd_si->list_of_sisu; > while(t_sisu) { > if (t_sisu->csi_add_rem == true) { > - LOG_WA("CSI create of '%s' rejected: > pending assignment for '%s'", > + LOG_NO("CSI create of '%s' in queue: > pending assignment for '%s'", > > opdata->objectName.value, t_sisu->su->name.value); > - if (avd_cb->avail_state_avd == > SA_AMF_HA_ACTIVE) { > - rc = SA_AIS_ERR_BAD_OPERATION; > - goto done; > - } > } > t_sisu = t_sisu->si_next; > }/* while(t_sisu) */ > @@ -626,7 +622,7 @@ static SaAisErrorT csi_ccb_completed_del > t_sisu = csi->si->list_of_sisu; > while(t_sisu) { > if (t_sisu->csi_add_rem == true) { > - LOG_WA("CSI remove of '%s' rejected: > pending assignment for '%s'", > + LOG_NO("CSI remove of '%s' rejected: > pending assignment for '%s'", > csi->name.value, > t_sisu->su->name.value); > if (avd_cb->avail_state_avd == > SA_AMF_HA_ACTIVE) { > rc = SA_AIS_ERR_BAD_OPERATION; > @@ -793,42 +789,33 @@ static void csi_ccb_apply_modify_hdlr(st > TRACE_LEAVE(); > } > > -/***************************************************************************** > - * Function: csi_ccb_apply_create_hdlr > - * > - * Purpose: This routine handles create operations on SaAmfCSI objects. > - * > +/** > + * @brief Assign csi to component as per compcsi configurations. > * > - * Input : Ccb Util Oper Data. > - * > - * Returns: None. > - * > - * NOTES : None. > - * > - * > - **************************************************************************/ > -static void csi_ccb_apply_create_hdlr(struct CcbUtilOperationData *opdata) > + * @param[in] csi pointer. > + * > + * @return OK if csi is assigned else NO_OP. > + */ > +SaAisErrorT csi_assign_hdlr(AVD_CSI *csi) [Hans] I am not so happy with this function name. What value does _hdlr add? How about "csi_assign_to_comp" ? > { > - AVD_CSI *csi = NULL; > AVD_COMP *t_comp; > AVD_SU_SI_REL *t_sisu; > SaBoolT first_sisu = true; > AVD_COMP_CSI_REL *compcsi; > AVD_COMPCS_TYPE *cst; > + SaAisErrorT rc = SA_AIS_ERR_NO_OP; > > - TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId, > opdata->objectName.value); > - > - > - if ((csi = avd_csi_get (&opdata->objectName)) == NULL) { > - /* this check is added because, some times there is possibility > that before getting ccb apply callback > - * we might get compcsi create checkpoint and csi will be > created as part of checkpoint processing > - */ > - csi = csi_create(&opdata->objectName); > - } > - csi_get_attr_and_add_to_model(csi, opdata->param.create.attrValues, > opdata->param.create.parentName); > - > - if (avd_cb->avail_state_avd != SA_AMF_HA_ACTIVE) > - goto done; > + /* Check whether csi assignment is already in progress and if yes, then > return. > + This csi will be assigned after the undergoing csi assignment gets > over.*/ > + if (csi->si->list_of_sisu != NULL) { > + for(t_sisu = csi->si->list_of_sisu; t_sisu != NULL; t_sisu = > t_sisu->si_next) { > + if (t_sisu->csi_add_rem == true) { > + LOG_NO("CSI create '%s' delayed: pending > assignment for '%s'", > + csi->name.value, > t_sisu->su->name.value); > + goto done; > + } > + } > + } > > /* Check whether si has been assigned to any SU. */ > if (NULL != csi->si->list_of_sisu) { > @@ -886,7 +873,7 @@ static void csi_ccb_apply_create_hdlr(st > } > if (NULL == t_comp) { > LOG_ER("Compcsi doesn't exist or > MaxActiveCSI/MaxStandbyCSI have reached for csi '%s'", > - opdata->objectName.value); > + csi->name.value); > goto done; > } > > @@ -907,6 +894,7 @@ static void csi_ccb_apply_create_hdlr(st > avd_susi_delete(avd_cb, t_sisu, true); > goto done; > } > + rc = SA_AIS_OK; > > } > t_sisu->csi_add_rem = true; > @@ -922,17 +910,32 @@ static void csi_ccb_apply_create_hdlr(st > csi->si->sg_of_si->si_func(avd_cb, csi->si); > } > done: > + return rc; > TRACE_LEAVE(); > } > > static void csi_ccb_apply_cb(CcbUtilOperationData_t *opdata) > { > + AVD_CSI *csi = NULL; > > TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId, > opdata->objectName.value); > > switch (opdata->operationType) { > case CCBUTIL_CREATE: > - csi_ccb_apply_create_hdlr(opdata); > + if ((csi = avd_csi_get (&opdata->objectName)) == NULL) { > + /* This check is added because, some times there is > + possibility that before getting ccb apply callback > + we might get compcsi create checkpoint and csi will > + be created as part of checkpoint processing */ > + csi = csi_create(&opdata->objectName); > + } > + csi_get_attr_and_add_to_model(csi, > + opdata->param.create.attrValues, > + opdata->param.create.parentName); > + > + if (avd_cb->avail_state_avd != SA_AMF_HA_ACTIVE) > + goto done; [Hans] as said previously, please keep the logic above in a function called csi_ccb_apply_create_hdlr which then _calls_ csi_assign > + csi_assign_hdlr(csi); [Hans] not checking return value here > break; > case CCBUTIL_MODIFY: > csi_ccb_apply_modify_hdlr(opdata); > @@ -944,7 +947,7 @@ static void csi_ccb_apply_cb(CcbUtilOper > osafassert(0); > break; > } > - > +done: > TRACE_LEAVE(); > } > > diff --git a/osaf/services/saf/avsv/avd/avd_sgproc.c > b/osaf/services/saf/avsv/avd/avd_sgproc.c > --- a/osaf/services/saf/avsv/avd/avd_sgproc.c > +++ b/osaf/services/saf/avsv/avd/avd_sgproc.c > @@ -808,6 +808,20 @@ void avd_su_si_assign_evh(AVD_CL_CB *cb, > } > t_sisu = t_sisu->si_next; > }/* while(t_sisu) */ > + if (t_sisu == NULL) { > + /* Since csi assignment is over, > walkthrough other > + unassigned CSIs for assignment. */ > + for (csi = susi->si->list_of_csi; csi; > csi = > + > csi->si_list_of_csi_next) { > + if (csi->list_compcsi == NULL) { > + /* Assign this csi and > when the assignment > + will be over for > this csi, then other > + unassigned CSIs will > be taken.*/ > + if > (csi_assign_hdlr(csi) == SA_AIS_OK) > + goto done; > + } > + } > + } > /* Comsume this message. */ > goto done; > } > diff --git a/osaf/services/saf/avsv/avd/include/avd_csi.h > b/osaf/services/saf/avsv/avd/include/avd_csi.h > --- a/osaf/services/saf/avsv/avd/include/avd_csi.h > +++ b/osaf/services/saf/avsv/avd/include/avd_csi.h > @@ -160,5 +160,6 @@ extern void avd_csi_delete(struct avd_cs > extern void csi_cmplt_delete(struct avd_csi_tag *csi, SaBoolT ckpt); > extern AVD_CSI *csi_create(const SaNameT *csi_name); > extern bool csi_assignment_validate(struct avd_sg_tag *sg); > +extern SaAisErrorT csi_assign_hdlr(AVD_CSI *csi); > > #endif ------------------------------------------------------------------------------ "Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE Instantly run your Selenium tests across 300+ browser/OS combos. Get unparalleled scalability from the best Selenium testing platform available Simple to use. Nothing to install. Get started now for free." http://p.sf.net/sfu/SauceLabs _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
