Hi Gary,
Any update on this.
I think all non-major comments can be taken post FC also.
If you do not any major comments, I would not like to push it today.
Thanks,
Praveen
On 26-Aug-16 6:40 PM, praveen malviya wrote:
> Hi Gary,
>
> Please see inline with [Praveen].
>
> Thanks,
> Praveen
>
>
>
> On 26-Aug-16 6:25 PM, Gary Lee wrote:
>> Hi Praveen
>>
>>
>> On 26/08/2016 10:19 PM, praveen malviya wrote:
>>> Hi Gary,
>>>
>>> Thanks for the comments.
>>> Please see responses inline with [Praveen]
>>>
>>>
>>> Thanks,
>>> Praveen
>>>
>>>
>>>> +// GL: very similar version in d2nmsg.c. Also this isn't freeing the
>>>> SaNameTs attrs.list
>>> [Praveen] I did not get it completely.
>>> The one in utils.cc is used by AMFD to free the message inside
>>> avd_d2n_msg_dequeue() and other one that is present in d2nmsg.c is
>>> used by AMFND to free the message.
>>> In utils.cc: delete [] (compcsi->info.attrs.list) is already present.
>>> In d2nmsg.c: AMFND will have to free memory allocated for extended
>>> names names (if any) by leap.
>>
>> I mean since 'list' is of type AVSV_ATTR_NAME_VAL, the SaNameTs inside
>> AVSV_ATTR_NAME_VAL are not freed.
> [Praveen] From utils.cc perpective it is connected with the below
> expalnation of memcpy(). Since we are using memcpy() we are not freeing
> in utils.cc for SaNameT for both SU_SI_ASSIGN message and this message.
> Since amfnd uses d2nmsg.cm it will free because in decoding there is
> allocation of memory by leap decoding APIs for SaNameT .
>
> I will do change for both SUSI assign and this new message.
> But in that case we do not require a separate function in utils.cc. We
> can directly use d2nmsg.cas it is generic. Do you agree?
>
> Also as of now these is not harming because we are not corrupting memory
> inside CSI->list_attributes even if there is any long dn configured.I
> will try to fix this on monday. otherwise post FC. Do you agree?
>
> Thanks,
> Praveen
>
>>>> static void free_d2n_compcsi_info(AVSV_DND_MSG *compcsi_msg) {
>>>> AVSV_D2N_COMPCSI_ASSIGN_MSG_INFO *compcsi =
>>>> &compcsi_msg->msg_info.d2n_compcsi_assign_msg_info;
>>>>
>>>> @@ -2030,6 +2031,7 @@
>>>> /* Scan the list of attributes for the CSI and add it to the
>>>> message */
>>>> while ((attr_ptr_db != nullptr) &&
>>>> (ptr_csiattr_msg->number <
>>>> compcsi->csi->num_attributes)) {
>>>> + // GL: AVSV_ATTR_NAME_VAL contains SaNameT. Does it need to
>>>> be deep copied.
>>> [Praveen] I got the same doubt while reviewing long dn patches because
>>> same mechamism is being done in avd_prep_csi_attr_info() for existing
>>> SUSI assign message. I thought it works because in case of long dn it
>>> will copy reference to the longdn and in case of short dn it will copy
>>> the original string. Since it is not freed in free_d2n_*() functions
>>> original referenced string in CSI is never gets corrupted.
>>> What do you think?
>>
>> Good point. We should fix avd_prep_csi_attr_info() too? I think there is
>> a danger the original SaNameT will no longer be available, since these
>> messages go into a queue that gets transmitted later?
>>
>> Thanks
>> Gary
>>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Opensaf-devel mailing list
> Opensaf-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
