[asterisk-dev] [Code Review] 2523: Pimp my SIP: extension state notifications

[Mark Michelson]

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

Ship it!


Like with the alwaysauthreject review, I'm giving this a "Ship it!" even though I've opened issues since the fixes are really simple and can just be performed when you merge your changes.


team/group/pimp_my_sip/res/res_sip_exten_state.c
<https://reviewboard.asterisk.org/r/2523/#comment17450>

    Really minor, but the final parameter should just be hdr->values[i].slen.
    
    Since pj_str_t is not null-terminated, that +1 means that you don't know what you might be copying into your buffer as the final character. As it turns out, it really doesn't matter since ast_copy_string() will immediately overwrite it with a null-terminator anyway.



team/group/pimp_my_sip/res/res_sip_exten_state.c
<https://reviewboard.asterisk.org/r/2523/#comment17451>

    s/a exten_state_subscription/an exten_state_subscription/



team/group/pimp_my_sip/res/res_sip_exten_state.c
<https://reviewboard.asterisk.org/r/2523/#comment17452>

    As much as I like the SCOPED_LOCK use here, I think the lock doesn't need to be held for as long as it is. The lock is intended to protect the list of providers, so the lock is really only needed when the new provider is added to the list at the very end of this function.
    
    The worrisome bit here is when create_and_register_handler() is called with the provider list lock held, because that calls ast_sip_register_subscription_handler(), which itself will lock its list of subscription handlers. By doing this, you've established a lock ordering that needs to always be followed. It's easier just to not have the provider list locked when making the function call since there's not a good reason to have it locked.


- Mark Michelson


On May 31, 2013, 11:07 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2523/
> -----------------------------------------------------------
> 
> (Updated May 31, 2013, 11:07 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21441 and ASTERISK-21442
>     https://issues.asterisk.org/jira/browse/ASTERISK-21441
>     https://issues.asterisk.org/jira/browse/ASTERISK-21442
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds support for extension state notifications in the new SIP channel driver.  A simple API was developed so that specific extension state providers can attach and create the relevant NOTIFY request body, according to their registered type, for the outgoing request.  The framework maintains the list of providers and looks up the appropriate provider upon a state change for an associated registered subscription.  For instance, a provider can be registered for a particular event/media type tuple (ex: presence and application/xml+pidf).  When a subscription comes in for a given event/media type it is the responsibility of the registered provider to create that NOTIFY body.
> 
> Unfortunately pjsip only allows an event and its associated media types to be registered all at once, so providers are not in themselves modules, but in order to maintain some division and to make it easier to move into a module later providers can and should reside in separate files (found in res/res_sip_providers/).
> 
> Included in this review is also an implementation of providers for the following types: event=presence; media_type=application/pidf+xml, application/xml+pidf, and application/cpim+pidf+xml.
> 
> 
> Diffs
> -----
> 
>   team/group/pimp_my_sip/include/asterisk/res_sip_exten_state.h PRE-CREATION 
>   team/group/pimp_my_sip/include/asterisk/res_sip_pubsub.h 390346 
>   team/group/pimp_my_sip/res/res_sip_exten_state.c PRE-CREATION 
>   team/group/pimp_my_sip/res/res_sip_exten_state.exports.in PRE-CREATION 
>   team/group/pimp_my_sip/res/res_sip_pidf.c PRE-CREATION 
>   team/group/pimp_my_sip/res/res_sip_pubsub.c 390346 
>   team/group/pimp_my_sip/res/res_sip_pubsub.exports.in 390346 
> 
> Diff: https://reviewboard.asterisk.org/r/2523/diff/
> 
> 
> Testing
> -------
> 
> Subscribed to a custom presence type and upon state change made sure the appropriate provider created the expected NOTIFY body in the request.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

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