see comment below/Thanks HansN On 09/09/2015 01:53 PM, Nagendra Kumar wrote: > Thanks for the clarification, few more needed, please check below. > >> -----Original Message----- >> From: Hans Nordebäck [mailto:hans.nordeb...@ericsson.com] >> Sent: 09 September 2015 17:09 >> To: Nagendra Kumar; Praveen Malviya; gary....@dektech.com.au >> Cc: opensaf-devel@lists.sourceforge.net >> Subject: Re: [PATCH 10 of 10] amfd: Make AVD_SUTYPE a class [#1142] >> >> Hi Nagu, >> >> please see my comment inlined with [HansN] /Thanks HansN >> >> On 09/09/2015 01:13 PM, Nagendra Kumar wrote: >>> Please check my comment below >>> >>> Thanks >>> -Nagu >>>> -----Original Message----- >>>> From: Hans Nordebäck [mailto:hans.nordeb...@ericsson.com] >>>> Sent: 09 September 2015 15:59 >>>> To: Nagendra Kumar; Praveen Malviya; gary....@dektech.com.au >>>> Cc: opensaf-devel@lists.sourceforge.net >>>> Subject: Re: [PATCH 10 of 10] amfd: Make AVD_SUTYPE a class [#1142] >>>> >>>> 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:hans.nordeb...@ericsson.com] >>>>>> Sent: 14 August 2015 20:20 >>>>>> To: Praveen Malviya; Nagendra Kumar; gary....@dektech.com.au >>>>>> Cc: opensaf-devel@lists.sourceforge.net >>>>>> 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. >>> [Nagu]: I got confused with erase taking first element as 'an element after >> remove' and then it erases from >>> 'removed item till end of the list i.e. range ?' Am I right ? can we >>> refactor it >> ? >> [HansN] >> The std::remove doesn't remove anything it returns a new iterator to a >> new end where objects to be erased has >> been transformed. Please see e.g. >> http://www.cplusplus.com/reference/algorithm/remove/?kw=remove > [Nagu]: Yes, I had read it before and I found it confusing. That means the > statement is equivalent to : > su_type->list_of_su.erase(Iterator, su_type-> list_of_su.end()) > So, does it erase a range of elements from iterator till end ? [HansN] yes, the elements to be erased are at the new iterator till end. > > >>>>>> + } >>>>>> } >>>>>> >>>>>> 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 Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
