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
