Ack from me too then (leveraging on Zoran's ack).
The only thing I reacted to was the fairly large section that was
removed in saImmOmClassDescriptionMemoryFree,
but it looks like you found an existing function that does the same
sub-job as that section of code.
/AndersBj
Zoran Milinkovic wrote:
> Hi Neel,
>
> I have tested saImmOmClassDescriptionMemoryFree_2() and it's ack.
> The rest of the code I just did a review without testing, and here are my
> comments:
>
> - reorder expressions in if statement to "if(rc == SA_AIS_ERR_LIBRARY &&
> returnParams && *returnParams != NULL)" for efficient comparison.
> @@ -3645,6 +3645,24 @@ static SaAisErrorT admin_op_invoke_commo
> + lock_fail1:
> + if(returnParams && *returnParams != NULL && rc == SA_AIS_ERR_LIBRARY) {
>
> - change attrDefinition into *attrDefinition and reorder if statement to: "if
> (rc == SA_AIS_ERR_LIBRARY && *attrDefinition)"
> @@ -4779,6 +4797,24 @@ SaAisErrorT saImmOmClassDescriptionGet_2
> + lock_fail1:
> +
> + if (attrDefinition && rc == SA_AIS_ERR_LIBRARY) {
>
> Best regards,
> Zoran
>
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: den 11 september 2013 14:44
> To: Anders Björnerstedt
> Cc: [email protected]
> Subject: [devel] [PATCH 1 of 1] IMM: fix the memory leaks when ncs_lock
> failed in IMM agent
>
> osaf/libs/agents/saf/imma/imma_oi_api.c | 6 +-
> osaf/libs/agents/saf/imma/imma_om_api.c | 84 +++++++++++++++------------
> 2 files changed, 50 insertions(+), 40 deletions(-)
>
>
> In the IMM API's if ncs_lock fails with SA_AIS_ERR_LIBRARY, memory if any
> aloocated in the API must be freed.
> In the saImmOmClassDescriptionMemoryFree_2 API is modified to use
> imma_freeAttrValue3
>
> diff --git a/osaf/libs/agents/saf/imma/imma_oi_api.c
> b/osaf/libs/agents/saf/imma/imma_oi_api.c
> --- a/osaf/libs/agents/saf/imma/imma_oi_api.c
> +++ b/osaf/libs/agents/saf/imma/imma_oi_api.c
> @@ -1457,7 +1457,7 @@ SaAisErrorT saImmOiImplementerClear(SaIm
> rc = SA_AIS_ERR_LIBRARY;
> /* Losing track of the pending reply count, but ERR_LIBRARY
> dominates*/
> TRACE_4("ERR_LIBRARY: LOCK failed");
> - goto lock_fail;
> + goto lock_fail1;
> }
> locked = true;
>
> @@ -1507,6 +1507,7 @@ SaAisErrorT saImmOiImplementerClear(SaIm
> if (locked)
> m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
>
> + lock_fail1:
> if (out_evt)
> free(out_evt);
>
> @@ -2465,7 +2466,7 @@ SaAisErrorT saImmOiRtObjectUpdate_2(SaIm
> rc = SA_AIS_ERR_LIBRARY;
> /* Losing track of the pending reply count, but ERR_LIBRARY
> dominates*/
> TRACE_4("ERR_LIBRARY: Lock failed");
> - goto lock_fail;
> + goto lock_fail1;
> }
> locked = true;
>
> @@ -2496,6 +2497,7 @@ SaAisErrorT saImmOiRtObjectUpdate_2(SaIm
>
> skip_over_send:
> bad_sync:
> + lock_fail1:
> if (evt.info.immnd.info.objModify.objectName.buf) { /*free-1 */
> free(evt.info.immnd.info.objModify.objectName.buf);
> evt.info.immnd.info.objModify.objectName.buf = NULL;
> diff --git a/osaf/libs/agents/saf/imma/imma_om_api.c
> b/osaf/libs/agents/saf/imma/imma_om_api.c
> --- a/osaf/libs/agents/saf/imma/imma_om_api.c
> +++ b/osaf/libs/agents/saf/imma/imma_om_api.c
> @@ -376,6 +376,7 @@ static SaAisErrorT initialize_common(SaI
> imma_callback_ipc_destroy(cl_node);
>
> ipc_init_fail:
> + lock_fail:
> if (rc != SA_AIS_OK) {
> free(cl_node);
> cl_node=NULL;
> @@ -384,7 +385,6 @@ static SaAisErrorT initialize_common(SaI
> if (locked)
> m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
>
> - lock_fail:
>
> if (out_evt) {
> free(out_evt);
> @@ -3604,7 +3604,7 @@ static SaAisErrorT admin_op_invoke_commo
> if (!locked && m_NCS_LOCK(&cb->cb_lock, NCS_LOCK_WRITE) !=
> NCSCC_RC_SUCCESS) {
> TRACE_4("ERR_LIBRARY: Lock failed");
> rc = SA_AIS_ERR_LIBRARY;
> - goto lock_fail;
> + goto lock_fail1;
> }
> locked = true;
>
> @@ -3645,6 +3645,24 @@ static SaAisErrorT admin_op_invoke_commo
> if (locked)
> m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
>
> + lock_fail1:
> + if(returnParams && *returnParams != NULL && rc == SA_AIS_ERR_LIBRARY) {
> + SaImmAdminOperationParamsT_2 **Params = *returnParams;
> + SaImmAdminOperationParamsT_2 *q = NULL;
> + unsigned int ix = 0;
> + while(Params[ix]) {
> + q = Params[ix];
> + imma_freeAttrValue3(q->paramBuffer, q->paramType);
> + free(q->paramName);
> + q->paramName = NULL;
> + free(q);
> + ++ix;
> + }
> +
> + free(Params);
> + }
> +
> +
> lock_fail:
>
> done:
> @@ -4754,7 +4772,7 @@ SaAisErrorT saImmOmClassDescriptionGet_2
> rc = SA_AIS_ERR_LIBRARY;
> TRACE_4("ERR_LIBRARY: Lock failed");
> /* Losing track of the pending reply count, but ERR_LIBRARY
> dominates*/
> - goto lock_fail;
> + goto lock_fail1;
> }
> locked = true;
>
> @@ -4779,6 +4797,24 @@ SaAisErrorT saImmOmClassDescriptionGet_2
> if (locked)
> m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
>
> + lock_fail1:
> +
> + if (attrDefinition && rc == SA_AIS_ERR_LIBRARY) {
> + SaImmAttrDefinitionT_2 **attr1 = *attrDefinition;
> + int i;
> + for (i = 0; attr1[i]; ++i) {
> + if (attr1[i]->attrDefaultValue) {
> + imma_freeAttrValue3(attr1[i]->attrDefaultValue,
> attr1[i]->attrValueType); /* free-4, free-5 */
> + attr1[i]->attrDefaultValue = 0;
> + }
> + free(attr1[i]->attrName); /*free-3 */
> + attr1[i]->attrName = 0;
> + free(attr1[i]); /*free-2 */
> + attr1[i] = 0;
> + }
> + free(attr1); /*free-1 */
> + }
> +
> lock_fail:
>
> TRACE_LEAVE();
> @@ -4821,36 +4857,7 @@ SaAisErrorT saImmOmClassDescriptionMemor
> int i;
> for (i = 0; attrDefinition[i]; ++i) {
> if (attrDefinition[i]->attrDefaultValue) {
> - SaStringT *strp;
> - SaAnyT *anyp;
> - switch (attrDefinition[i]->attrValueType) {
> - case SA_IMM_ATTR_SAINT32T:
> //intended fall through
> - case SA_IMM_ATTR_SAUINT32T:
> //intended fall through
> - case SA_IMM_ATTR_SAINT64T:
> //intended fall through
> - case SA_IMM_ATTR_SAUINT64T:
> //intended fall through
> - case SA_IMM_ATTR_SATIMET:
> //intended fall through
> - case SA_IMM_ATTR_SAFLOATT:
> //intended fall through
> - case SA_IMM_ATTR_SADOUBLET:
> - case SA_IMM_ATTR_SANAMET:
> -
> free(attrDefinition[i]->attrDefaultValue); /*free-4 */
> - break;
> -
> - case SA_IMM_ATTR_SASTRINGT:
> - strp = (SaStringT
> *)attrDefinition[i]->attrDefaultValue;
> - free(*strp); /*free-5 */
> - free(strp); /*free-4 */
> - break;
> -
> - case SA_IMM_ATTR_SAANYT:
> - anyp = (SaAnyT
> *)attrDefinition[i]->attrDefaultValue;
> - free(anyp->bufferAddr);
> /*free-5 */
> - anyp->bufferAddr = 0;
> - free(anyp); /*free-4 */
> - break;
> -
> - default:
> - abort();
> - }//switch
> +
> imma_freeAttrValue3(attrDefinition[i]->attrDefaultValue,
> attrDefinition[i]->attrValueType); /* free-4, free-5 */
> attrDefinition[i]->attrDefaultValue = 0;
> }
> free(attrDefinition[i]->attrName); /*free-3 */
> @@ -5462,7 +5469,7 @@ mds_send_fail:
> if (m_NCS_LOCK(&cb->cb_lock, NCS_LOCK_WRITE) != NCSCC_RC_SUCCESS) {
> rc = SA_AIS_ERR_LIBRARY;
> TRACE_4("ERR_LIBRARY: Lock error");
> - goto release_cb;
> + goto lock_fail;
> }
> locked = true;
>
> @@ -5500,6 +5507,8 @@ mds_send_fail:
> m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
> }
>
> +lock_fail:
> +
> if(out_evt) {
> if(out_evt->info.imma.info.searchNextRsp) {
>
> if(out_evt->info.imma.info.searchNextRsp->attrValuesList) {
> @@ -6256,7 +6265,7 @@ SaAisErrorT saImmOmSearchInitialize_2(Sa
> if (m_NCS_LOCK(&cb->cb_lock, NCS_LOCK_WRITE) != NCSCC_RC_SUCCESS) {
> rc = SA_AIS_ERR_LIBRARY;
> TRACE_4("ERR_LIBRARY: Lock error");
> - goto release_cb;
> + goto lock_fail;
> }
> locked = true;
>
> @@ -6326,7 +6335,7 @@ SaAisErrorT saImmOmSearchInitialize_2(Sa
> if (locked) {
> m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
> }
> -
> +lock_fail:
> if(out_evt) {
> free(out_evt);
> out_evt=NULL;
> @@ -6469,8 +6478,7 @@ SaAisErrorT saImmOmSearchNext_2(SaImmSea
>
> if (m_NCS_LOCK(&cb->cb_lock, NCS_LOCK_WRITE) != NCSCC_RC_SUCCESS) {
> TRACE_4("ERR_LIBRARY: Lock error");
> - return SA_AIS_ERR_LIBRARY;
> - /*Error case will leak memory. */
> + goto release_evt;
> }
> locked = true;
>
>
> ------------------------------------------------------------------------------
> How ServiceNow helps IT people transform IT departments:
> 1. Consolidate legacy IT systems to a single system of record for IT
> 2. Standardize and globalize service processes across IT
> 3. Implement zero-touch automation to replace manual, redundant tasks
> http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
> _______________________________________________
> Opensaf-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>
------------------------------------------------------------------------------
LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
1,500+ hours of tutorials including VisualStudio 2012, Windows 8, SharePoint
2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack includes
Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/20/13.
http://pubads.g.doubleclick.net/gampad/clk?id=58041151&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
