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