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 > >>>> + } >>>> } >>>> >>>> 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
