Hi Canh,
Please find one comment inline with [Praveen].
Thanks,
Praveen
On 28-Sep-16 9:05 AM, Canh Van Truong wrote:
> osaf/libs/agents/saf/ntfa/ntfa_api.c | 2 +
> osaf/libs/agents/saf/ntfa/ntfa_util.c | 68
> +++++++++++++++++++--------
> tests/ntfsv/tet_saNtfNotificationSubscribe.c | 32 +++++++++++++
> 3 files changed, 81 insertions(+), 21 deletions(-)
>
>
> In finalize(), ntfa deletes client and does not remove the subscriber that
> relate to this client
> from subscriber list. The subscriber with subscriptionId keep in list, then
> we cannot subscribe
> with this subscriptionId.
>
> The patch does:
> - Deleting all subscribers that relate to finalized client.
> - Checking pointer before using in subscriberListItemRemove()
>
> diff --git a/osaf/libs/agents/saf/ntfa/ntfa_api.c
> b/osaf/libs/agents/saf/ntfa/ntfa_api.c
> --- a/osaf/libs/agents/saf/ntfa/ntfa_api.c
> +++ b/osaf/libs/agents/saf/ntfa/ntfa_api.c
> @@ -2059,6 +2059,7 @@ static void subscriberListItemRemove(SaN
> ntfa_subscriber_list_t *listPtr = NULL;
> osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0);
> listPtr = listItemGet(subscriptionId);
> + if (listPtr == NULL) goto done;
> if (listPtr->next != NULL) {
> listPtr->next->prev = listPtr->prev;
> }
> @@ -2074,6 +2075,7 @@ static void subscriberListItemRemove(SaN
> TRACE_1("REMOVE: listPtr->SubscriptionId %d",
> listPtr->subscriberListSubscriptionId);
> ntfa_del_ntf_filter_ptrs(&listPtr->filters);
> free(listPtr);
> +done:
> osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0);
> }
>
> diff --git a/osaf/libs/agents/saf/ntfa/ntfa_util.c
> b/osaf/libs/agents/saf/ntfa/ntfa_util.c
> --- a/osaf/libs/agents/saf/ntfa/ntfa_util.c
> +++ b/osaf/libs/agents/saf/ntfa/ntfa_util.c
> @@ -743,6 +743,46 @@ void ntfa_subscriber_list_del()
> }
> subscriberNoList = NULL;
> }
> +
> +/****************************************************************************
> + Name : subscriber_removed_by_handle
> +
> + Description : This routine delete subscribers of this client out of the
> + subcriberNoList
> +
> + Arguments : SaNtfHandleT ntfHandle
> +
> + Return Values : None
> +
> + Notes :
> +******************************************************************************/
> +void ntfa_subscriber_del_by_handle(SaNtfHandleT ntfHandle)
> +{
> + TRACE_ENTER();
> + ntfa_subscriber_list_t* subscriber_hdl = subscriberNoList;
> + while (subscriber_hdl != NULL) {
> + ntfa_subscriber_list_t *rm_subscriber = subscriber_hdl;
> + subscriber_hdl = subscriber_hdl->next;
> + if (ntfHandle == rm_subscriber->subscriberListNtfHandle) {
> + if (rm_subscriber->next != NULL) {
> + rm_subscriber->next->prev = rm_subscriber->prev;
> + }
> +
> + if (rm_subscriber->prev != NULL) {
> + rm_subscriber->prev->next = rm_subscriber->next;
> + } else {
> + if (rm_subscriber->next != NULL)
> + subscriberNoList = rm_subscriber->next;
> + else
> + subscriberNoList = NULL;
> + }
> + ntfa_del_ntf_filter_ptrs(&rm_subscriber->filters);
> + free(rm_subscriber);
> + }
> + }
> + TRACE_LEAVE();
> +}
> +
> /****************************************************************************
> Name : ntfa_hdl_list_del
>
> @@ -910,27 +950,7 @@ void ntfa_hdl_rec_force_del(ntfa_client_
> ntfa_msg_destroy(cbk_msg);
> }
> /* delete subscriber of this client out of the subcriberNoList*/
> - ntfa_subscriber_list_t* subscriber_hdl = subscriberNoList;
> - while (subscriber_hdl != NULL) {
> - ntfa_subscriber_list_t *rm_subscriber = subscriber_hdl;
> - subscriber_hdl = subscriber_hdl->next;
> - if (rm_node->local_hdl ==
> rm_subscriber->subscriberListNtfHandle) {
> - if (rm_subscriber->next != NULL) {
> - rm_subscriber->next->prev = rm_subscriber->prev;
> - }
> -
> - if (rm_subscriber->prev != NULL) {
> - rm_subscriber->prev->next = rm_subscriber->next;
> - } else {
> - if (rm_subscriber->next != NULL)
> - subscriberNoList = rm_subscriber->next;
> - else
> - subscriberNoList = NULL;
> - }
> - ntfa_del_ntf_filter_ptrs(&rm_subscriber->filters);
> - free(rm_subscriber);
> - }
> - }
> + ntfa_subscriber_del_by_handle(rm_node->local_hdl);
> /* Now delete client */
> m_NCS_IPC_DETACH(&rm_node->mbx, ntfa_clear_mbx, NULL);
> m_NCS_IPC_RELEASE(&rm_node->mbx, NULL);
> @@ -946,6 +966,7 @@ void ntfa_hdl_rec_force_del(ntfa_client_
>
> TRACE_LEAVE();
> }
> +
> /****************************************************************************
> Name : ntfa_hdl_rec_del
>
> @@ -970,6 +991,11 @@ uint32_t ntfa_hdl_rec_del(ntfa_client_hd
> TRACE_ENTER();
> /* TODO: free all resources allocated by the client */
>
> + /* Remove subscribers of this client if there are any in
> subcriberNoList */
> + pthread_mutex_lock(&ntfa_cb.cb_lock);
> + ntfa_subscriber_del_by_handle(rm_node->local_hdl);
> + pthread_mutex_unlock(&ntfa_cb.cb_lock);
> +
[Praveen] These is a note function header of saNtfFinalize() API which
states that taking lock in this API needs to be avoided as it causes
deadlock situaion. This function ntfa_hdl_rec_del() is called from the
saNtfFinalize() API.
Can't we fix this issue by avoiding mutex?
> /* If the to be removed record is the first record */
> if (list_iter == rm_node) {
> *list_head = rm_node->next;
> diff --git a/tests/ntfsv/tet_saNtfNotificationSubscribe.c
> b/tests/ntfsv/tet_saNtfNotificationSubscribe.c
> --- a/tests/ntfsv/tet_saNtfNotificationSubscribe.c
> +++ b/tests/ntfsv/tet_saNtfNotificationSubscribe.c
> @@ -157,6 +157,37 @@ void saNtfNotificationSubscribe_04(void)
> test_validate(rc, SA_AIS_ERR_EXIST);
> }
>
> +SaAisErrorT saNtfSubscribe(SaNtfSubscriptionIdT subscriptionId)
> +{
> + SaAisErrorT ret;
> + SaNtfObjectCreateDeleteNotificationFilterT obcf;
> + SaNtfNotificationTypeFilterHandlesT FilterHandles;
> + memset(&FilterHandles, 0, sizeof(FilterHandles));
> +
> + ret = saNtfObjectCreateDeleteNotificationFilterAllocate(ntfHandle,
> &obcf,0,0,0,1,0);
> + obcf.notificationFilterHeader.notificationClassIds->vendorId =
> SA_NTF_VENDOR_ID_SAF;
> + obcf.notificationFilterHeader.notificationClassIds->majorId = 222;
> + obcf.notificationFilterHeader.notificationClassIds->minorId = 222;
> + FilterHandles.objectCreateDeleteFilterHandle =
> obcf.notificationFilterHandle;
> + ret = saNtfNotificationSubscribe(&FilterHandles, subscriptionId);
> + return ret;
> +}
> +
> +void saNtfNotificationSubscribe_05(void)
> +{
> + SaNtfHandleT ntfHandle1 = 0;
> + safassert(saNtfInitialize(&ntfHandle1, &ntfCallbacks, &ntfVersion),
> SA_AIS_OK);
> + safassert(saNtfInitialize(&ntfHandle, &ntfCallbacks, &ntfVersion),
> SA_AIS_OK);
> + safassert(saNtfSubscribe(666), SA_AIS_OK);
> + safassert(saNtfFinalize(ntfHandle), SA_AIS_OK);
> +
> + safassert(saNtfInitialize(&ntfHandle, &ntfCallbacks, &ntfVersion),
> SA_AIS_OK);
> + rc = saNtfSubscribe(666);
> + safassert(saNtfFinalize(ntfHandle), SA_AIS_OK);
> + safassert(saNtfFinalize(ntfHandle1), SA_AIS_OK);
> + test_validate(rc, SA_AIS_OK);
> +}
> +
> __attribute__ ((constructor)) static void
> saNtfNotificationSubscribe_constructor(void)
> {
> test_suite_add(10, "Consumer operations - subscribe");
> @@ -164,6 +195,7 @@ void saNtfNotificationSubscribe_04(void)
> test_case_add(10, saNtfNotificationSubscribe_02,
> "saNtfNotificationSubscribe null handle SA_AIS_ERR_INVALID_PARAM");
> test_case_add(10, saNtfNotificationSubscribe_03,
> "saNtfNotificationSubscribe All filter handles null
> SA_AIS_ERR_INVALID_PARAM");
> test_case_add(10, saNtfNotificationSubscribe_04,
> "saNtfNotificationSubscribe subscriptionId exist SA_AIS_ERR_EXIST");
> + test_case_add(10, saNtfNotificationSubscribe_05,
> "saNtfNotificationSubscribe subscriptionId if old handle has subscriptionId
> had been finalized SA_AIS_OK");
> }
>
>
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
