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

[Joshua Colp]

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



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

    Just go ahead and require the provider to fill this in, minor really.



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

    Document the return values in the doxygen.



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

    Document the return values.



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

    This won't ever be reached because res_sip_pubsub does this check already.



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

    Same here, check is already done in res_sip_pubsub thus will never be NULL.



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

    What's this magic value?



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

    Where did 256 come from?



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

    Pass in the fields to be set as arguments here, and bump your refcounts if needed here as well. Makes all the code exist in one place.



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

    ast_copy_string(exten_state_sub->context, endpoint->context, sizeof(exten_state_sub->context));



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

    I think it would be better to have this where the reference is actually held (add_datastore)



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

    If there is no more exten state subscription here wouldn't it make more sense to terminate the subscription? (Can you think of a case where this would happen?)



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

    Once my patch to fix this gets merged then this can be less silly. :D



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

    Allocate this with an rwlock instead of mutex.



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

    I'd go so far as to say I wouldn't like this to be merged until my other patch is in, cause this is just plain sadness. :(



team/group/pimp_my_sip/res/res_sip_providers/include/res_sip_providers.h
<https://reviewboard.asterisk.org/r/2523/#comment16846>

    I look forward to this disappearing.


- Joshua Colp


On May 14, 2013, 8:37 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2523/
> -----------------------------------------------------------
> 
> (Updated May 14, 2013, 8:37 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 388724 
>   team/group/pimp_my_sip/res/Makefile 388724 
>   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 388724 
>   team/group/pimp_my_sip/res/res_sip_pubsub.exports.in 388724 
> 
> 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/20130516/15c0b445/attachment-0001.htm>