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