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

Thu May 9 15:25:41 CDT 2013

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


team/group/pimp_my_sip/include/asterisk/res_sip_exten_state.h
<https://reviewboard.asterisk.org/r/2523/#comment16594>

    Josh mentioned that variable naming could use some work, so I'm going to chime in with specific places where I agree.
    
    In this case, ast_exten_state sounds very generic and not like something that only applies to SIP. Using something like res_sip_exten_state or just sip_exten_state gives a better indication that this structure is SIP-specific.


team/group/pimp_my_sip/include/asterisk/res_sip_exten_state.h
<https://reviewboard.asterisk.org/r/2523/#comment16590>

    Change the type of this from int to enum ast_extension_states.


team/group/pimp_my_sip/include/asterisk/res_sip_exten_state.h
<https://reviewboard.asterisk.org/r/2523/#comment16595>

    My same sentiment applies to ast_exten_state_provider as it did to ast_exten_state


team/group/pimp_my_sip/include/asterisk/res_sip_exten_state.h
<https://reviewboard.asterisk.org/r/2523/#comment16601>

    Add parameter documentation to this. Specifically, it's a good idea to know what local and remote are. Are they URIs? Are they extensions names?


team/group/pimp_my_sip/include/asterisk/res_sip_exten_state.h
<https://reviewboard.asterisk.org/r/2523/#comment16596>

    I'm going to agree with Josh that this should be able to return a success/failure result.
    
    Also, put a space after the comma on the second line.


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

    Not yours but since you're in the area, you may as well remove the red blob here.


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

    The cast isn't necessary.


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

    s/an/a/


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

    red blob


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

    This function could stand to have a better name.


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

    This function could use a better name.


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

    Couple of red blobs.


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

    red blob


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

    Another blob of redness.


team/group/pimp_my_sip/res/res_sip_providers/res_sip_pidf.c
<https://reviewboard.asterisk.org/r/2523/#comment16614>

    Did you look into using pjsip/include/pjsip-simple/pidf.h in order to generate a PIDF body?
    
    Similarly, they have pjsip/include/pjsip-simple/xpidf.h for the other type of NOTIFY body you create in this file.


team/group/pimp_my_sip/res/res_sip_providers/res_sip_pidf.c
<https://reviewboard.asterisk.org/r/2523/#comment16613>

    Where did you find out about this accept type? Is this something that's actually used? Did you maybe mean "application/xpidf+xml"?


- Mark Michelson


On May 8, 2013, 10:17 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2523/
> -----------------------------------------------------------
> 
> (Updated May 8, 2013, 10:17 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 388073 
>   team/group/pimp_my_sip/res/Makefile 388073 
>   team/group/pimp_my_sip/res/res_sip_exten_state.c PRE-CREATION 
>   team/group/pimp_my_sip/res/res_sip_providers/include/res_sip_providers.h PRE-CREATION 
>   team/group/pimp_my_sip/res/res_sip_providers/res_sip_pidf.c PRE-CREATION 
>   team/group/pimp_my_sip/res/res_sip_pubsub.c 388073 
>   team/group/pimp_my_sip/res/res_sip_pubsub.exports.in 388073 
> 
> 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/20130509/48d941d1/attachment-0001.htm>