Re: [devel] [PATCH 1/1] imma: Do not finalize previously instialized privateOmHandle in saImmOiAugmentCcbInitialize [#2827]

2018-04-16 Thread Hoa Le 

Hi,

Thanks for the review, I am going to push the change with your suggestion.

--
Best regards,
Hoa Le

On 04/13/2018 02:09 PM, Vu Minh Nguyen wrote:

Hi Hoa,

Ack from me with minor suggestion, started with [Vu].

Regards, Vu


-Original Message-
From: Hoa Le [mailto:hoa...@dektech.com.au]
Sent: Friday, April 6, 2018 10:52 AM
To: ravisekhar.ko...@oracle.com; vu.m.ngu...@dektech.com.au;
zoran.milinko...@ericsson.com
Cc: opensaf-devel@lists.sourceforge.net; Hoa Le 
Subject: [PATCH 1/1] imma: Do not finalize previously instialized
privateOmHandle in saImmOiAugmentCcbInitialize [#2827]

Currently in saImmOiAugmentCcbInitialize, if OI augmented ccb handle
initialization request does not result in OK or TRY_AGAIN, the
privateOmHandle will be finalized and the private OM will be destroyed.

That is fine if the privateOmHandle is initialized in the current
session. But if the privateOmHandle was already initialized in another
previous callback, which means the privateOmHandle record was added to
imma_oi_ccb_record of the corresponding client node, finalizing this
privateOmHandle without cleaning-up imma_oi_ccb_record may call up
unexpected behavior of the Implementer.

This patch checks if the privateOmHandle is the previous initialized
privateOmHandle to avoid finalizing it. This privateOmHandle will be
automatically finalized and cleanup-up in CCB abort or CCB apply.
---
  src/imm/agent/imma_oi_api.cc | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/imm/agent/imma_oi_api.cc b/src/imm/agent/imma_oi_api.cc
index 29fb39d..187e891 100644
--- a/src/imm/agent/imma_oi_api.cc
+++ b/src/imm/agent/imma_oi_api.cc
@@ -4098,9 +4098,11 @@ done:
  TRACE("ownerHandle:%llx", *ownerHandle);

} else if (privateOmHandle) {
-osafassert(immsv_om_handle_finalize);
-immsv_om_handle_finalize(
-privateOmHandle); /* Also finalizes admo handles & ccb handles*/
+if (privateOmHandle != ccb_oi_record->privateAugOmHandle) {

[Vu] Is it good to combine 02 `if` conditions into one? Such as:
else if (privateOmHandle && privateOmHandle !=
ccb_oi_record->privateAugOmHandle) {
osafassert(immsv_om_handle_finalize);
immsv_om_handle_finalize(privateOmHandle);
}


+  osafassert(immsv_om_handle_finalize);
+  immsv_om_handle_finalize(
+  privateOmHandle); /* Also finalizes admo handles & ccb

handles*/

+}
}

if (out_evt) {
--
2.7.4





--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1/1] imma: Do not finalize previously instialized privateOmHandle in saImmOiAugmentCcbInitialize [#2827]

2018-04-13 Thread Vu Minh Nguyen 
Hi Hoa,

Ack from me with minor suggestion, started with [Vu].

Regards, Vu

> -Original Message-
> From: Hoa Le [mailto:hoa...@dektech.com.au]
> Sent: Friday, April 6, 2018 10:52 AM
> To: ravisekhar.ko...@oracle.com; vu.m.ngu...@dektech.com.au;
> zoran.milinko...@ericsson.com
> Cc: opensaf-devel@lists.sourceforge.net; Hoa Le 
> Subject: [PATCH 1/1] imma: Do not finalize previously instialized
> privateOmHandle in saImmOiAugmentCcbInitialize [#2827]
> 
> Currently in saImmOiAugmentCcbInitialize, if OI augmented ccb handle
> initialization request does not result in OK or TRY_AGAIN, the
> privateOmHandle will be finalized and the private OM will be destroyed.
> 
> That is fine if the privateOmHandle is initialized in the current
> session. But if the privateOmHandle was already initialized in another
> previous callback, which means the privateOmHandle record was added to
> imma_oi_ccb_record of the corresponding client node, finalizing this
> privateOmHandle without cleaning-up imma_oi_ccb_record may call up
> unexpected behavior of the Implementer.
> 
> This patch checks if the privateOmHandle is the previous initialized
> privateOmHandle to avoid finalizing it. This privateOmHandle will be
> automatically finalized and cleanup-up in CCB abort or CCB apply.
> ---
>  src/imm/agent/imma_oi_api.cc | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/imm/agent/imma_oi_api.cc b/src/imm/agent/imma_oi_api.cc
> index 29fb39d..187e891 100644
> --- a/src/imm/agent/imma_oi_api.cc
> +++ b/src/imm/agent/imma_oi_api.cc
> @@ -4098,9 +4098,11 @@ done:
>  TRACE("ownerHandle:%llx", *ownerHandle);
> 
>} else if (privateOmHandle) {
> -osafassert(immsv_om_handle_finalize);
> -immsv_om_handle_finalize(
> -privateOmHandle); /* Also finalizes admo handles & ccb handles*/
> +if (privateOmHandle != ccb_oi_record->privateAugOmHandle) {
[Vu] Is it good to combine 02 `if` conditions into one? Such as:
else if (privateOmHandle && privateOmHandle !=
ccb_oi_record->privateAugOmHandle) {
osafassert(immsv_om_handle_finalize);
immsv_om_handle_finalize(privateOmHandle);
}

> +  osafassert(immsv_om_handle_finalize);
> +  immsv_om_handle_finalize(
> +  privateOmHandle); /* Also finalizes admo handles & ccb
handles*/
> +}
>}
> 
>if (out_evt) {
> --
> 2.7.4



--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel