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

Thu May 9 14:36:14 CDT 2013

> On May 9, 2013, 6 p.m., Joshua Colp wrote:
> > team/group/pimp_my_sip/res/res_sip_exten_state.c, lines 646-651
> > <https://reviewboard.asterisk.org/r/2523/diff/1/?file=37491#file37491line646>
> >
> >     What's the exact limitation cause this makes me a sad panda.

I can answer this!

PJSIP's evsub API provides a function called pjsip_evsub_register_pkg(). Amongst its parameters, it takes an event name and an array of "accept" headers that it knows how to handle.

The big issue is that you cannot call pjsip_evsub_register_pkg() more than once with the same event name. This means that if you called pjsip_evsub_register_pkg() once with "presence" as the event name and "application/xml+pidf" as the accept type, and then you attempted to call pjsip_evsub_register_pkg() a second time with "presence" as the event name and "application/xml+xpidf" as the accept type, it would trigger an assertion. Even though two separate accept types were passed to the function, the same event name was used for both invocations. What this means is that we have to call pjsip_evsub_register_pkg() for any given event type exactly once. This means that presence state providers that write different NOTIFY body types but for identical event names can't dynamically register themselves.

> On May 9, 2013, 6 p.m., Joshua Colp wrote:
> > team/group/pimp_my_sip/res/res_sip_exten_state.c, line 173
> > <https://reviewboard.asterisk.org/r/2523/diff/1/?file=37491#file37491line173>
> >
> >     const this

I'd also make it static.

> On May 9, 2013, 6 p.m., Joshua Colp wrote:
> > team/group/pimp_my_sip/res/res_sip_pubsub.c, lines 152-153
> > <https://reviewboard.asterisk.org/r/2523/diff/1/?file=37494#file37494line152>
> >
> >     So why couldn't this stuff be used? (Curiosity)

I can answer this as well!

There are two reasons this was removed:

1) It doesn't add any real value. When you create a subscription in PJSIP, you provide a set of callbacks (pjsip_evsub_user struct) that get called into when events happen on the subscription. The presence and MWI layers in PJSIP can behave automatically if you do not provide certain callbacks of your own. However, if you do provide your own callbacks, then the MWI and presence layers simply call up into your callback instead of behaving automatically. In our case, all subscriptions funnel through the pubsub core in res_sip_pubsub. This means we fill in all of the callbacks always. This means that any of the automatic behavior we would have got from the presence layer is bypassed entirely.

2) Our exten_state code is going to be used both for presence and other event packages that relay extension state. Since all of the extension state code goes through this layer that Kevin has written, it means that it's a lot easier to just use the base evsub code instead of having special cases for presence vs. dialog or other event types.

With regards to 1), once this changeset goes in, I'm likely to remove the MWI-specific stuff as well since it also adds no real value.

- Mark

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

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