Yes I agree that it is a better semantics to let the API own the allocated memory in case the API call was successful, and let the user own the allocated memory in case the API call failed. However if we want to change the semantics of the API in this way we must go through all the places where it is used and modify the code appropriately. From what I can see, the users of MDS_DIRECT_SEND currently don't free the allocated memory if the MDS call fails.
regards, Anders Widell On 03/09/2017 03:39 AM, Vo Minh Hoang wrote: > > Dear Anders Widell, > > Yes, as you mentioned, this patch offer a change for this behavior. > > The reason is as bellowing: > > * Currently, when API succeeds, memory is free in user defined > callback, not really by MDS itself. Only in some cases API fails, > MDS frees memory, in some cases MDS does not. So source code is > not strictly same as PR document. > * MDS do not have method to check user init > memory by using the macro m_MDS_ALLOC_DIRECT_BUFF(x) so this > behavior is non-defensive coding. > * Personally, I think that memory should be init/free in the same > level except specific reason, I cannot find reason in this case. > > Thank you and best regards, > > Hoang > > *From:*Anders Widell [mailto:anders.wid...@ericsson.com] > *Sent:* Wednesday, March 8, 2017 9:49 PM > *To:* Hoang Vo <hoang.m...@dektech.com.au>; mahesh.va...@oracle.com > *Cc:* opensaf-devel@lists.sourceforge.net > *Subject:* Re: [PATCH 1 of 2] mds: handle memory leak [#1860] > > Hi! > > I am trying to understand how these MDS APIs are intended to work. > When I read Section 3.2.9.1.7 in OpenSAF_MDSv_PR.odt in the > opensaf-internal-docs Mercurial repository, it looks like it is the > responsibility of the MDS library to free the provided buffer that was > passed in to MDS_DIRECT_SEND, no matter if the MDS API call succeeds > or fails. Doesn't this patch change this behaviour in MDS? > > regards, > > Anders Widell > > On 03/06/2017 09:00 AM, Hoang Vo wrote: > > src/base/sysf_mem.c | 3 +++ > > src/mds/mds_c_sndrcv.c | 34 +++++++++++++++------------------- > > 2 files changed, 18 insertions(+), 19 deletions(-) > > > > > > Some error handling does not clean internal memory. > > Error handling in dirrect send case clear user memory seem inconsistence, > > mds should let creater manage its memory in error cases. > > > > action: implement as proposed. > > > > diff --git a/src/base/sysf_mem.c b/src/base/sysf_mem.c > > --- a/src/base/sysf_mem.c > > +++ b/src/base/sysf_mem.c > > @@ -1,6 +1,7 @@ > > /* -*- OpenSAF -*- > > * > > * (C) Copyright 2008 The OpenSAF Foundation > > + * Copyright Ericsson AB 2017 - All Rights Reserved. > > * > > * This program is distributed in the hope that it will be useful, but > > * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > > @@ -428,6 +429,8 @@ USRBUF *sysf_alloc_pkt(unsigned char poo > > if (pool_id >= UB_MAX_POOLS) { > > m_PMGR_UNLK(&gl_ub_pool_mgr.lock); > > m_LEAP_DBG_SINK_VOID; > > + m_NCS_MEM_FREE(ub, NCS_MEM_REGION_IO_DATA_HDR, > NCS_SERVICE_ID_OS_SVCS, 2); > > + ub = (USRBUF *)0; > > return NULL; > > } > > ud = (USRDATA > *)gl_ub_pool_mgr.pools[pool_id].mem_alloc(sizeof(USRDATA), pool_id, priority); > > diff --git a/src/mds/mds_c_sndrcv.c b/src/mds/mds_c_sndrcv.c > > --- a/src/mds/mds_c_sndrcv.c > > +++ b/src/mds/mds_c_sndrcv.c > > @@ -1,6 +1,7 @@ > > /* -*- OpenSAF -*- > > * > > * (C) Copyright 2008 The OpenSAF Foundation > > + * Copyright Ericsson AB 2017 - All Rights Reserved. > > * > > * This program is distributed in the hope that it will be useful, but > > * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > > @@ -420,10 +421,6 @@ static uint32_t mds_mcm_direct_send(NCSM > > memset(&req, 0, sizeof(req)); > > if ((info->info.svc_direct_send.i_priority < MDS_SEND_PRIORITY_LOW) || > > (info->info.svc_direct_send.i_priority > > MDS_SEND_PRIORITY_VERY_HIGH)) { > > - if (info->info.svc_direct_send.i_direct_buff != NULL) { > > - > m_MDS_FREE_DIRECT_BUFF(info->info.svc_direct_send.i_direct_buff); > > - info->info.svc_direct_send.i_direct_buff = NULL; > > - } > > m_MDS_LOG_ERR("MDS_SND_RCV: Priority defined is not in > range\n"); > > return NCSCC_RC_FAILURE; > > } > > @@ -432,13 +429,11 @@ static uint32_t mds_mcm_direct_send(NCSM > > m_MDS_LOG_ERR("MDS_SND_RCV: Send Message Direct Buff is > NULL\n"); > > return NCSCC_RC_FAILURE; > > } else if (info->info.svc_direct_send.i_direct_buff_len > > MDS_DIRECT_BUF_MAXSIZE) { > > - mds_free_direct_buff(info->info.svc_direct_send.i_direct_buff); > > m_MDS_LOG_ERR("MDS_SND_RCV: Send Message Direct Buff Len is > greater than SEND SIZE\n"); > > return NCSCC_RC_FAILURE; > > } > > > > if ((info->info.svc_direct_send.i_to_svc == 0) || (info->i_svc_id == > 0)) { > > - m_MDS_FREE_DIRECT_BUFF(info->info.svc_direct_send.i_direct_buff); > > m_MDS_LOG_ERR("MDS_SND_RCV: Source or Dest service provided is > Null, src svc_id = %s(%d), dest svc_id = %s(%d) \n", > > get_svc_names(info->i_svc_id), info->i_svc_id, > get_svc_names(info->info.svc_direct_send.i_to_svc), > info->info.svc_direct_send.i_to_svc); > > return NCSCC_RC_FAILURE; > > @@ -633,11 +628,6 @@ static uint32_t mds_mcm_direct_send(NCSM > > status = NCSCC_RC_FAILURE; > > break; > > } > > - if (status == MDS_INT_RC_DIRECT_SEND_FAIL) { > > - /* Free the MDS Direct Buff */ > > - m_MDS_FREE_DIRECT_BUFF(info->info.svc_direct_send.i_direct_buff); > > - status = NCSCC_RC_FAILURE; > > - } > > return status; > > } > > > > @@ -2014,6 +2004,7 @@ static uint32_t mcm_process_await_active > > return Sucess; */ > > > > MDTM_SEND_REQ req; > > + uint32_t rc = NCSCC_RC_SUCCESS; > > > > NCSMDS_CALLBACK_INFO cbinfo; > > MDS_BCAST_BUFF_LIST *bcast_ptr = NULL; > > @@ -2022,9 +2013,6 @@ static uint32_t mcm_process_await_active > > > > if (svc_cb->i_fail_no_active_sends) { > > m_MDS_LOG_ERR("MDS_SND_RCV: Returning in noactive state as the > option is not to buffer the msgs"); > > - if (to_msg->msg_type == MSG_DIRECT_BUFF) { > > - m_MDS_FREE_DIRECT_BUFF(to_msg->data.info.buff); > > - } > > return MDS_RC_MSG_NO_BUFFERING; > > } > > > > @@ -2071,7 +2059,8 @@ static uint32_t mcm_process_await_active > > if (status != NCSCC_RC_SUCCESS) { > > m_MDS_LOG_ERR("MDS_SND_RCV: CALLBACK ENC failed > for svc_id = %s(%d)\n", > > get_svc_names(svc_cb->svc_id), svc_cb->svc_id); > > - return NCSCC_RC_FAILURE; > > + rc = NCSCC_RC_FAILURE; > > + goto free_mem; > > } > > > > if (((snd_type == MDS_SENDTYPE_BCAST) || (snd_type == > MDS_SENDTYPE_RBCAST))) { > > @@ -2080,7 +2069,8 @@ static uint32_t mcm_process_await_active > > > to_msg->rem_svc_sub_part_ver, cbinfo.info.enc.o_msg_fmt_ver, > > > MDS_SVC_ARCHWORD_TYPE_UNSPECIFIED)) { > > m_MDS_LOG_ERR("MDS_C_SNDRCV: Addition to > bcast list failed in enc case"); > > - return NCSCC_RC_FAILURE; > > + rc = NCSCC_RC_FAILURE; > > + goto free_mem; > > } > > } > > req.msg.encoding = MDS_ENC_TYPE_FULL; > > @@ -2103,8 +2093,14 @@ static uint32_t mcm_process_await_active > > > > m_MDS_LOG_INFO("MDS_SND_RCV: Adding in await active tbl\n"); > > > > - return mds_await_active_tbl_add(send_hdl, req); > > - > > + rc = mds_await_active_tbl_add(send_hdl, req); > > + > > +free_mem: > > + if (rc != NCSCC_RC_SUCCESS) { > > + mds_mcm_free_msg_uba_start(req.msg); > > + } > > + > > + return rc; > > } > > > > > /**************************************************************************** > > @@ -2540,6 +2536,7 @@ static uint32_t mds_await_active_tbl_del > > bk_ptr->next_msg = > mov_ptr->next_msg; > > } > > m_MDS_LOG_INFO("MDS_SND_RCV: Await > active entry successfully deleted\n"); > > + > mds_mcm_free_msg_uba_start(mov_ptr->req.msg); > > m_MMGR_FREE_AWAIT_ACTIVE(mov_ptr); > > return NCSCC_RC_SUCCESS; > > } > > @@ -6061,7 +6058,6 @@ static uint32_t mds_mailbox_proc(MDS_MCM > > m_MMGR_FREE_MSGELEM(msgelem); > > return status; > > } > > - return NCSCC_RC_FAILURE; > > } > > > > > /**************************************************************************** > ------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
