Hi Nagu, There are problems building for opensaf-4.6.x with this patch, as 4.6.x don't have c++11 support enabled.
/Thanks HansN -----Original Message----- From: Nagendra Kumar [mailto:[email protected]] Sent: den 4 mars 2016 11:14 To: Hans Nordebäck; Praveen Malviya; Minh Chau H; Gary Lee Cc: [email protected] Subject: RE: [devel] [PATCH 1 of 1] amfd: delete su and its child objects at stdby amfd [#1683] Hi Hans N, Thanks for your review. I thought that accessing based on node name is bit safer than using pointers as in each loop the link is being deleted. But, anyway, I can clean this up some time later. Thanks -Nagu > -----Original Message----- > From: Hans Nordebäck [mailto:[email protected]] > Sent: 04 March 2016 15:35 > To: Nagendra Kumar; Praveen Malviya; Minh Chau H; Gary Lee > Cc: [email protected] > Subject: Re: [devel] [PATCH 1 of 1] amfd: delete su and its child > objects at stdby amfd [#1683] > > Hi Nagu, > > ack code review only. One minor comment below, > > these loops, etc: > > std::set<std::string> su_list; > std::set<std::string> comp_list; > for (const auto& su : node->list_of_su) > su_list.insert(Amf::to_string(&su->name)); > : > for (const auto& comp : su->list_of_comp) > comp_list.insert(Amf::to_string(&comp->comp_info.name)); > > > can be replaced by: > > > if (node->list_of_su.empty() != true) { > std::vector<AVD_SU*> su_list = node->list_of_su; > for (auto iter = su_list.begin(); iter != su_list.end(); > ++iter) { > AVD_SU *su = *iter; > TRACE("Standby Amfd, su '%s' not deleted", su->name.value); > std::vector<AVD_COMP*> comp_list = su->list_of_comp; > for (auto iter1 = comp_list.begin(); iter1 != > comp_list.end(); ++iter1) { > AVD_COMP *comp = *iter1; > TRACE("Standby Amfd, comp '%s' not deleted", > comp->comp_info.name.value); > > std::map<std::string, AVD_COMPCS_TYPE*>::iterator it = > compcstype_db->begin(); > while (it != compcstype_db->end()) { > AVD_COMPCS_TYPE *compcstype = it->second; > if (compcstype->comp == comp) { > TRACE("Standby Amfd, compcstype '%s' not > deleted", > compcstype->name.value); > it = compcstype_db->erase(it); > delete compcstype; > } > else > it++; > } > > I think it makes the code a bit cleaner. > > /Thanks HansN > > On 03/04/2016 08:19 AM, Nagendra Kumar wrote: > > Hi Hans N, > > Please find the modified attached patch. Please find some > time to review. > > > > Thanks > > -Nagu > >> -----Original Message----- > >> From: Hans Nordebäck [mailto:[email protected]] > >> Sent: 03 March 2016 12:48 > >> To: Nagendra Kumar; Praveen Malviya; Minh Chau H; Gary Lee > >> Cc: [email protected] > >> Subject: RE: [devel] [PATCH 1 of 1] amfd: delete su and its child > >> objects at stdby amfd [#1683] > >> > >> Hi Nagu, > >> > >> good , my questions below was related to the three iterators and > >> deleting while iterating. /Thanks HansN > >> > >> -----Original Message----- > >> From: Nagendra Kumar [mailto:[email protected]] > >> Sent: den 3 mars 2016 07:01 > >> To: Hans Nordebäck; Praveen Malviya; Minh Chau H; Gary Lee > >> Cc: [email protected] > >> Subject: RE: [devel] [PATCH 1 of 1] amfd: delete su and its child > >> objects at stdby amfd [#1683] > >> > >> Hi Hans, > >> I will send the modified patch. > >> > >> Thanks > >> -Nagu > >> > >>> -----Original Message----- > >>> From: Nagendra Kumar > >>> Sent: 03 March 2016 10:50 > >>> To: Hans Nordebäck; Praveen Malviya; Minh Chau H; Gary Lee > >>> Cc: [email protected] > >>> Subject: Re: [devel] [PATCH 1 of 1] amfd: delete su and its child > >>> objects at stdby amfd [#1683] > >>> > >>> Hi Hans N, > >>> Thanks for your review. If I incorporate the comment, Is that > >> Ack ? > >>> Thanks > >>> -Nagu > >>>> -----Original Message----- > >>>> From: Hans Nordebäck [mailto:[email protected]] > >>>> Sent: 29 February 2016 19:30 > >>>> To: Nagendra Kumar; Praveen Malviya; Minh Chau H; Gary Lee > >>>> Cc: [email protected]; Hans Nordebäck > >>>> Subject: RE: [PATCH 1 of 1] amfd: delete su and its child objects > >>>> at stdby amfd [#1683] > >>>> > >>>> Hi Nagu, minor correction below, (not tested or compiled though): > >>>> > >>>> std::map<std::string, AVD_COMPCS_TYPE*>::iterator it = > >>>> compcstype_db- > >>>>> begin(); > >>>> while (it != compcstype_db->end() { > >>>> AVD_COMPCS_TYPE *compcstype = it->second; if (compcstype- > >comp > >> == > >>>> comp) { > >>>> TRACE("Standby Amfd, compcstype '%s' not deleted", > >>>> compcstype->name.value); > >>>> it = compcstype_db->erase(it)); > >>>> delete compcstype; > >>>> else > >>>> it++ > >>>> } > >>>> } > >>>> > >>>> /Thanks HansN > >>>> -----Original Message----- > >>>> From: Hans Nordebäck > >>>> Sent: den 29 februari 2016 14:29 > >>>> To: [email protected]; [email protected]; Minh > Chau > >> H; > >>>> Gary Lee > >>>> Cc: [email protected]; Hans Nordebäck > >>>> Subject: Re: [PATCH 1 of 1] amfd: delete su and its child objects > >>>> at stdby amfd [#1683] > >>>> > >>>> Hi Nagu, > >>>> > >>>> the deepest for loop can be changed to: > >>>> > >>>> > >>>> and db_template.h is changed to: > >>>> > >>>> // c++11, returns iterator > >>>> iterator erase(const iterator &it) {return db.erase(it);} > >>>> > >>>> iterator begin() {return db.begin();} > >>>> iterator end() {return db.end();} > >>>> > >>>> const_iterator cbegin() const {return db.cbegin();} > >>>> const_iterator cend() const {return db.cend();} > >>>> > >>>> as c++11 returns an iterator in erase. It should be backwards > >>>> compatible and all present calls to begin can be changed later as > needed. > >>>> > >>>> The other two loops over su and comp, why is the iterator not > updated? > >>>> Elements in node list_of_su and > >>>> su list_of_comp is deleted while iterating, an alternative could > >>>> be to iterate over a copy of these lists and then the deletes > >>>> should not affect respective iterators. > >>>> > >>>> /Thanks HansN > >>>> > >>>> > >>>> > >>>> > >>>> On 02/26/2016 07:52 AM, [email protected] wrote: > >>>>> osaf/services/saf/amf/amfd/comp.cc | 2 +- > >>>>> osaf/services/saf/amf/amfd/include/comp.h | 1 + > >>>>> osaf/services/saf/amf/amfd/include/su.h | 2 +- > >>>>> osaf/services/saf/amf/amfd/node.cc | 54 > >>>> +++++++++++++++++++++++++++++++ > >>>>> osaf/services/saf/amf/amfd/su.cc | 2 +- > >>>>> osaf/services/saf/amf/amfd/util.cc | 16 +++++++++ > >>>>> 6 files changed, 74 insertions(+), 3 deletions(-) > >>>>> > >>>>> > >>>>> If SUs are deleted at Act Amfd when Stdby Amfd has finished > >>>>> reading objects from Imm database, Stdby Amfd still has those SUs. > >>>>> Now, if node hosting those Sus are deleted, Act Amfd says Ok, > >>>>> but Stdby Amfd says it still has some SUs left on that node to be > >>>>> deleted. > >>>>> Hence in apply callback, Stdby Amfd crashes. > >>>>> So, this patch makes sure that in this particular scenarios, if > >>>>> node is being deleted and if there are some Sus and its child > >>>>> obkects are still there, Stdby Amfd deletes them. > >>>>> > >>>>> diff --git a/osaf/services/saf/amf/amfd/comp.cc > >>>>> b/osaf/services/saf/amf/amfd/comp.cc > >>>>> --- a/osaf/services/saf/amf/amfd/comp.cc > >>>>> +++ b/osaf/services/saf/amf/amfd/comp.cc > >>>>> @@ -1577,7 +1577,7 @@ static void comp_ccb_apply_modify_hdlr(s > >>>>> TRACE_LEAVE(); > >>>>> } > >>>>> > >>>>> -static void comp_ccb_apply_delete_hdlr(struct > >>>>> CcbUtilOperationData > >>>>> *opdata) > >>>>> +void comp_ccb_apply_delete_hdlr(struct CcbUtilOperationData > >>>>> +*opdata) > >>>>> { > >>>>> TRACE_ENTER(); > >>>>> > >>>>> diff --git a/osaf/services/saf/amf/amfd/include/comp.h > >>>>> b/osaf/services/saf/amf/amfd/include/comp.h > >>>>> --- a/osaf/services/saf/amf/amfd/include/comp.h > >>>>> +++ b/osaf/services/saf/amf/amfd/include/comp.h > >>>>> @@ -275,4 +275,5 @@ bool comp_is_preinstantiable(const AVD_C > >>>>> extern bool is_comp_assigned_any_csi(AVD_COMP *comp); > >>>>> extern SaAisErrorT check_comp_stability(const AVD_COMP*); > >>>>> extern AVD_CTCS_TYPE *get_ctcstype(const SaNameT > >>> *comptype_name, > >>>>> const SaNameT *cstype_name); > >>>>> +extern void comp_ccb_apply_delete_hdlr(struct > >>>>> +CcbUtilOperationData *opdata); > >>>>> #endif > >>>>> 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 > >>>>> @@ -171,5 +171,5 @@ extern SaAisErrorT avd_su_config_get(con > >>>>> * Class constructor, must be called before any other function > >>>>> */ > >>>>> extern void avd_su_constructor(void); > >>>>> - > >>>>> +extern void su_ccb_apply_delete_hdlr(struct > >>>>> +CcbUtilOperationData *opdata); > >>>>> #endif > >>>>> diff --git a/osaf/services/saf/amf/amfd/node.cc > >>>>> b/osaf/services/saf/amf/amfd/node.cc > >>>>> --- a/osaf/services/saf/amf/amfd/node.cc > >>>>> +++ b/osaf/services/saf/amf/amfd/node.cc > >>>>> @@ -165,9 +165,56 @@ AVD_AVND *avd_node_new(const SaNameT > >>> *dn > >>>>> void avd_node_delete(AVD_AVND *node) > >>>>> { > >>>>> + TRACE_ENTER(); > >>>>> osafassert(node->pg_csi_list.n_nodes == 0); > >>>>> if (node->node_info.nodeId) > >>>>> avd_node_delete_nodeid(node); > >>>>> + /* Check if the SUs and related objects are still left. This can > >>>>> + happen on Standby Amfd when it has just read the > configuration > >>>>> + and before it becomes applier, Act Amfd deletes SUs. Those > SUs > >>>>> + will be left out at Standby Amfd. Though this could be > rare.*/ > >>>>> + if (avd_cb->avail_state_avd != SA_AMF_HA_ACTIVE) { > >>>>> + if (node->list_of_su.empty() != true) { > >>>>> + std::vector<AVD_SU*> *su_list = nullptr; > >>>>> + su_list = &node->list_of_su; > >>>>> + std::vector<AVD_SU*>::const_iterator > su_iter; > >>>>> + for (su_iter = su_list->begin(); su_iter != > su_list- > >>>>> end(); ) { > >>>>> + AVD_SU *su = *su_iter; > >>>>> + TRACE("Standby Amfd, su '%s' not > deleted", > >>>> su->name.value); > >>>>> + std::vector<AVD_COMP*> > *comp_list = > >>>> nullptr; > >>>>> + comp_list = &su->list_of_comp; > >>>>> + > std::vector<AVD_COMP*>::const_iterator > >>>> comp_iter; > >>>>> + for (comp_iter = comp_list->begin(); > >>>> comp_iter != comp_list->end(); ) { > >>>>> + AVD_COMP *comp = > *comp_iter; > >>>>> + TRACE("Standby Amfd, comp > '%s' not > >>>> deleted", comp->comp_info.name.value); > >>>>> + /* Delete the CompCsType. > */ > >>>>> + for (std::map<std::string, > >>>> AVD_COMPCS_TYPE*>::const_iterator it = > >>>>> + > compcstype_db- > >>>>> begin(); > >>>>> + it != > compcstype_db- > >>>>> end(); it++) { > >>>>> + AVD_COMPCS_TYPE > >>>> *compcstype = it->second; > >>>>> + if (compcstype- > >comp == > >>>> comp) { > >>>>> + > TRACE("Standby > >>>> Amfd, compcstype '%s' not deleted", > >>>>> + > >>>> compcstype->name.value); > >>>>> + > compcstype_db- > >>>>> erase(Amf::to_string(&compcstype->name)); > >>>>> + delete > compcstype; > >>>>> + } > >>>>> + } > >>>>> + > >>>>> + /* Delete the Comp. */ > >>>>> + struct CcbUtilOperationData > opdata; > >>>>> + > memset(&opdata.objectName, 0, > >>>> sizeof(SaNameT)); > >>>>> + > memcpy(opdata.objectName.value, > >>>> comp->comp_info.name.value, > >>>>> + comp- > >>>>> comp_info.name.length); > >>>>> + opdata.objectName.length = > comp- > >>>>> comp_info.name.length; > >>>>> + > >>>> comp_ccb_apply_delete_hdlr(&opdata); > >>>>> + } > >>>>> + /* Delete the SU. */ > >>>>> + struct CcbUtilOperationData opdata; > >>>>> + opdata.userData = su; > >>>>> + su_ccb_apply_delete_hdlr(&opdata); > >>>>> + } > >>>>> + } > >>>>> + } > >>>>> m_AVSV_SEND_CKPT_UPDT_ASYNC_RMV(avd_cb, node, > >>>> AVSV_CKPT_AVD_NODE_CONFIG); > >>>>> node_name_db->erase(Amf::to_string(&node->name)); > >>>>> delete [] node->node_name; > >>>>> @@ -493,6 +540,10 @@ static SaAisErrorT node_ccb_completed_de > >>>>> return SA_AIS_ERR_BAD_OPERATION; > >>>>> } > >>>>> > >>>>> + if (avd_cb->avail_state_avd != SA_AMF_HA_ACTIVE) { > >>>>> + goto done_ok; > >>>>> + } > >>>>> + > >>>>> /* Check to see that no SUs exists on this node */ > >>>>> if (node->list_of_su.empty() != true) { > >>>>> /* check whether there exists a delete operation for > @@ - > >>>> 569,6 > >>>>> +620,9 @@ static SaAisErrorT node_ccb_completed_de > >>>>> } > >>>>> } > >>>>> } > >>>>> + > >>>>> +done_ok: > >>>>> + rc = SA_AIS_OK; > >>>>> opdata->userData = node; > >>>>> done: > >>>>> TRACE_LEAVE(); > >>>>> 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 > >>>>> @@ -1783,7 +1783,7 @@ static void su_ccb_apply_modify_hdlr(str > >>>>> * > >>>>> * @param su > >>>>> */ > >>>>> -static void su_ccb_apply_delete_hdlr(struct > >>>>> CcbUtilOperationData > >>>>> *opdata) > >>>>> +void su_ccb_apply_delete_hdlr(struct CcbUtilOperationData > >>>>> +*opdata) > >>>>> { > >>>>> AVD_SU *su = static_cast<AVD_SU*>(opdata->userData); > >>>>> AVD_AVND *su_node_ptr; > >>>>> diff --git a/osaf/services/saf/amf/amfd/util.cc > >>>>> b/osaf/services/saf/amf/amfd/util.cc > >>>>> --- a/osaf/services/saf/amf/amfd/util.cc > >>>>> +++ b/osaf/services/saf/amf/amfd/util.cc > >>>>> @@ -1551,6 +1551,22 @@ int amfd_file_dump(const char *filename) > >>>>> fprintf(f, " saAmfCompCurrProxyName: > %s\n", > >>>> comp->saAmfCompCurrProxyName.value); > >>>>> } > >>>>> > >>>>> + fprintf(f, "COMPCS_TYPE:\n"); > >>>>> + for (std::map<std::string, > >>>>> + AVD_COMPCS_TYPE*>::const_iterator it = > >>>> compcstype_db->begin(); > >>>>> + it != compcstype_db->end(); it++) { > >>>>> + const AVD_COMPCS_TYPE *compcs_type = it->second; > >>>>> + fprintf(f, " dn: %s\n", compcs_type->name.value); > >>>>> + fprintf(f, " saAmfCompNumMaxActiveCSIs: %u\n", > >>>>> + > >>>>> compcs_type->saAmfCompNumMaxActiveCSIs); > >>>>> + fprintf(f, " saAmfCompNumMaxStandbyCSIs: %u\n", > >>>>> + > >>>>> compcs_type->saAmfCompNumMaxStandbyCSIs); > >>>>> + fprintf(f, " saAmfCompNumCurrActiveCSIs: %u\n", > >>>>> + > >>>>> compcs_type->saAmfCompNumCurrActiveCSIs); > >>>>> + fprintf(f, " saAmfCompNumCurrStandbyCSIs: %u\n", > >>>>> + > >>>>> compcs_type->saAmfCompNumCurrStandbyCSIs); > >>>>> + } > >>>>> + > >>>>> + > >>>>> fprintf(f, "Node Groups:\n"); > >>>>> for (std::map<std::string, AVD_AMF_NG*>::const_iterator it > = > >>>> nodegroup_db->begin(); > >>>>> it != nodegroup_db->end(); it++) { > >>> ------------------------------------------------------------------ > >>> ---- > >>> -------- > >>> Site24x7 APM Insight: Get Deep Visibility into Application > >>> Performance APM + Mobile APM + RUM: Monitor 3 App instances at > >>> just $35/Month Monitor end-to-end web transactions and take > >>> corrective actions now Troubleshoot faster and improve end-user > >>> experience. Signup Now! > >>> http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 > >>> _______________________________________________ > >>> Opensaf-devel mailing list > >>> [email protected] > >>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel > > ------------------------------------------------------------------------------ Transform Data into Opportunity. Accelerate data analysis in your applications with Intel Data Analytics Acceleration Library. Click to learn more. http://makebettercode.com/inteldaal-eval _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
