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

Thu May 9 13:00:06 CDT 2013

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


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

    This can be a RDLOCK.


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

    You can scope type to here.


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

    I think it would be better to have the provider specify subtype and type separately, and parse them accordingly in here versus doing it on the NOTIFY.


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

    This is unsafe. You set this to the context pointer in endpoint but it is *NOT* guaranteed endpoint will be valid after you drop your reference.


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

    Incomplete, it's not given an event type - it's given a message which contains an event type.


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

    You've used 25 around in multiple places - is this fixed size big enough? Should it be a #define?


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

    const this


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

    Nit pick: Since you've got so many ao2_cleanup cases you may as well RAII_VAR this and then at the end on success bump the ref count up via ao2_ref


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

    Per my earlier comment store these separately, less work in here.


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

    At this is required for this to actually be useful - why not do a check when a provider registers instead?


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

    Do you foresee any possible uses for this returning a value and thus being able to signal whether an error occurred or not?


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

    Turn this into an ao2 object with a destructor that probably gets rid of stuff. As well - why alloc an ast_exten_state? Why not make it part of this structure?


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

    You aren't actually freeing udata itself. If you do what I mention above you can just RAII_VAR this and not worry about it, or call ao2_cleanup at the end.


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

    These could fail.


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

    This is unsafe. endpoint is not guaranteed to persist.


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

    This has a reference to the state subscription, so the reference should be bumped up/down accordingly or else badness may occur in race conditions.


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

    Check the return value of this.


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

    Old development debugging log message.


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

    Ditto.


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

    What's the exact limitation cause this makes me a sad panda.


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

    So why couldn't this stuff be used? (Curiosity)


- Joshua Colp


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