[Asterisk-code-review] res pjsip mwi: unsubscribe unsolicited MWI on deleting endpo... (asterisk[13])

[Alexei Gradinari]

Alexei Gradinari has posted comments on this change. ( https://gerrit.asterisk.org/5802 )

Change subject: res_pjsip_mwi: unsubscribe unsolicited MWI on deleting endpoint last contact
......................................................................


Patch Set 1:

(3 comments)

https://gerrit.asterisk.org/#/c/5802/1/res/res_pjsip_mwi.c
File res/res_pjsip_mwi.c:

https://gerrit.asterisk.org/#/c/5802/1/res/res_pjsip_mwi.c@1245
PS1, Line 1245: 	endpoint = ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), "endpoint", contact->endpoint_name);
> Shouldn't contact->endpoint already be a pointer to the endpoint object?
I checked it, on delete the pointer is null.


https://gerrit.asterisk.org/#/c/5802/1/res/res_pjsip_mwi.c@1251
PS1, Line 1251: 	ao2_lock(unsolicited_mwi);
              : 	mwi_subs = ao2_find(unsolicited_mwi, contact->endpoint_name,
              : 		OBJ_SEARCH_KEY | OBJ_MULTIPLE | OBJ_NOLOCK | OBJ_UNLINK);
              : 	if (mwi_subs) {
              : 		for (; (mwi_sub = ao2_iterator_next(mwi_subs)); ao2_cleanup(mwi_sub)) {
              : 			unsubscribe(mwi_sub, NULL, 0);
              : 		}
              : 		ao2_iterator_destroy(mwi_subs);
              : 	}
              : 	ao2_unlock(unsolicited_mwi);
> The same logic is in mwi_contact_added.  Can you re-use it?
It's not simple reusable because in mwi_contact_added the lock contains both unsubscribe and subscribe. 

I do not fully understand why removing the old subscriptions first and then applying the new subscriptions in mwi_contact_added.

I think the mwi_contact_deleted can be simplified if it's not necessary to remove/add subscription, i.e.
if deleted the last contact then remove all subscription
if exists the contact then do nothing with subscription.


https://gerrit.asterisk.org/#/c/5802/1/res/res_pjsip_mwi.c@1267
PS1, Line 1267: 	ao2_lock(unsolicited_mwi);
              : 	create_mwi_subscriptions_for_endpoint(endpoint, NULL, 0);
              : 	ao2_unlock(unsolicited_mwi);
              : }
> I don't understand why you are re-creating the subscriptions again.
Because if there is another contact we should have the subscription again



-- 
To view, visit https://gerrit.asterisk.org/5802
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I33e174e0b9dba0998927d16d6d100fda5c7254e0
Gerrit-Change-Number: 5802
Gerrit-PatchSet: 1
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Comment-Date: Tue, 13 Jun 2017 14:30:51 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170613/2daa4033/attachment.html>