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
