I have refloated this patch after incorporating comments. Please see inline marked status "done" for the incorporated comment.
Thanks Praveen On 24-Jun-13 4:30 PM, Hans Feldt wrote: > Hi, > > AVD_SU saAmfSUFailover should be changed to bool (instead of SaBoolT), > that will avoid a number of casts. done. > See inline. > > Thanks, > Hans > > On 7 June 2013 08:39,<[email protected]> wrote: >> osaf/services/saf/avsv/avd/avd_comp.cc | 9 +- >> osaf/services/saf/avsv/avd/avd_sg.cc | 10 ++ >> osaf/services/saf/avsv/avd/avd_su.cc | 79 +++++++++++++++++++- >> osaf/services/saf/avsv/avd/avd_sutype.cc | 104 >> +++++++++++++++++++++++++++- >> osaf/services/saf/avsv/avd/include/avd_su.h | 2 + >> 5 files changed, 196 insertions(+), 8 deletions(-) >> >> >> The patch allows modify operation on attribute saAmfSutDefSUFailover of >> class SaAmfSUType. >> Modificaion of saAmfSUFailover in SaAmfSU is allowed only when SG is >> stable. >> >> diff --git a/osaf/services/saf/avsv/avd/avd_comp.cc >> b/osaf/services/saf/avsv/avd/avd_comp.cc >> --- a/osaf/services/saf/avsv/avd/avd_comp.cc >> +++ b/osaf/services/saf/avsv/avd/avd_comp.cc >> @@ -1449,8 +1449,13 @@ static void comp_ccb_apply_delete_hdlr(s >> /* if SU is not being deleted and the PreInstantiable state has >> changed >> * then update the IMM with the new value for >> saAmfSUPreInstantiable */ >> if (su_delete == SA_FALSE && old_val != >> comp->su->saAmfSUPreInstantiable) { >> - avd_saImmOiRtObjectUpdate(&comp->su->name, >> const_cast<SaImmAttrNameT>("saAmfSUPreInstantiable"), SA_IMM_ATTR_SAUINT32T, >> - &comp->su->saAmfSUPreInstantiable); >> + avd_saImmOiRtObjectUpdate(&comp->su->name, >> const_cast<SaImmAttrNameT>("saAmfSUPreInstantiable"), >> + SA_IMM_ATTR_SAUINT32T, >> &comp->su->saAmfSUPreInstantiable); >> + /* If SU becomes NPI then enable saAmfSUFailover flag Sec >> 3.11.1.3.2 AMF-B.04.01 spec */ >> + if (comp->su->saAmfSUPreInstantiable == false) { > do not check for == false, negate the expression instead (using !) done >> + comp->su->saAmfSUFailover = >> static_cast<SaBoolT>(true); >> + su_nd_attribute_update(comp->su, saAmfSUFailOver_ID); >> + } >> } >> >> /* send a message to the AVND deleting the >> diff --git a/osaf/services/saf/avsv/avd/avd_sg.cc >> b/osaf/services/saf/avsv/avd/avd_sg.cc >> --- a/osaf/services/saf/avsv/avd/avd_sg.cc >> +++ b/osaf/services/saf/avsv/avd/avd_sg.cc >> @@ -1386,6 +1386,8 @@ static void avd_verify_equal_ranked_su(A >> */ >> void avd_sg_adjust_config(AVD_SG *sg) >> { >> + AVD_SU *su; > move down to block where needed done >> + >> // SUs in an SG are equal, only need to look at the first one >> if ((sg->list_of_su != NULL) && >> !sg->list_of_su->saAmfSUPreInstantiable) { >> // NPI SUs can only be assigned one SI, see 3.2.1.1 >> @@ -1400,6 +1402,14 @@ void avd_sg_adjust_config(AVD_SG *sg) >> sg->saAmfSGMaxStandbySIsperSU, >> sg->name.value); >> } >> sg->saAmfSGMaxStandbySIsperSU = 1; >> + /* saAmfSUFailover must be true for a NPI SU sec 3.11.1.3.2 >> AMF-B.04.01 spec */ >> + for (su = sg->list_of_su; su != NULL; su = >> su->sg_list_su_next) { >> + if (!su->saAmfSUFailover) { >> + su->saAmfSUFailover = >> static_cast<SaBoolT>(true); >> + su_nd_attribute_update(su, >> saAmfSUFailOver_ID); >> + } >> + } >> + >> } >> >> /* adjust saAmfSGNumPrefInserviceSUs if not configured */ >> diff --git a/osaf/services/saf/avsv/avd/avd_su.cc >> b/osaf/services/saf/avsv/avd/avd_su.cc >> --- a/osaf/services/saf/avsv/avd/avd_su.cc >> +++ b/osaf/services/saf/avsv/avd/avd_su.cc >> @@ -481,6 +481,8 @@ static AVD_SU *su_create(const SaNameT * >> if (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSUFailover"), >> attributes, 0, &su->saAmfSUFailover) != SA_AIS_OK) { >> su->saAmfSUFailover = >> static_cast<SaBoolT>(sut->saAmfSutDefSUFailover); >> } >> + else >> + su->sufailover_conf = true; > "sufailover_conf": I think we need to establish a pattern here since > we have the problem in different scenarios. > > First, spell out "configured". > > Second I think we should use the same prefix as the variable it is > associated with. This gives us: > > "saAmfSUFailover_configured" done >> >> (void)immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSUMaintenanceCampaign"), >> attributes, 0, &su->saAmfSUMaintenanceCampaign); >> >> @@ -1228,9 +1230,16 @@ static SaAisErrorT su_ccb_completed_modi >> continue; >> >> if (!strcmp(attr_mod->modAttr.attrName, "saAmfSUFailover")) >> { >> - bool su_failover = *((SaTimeT >> *)attr_mod->modAttr.attrValues[0]); >> + AVD_SU *su; >> + uint32_t su_failover = *((SaUint32T >> *)attr_mod->modAttr.attrValues[0]); >> + su = avd_su_get(&opdata->objectName); > define and initialize su variable in one go. done >> + if (su->sg_of_su->sg_fsm_state != AVD_SG_FSM_STABLE) >> { >> + rc = SA_AIS_ERR_TRY_AGAIN; >> + LOG_WA("SG state is not stable"); > change log to trace or remove done >> + goto done; >> + } > Can't we just remove this "STABLE" check? > >> if (su_failover > SA_TRUE) { >> - LOG_ER("Invalid saAmfSUFailover %u", >> su_failover); >> + LOG_ER("Invalid saAmfSUFailover SU:'%s'", >> su->name.value); >> rc = SA_AIS_ERR_BAD_OPERATION; >> goto done; >> } >> @@ -1410,10 +1419,15 @@ static void su_ccb_apply_modify_hdlr(str >> value_is_deleted = false; >> >> if (!strcmp(attr_mod->modAttr.attrName, "saAmfSUFailover")) >> { >> - if (value_is_deleted) >> + if (value_is_deleted) { >> su->saAmfSUFailover = >> static_cast<SaBoolT>(su->su_type->saAmfSutDefSUFailover); >> - else >> + su->sufailover_conf = false; >> + } >> + else { >> su->saAmfSUFailover = >> static_cast<SaBoolT>(*((SaUint32T *)attr_mod->modAttr.attrValues[0])); >> + su->sufailover_conf = true; >> + } >> + su_nd_attribute_update(su, saAmfSUFailOver_ID); >> } else if (!strcmp(attr_mod->modAttr.attrName, >> "saAmfSUMaintenanceCampaign")) { >> if (value_is_deleted) { >> su->saAmfSUMaintenanceCampaign.length = 0; >> @@ -1432,7 +1446,14 @@ static void su_ccb_apply_modify_hdlr(str >> su->saAmfSUType = sutype_name; >> su->su_type = sut; >> avd_sutype_add_su(su); >> - su->saAmfSUFailover = >> static_cast<SaBoolT>(sut->saAmfSutDefSUFailover); >> + if (su->saAmfSUPreInstantiable) { >> + su->saAmfSUFailover = >> static_cast<SaBoolT>(sut->saAmfSutDefSUFailover); >> + su->sufailover_conf = false; >> + } else { >> + su->saAmfSUFailover = >> static_cast<SaBoolT>(true); >> + su->sufailover_conf = true; >> + } >> + su_nd_attribute_update(su, saAmfSUFailOver_ID); >> su->su_is_external = sut->saAmfSutIsExternal; >> } else >> osafassert(0); >> @@ -1565,3 +1586,51 @@ void avd_su_constructor(void) >> avd_class_impl_set(const_cast<SaImmClassNameT>("SaAmfSU"), >> su_rt_attr_cb, su_admin_op_cb, su_ccb_completed_cb, su_ccb_apply_cb); >> } >> >> +/** >> + * update these attributes on the nd > rather send attribute update to node director done >> + * @param su >> + * @param attrib_id >> + * @param value > no value! done >> + */ >> +void su_nd_attribute_update(AVD_SU *su, uint32_t attrib_id) > add const to su > for attrib_id use enum AVSV_AMF_SU_ATTR_ID done >> +{ >> + AVD_AVND *su_node_ptr = NULL; >> + AVSV_PARAM_INFO param; >> + memset(((uint8_t *)¶m), '\0', sizeof(AVSV_PARAM_INFO)); >> + >> + TRACE_ENTER(); >> + >> + if (avd_cb->avail_state_avd != SA_AMF_HA_ACTIVE) { >> + TRACE_LEAVE2("avd is not in active state"); >> + return; >> + } >> + m_AVD_GET_SU_NODE_PTR(avd_cb, su, su_node_ptr); >> + param.class_id = AVSV_SA_AMF_SU; >> + param.act = AVSV_OBJ_OPR_MOD; >> + param.name = su->name; >> + >> + switch (attrib_id) { >> + case saAmfSUFailOver_ID: >> + { >> + uint32_t sufailover; >> + param.attr_id = saAmfSUFailOver_ID; >> + param.value_len = sizeof(uint32_t); >> + sufailover = htonl(su->saAmfSUFailover); >> + memcpy(¶m.value[0], &sufailover, param.value_len); >> + break; >> + } >> + default: >> + osafassert(0); >> + } >> + >> + /*Update this value on the node hosting this SU*/ >> + if ((su_node_ptr) && ((su_node_ptr->node_state == >> AVD_AVND_STATE_PRESENT) || >> + (su_node_ptr->node_state == >> AVD_AVND_STATE_NO_CONFIG) || >> + (su_node_ptr->node_state == >> AVD_AVND_STATE_NCS_INIT))) { >> + if (avd_snd_op_req_msg(avd_cb, su_node_ptr, ¶m) != >> NCSCC_RC_SUCCESS) { >> + LOG_ER("%s:failed for %s",__FUNCTION__, >> su_node_ptr->name.value); >> + } >> + } >> + >> + TRACE_LEAVE(); >> +} >> diff --git a/osaf/services/saf/avsv/avd/avd_sutype.cc >> b/osaf/services/saf/avsv/avd/avd_sutype.cc >> --- a/osaf/services/saf/avsv/avd/avd_sutype.cc >> +++ b/osaf/services/saf/avsv/avd/avd_sutype.cc >> @@ -28,6 +28,8 @@ >> #include <avd_proc.h> >> >> static NCS_PATRICIA_TREE sutype_db; >> +static void sutype_ccb_apply_modify_hdlr(struct CcbUtilOperationData >> *opdata); >> +static SaAisErrorT sutype_ccb_completed_modify_hdlr(CcbUtilOperationData_t >> *opdata); > move the source for the above functions to before they are used and > remove the above forward declarations done >> static struct avd_sutype *sutype_new(const SaNameT *dn) >> { >> @@ -249,6 +251,9 @@ static void sutype_ccb_apply_cb(CcbUtilO >> osafassert(sut); >> sutype_db_add(sut); >> break; >> + case CCBUTIL_MODIFY: >> + sutype_ccb_apply_modify_hdlr(opdata); >> + break; >> case CCBUTIL_DELETE: >> sut = avd_sutype_get(&opdata->objectName); >> sutype_db_delete(sut); >> @@ -278,7 +283,7 @@ static SaAisErrorT sutype_ccb_completed_ >> rc = SA_AIS_OK; >> break; >> case CCBUTIL_MODIFY: >> - LOG_ER("Modification of SaAmfSUType not supported"); >> + rc = sutype_ccb_completed_modify_hdlr(opdata); >> break; >> case CCBUTIL_DELETE: >> sut = avd_sutype_get(&opdata->objectName); >> @@ -356,3 +361,100 @@ void avd_sutype_constructor(void) >> avd_class_impl_set(const_cast<SaImmClassNameT>("SaAmfSUBaseType"), >> NULL, NULL, avd_imm_default_OK_completed_cb, NULL); >> avd_class_impl_set(const_cast<SaImmClassNameT>("SaAmfSUType"), >> NULL, NULL, sutype_ccb_completed_cb, sutype_ccb_apply_cb); >> } >> + >> +/** >> + * This function handles modify operations on SaAmfSUType objects. >> + * @param ptr to opdata >> + */ >> +static void sutype_ccb_apply_modify_hdlr(struct CcbUtilOperationData >> *opdata) >> +{ >> + const SaImmAttrModificationT_2 *attr_mod; >> + int i = 0; >> + avd_sutype *sut; >> + AVD_SU *su; > move to block where needed done >> + uint32_t old_value; >> + >> + TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId, >> opdata->objectName.value); >> + >> + sut = avd_sutype_get(&opdata->objectName); >> + >> + while ((attr_mod = opdata->param.modify.attrMods[i++]) != NULL) { >> + if (!strcmp(attr_mod->modAttr.attrName, >> "saAmfSutDefSUFailover")) { >> + old_value = sut->saAmfSutDefSUFailover; >> + sut->saAmfSutDefSUFailover = *((SaUint32T >> *)attr_mod->modAttr.attrValues[0]); >> + /* Modify saAmfSUFailover for SUs which had >> inherited saAmfSutDefSUFailover. >> + Modification will not be done for the NPI SU >> */ >> + if (old_value != sut->saAmfSutDefSUFailover) { >> + su = sut->list_of_su; > define and initialize su in one go done >> + while (su != NULL) { > use for loop done >> + if ((!su->sufailover_conf) && >> (su->saAmfSUPreInstantiable)) { >> + su->saAmfSUFailover = >> static_cast<SaBoolT>(sut->saAmfSutDefSUFailover); >> + su_nd_attribute_update(su, >> saAmfSUFailOver_ID); >> + } >> + su = su->su_list_su_type_next; >> + } >> + } >> + } else >> + osafassert(0); >> + } >> + >> + TRACE_LEAVE(); >> +} >> + >> +/** >> + * This routine validates modify CCB operations on SaAmfSUType objects. >> + * @param Ccb Util Oper Data >> + */ >> + >> +static SaAisErrorT sutype_ccb_completed_modify_hdlr(CcbUtilOperationData_t >> *opdata) >> +{ >> + SaAisErrorT rc = SA_AIS_OK; >> + const SaImmAttrModificationT_2 *attr_mod; >> + int i = 0; >> + avd_sutype *sut; >> + >> + sut = avd_sutype_get(&opdata->objectName); >> + >> + TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId, >> opdata->objectName.value); >> + while ((attr_mod = opdata->param.modify.attrMods[i++]) != NULL) { >> + >> + if ((attr_mod->modType == SA_IMM_ATTR_VALUES_DELETE) || >> (attr_mod->modAttr.attrValues == NULL)) { >> + LOG_ER("Attributes cannot be deleted in >> SaAmfSUType"); >> + rc = SA_AIS_ERR_BAD_OPERATION; >> + goto done; >> + } >> + >> + if (!strcmp(attr_mod->modAttr.attrName, >> "saAmfSutDefSUFailover")) { >> + AVD_SU *su = sut->list_of_su; >> + uint32_t sut_failover = *((SaUint32T >> *)attr_mod->modAttr.attrValues[0]); >> + >> + if (sut_failover > SA_TRUE) { >> + LOG_ER("invalid saAmfSutDefSUFailover >> in:'%s'", su->name.value); >> + rc = SA_AIS_ERR_BAD_OPERATION; >> + goto done; >> + } >> + >> + while (su != NULL) { > use for loop done >> + if (su->sufailover_conf) { >> + su = su->su_list_su_type_next; > can be remove with for loop done >> + continue; >> + } >> + >> + if (su->sg_of_su->sg_fsm_state != >> AVD_SG_FSM_STABLE) { >> + rc = SA_AIS_ERR_TRY_AGAIN; >> + LOG_WA("SG state is not stable"); >> + goto done; >> + } >> + su = su->su_list_su_type_next; >> + } >> + } else { >> + LOG_ER("attribute is non writable:'%s' ", >> attr_mod->modAttr.attrName); >> + rc = SA_AIS_ERR_BAD_OPERATION; >> + goto done; >> + } >> + } >> + >> +done: >> + TRACE_LEAVE(); >> + return rc; >> +} >> diff --git a/osaf/services/saf/avsv/avd/include/avd_su.h >> b/osaf/services/saf/avsv/avd/include/avd_su.h >> --- a/osaf/services/saf/avsv/avd/include/avd_su.h >> +++ b/osaf/services/saf/avsv/avd/include/avd_su.h >> @@ -64,6 +64,7 @@ typedef struct avd_su_tag { >> uint32_t saAmfSURank; >> SaNameT saAmfSUHostNodeOrNodeGroup; >> SaBoolT saAmfSUFailover; >> + bool sufailover_conf; /* True when user configures saAmfSUFailover >> else false */ >> SaNameT saAmfSUMaintenanceCampaign; >> >> /* runtime attributes */ >> @@ -239,5 +240,6 @@ extern void avd_su_inc_curr_stdby_si(AVD >> extern void avd_su_dec_curr_stdby_si(AVD_SU *su); >> extern uint32_t avd_su_get_current_no_of_assignments(AVD_SU *su, >> SaAmfHAStateT ha_state); >> extern AVD_SU *avd_su_get_or_create(const SaNameT *dn); >> +extern void su_nd_attribute_update(AVD_SU *su, uint32_t attrib_id); >> >> #endif >> >> ------------------------------------------------------------------------------ >> How ServiceNow helps IT people transform IT departments: >> 1. A cloud service to automate IT design, transition and operations >> 2. Dashboards that offer high-level views of enterprise services >> 3. A single system of record for all IT processes >> http://p.sf.net/sfu/servicenow-d2d-j >> _______________________________________________ >> Opensaf-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
