[Asterisk-code-review] res_pjsip_mwi: add better handling of solicited vs unsolicited subscr... (...asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/12791 )

Change subject: res_pjsip_mwi: add better handling of solicited vs unsolicited subscriptions
......................................................................


Patch Set 1:

(4 comments)

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

https://gerrit.asterisk.org/#/c/12791/1/res/res_pjsip_mwi.c@725 
PS1, Line 725: 	 * an unsolicited subscription for any of them.
> Since we have endpoint, can't we just get the list of mailboxes from it?
I did it this way so it wouldn't have to iterate over the full list of endpoint mailboxes when checking if it needed to add the unsolicited subscription back, but just a subset of them.

Probably an unnecessary optimization though as the number of mailboxes probably isn't that large. What's typical use? 1-3 mailboxes?


https://gerrit.asterisk.org/#/c/12791/1/res/res_pjsip_mwi.c@841 
PS1, Line 841: static int is_solicited_allowed(struct ast_sip_endpoint *endpoint, const char *mailbox)
> Does this also need to check that the mailbox is configured in any of the endpoint's aors?
Not for this. It's called while looping over the mailboxes specified on the endpoint's aor. So the given mailbox is a mailbox from the aor line.


https://gerrit.asterisk.org/#/c/12791/1/res/res_pjsip_mwi.c@892 
PS1, Line 892: static int is_unsolicited_allowed(struct ast_sip_endpoint *endpoint, const char *mailbox)
> Does this also need to check that the mailbox is actually currently configured in endpoint->subscrip […]
Not for this one either. The passed in mailbox is one from the endpoint's mailboxes. Unless I was to keep the optimization above upon subscription shutdown. Then yes I'd need to check.


https://gerrit.asterisk.org/#/c/12791/1/res/res_pjsip_mwi.c@967 
PS1, Line 967: 	ao2_lock(unsolicited_mwi);
> A full lock of the container seems like overkill on the off chance a reload is taking place. […]
hrm unfortunately I don't think an rdlock will work here since the unsolicited_mwi might potentially be changed due to a solicited subscription replacing an unsolicited one.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/12791
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Iec2ec12d9431097e97ed5f37119963aee41af7b1
Gerrit-Change-Number: 12791
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 26 Aug 2019 15:34:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190826/0abb0a8c/attachment.html>