Hi Nagu, please see comment inlined with [HansN] /Thanks HansN
On 09/09/2015 11:58 AM, Nagendra Kumar wrote: > Please find comment inlined with [Nagu]. > > Thanks > -Nagu >> -----Original Message----- >> From: Hans Nordeback [mailto:[email protected]] >> Sent: 14 August 2015 20:20 >> To: Praveen Malviya; Nagendra Kumar; [email protected] >> Cc: [email protected] >> Subject: [PATCH 10 of 10] amfd: Make AVD_SUTYPE a class [#1142] >> >> osaf/services/saf/amf/amfd/include/su.h | 3 +- >> osaf/services/saf/amf/amfd/include/sutcomptype.h | 2 +- >> osaf/services/saf/amf/amfd/include/sutype.h | 24 +++- >> osaf/services/saf/amf/amfd/sgtype.cc | 2 +- >> osaf/services/saf/amf/amfd/su.cc | 6 +- >> osaf/services/saf/amf/amfd/sutcomptype.cc | 2 +- >> osaf/services/saf/amf/amfd/sutype.cc | 108 >> +++++++++------------- >> 7 files changed, 70 insertions(+), 77 deletions(-) >> >> >> diff --git a/osaf/services/saf/amf/amfd/include/su.h >> b/osaf/services/saf/amf/amfd/include/su.h >> --- a/osaf/services/saf/amf/amfd/include/su.h >> +++ b/osaf/services/saf/amf/amfd/include/su.h >> @@ -35,6 +35,7 @@ >> #include "include/db_template.h" >> >> class AVD_SG; >> +class AVD_SUTYPE; >> >> /** >> * AMF director Service Unit representation. >> @@ -94,7 +95,7 @@ class AVD_SU { >> >> AVD_SU *sg_list_su_next; /* the next SU in the SG */ >> AVD_SU *avnd_list_su_next; /* the next SU in the AvND */ >> - struct avd_sutype *su_type; >> + AVD_SUTYPE *su_type; >> AVD_SU *su_list_su_type_next; >> >> void set_su_failover(bool value); >> diff --git a/osaf/services/saf/amf/amfd/include/sutcomptype.h >> b/osaf/services/saf/amf/amfd/include/sutcomptype.h >> --- a/osaf/services/saf/amf/amfd/include/sutcomptype.h >> +++ b/osaf/services/saf/amf/amfd/include/sutcomptype.h >> @@ -36,7 +36,7 @@ typedef struct { >> } AVD_SUTCOMP_TYPE; >> extern AmfDb<std::string, AVD_SUTCOMP_TYPE> *sutcomptype_db; >> >> -SaAisErrorT avd_sutcomptype_config_get(SaNameT *sutype_name, struct >> avd_sutype *sut); >> +SaAisErrorT avd_sutcomptype_config_get(SaNameT *sutype_name, >> AVD_SUTYPE *sut); >> void avd_sutcomptype_constructor(void); >> >> #endif >> diff --git a/osaf/services/saf/amf/amfd/include/sutype.h >> b/osaf/services/saf/amf/amfd/include/sutype.h >> --- a/osaf/services/saf/amf/amfd/include/sutype.h >> +++ b/osaf/services/saf/amf/amfd/include/sutype.h >> @@ -20,16 +20,24 @@ >> >> #include <saAis.h> >> #include <su.h> >> +#include <vector> >> >> -struct avd_sutype { >> - SaNameT name; >> - SaUint32T saAmfSutIsExternal; >> - SaUint32T saAmfSutDefSUFailover; >> - SaNameT *saAmfSutProvidesSvcTypes; /* array of DNs, size in >> number_svc_types */ >> - unsigned int number_svc_types; /* size of array >> saAmfSutProvidesSvcTypes */ >> - AVD_SU *list_of_su; >> +class AVD_SUTYPE { >> + public: >> + explicit AVD_SUTYPE(const SaNameT *dn); >> + SaNameT name {}; >> + SaUint32T saAmfSutIsExternal {}; >> + SaUint32T saAmfSutDefSUFailover {}; >> + SaNameT *saAmfSutProvidesSvcTypes {}; /* array of DNs, size in >> number_svc_types */ >> + unsigned int number_svc_types {}; /* size of array >> saAmfSutProvidesSvcTypes */ >> + std::vector<AVD_SU*> list_of_su {}; >> + private: >> + AVD_SUTYPE(); >> + // disallow copy and assign >> + AVD_SUTYPE(const AVD_SUTYPE&); >> + void operator=(const AVD_SUTYPE&); >> }; >> -extern AmfDb<std::string, avd_sutype> *sutype_db; >> +extern AmfDb<std::string, AVD_SUTYPE> *sutype_db; >> >> /** >> * Get SaAmfSUType from IMM and create internal objects >> diff --git a/osaf/services/saf/amf/amfd/sgtype.cc >> b/osaf/services/saf/amf/amfd/sgtype.cc >> --- a/osaf/services/saf/amf/amfd/sgtype.cc >> +++ b/osaf/services/saf/amf/amfd/sgtype.cc >> @@ -84,7 +84,7 @@ static int is_config_valid(const SaNameT >> char *parent; >> SaUint32T value; >> SaBoolT abool; >> - struct avd_sutype *sut; >> + AVD_SUTYPE *sut; >> const SaImmAttrValuesT_2 *attr; >> >> if ((parent = strchr((char*)dn->value, ',')) == NULL) { >> diff --git a/osaf/services/saf/amf/amfd/su.cc >> b/osaf/services/saf/amf/amfd/su.cc >> --- a/osaf/services/saf/amf/amfd/su.cc >> +++ b/osaf/services/saf/amf/amfd/su.cc >> @@ -230,7 +230,7 @@ static int is_config_valid(const SaNameT >> SaAmfAdminStateT admstate; >> char *parent; >> SaUint32T saAmfSutIsExternal; >> - struct avd_sutype *sut = NULL; >> + AVD_SUTYPE *sut = NULL; >> CcbUtilOperationData_t *tmp; >> AVD_SG *sg; >> >> @@ -416,7 +416,7 @@ static AVD_SU *su_create(const SaNameT * >> { >> int rc = -1; >> AVD_SU *su; >> - struct avd_sutype *sut; >> + AVD_SUTYPE *sut; >> SaAisErrorT error; >> >> TRACE_ENTER2("'%s'", dn->value); >> @@ -1691,7 +1691,7 @@ static void su_ccb_apply_modify_hdlr(str >> su- >>> saAmfSUMaintenanceCampaign.value, su->name.value); >> } >> } else if (!strcmp(attr_mod->modAttr.attrName, >> "saAmfSUType")) { >> - struct avd_sutype *sut; >> + AVD_SUTYPE *sut; >> SaNameT sutype_name = *(SaNameT*) attr_mod- >>> modAttr.attrValues[0]; >> TRACE("Modified saAmfSUType from '%s' to '%s'", su- >>> saAmfSUType.value, sutype_name.value); >> sut = sutype_db- >>> find(Amf::to_string(&sutype_name)); >> diff --git a/osaf/services/saf/amf/amfd/sutcomptype.cc >> b/osaf/services/saf/amf/amfd/sutcomptype.cc >> --- a/osaf/services/saf/amf/amfd/sutcomptype.cc >> +++ b/osaf/services/saf/amf/amfd/sutcomptype.cc >> @@ -65,7 +65,7 @@ static int is_config_valid(const SaNameT >> return 1; >> } >> >> -SaAisErrorT avd_sutcomptype_config_get(SaNameT *sutype_name, struct >> avd_sutype *sut) >> +SaAisErrorT avd_sutcomptype_config_get(SaNameT *sutype_name, >> AVD_SUTYPE *sut) >> { >> AVD_SUTCOMP_TYPE *sutcomptype; >> SaAisErrorT error; >> diff --git a/osaf/services/saf/amf/amfd/sutype.cc >> b/osaf/services/saf/amf/amfd/sutype.cc >> --- a/osaf/services/saf/amf/amfd/sutype.cc >> +++ b/osaf/services/saf/amf/amfd/sutype.cc >> @@ -27,37 +27,41 @@ >> #include <cluster.h> >> #include <ntf.h> >> #include <proc.h> >> +#include <algorithm> >> >> -AmfDb<std::string, avd_sutype> *sutype_db = NULL; >> +AmfDb<std::string, AVD_SUTYPE> *sutype_db = NULL; >> >> -static struct avd_sutype *sutype_new(const SaNameT *dn) >> +// >> +AVD_SUTYPE::AVD_SUTYPE(const SaNameT *dn) { >> + memcpy(&name.value, dn->value, dn->length); >> + name.length = dn->length; >> +} >> + >> +static AVD_SUTYPE *sutype_new(const SaNameT *dn) >> { >> - struct avd_sutype *sutype = new avd_sutype(); >> - >> - memcpy(sutype->name.value, dn->value, dn->length); >> - sutype->name.length = dn->length; >> + AVD_SUTYPE *sutype = new AVD_SUTYPE(dn); >> >> return sutype; >> } >> >> -static void sutype_delete(struct avd_sutype **sutype) >> +static void sutype_delete(AVD_SUTYPE **sutype) >> { >> - osafassert(NULL == (*sutype)->list_of_su); >> + osafassert(true == (*sutype)->list_of_su.empty()); >> delete [] (*sutype)->saAmfSutProvidesSvcTypes; >> delete *sutype; >> *sutype = NULL; >> } >> >> -static void sutype_db_add(struct avd_sutype *sutype) >> +static void sutype_db_add(AVD_SUTYPE *sutype) >> { >> unsigned int rc = sutype_db->insert(Amf::to_string(&sutype- >>> name),sutype); >> osafassert(rc == NCSCC_RC_SUCCESS); >> } >> >> -static struct avd_sutype *sutype_create(const SaNameT *dn, const >> SaImmAttrValuesT_2 **attributes) >> +static AVD_SUTYPE *sutype_create(const SaNameT *dn, const >> SaImmAttrValuesT_2 **attributes) >> { >> const SaImmAttrValuesT_2 *attr; >> - struct avd_sutype *sutype; >> + AVD_SUTYPE *sutype; >> int rc = 0; >> unsigned i = 0; >> SaAisErrorT error; >> @@ -166,7 +170,7 @@ static int is_config_valid(const SaNameT >> >> SaAisErrorT avd_sutype_config_get(void) >> { >> - struct avd_sutype *sut; >> + AVD_SUTYPE *sut; >> SaAisErrorT error; >> SaImmSearchHandleT searchHandle; >> SaImmSearchParametersT_2 searchParam; >> @@ -228,7 +232,7 @@ static void sutype_ccb_apply_modify_hdlr >> int i = 0; >> >> TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId, opdata- >>> objectName.value); >> - avd_sutype *sut = sutype_db->find(Amf::to_string(&opdata- >>> objectName)); >> + AVD_SUTYPE *sut = sutype_db->find(Amf::to_string(&opdata- >>> objectName)); >> while ((attr_mod = opdata->param.modify.attrMods[i++]) != NULL) { >> if (!strcmp(attr_mod->modAttr.attrName, >> "saAmfSutDefSUFailover")) { >> @@ -239,7 +243,7 @@ static void sutype_ccb_apply_modify_hdlr >> /* Modify saAmfSUFailover for SUs which had >> inherited saAmfSutDefSUFailover. >> Modification will not be done for the NPI SU >> */ >> if (old_value != sut->saAmfSutDefSUFailover) { >> - for (AVD_SU *su = sut->list_of_su; su; su = su- >>> su_list_su_type_next) { >> + for (const auto& su : sut->list_of_su) { >> if ((!su->saAmfSUFailover_configured) >> && (su->saAmfSUPreInstantiable)) { >> su- >>> set_su_failover(static_cast<bool>(sut->saAmfSutDefSUFailover)); >> } >> @@ -255,7 +259,7 @@ static void sutype_ccb_apply_modify_hdlr >> >> static void sutype_ccb_apply_cb(CcbUtilOperationData_t *opdata) >> { >> - struct avd_sutype *sut; >> + AVD_SUTYPE *sut; >> >> TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId, opdata- >>> objectName.value); >> @@ -291,7 +295,7 @@ static SaAisErrorT sutype_ccb_completed_ >> SaAisErrorT rc = SA_AIS_OK; >> const SaImmAttrModificationT_2 *attr_mod; >> int i = 0; >> - avd_sutype *sut = sutype_db->find(Amf::to_string(&opdata- >>> objectName)); >> + AVD_SUTYPE *sut = sutype_db->find(Amf::to_string(&opdata- >>> objectName)); >> TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId, opdata- >>> objectName.value); >> while ((attr_mod = opdata->param.modify.attrMods[i++]) != NULL) { >> @@ -311,7 +315,7 @@ static SaAisErrorT sutype_ccb_completed_ >> goto done; >> } >> >> - for (AVD_SU *su = sut->list_of_su; su; su = su- >>> su_list_su_type_next) { >> + for (const auto& su : sut->list_of_su) { >> if (su->saAmfSUFailover_configured) >> continue; >> >> @@ -341,8 +345,7 @@ done: >> static SaAisErrorT sutype_ccb_completed_cb(CcbUtilOperationData_t >> *opdata) >> { >> SaAisErrorT rc = SA_AIS_ERR_BAD_OPERATION; >> - struct avd_sutype *sut; >> - AVD_SU *su; >> + AVD_SUTYPE *sut; >> bool su_exist = false; >> CcbUtilOperationData_t *t_opData; >> >> @@ -358,24 +361,23 @@ static SaAisErrorT sutype_ccb_completed_ >> break; >> case CCBUTIL_DELETE: >> sut = sutype_db->find(Amf::to_string(&opdata->objectName)); >> - if (NULL != sut->list_of_su) { >> - /* check whether there exists a delete operation for >> - * each of the SU in the su_type list in the current CCB >> - */ >> - su = sut->list_of_su; >> - while (su != NULL) { >> - t_opData = >> ccbutil_getCcbOpDataByDN(opdata->ccbId, &su->name); >> - if ((t_opData == NULL) || (t_opData- >>> operationType != CCBUTIL_DELETE)) { >> - su_exist = true; >> - break; >> - } >> - su = su->su_list_su_type_next; >> - } >> - if (su_exist == true) { >> - report_ccb_validation_error(opdata, >> "SaAmfSUType '%s'is in use",sut->name.value); >> - goto done; >> + >> + /* check whether there exists a delete operation for >> + * each of the SU in the su_type list in the current CCB >> + */ >> + >> + for (const auto& su : sut->list_of_su) { >> + t_opData = ccbutil_getCcbOpDataByDN(opdata- >>> ccbId, &su->name); >> + if ((t_opData == NULL) || (t_opData->operationType >> != CCBUTIL_DELETE)) { >> + su_exist = true; >> + break; >> } >> } >> + >> + if (su_exist == true) { >> + report_ccb_validation_error(opdata, "SaAmfSUType >> '%s'is in use",sut->name.value); >> + goto done; >> + } >> rc = SA_AIS_OK; >> break; >> default: >> @@ -391,41 +393,23 @@ done: >> /* Add SU to list in SU Type */ >> void avd_sutype_add_su(AVD_SU* su) >> { >> - struct avd_sutype *sut = su->su_type; >> - su->su_list_su_type_next = sut->list_of_su; >> - sut->list_of_su = su; >> + AVD_SUTYPE *sut = su->su_type; >> + sut->list_of_su.push_back(su); >> } >> >> -void avd_sutype_remove_su(AVD_SU* su) >> -{ >> - AVD_SU *i_su = NULL; >> - AVD_SU *prev_su = NULL; >> +void avd_sutype_remove_su(AVD_SU* su) { >> + AVD_SUTYPE *su_type = su->su_type; >> >> - if (su->su_type != NULL) { >> - i_su = su->su_type->list_of_su; >> - >> - while ((i_su != NULL) && (i_su != su)) { >> - prev_su = i_su; >> - i_su = i_su->su_list_su_type_next; >> - } >> - >> - if (i_su == su) { >> - if (prev_su == NULL) { >> - su->su_type->list_of_su = su- >>> su_list_su_type_next; >> - } else { >> - prev_su->su_list_su_type_next = su- >>> su_list_su_type_next; >> - } >> - >> - su->su_list_su_type_next = NULL; >> - su->su_type = NULL; >> - } >> - } >> + if (su_type != nullptr) { >> + su_type->list_of_su.erase(std::remove(su_type->list_of_su.begin(), >> + su_type->list_of_su.end(), su), >> su_type- >>> list_of_su.end()); > [Nagu]: Looks complex to understand, can it be done as like #9th of patch > series? [HansN] It can be done like #9th patch, the reason std::find is used in #9 is because of the osafassert, that checks if si exists in svc_type->list_of_si, this is not needed in #10. The use of std::remove, in #10, is to show an alternative and (in my opinion) easier way to express the remove/erase idiom. > >> + } >> } >> >> void avd_sutype_constructor(void) >> { >> >> - sutype_db = new AmfDb<std::string, avd_sutype>; >> + sutype_db = new AmfDb<std::string, AVD_SUTYPE>; >> avd_class_impl_set("SaAmfSUBaseType", NULL, NULL, >> avd_imm_default_OK_completed_cb, NULL); >> avd_class_impl_set("SaAmfSUType", NULL, NULL, ------------------------------------------------------------------------------ Monitor Your Dynamic Infrastructure at Any Scale With Datadog! Get real-time metrics from all of your servers, apps and tools in one place. SourceForge users - Click here to start your Free Trial of Datadog now! http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140 _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
