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:[email protected]]
> Sent: Friday, November 25, 2016 5:14 PM
> To: 'A V Mahesh' <[email protected]>
> Cc: [email protected]
> 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:[email protected]]
> Sent: Friday, November 25, 2016 3:28 PM
> To: Vo Minh Hoang <[email protected]>
> Cc: [email protected]; [email protected]
> 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:[email protected]]
>> Sent: Friday, November 25, 2016 1:20 PM
>> To: Vo Minh Hoang <[email protected]>
>> Cc: [email protected]; [email protected]
>> 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:[email protected]]
>>> Sent: Friday, November 25, 2016 12:12 PM
>>> To: Hoang Vo <[email protected]>; [email protected]
>>> Cc: [email protected]
>>> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
