Hi , On 11/25/2016 4:24 PM, Vo Minh Hoang wrote: > Maybe too much but would you please help me the solution to check > cpnd_sec_hdr_update() result in this case. Ok I will provide.
-AVM On 11/25/2016 4:24 PM, Vo Minh Hoang wrote: > Dear Mahesh, > > Thank you very much for your comment. > > Maybe too much but would you please help me the solution to check > cpnd_sec_hdr_update() result in this case. > > Calling it again is not really good when in case of insufficient resource > like now, the first call can return error but the second call after that > might return OK. > cpnd_ckpt_sec_del() itself do not have params that carry error information. > Global variable is should not be used also. > > Sincerely, > Hoang > > > -----Original Message----- > From: A V Mahesh [mailto:mahesh.va...@oracle.com] > Sent: Friday, November 25, 2016 5:37 PM > To: Vo Minh Hoang <hoang.m...@dektech.com.au> > Cc: opensaf-devel@lists.sourceforge.net > Subject: Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while > section_hdr_update_fail [#2207] > > Hi Hoang, > > The better approach will be , keep existing cpnd_ckpt_sec_del() as it is, > keep condition , avoid functionality the that are not required in > cpnd_ckpt_sec_del(), in case of cpnd_sec_hdr_update() failure . > > -AVM > > On 11/25/2016 3:58 PM, Vo Minh Hoang wrote: >> Dear Mahesh, >> >> Thank you very much for your review and comments. >> I understood that I missed the point at n_secs calculation. >> >> Because checking cpnd_sec_hdr_update() result by calling it again is >> quite weird. >> Is that acceptable if I move n_secs-- part to cpnd_ckpt_sec_del_db() >> function? something like this: >> ====================================================================== >> ====== >> ======== >> >> .......... >> if (localSecMap) { >> if (sectionInfo) { >> LocalSectionIdMap::iterator >> it(localSecMap->find(sectionInfo->lcl_sec_id)); >> >> if (it != localSecMap->end()) >> localSecMap->erase(it); >> } >> } >> else { >> LOG_ER("can't find local sec map in cpnd_ckpt_sec_del"); >> osafassert(false); >> } >> // Move following: >> if (sectionInfo) { >> cp_node->replica_info.n_secs--; >> cp_node->replica_info.mem_used = cp_node->replica_info.mem_used - >> (sectionInfo->sec_size); >> } >> ........... >> >> ====================================================================== >> ====== >> ======= >> The ideal is that cpnd_ckpt_sec_del() keep original >> the cpnd_ckpt_sec_del_db() is revert of action before > cpnd_sec_hdr_update() >> cpnd_ckpt_sec_del_db() can be called after cpnd_ckpt_sec_del() many >> time without affecting system. >> >> Please consider and tell me your judgement. >> Best regards, >> Hoang >> >> -----Original Message----- >> From: Vo Minh Hoang [mailto:hoang.m...@dektech.com.au] >> Sent: Friday, November 25, 2016 5:14 PM >> To: 'A V Mahesh' <mahesh.va...@oracle.com> >> Cc: opensaf-devel@lists.sourceforge.net >> Subject: Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while >> section_hdr_update_fail [#2207] >> >> Dear Mahesh, >> >> Thank you very much for your review and comments. >> I understood that I missed the point at n_secs calculation. >> >> Because checking cpnd_sec_hdr_update() >> >> -----Original Message----- >> From: A V Mahesh [mailto:mahesh.va...@oracle.com] >> Sent: Friday, November 25, 2016 3:28 PM >> To: Vo Minh Hoang <hoang.m...@dektech.com.au> >> Cc: anders.wid...@ericsson.com; opensaf-devel@lists.sourceforge.net >> Subject: Re: [PATCH 1 of 1] cpnd: fix error handling while >> section_hdr_update_fail [#2207] >> >> Hi Hoang, >> >> >>I am sorry that I cannot find the ER that you point out. >> >> Ok I understand that `map & localSecMap` will not become become null >> even all elements are removed in fist attempt of >> cpnd_ckpt_sec_del_db() invocation, so we will not core dump. >> >> But section DB still corrupted, because of cpnd_ckpt_sec_de() function >> split ( this patch ) >> >> Irrelevant of cpnd_sec_hdr_update() success or failure, the >> `cp_node->replica_info.n_secs++;` was occurring before >> cpnd_sec_hdr_update(), so we need to do >> `cp_node->replica_info.n_secs--;` >> >> If cpnd_sec_hdr_update() successful and cpnd_ckpt_hdr_update() fails, >> we also need to do roll-backing SECTION HEADER with roll backed >> sectionInfo , so we need to UPDATE THE SECTION HEADER. >> >> If you see original cpnd_ckpt_sec_del() function this is required >> irrelevant of cpnd_sec_hdr_update() success or failure >> ====================================================================== >> ====== >> ======================= >> if (sectionInfo) { >> cp_node->replica_info.n_secs--; >> cp_node->replica_info.mem_used = cp_node->replica_info.mem_used >> - (sectionInfo->sec_size); >> >> // UPDATE THE SECTION HEADER >> uint32_t rc(cpnd_sec_hdr_update(sectionInfo, cp_node)); >> if (rc == NCSCC_RC_FAILURE) { >> TRACE_4("cpnd sect hdr update failed"); >> } >> ====================================================================== >> ====== >> ======================= >> >> So splitting the function is not required you need to only ensure >> cpnd_ckpt_hdr_update() done or not yet done put some filter and do >> UPDATE THE CHECKPOINT HEADER based on >> cpnd_ckpt_hdr_update() >> ====================================================================== >> ====== >> ======================= >> >> if ( cpnd_ckpt_hdr_update() == was successful ) { // UPDATE THE >> CHECKPOINT HEADER >> rc = cpnd_ckpt_hdr_update(cp_node); >> if (rc == NCSCC_RC_FAILURE) { >> TRACE_4("cpnd ckpt hdr update failed"); >> } >> } >> ====================================================================== >> ====== >> ======================= >> >> -AVM >> >> >> On 11/25/2016 12:08 PM, Vo Minh Hoang wrote: >>> Dear Mahesh, >>> >>> The first call erase a element with id from map. >>> The second call with same id, no element found so iterator will point >>> to >>> map->end(). >>> Because of that map->erase() does not execute. >>> >>> I am sorry that I cannot find the ER that you point out. >>> >>> Sincerely, >>> Hoang >>> >>> -----Original Message----- >>> From: A V Mahesh [mailto:mahesh.va...@oracle.com] >>> Sent: Friday, November 25, 2016 1:20 PM >>> To: Vo Minh Hoang <hoang.m...@dektech.com.au> >>> Cc: anders.wid...@ericsson.com; opensaf-devel@lists.sourceforge.net >>> Subject: Re: [PATCH 1 of 1] cpnd: fix error handling while >>> section_hdr_update_fail [#2207] >>> >>> Hi Hoang, >>> >>> The second invocation of cpnd_ckpt_sec_del_db() with generate code dump. >>> >>> ============================================================= >>> >>> SectionMap *map(static_cast<SectionMap >>> *>(cp_node->replica_info.section_db)); >>> >>> if (map) { >>> SectionMap::iterator it(map->find(id)); >>> >>> if (it != map->end()) { >>> sectionInfo = it->second; >>> map->erase(it); >>> } >>> } >>> else { >>> LOG_ER("can't find map in cpnd_ckpt_sec_del"); >>> *osafassert(false);* >>> } >>> ============================================================= >>> >>> >>> -AVM >>> >>> On 11/25/2016 11:16 AM, Vo Minh Hoang wrote: >>>> Dear Mahesh, >>>> >>>> Thank you very much for your comment. >>>> >>>> Would you please verify again my understanding about this. >>>> >>>> Old cpnd_ckpt_sec_del() function and new cpnd_ckpt_sec_del_db() >>>> search for the sectionInfo in 2 maps by its id and remove if found. >>>> In case ckpt_hdr_update_fails, the cpnd_ckpt_sec_del_db() is called >>>> twice, the first one remove sectionInfo from maps, the second one >>>> does not found sectionInfo and cannot remove anything, will not >>>> generate >> error. >>>> Sincerely, >>>> Hoang >>>> >>>> -----Original Message----- >>>> From: A V Mahesh [mailto:mahesh.va...@oracle.com] >>>> Sent: Friday, November 25, 2016 12:12 PM >>>> To: Hoang Vo <hoang.m...@dektech.com.au>; anders.wid...@ericsson.com >>>> Cc: opensaf-devel@lists.sourceforge.net >>>> Subject: Re: [PATCH 1 of 1] cpnd: fix error handling while >>>> section_hdr_update_fail [#2207] >>>> >>>> Hi Hoang, >>>> >>>> NACK >>>> >>>> This patch has some fundamental problem, this will corrupt the cpsv >>>> database . >>>> This patch instead of resolving the issue , increase the percentage >>>> of problem occurrence . >>>> >>>> Basically this patch splits cpnd_ckpt_sec_del() in to two parts >>>> cpnd_ckpt_sec_del_db() & cpnd_ckpt_sec_del() and the new >>>> cpnd_ckpt_sec_del() invokes cpnd_ckpt_sec_del_db() to fullfil the >>>> old function behavior , so that to support other old invocations >>>> and changed the error cleanup in >>>> cpnd_ckpt_sec_add() as follows : >>>> >>>> ckpt_hdr_update_fails: >>>> cpnd_ckpt_sec_del(cp_node, id); >>>> >>>> section_hdr_update_fails: >>>> cpnd_ckpt_sec_del_db(cp_node, id); >>>> >>>> This means cpnd_ckpt_sec_del_db() is always called twice in case of >>>> ckpt_hdr_update_fails , which will generate core >>>> >>>> -AVM >>>> >>>> >>>> On 11/24/2016 3:26 PM, Hoang Vo wrote: >>>>> osaf/libs/common/cpsv/include/cpnd_sec.h | 3 +++ >>>>> osaf/services/saf/cpsv/cpnd/cpnd_db.c | 4 +++- >>>>> osaf/services/saf/cpsv/cpnd/cpnd_sec.cc | 31 >>>> +++++++++++++++++++++++++++---- >>>>> 3 files changed, 33 insertions(+), 5 deletions(-) >>>>> >>>>> >>>>> problem: >>>>> the steps to add a section is add_db_tree -> update_sec_hdr -> >>>>> update_ckpt_hdr so if an error occur cpsv should handle error in >>>>> reverse >>>> order. >>>>> currently, section_hdr_update_fails, cpsv revert ckpt_hdr also that >>>>> case error >>>>> >>>>> solution: >>>>> only revert db_tree in case section_hdr_update_fails >>>>> >>>>> diff --git a/osaf/libs/common/cpsv/include/cpnd_sec.h >>>>> b/osaf/libs/common/cpsv/include/cpnd_sec.h >>>>> --- a/osaf/libs/common/cpsv/include/cpnd_sec.h >>>>> +++ b/osaf/libs/common/cpsv/include/cpnd_sec.h >>>>> @@ -39,6 +39,9 @@ CPND_CKPT_SECTION_INFO * >>>>> cpnd_ckpt_sec_get_create(const CPND_CKPT_NODE *, const >>>>> SaCkptSectionIdT *); >>>>> >>>>> CPND_CKPT_SECTION_INFO * >>>>> +cpnd_ckpt_sec_del_db(CPND_CKPT_NODE *, SaCkptSectionIdT *); >>>>> + >>>>> +CPND_CKPT_SECTION_INFO * >>>>> cpnd_ckpt_sec_del(CPND_CKPT_NODE *, SaCkptSectionIdT *); >>>>> >>>>> CPND_CKPT_SECTION_INFO * >>>>> diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_db.c >>>>> b/osaf/services/saf/cpsv/cpnd/cpnd_db.c >>>>> --- a/osaf/services/saf/cpsv/cpnd/cpnd_db.c >>>>> +++ b/osaf/services/saf/cpsv/cpnd/cpnd_db.c >>>>> @@ -468,10 +468,12 @@ CPND_CKPT_SECTION_INFO *cpnd_ckpt_sec_ad >>>>> TRACE_LEAVE(); >>>>> return pSecPtr; >>>>> >>>>> - section_hdr_update_fails: >>>>> ckpt_hdr_update_fails: >>>>> cpnd_ckpt_sec_del(cp_node, id); >>>>> >>>>> + section_hdr_update_fails: >>>>> + cpnd_ckpt_sec_del_db(cp_node, id); >>>>> + >>>>> section_add_fails: >>>>> if (pSecPtr->sec_id.id != NULL) >>>>> m_MMGR_FREE_CPND_DEFAULT(pSecPtr->sec_id.id); >>>>> diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_sec.cc >>>>> b/osaf/services/saf/cpsv/cpnd/cpnd_sec.cc >>>>> --- a/osaf/services/saf/cpsv/cpnd/cpnd_sec.cc >>>>> +++ b/osaf/services/saf/cpsv/cpnd/cpnd_sec.cc >>>>> @@ -157,19 +157,19 @@ cpnd_ckpt_sec_find(const CPND_CKPT_NODE >>>>> } >>>>> >>>>> >>>> /******************************************************************* >>>> * >>>> * >>>> ****** >>>> * >>>>> - * Name : cpnd_ckpt_sec_del >>>>> + * Name : cpnd_ckpt_sec_del_db >>>>> * >>>>> - * Description : Function to remove the section from a checkpoint. >>>>> + * Description : Function to remove the section from a checkpoint > map >>>> db. >>>>> * >>>>> * Arguments : CPND_CKPT_NODE *cp_node - Check point node. >>>>> * : SaCkptSectionIdT id - Section Identifier >>>>> - * >>>>> + * >>>>> * Return Values : ptr to CPND_CKPT_SECTION_INFO/NULL; >>>>> * >>>>> * Notes : None. >>>>> >>>> ******************************************************************** >>>> * >>>> * >>>> ****** >>>> */ >>>>> CPND_CKPT_SECTION_INFO * >>>>> -cpnd_ckpt_sec_del(CPND_CKPT_NODE *cp_node, SaCkptSectionIdT *id) >>>>> +cpnd_ckpt_sec_del_db(CPND_CKPT_NODE *cp_node, SaCkptSectionIdT >>>>> +*id) >>>>> { >>>>> CPND_CKPT_SECTION_INFO *sectionInfo(0); >>>>> >>>>> @@ -206,6 +206,29 @@ cpnd_ckpt_sec_del(CPND_CKPT_NODE *cp_nod >>>>> osafassert(false); >>>>> } >>>>> >>>>> + TRACE_LEAVE(); >>>>> + >>>>> + return sectionInfo; >>>>> +} >>>>> + >>>>> >>>> +/****************************************************************** >>>> +* >>>> +* >>>> +****** >>>> ** >>>>> + * Name : cpnd_ckpt_sec_del >>>>> + * >>>>> + * Description : Function to remove the section from a checkpoint. >>>>> + * >>>>> + * Arguments : CPND_CKPT_NODE *cp_node - Check point node. >>>>> + * : SaCkptSectionIdT id - Section Identifier >>>>> + * >>>>> + * Return Values : ptr to CPND_CKPT_SECTION_INFO/NULL; >>>>> + * >>>>> + * Notes : None. >>>>> + >>>>> +****************************************************************** >>>>> +* >>>>> +* >>>>> +* >>>>> +********/ >>>>> +CPND_CKPT_SECTION_INFO * >>>>> +cpnd_ckpt_sec_del(CPND_CKPT_NODE *cp_node, SaCkptSectionIdT *id) { >>>>> + TRACE_ENTER(); >>>>> + CPND_CKPT_SECTION_INFO *sectionInfo = >>>>> +cpnd_ckpt_sec_del_db(cp_node, id); >>>>> + >>>>> if (sectionInfo) { >>>>> cp_node->replica_info.n_secs--; >>>>> cp_node->replica_info.mem_used = >>>>> cp_node->replica_info.mem_used >>>>> - (sectionInfo->sec_size); >> >> ---------------------------------------------------------------------- >> ------ >> -- >> _______________________________________________ >> Opensaf-devel mailing list >> Opensaf-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/opensaf-devel >> > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
