Hi Minh, AMFA currently does not remember the allocated memory. It relies on the application always to free the memory. In saAmfFinalize() also, it does not free the memory. I think AMFA should remember memory by associating it with handle because process which is starting PG tracking may not be a component and may not call free()/Free_4() and just relies on saAmfFinalize() call to release all the resources.
I think as of now please go ahead with your suggested solution. From finalize perspective this is a defect and applicable to all the branches. So please raise a ticket for that. Thanks, Praveen On 22-Aug-16 12:08 PM, minh chau wrote: > Hi Praveen, > > The case you just mentioned is still in callback context, so Agent can > help application to release the allocated notification. But still > another case: > > + SaAmfProtectionGroupNotificationBufferT buff; > + buff.notification = NULL; > + rc = saAmfProtectionGroupTrack_4(my_amf_hdl, &track_csi, > SA_TRACK_CURRENT, &buff); > + if (rc != SA_AIS_OK) { > + syslog(LOG_ERR, "saAmfProtectionGroupTrack FAILED - %u", rc); > + goto done; > + } > > In this case Agent has to allocate notification but it's not in Agent's > context. > Application has to call API Free_4(buff.notification) to free up > notification. > In order to iterate to free longDn(s) inside Free_4(), Agent has to > memorize a list numberOfItems for every single call as above Track_4(), > or Agent can add sentinel element to the allocated notification. > > Thanks, > Minh > > On 22/08/16 15:34, praveen malviya wrote: >> Hi, >> The callback looks like this: >> typedef void >> (*SaAmfProtectionGroupTrackCallbackT_4)( >> const SaNameT *csiName, >> SaAmfProtectionGroupNotificationBufferT_4 *notificationBuffer, >> SaUint32T numberOfMembers, >> SaAisErrorT error); >> >> Inside this callback, application is supposed to call >> saAmfProtectionGroupNotificationFree_4(). So agent must be able to >> deduce this information as SaAmfProtectionGroupNotificationBufferT_4 >> contains numberOfItems and also numberOfMembers is available from >> callback. >> Since B.04.01 APIs are not fully implemented, agent copies from old >> type of structure to new type in ava_cpy_protection_group_ntf(). >> >> >> Thanks, >> Praveen >> >> On 22-Aug-16 10:51 AM, minh chau wrote: >>> Hi Praveen, >>> >>> The problem with B.04.01 is the API: >>> saAmfProtectionGroupNotificationFree_4(SaAmfHandleT hdl, >>> SaAmfProtectionGroupNotificationT_4 *notification) does not have >>> numberOfItems. >>> Agent does not know how many element in *notification, each of element >>> can hide a longDn inside it. >>> >>> Thanks, >>> Minh >>> >>> >>> On 22/08/16 15:04, praveen malviya wrote: >>>> Hi Minh, >>>> >>>> SaAmfProtectionGroupNotificationBufferT_4() contains numberOfItems to >>>> iterate over. In case of B.04.01, it should be simple as agent can >>>> call direclty osaf_extended_name_free() during iteration inside >>>> saAmfProtectionGroupNotificationFree_4(). So I think, only a for loop >>>> which will iterate over numberOfItems is required. >>>> >>>> Problem was in B.01.01 case, where application will have to iterate >>>> and free the memory. For this, Long has already suggested and that >>>> needs to be documented. >>>> >>>> >>>> Thanks, >>>> Praveen >>>> >>>> >>>> On 20-Aug-16 2:22 PM, minh chau wrote: >>>>> Hi Long, Praveen, >>>>> >>>>> Regarding this TODO >>>>> + if(notification) { >>>>> + // TODO (minhchau): memleak if notification is an array >>>>> + osaf_extended_name_free(¬ification->member.compName); >>>>> free(notification); >>>>> + } >>>>> >>>>> Client currently uses saAmfProtectionGroupNotificationFree_4(handle, >>>>> buff->notification) to free the notification in buffer. >>>>> If @buff->notification is a list of shortDn only, that should work as >>>>> before, as agent will call this inside >>>>> saAmfProtectionGroupNotificationFree_4 >>>>> >>>>> /* free memory */ >>>>> if(notification) >>>>> free(notification); >>>>> >>>>> It will cause memory leak if @buff->notification contains a list of >>>>> longDN notifications. >>>>> The leak is longDn of compName in each notification after the the >>>>> first >>>>> one in the array @buff->notification. >>>>> >>>>> Agent can add a sentinel element when agent allocates >>>>> @buff->notification, set this last element as NULL >>>>> In Free() API, agent could iterate and free longDn in each element of >>>>> array @buff->notification until agent reaches NULL element. >>>>> >>>>> Do you think it could work? >>>> >>>>> >>>>> Thanks, >>>>> Minh >>>>> >>>>> On 19/08/16 21:13, Long Nguyen wrote: >>>>>> Hi Praveen, >>>>>> >>>>>> Please see my answers marked with [Long]. >>>>>> >>>>>> Best regards, >>>>>> Long Nguyen. >>>>>> >>>>>> On 8/19/2016 6:01 PM, praveen malviya wrote: >>>>>>> Hi Long, >>>>>>> >>>>>>> I see one problem if B.01.01 application frees the memory in pg >>>>>>> tracking callback. >>>>>>> Please see inline. >>>>>>> >>>>>>> Thanks, >>>>>>> Praveen >>>>>>> On 19-Aug-16 12:00 PM, Long HB Nguyen wrote: >>>>>>>> osaf/libs/agents/saf/amfa/amf_agent.cc | 1 + >>>>>>>> osaf/libs/agents/saf/amfa/ava_hdl.cc | 2 -- >>>>>>>> 2 files changed, 1 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> >>>>>>>> diff --git a/osaf/libs/agents/saf/amfa/amf_agent.cc >>>>>>>> b/osaf/libs/agents/saf/amfa/amf_agent.cc >>>>>>>> --- a/osaf/libs/agents/saf/amfa/amf_agent.cc >>>>>>>> +++ b/osaf/libs/agents/saf/amfa/amf_agent.cc >>>>>>>> @@ -2450,6 +2450,7 @@ SaAisErrorT AmfAgent::ProtectionGroupTra >>>>>>>> ava_cpy_protection_group_ntf(buf->notification, >>>>>>>> rsp_buf->notification, >>>>>>>> buf->numberOfItems, >>>>>>>> SA_AMF_HARS_READY_FOR_ASSIGNMENT); >>>>>>>> rc = SA_AIS_ERR_NO_SPACE; >>>>>>>> + buf->numberOfItems = rsp_buf->numberOfItems; >>>>>>>> } >>>>>>>> } else { /* if(create_memory == false) */ >>>>>>>> >>>>>>>> diff --git a/osaf/libs/agents/saf/amfa/ava_hdl.cc >>>>>>>> b/osaf/libs/agents/saf/amfa/ava_hdl.cc >>>>>>>> --- a/osaf/libs/agents/saf/amfa/ava_hdl.cc >>>>>>>> +++ b/osaf/libs/agents/saf/amfa/ava_hdl.cc >>>>>>>> @@ -697,7 +697,6 @@ uint32_t ava_hdl_cbk_rec_prc(AVSV_AMF_CB >>>>>>>> ((SaAmfCallbacksT_4*)reg_cbk)->saAmfProtectionGroupTrackCallback(&pg_track->csi_name, >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> &buf, >>>>>>>> pg_track->mem_num, pg_track->err); >>>>>>>> - free(buf.notification); >>>>>>>> } else { >>>>>>>> pg_track->err = SA_AIS_ERR_NO_MEMORY; >>>>>>>> LOG_CR("Notification is NULL: Invoking >>>>>>>> PGTrack Callback with error SA_AIS_ERR_NO_MEMORY"); >>>>>>>> @@ -740,7 +739,6 @@ uint32_t ava_hdl_cbk_rec_prc(AVSV_AMF_CB >>>>>>>> ((SaAmfCallbacksT >>>>>>>> *)reg_cbk)->saAmfProtectionGroupTrackCallback(&pg_track->csi_name, >>>>>>>> &buf, >>>>>>>> pg_track->mem_num, pg_track->err); >>>>>>>> - free(buf.notification); >>>>>>> For B.04.01 API, saAmfProtectionGroupNotificationFree_4() is taking >>>>>>> care of freeing any extended name. For >>>>>>> saAmfProtectionGroupNotificationFree(), it is the application's >>>>>>> responsibility to free the memory. But how it will free any extended >>>>>>> name. >>>>>>> I think there is no API equivalent to osaf_extended_name_free() for >>>>>>> application. Is there any way? >>>>>>> [Long] I think we can do somethings like in applications: >>>>>>> if (strlen(saAisNameBorrow(buff.notification[i].member.comp_name)) > >>>>>>> SA_MAX_UNEXTENDED_NAME_LENGTH) >>>>>>> free(saAisNameBorrow(buff.notification[i].member.comp_name)); >>>>>>> >>>>>>> Otherwise we can document it that: >>>>>>> -if compName is not long dn in the notification buffer, then >>>>>>> application has to free the memory.This will provide backward >>>>>>> compatibility and spec compliance. >>>>>>> >>>>>>> -if compName is longdn in notification then application should not >>>>>>> free the memory. Agent will free the memory after callback is >>>>>>> completed. So any B.01.01 application adapting to long dn will take >>>>>>> care of this when modifying the application. In that case we need to >>>>>>> do something like this : >>>>>>> for i in notificationBuffer->notification[i] >>>>>>> if >>>>>>> (osaf_is_an_extended_name(notificationBuffer->notification[i].member.compName)){ >>>>>>> >>>>>>> >>>>>>> >>>>>>> longdn_found = true; >>>>>>> osaf_extended_name_free(); >>>>>>> } >>>>>>> } >>>>>>> if(longdn_found) >>>>>>> free(buf.notification) >>>>>>> >>>>>>> >>>>>>> Also there is a Todo in amf_agent.cc " // TODO (minhchau): >>>>>>> memleak if >>>>>>> notification is an array". >>>>>>> [Long] Thanks, I will check it. >>>>>>> >>>>>>> Thanks, >>>>>>> Praveen >>>>>>> >>>>>>>> } else { >>>>>>>> pg_track->err = SA_AIS_ERR_NO_MEMORY; >>>>>>>> LOG_CR("Notification is NULL: Invoking >>>>>>>> PGTrack Callback with error SA_AIS_ERR_NO_MEMORY"); >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel