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

[Joshua Colp]

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



/team/group/pimp_my_sip/include/asterisk/res_sip.h
<https://reviewboard.asterisk.org/r/2456/#comment15989>

    Why add another API call instead of just using ast_sip_dialog_set_serializer with NULL?



/team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h
<https://reviewboard.asterisk.org/r/2456/#comment15991>

    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.



/team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h
<https://reviewboard.asterisk.org/r/2456/#comment15990>

    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?



/team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h
<https://reviewboard.asterisk.org/r/2456/#comment15993>

    Does this increase the reference count or not?



/team/group/pimp_my_sip/res/res_sip_mwi.c
<https://reviewboard.asterisk.org/r/2456/#comment15997>

    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.



/team/group/pimp_my_sip/res/res_sip_mwi.c
<https://reviewboard.asterisk.org/r/2456/#comment15994>

    Instead of doing this reason_str_ptr why can't you just initialize reason_str to NULL?



/team/group/pimp_my_sip/res/res_sip_mwi.c
<https://reviewboard.asterisk.org/r/2456/#comment15971>

    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.



/team/group/pimp_my_sip/res/res_sip_pubsub.c
<https://reviewboard.asterisk.org/r/2456/#comment15995>

    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?



/team/group/pimp_my_sip/res/res_sip_pubsub.c
<https://reviewboard.asterisk.org/r/2456/#comment15996>

    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.



/team/group/pimp_my_sip/res/res_sip_pubsub.c
<https://reviewboard.asterisk.org/r/2456/#comment16001>

    Are you going to take care of this todo? ;)


- Joshua Colp


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/20130418/e1c68b90/attachment-0001.htm>