subject:"\[devel\] \[PATCH 1 of 1\] cpnd\: fix error handling while section_hdr_update

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-30 Thread A V Mahesh

Hi Hoang,

ACK for the attached patch .

-AVM


On 11/29/2016 6:01 PM, A V Mahesh wrote:
> HI All,
>
> Validation of this patch dependent on #2202 , so please find the attached
> re-based patch of this (#2207) ticket.
>
> Hoang  will re-publish the V3 patch once he is in office.
>
> -AVM
>
> On 11/29/2016 4:23 PM, Hoang Vo wrote:
>>   osaf/libs/common/cpsv/include/cpnd_sec.h |   2 +-
>>   osaf/services/saf/cpsv/cpnd/cpnd_db.c|   5 -
>>   osaf/services/saf/cpsv/cpnd/cpnd_evt.c   |   8 
>>   osaf/services/saf/cpsv/cpnd/cpnd_proc.c  |   2 +-
>>   osaf/services/saf/cpsv/cpnd/cpnd_sec.cc  |  25 
>> ++---
>>   5 files changed, 24 insertions(+), 18 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,7 +39,7 @@ CPND_CKPT_SECTION_INFO *
>>   cpnd_ckpt_sec_get_create(const CPND_CKPT_NODE *, const 
>> SaCkptSectionIdT *);
>> CPND_CKPT_SECTION_INFO *
>> -cpnd_ckpt_sec_del(CPND_CKPT_NODE *, SaCkptSectionIdT *);
>> +cpnd_ckpt_sec_del(CPND_CKPT_NODE *, SaCkptSectionIdT *, bool);
>> CPND_CKPT_SECTION_INFO *
>>   cpnd_get_sect_with_id(const CPND_CKPT_NODE *, uint32_t lcl_sec_id);
>> 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
>> @@ -391,6 +391,7 @@ CPND_CKPT_SECTION_INFO *cpnd_ckpt_sec_ad
>>   int32_t lcl_sec_id = 0;
>>   uint32_t rc = NCSCC_RC_SUCCESS;
>>   uint32_t value = 0, i = 0, j = 0;
>> +bool hdr_update = true;
>> TRACE_ENTER();
>>   /* get the lcl_sec_id */
>> @@ -469,8 +470,10 @@ CPND_CKPT_SECTION_INFO *cpnd_ckpt_sec_ad
>>   return pSecPtr;
>>  section_hdr_update_fails:
>> +hdr_update = false;
>> +
>>ckpt_hdr_update_fails:
>> -cpnd_ckpt_sec_del(cp_node, id);
>> +cpnd_ckpt_sec_del(cp_node, id, hdr_update);
>>  section_add_fails:
>>   if (pSecPtr->sec_id.id != NULL)
>> diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_evt.c 
>> b/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
>> --- a/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
>> +++ b/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
>> @@ -2357,12 +2357,12 @@ static uint32_t cpnd_evt_proc_ckpt_sect_
>>   /* delete the section */
>>   if (gen_sec_id)
>>   tmp_sec_info =
>> - cpnd_ckpt_sec_del(cp_node, _info->sec_id);
>> + cpnd_ckpt_sec_del(cp_node, _info->sec_id, true);
>>   else
>>   tmp_sec_info =
>> cpnd_ckpt_sec_del(cp_node,
>> evt->info.sec_creatReq.
>> - sec_attri.sectionId);
>> + sec_attri.sectionId, true);
>> if (tmp_sec_info == sec_info) {
>>   cp_node->replica_info.
>> @@ -2494,7 +2494,7 @@ static uint32_t cpnd_evt_proc_ckpt_sect_
>>   rc = cpnd_ckpt_sec_find(cp_node, >info.sec_delReq.sec_id);
>>   if (rc == NCSCC_RC_SUCCESS) {
>>   -sec_info = cpnd_ckpt_sec_del(cp_node, 
>> >info.sec_delReq.sec_id);
>> +sec_info = cpnd_ckpt_sec_del(cp_node, 
>> >info.sec_delReq.sec_id, true);
>>   /* resetting lcl_sec_id mapping */
>>   if (sec_info == NULL) {
>>   rc = NCSCC_RC_FAILURE;
>> @@ -2774,7 +2774,7 @@ static uint32_t cpnd_evt_proc_nd2nd_ckpt
>>   send_evt.info.cpnd.info.sec_delete_rsp.error = 
>> SA_AIS_ERR_TRY_AGAIN;
>>   goto nd_rsp;
>>   }
>> -sec_info = cpnd_ckpt_sec_del(cp_node, 
>> >info.sec_delete_req.sec_id);
>> +sec_info = cpnd_ckpt_sec_del(cp_node, 
>> >info.sec_delete_req.sec_id, true);
>>   if (sec_info == NULL) {
>>   if 
>> (m_CPND_IS_COLLOCATED_ATTR_SET(cp_node->create_attrib.creationFlags)) {
>>   TRACE_4("cpnd ckpt sect del failed for 
>> sec_id:%s,ckpt_id:%llx",
>> diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c 
>> b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
>> --- a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
>> +++ b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
>> @@ -1544,7 +1544,7 @@ uint32_t cpnd_proc_sec_expiry(CPND_CB *c
>>   return NCSCC_RC_FAILURE;
>>   }
>>   -cpnd_ckpt_sec_del(cp_node, _info->sec_id);
>> +cpnd_ckpt_sec_del(cp_node, _info->sec_id, true);
>> cp_node->replica_info.shm_sec_mapping[pSec_info->lcl_sec_id] = 1;
>> /* send out destory to all cpnd's maintaining this ckpt */
>> diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_sec.cc 
>>

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-29 Thread A V Mahesh


HI All,

Validation of this patch dependent on #2202 , so please find the attached
re-based patch of this (#2207) ticket.

Hoang  will re-publish the V3 patch once he is in office.

-AVM

On 11/29/2016 4:23 PM, Hoang Vo wrote:

  osaf/libs/common/cpsv/include/cpnd_sec.h |   2 +-
  osaf/services/saf/cpsv/cpnd/cpnd_db.c|   5 -
  osaf/services/saf/cpsv/cpnd/cpnd_evt.c   |   8 
  osaf/services/saf/cpsv/cpnd/cpnd_proc.c  |   2 +-
  osaf/services/saf/cpsv/cpnd/cpnd_sec.cc  |  25 ++---
  5 files changed, 24 insertions(+), 18 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,7 +39,7 @@ CPND_CKPT_SECTION_INFO *
  cpnd_ckpt_sec_get_create(const CPND_CKPT_NODE *, const SaCkptSectionIdT *);
  
  CPND_CKPT_SECTION_INFO *

-cpnd_ckpt_sec_del(CPND_CKPT_NODE *, SaCkptSectionIdT *);
+cpnd_ckpt_sec_del(CPND_CKPT_NODE *, SaCkptSectionIdT *, bool);
  
  CPND_CKPT_SECTION_INFO *

  cpnd_get_sect_with_id(const CPND_CKPT_NODE *, uint32_t lcl_sec_id);
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
@@ -391,6 +391,7 @@ CPND_CKPT_SECTION_INFO *cpnd_ckpt_sec_ad
int32_t lcl_sec_id = 0;
uint32_t rc = NCSCC_RC_SUCCESS;
uint32_t value = 0, i = 0, j = 0;
+   bool hdr_update = true;
  
  	TRACE_ENTER();

/* get the lcl_sec_id */
@@ -469,8 +470,10 @@ CPND_CKPT_SECTION_INFO *cpnd_ckpt_sec_ad
return pSecPtr;
  
   section_hdr_update_fails:

+   hdr_update = false;
+
   ckpt_hdr_update_fails:
-   cpnd_ckpt_sec_del(cp_node, id);
+   cpnd_ckpt_sec_del(cp_node, id, hdr_update);
  
   section_add_fails:

if (pSecPtr->sec_id.id != NULL)
diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_evt.c 
b/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
--- a/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
+++ b/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
@@ -2357,12 +2357,12 @@ static uint32_t cpnd_evt_proc_ckpt_sect_
/* delete the 
section */
if (gen_sec_id)

tmp_sec_info =
-   
cpnd_ckpt_sec_del(cp_node, _info->sec_id);
+   
cpnd_ckpt_sec_del(cp_node, _info->sec_id, true);
else

tmp_sec_info =

cpnd_ckpt_sec_del(cp_node,
   
 evt->info.sec_creatReq.
-   
sec_attri.sectionId);
+   
sec_attri.sectionId, true);
  
  if (tmp_sec_info == sec_info) {


cp_node->replica_info.
@@ -2494,7 +2494,7 @@ static uint32_t cpnd_evt_proc_ckpt_sect_
rc = cpnd_ckpt_sec_find(cp_node, >info.sec_delReq.sec_id);
if (rc == NCSCC_RC_SUCCESS) {
  
-		sec_info = cpnd_ckpt_sec_del(cp_node, >info.sec_delReq.sec_id);

+   sec_info = cpnd_ckpt_sec_del(cp_node, 
>info.sec_delReq.sec_id, true);
/* resetting lcl_sec_id mapping */
if (sec_info == NULL) {
rc = NCSCC_RC_FAILURE;
@@ -2774,7 +2774,7 @@ static uint32_t cpnd_evt_proc_nd2nd_ckpt
send_evt.info.cpnd.info.sec_delete_rsp.error = 
SA_AIS_ERR_TRY_AGAIN;
goto nd_rsp;
}
-   sec_info = cpnd_ckpt_sec_del(cp_node, >info.sec_delete_req.sec_id);
+   sec_info = cpnd_ckpt_sec_del(cp_node, >info.sec_delete_req.sec_id, 
true);
if (sec_info == NULL) {
if 
(m_CPND_IS_COLLOCATED_ATTR_SET(cp_node->create_attrib.creationFlags)) {
TRACE_4("cpnd ckpt sect del failed for 
sec_id:%s,ckpt_id:%llx",
diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c 
b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
--- a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
+++ b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
@@ -1544,7 +1544,7 @@ uint32_t cpnd_proc_sec_expiry(CPND_CB *c
return

[devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-29 Thread Hoang Vo

 osaf/libs/common/cpsv/include/cpnd_sec.h |   2 +-
 osaf/services/saf/cpsv/cpnd/cpnd_db.c|   5 -
 osaf/services/saf/cpsv/cpnd/cpnd_evt.c   |   8 
 osaf/services/saf/cpsv/cpnd/cpnd_proc.c  |   2 +-
 osaf/services/saf/cpsv/cpnd/cpnd_sec.cc  |  25 ++---
 5 files changed, 24 insertions(+), 18 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,7 +39,7 @@ CPND_CKPT_SECTION_INFO *
 cpnd_ckpt_sec_get_create(const CPND_CKPT_NODE *, const SaCkptSectionIdT *);
 
 CPND_CKPT_SECTION_INFO *
-cpnd_ckpt_sec_del(CPND_CKPT_NODE *, SaCkptSectionIdT *);
+cpnd_ckpt_sec_del(CPND_CKPT_NODE *, SaCkptSectionIdT *, bool);
 
 CPND_CKPT_SECTION_INFO *
 cpnd_get_sect_with_id(const CPND_CKPT_NODE *, uint32_t lcl_sec_id);
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
@@ -391,6 +391,7 @@ CPND_CKPT_SECTION_INFO *cpnd_ckpt_sec_ad
int32_t lcl_sec_id = 0;
uint32_t rc = NCSCC_RC_SUCCESS;
uint32_t value = 0, i = 0, j = 0;
+   bool hdr_update = true;
 
TRACE_ENTER();
/* get the lcl_sec_id */
@@ -469,8 +470,10 @@ CPND_CKPT_SECTION_INFO *cpnd_ckpt_sec_ad
return pSecPtr;
 
  section_hdr_update_fails:
+   hdr_update = false;
+
  ckpt_hdr_update_fails:
-   cpnd_ckpt_sec_del(cp_node, id);
+   cpnd_ckpt_sec_del(cp_node, id, hdr_update);
 
  section_add_fails:
if (pSecPtr->sec_id.id != NULL) 
diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_evt.c 
b/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
--- a/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
+++ b/osaf/services/saf/cpsv/cpnd/cpnd_evt.c
@@ -2357,12 +2357,12 @@ static uint32_t cpnd_evt_proc_ckpt_sect_
/* delete the 
section */
if (gen_sec_id)

tmp_sec_info =
-   
cpnd_ckpt_sec_del(cp_node, _info->sec_id);
+   
cpnd_ckpt_sec_del(cp_node, _info->sec_id, true);
else

tmp_sec_info =

cpnd_ckpt_sec_del(cp_node,

evt->info.sec_creatReq.
-   
sec_attri.sectionId);
+   
sec_attri.sectionId, true);
 
if 
(tmp_sec_info == sec_info) {

cp_node->replica_info.
@@ -2494,7 +2494,7 @@ static uint32_t cpnd_evt_proc_ckpt_sect_
rc = cpnd_ckpt_sec_find(cp_node, >info.sec_delReq.sec_id);
if (rc == NCSCC_RC_SUCCESS) {
 
-   sec_info = cpnd_ckpt_sec_del(cp_node, 
>info.sec_delReq.sec_id);
+   sec_info = cpnd_ckpt_sec_del(cp_node, 
>info.sec_delReq.sec_id, true);
/* resetting lcl_sec_id mapping */
if (sec_info == NULL) {
rc = NCSCC_RC_FAILURE;
@@ -2774,7 +2774,7 @@ static uint32_t cpnd_evt_proc_nd2nd_ckpt
send_evt.info.cpnd.info.sec_delete_rsp.error = 
SA_AIS_ERR_TRY_AGAIN;
goto nd_rsp;
}
-   sec_info = cpnd_ckpt_sec_del(cp_node, >info.sec_delete_req.sec_id);
+   sec_info = cpnd_ckpt_sec_del(cp_node, >info.sec_delete_req.sec_id, 
true);
if (sec_info == NULL) {
if 
(m_CPND_IS_COLLOCATED_ATTR_SET(cp_node->create_attrib.creationFlags)) {
TRACE_4("cpnd ckpt sect del failed for 
sec_id:%s,ckpt_id:%llx",
diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c 
b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
--- a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
+++ b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
@@ -1544,7 +1544,7 @@ uint32_t cpnd_proc_sec_expiry(CPND_CB *c
return NCSCC_RC_FAILURE;
}
 
-   cpnd_ckpt_sec_del(cp_node, _info->sec_id);
+   cpnd_ckpt_sec_del(cp_node, _info->sec_id, true);

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-25 Thread A V Mahesh

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

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-25 Thread Vo Minh Hoang

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

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-25 Thread A V Mahesh

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");
>   }
> }
> ==

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-25 Thread Vo Minh Hoang

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  cpn

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-25 Thread Vo Minh Hoang

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 
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 
> 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 *>(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 ; 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 splitscpnd_ckpt_sec_del()  in to  two parts
>>

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-25 Thread A V Mahesh

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 
> 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 *>(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 ; 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 splitscpnd_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

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-24 Thread Vo Minh Hoang

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 
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(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 ; 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 splitscpnd_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
>>}
>>
>>
>

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-24 Thread A V Mahesh

Hi Hoang,

The second invocation of  cpnd_ckpt_sec_del_db() with generate code dump.

=

SectionMap *map(static_cast(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 ; 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 splitscpnd_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.
>>
>

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-24 Thread Vo Minh Hoang

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 ; 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 splitscpnd_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
> + *
> + *

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-24 Thread A V Mahesh

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

[devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

2016-11-24 Thread Hoang Vo

 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

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

[devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

Re: [devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

[devel] [PATCH 1 of 1] cpnd: fix error handling while section_hdr_update_fail [#2207]

14 matches

Site Navigation

Mail list logo

Footer information