Re: [devel] [PATCH 1/1] imma: Do not finalize previously instialized privateOmHandle in saImmOiAugmentCcbInitialize [#2827]
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]
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