[asterisk-dev] [Code Review] 2456: Pimp My SIP: Add SUBSCRIBE/NOTIFY support and MWI support

Fri Apr 19 13:47:50 CDT 2013

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h, lines 74-76
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36097#file36097line74>
> >
> >     Random thought - could more of this be pushed to the pubsub core? Ie: Splitting this up into a "can accept" callback with the existing new subscribe - then if the can accept callback returns okay take care of the common stuff. The less work for external modules, the better imo.

I've given this some thought and I'm not really seeing the benefit of this. Take res_sip_mwi.c's new_subscribe() as an example. Currently it does the following:

1) Uses the request URI to look up an AOR
2) Looks up the mailboxes for that AOR
3) Allocates subscriptions (stasis subscriptions for appropriate mailboxes and SIP subscription)
4) Accepts the SIP subscription with a 200 OK response
5) Sends an initial NOTIFY

The only thing in there that is truly common to all event packages is step 4, to accept the subscription with a 200 OK. So it may be possible to perform the steps up to step 4 in one callback, have the pubsub core do step 4, and then call back into the subscription handler for step 5. I don't really see the benefit of this though.

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h, lines 98-101
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36097#file36097line98>
> >
> >     This goes for the following stuff as well - what is the lifecycle for ast_sip_subscription_response_data? Should it be allocated and then freed by the caller?

Yeah, this is not well documented, and it's not really clear from looking at the code what is expected. I'll get this fixed up.

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h, line 191
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36097#file36097line191>
> >
> >     Does this increase the reference count or not?

Yes it increases the refcount. I'll add this to the docs.

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_mwi.c, line 263
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36102#file36102line263>
> >
> >     This effectively limits the unsolicited MWI to a single contact on the endpoint. I think this is unacceptable and that we are going to have to do away with the idea that we are only going to send to one target.

Since ast_sip_create_request can take a remote URI as an argument, I can get all contacts for the endpoint and then iterate over each, creating and sending requests for each configured contact. In pseudocode:

for aor_name in endpoint->aors:
    aor = ast_sip_location_retrieve_aor(aor_name)
    contacts = ast_sip_location_retrieve_aor_contacts(aor)
    for contact in contacts:
        ast_sip_create_request("NOTIFY", NULL, endpoint, contact->uri, &tdata)
        (add appropriate subscription headers and body)
        ast_sip_send_request(tdata, NULL, endpoint)

Would that do the trick?

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_mwi.c, line 296
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36102#file36102line296>
> >
> >     Instead of doing this reason_str_ptr why can't you just initialize reason_str to NULL?

reason_str is not a pointer. I can initialize it to have reason_str.ptr = NULL and reason_str.slen = 0, but that's not the same as passing in a NULL pointer to pjsip_mwi_notify().

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_mwi.c, lines 582-583
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36102#file36102line582>
> >
> >     This is *expensive* right now. If there are many configured endpoints it will use MUCH memory. I don't have any other suggestion of how to do it though, for the moment.

Yeah, one thing that would help is something we've talked about in the past: to enact an observer system for sorcery such that modules that are interested in the creation/modification/deletion of certain sorcery types can be informed of such changes. That way, res_sip_mwi would just be given individual endpoints to mess with instead of having to generate the entire container of endpoints each time that res_sip_mwi.so is loaded/reloaded. This may actually be a good use for stasis. Sorcery can generate stasis events and interested parties can subscribe to sorcery stasis events.

This also would have the benefit of not requiring a separate reloads of both res_sip.so and res_sip_mwi.so when endpoint mailbox configuration changes.

I think we should set up a task for getting sorcery observers put in place, but I don't think that should block this change from being merged.

One other smaller change that might help mitigate the memory usage a bit would be to add a flag understood by ast_sorcery_retrieve_by_fields() that acts as a negation flag. So for instance, it would allow me to retrieve all endpoints that do NOT have mailboxes set to an empty string. This way, I could retrieve a subset of the endpoints rather than all of them. This still will not be of much help if all configured endpoints have mailboxes though.

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_pubsub.c, lines 119-125
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36103#file36103line119>
> >
> >     I'm going all hypothetical here but is it possible for this to get called within a pjsip thread that has the dialog already locked?

Hm, hypothetically yes. For MWI, it is highly unlikely since the subscription should always be destroyed from a stasis thread. I think it's a good idea to re-tool the destructor so that it must be called from a servant thread. This means that if you have any stasis callbacks or any other similar Asterisk threads that decrement the refcount of the subscription, your best bet is to push all processing off into a servant so that if the refcount reaches zero, the destructor will run in a servant and there will be no need to push serializer removal as a separate task.

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_pubsub.c, lines 155-174
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36103#file36103line155>
> >
> >     Since this uses the pjsip implementation for a few event types doesn't this then force modules to also use the pjsip implementation? If this is acceptable then it should at least be documented.

Yes, I'll document this.

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_pubsub.c, line 336
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36103#file36103line336>
> >
> >     Are you going to take care of this todo? ;)

Yeah, I suppose I should.

> On April 18, 2013, 5:50 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/include/asterisk/res_sip.h, lines 794-803
> > <https://reviewboard.asterisk.org/r/2456/diff/1/?file=36096#file36096line794>
> >
> >     Why add another API call instead of just using ast_sip_dialog_set_serializer with NULL?

That is a much better idea. Just to be clear, I'll add a note to ast_sip_dialog_set_serializer() to indicate that passing a NULL serializer is a valid way of removing the serializer from the dialog.

- Mark

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2456/#review8301
-----------------------------------------------------------

On April 16, 2013, 7:12 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2456/
> -----------------------------------------------------------
> 
> (Updated April 16, 2013, 7:12 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Bugs: ASTERISK-21259 and ASTERISK-21260
>     https://issues.asterisk.org/jira/browse/ASTERISK-21259
>     https://issues.asterisk.org/jira/browse/ASTERISK-21260
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This adds an API for SUBSCRIBE/NOTIFY support within chan_sip and provides an MWI module as the first user of the API.
> 
> The API allows for subscription handlers to register themselves as being able to handle specific event types and what sort of body types they accept. The API allows for subscription handlers to function either as subscribers or notifiers.
> 
> Subscription handlers are called into during certain milestones of a subscription. It is all documented in res_sip_pubsub.h
> 
> I added MWI support as a way of proving the concept of the API. MWI was both a good and a bad choice for this. MWI can be either solicited or unsolicited. Unsolicited MWI does not make use of the new API at all, but solicited MWI does. The solicited MWI is relatively straightforward as compared to unsolicited.
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/include/asterisk/res_sip.h 385908 
>   /team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h PRE-CREATION 
>   /team/group/pimp_my_sip/res/res_sip.exports.in 385908 
>   /team/group/pimp_my_sip/res/res_sip/location.c 385908 
>   /team/group/pimp_my_sip/res/res_sip/sip_configuration.c 385908 
>   /team/group/pimp_my_sip/res/res_sip/sip_distributor.c 385908 
>   /team/group/pimp_my_sip/res/res_sip_mwi.c PRE-CREATION 
>   /team/group/pimp_my_sip/res/res_sip_pubsub.c PRE-CREATION 
>   /team/group/pimp_my_sip/res/res_sip_pubsub.exports.in PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2456/diff/
> 
> 
> Testing
> -------
> 
> I tested both solicited and unsolicited MWI.
> 
> I tested unsolicited MWI using a Polycom phone. I ensured that as voicemail state changed, the phone was updated with NOTIFYs that contained the proper message count.
> 
> I tested solicited MWI using both Jitsi and SIPp. I used Jitsi in order to ensure that the subscription was handled correctly and that MWI NOTIFYs were sent when the voicemail state changed. I used SIPp to test that unsubscribing from MWI notifications worked properly.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130419/ce13ef25/attachment-0001.htm>