Hi Minh and Praveen,
I have just sent out the patch (amfa: fix memory leak in protection group).
Please help to review. Thanks so much.
Best regards,
Long Nguyen.
On 8/26/2016 12:47 PM, minh.c...@dektech.com.au wrote:
> Hi Praveen,
>
> Long is preparing the patch adding sentinel element.
>
> Thanks,
> Minh
>
>> Hi Minh,
>>
>> I think this is subject to interpretation. After finalize(), handle
>> becomes invalid. So an application cannot call Free_4() to free any
>> memory. In such a case, freeing any resources associated with this
>> handle in finalize() seems to be ok.
>> Anyways freeing in finalize() can be postponed for any real use case
>> to come up.In that case please go ahead by adding sentinel element.
>> I think there are not other pending things in #1642 other than this.
>>
>> Thanks,
>> Praveen
>>
>>
>>
>> On 26-Aug-16 7:35 AM, minh chau wrote:
>>> Hi Praveen,
>>>
>>> Just to confirm if I understand correctly the problem you mentioned in
>>> saAmfFinalize(). As the specification says application should call
>>> free()/Free_4() to release the allocated memory, if application does not
>>> release memory then it's most likely application misuses API. Or do you
>>> mean saAmfFinalize() should do the same as saNtfFinalize()? if this is
>>> the case it looks just an enhancement to guard AMFA?
>>>
>>> Thanks
>>> Minh
>>>
>>> On 25/08/16 20:04, praveen malviya wrote:
>>>> 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
