[asterisk-dev] [Code Review] 2710: Support externally initiated parking requests; remove a bunch of dead parking code from features.c; refactor parking bridge features slightly

[Matt Jordan]

> On July 31, 2013, 6:38 p.m., Mark Michelson wrote:
> > Nothing I've found here is particularly bad, so I've opened no issues.
> > 
> > The one thing I find a bit strange in this review is the presence of the parking provider function table that has to be retrieved. What's more common in Asterisk is to hide such a function table behind corresponding public API calls. So instead of having to retrieve the parking provider and call its parking_park_call function, you'd just have an ast_parking_park_call() function that would retrieve the parking provider and call the provider's function for you. The one merit you could chalk up to your approach is that you grab a reference to the parking provider and have that reference for as long as you require it. The problem is that res_parking currently does not permit unloading, so there's no way that you would run into an issue doing it the way I described.
> > 
> > I don't feel strongly enough to make you change from what works though.

That's actually why I took that approach - I was hoping that grabbing the ao2 object would also allow bumping the module reference count at some future date, which would then live for the lifetime of that ao2 object. That may be a bit optimistic however.


- Matt


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


On July 29, 2013, 3:52 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2710/
> -----------------------------------------------------------
> 
> (Updated July 29, 2013, 3:52 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22134
>     https://issues.asterisk.org/jira/browse/ASTERISK-22134
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This task started out as a goal to remove as much dead parking code from features.c as possible.
> 
> While doing so, we realized that we still had a few ways certain channel drivers (chan_skinny, chan_mgcp, and chan_dahdi) could initiate a parking request that we hadn't handled. So that got bundled in.
> 
> Once that got bundled in, it became useful to refactor (a little) the way in which the Bridging API - and external consumes of parking - interact with parking features. At that point, I decided I had fallen victim to the scope creep monster and cut it off.
> 
> So, this patch includes:
> 
> * A parking bridge features virtual table that provides access to the parking functionality that the Bridging API needs. This includes requests to park an entire 'call' (with little or no additional information, thank you chan_skinny), perform a blind transfer to a parking extension, determine if an extension is a parking extension, as well as the actual "do the parking" request from the Bridging API.
> 
> * Refactoring in chan_mgcp, chan_skinny, and chan_dahdi to make use of the new functions
> 
> * The removal of some - but not all - dead parking code from features.c
> 
> This also fixed blind transferring a multi-party bridge to a parking lot (which was implemented, but had at least one code path where using the parking features kK might not have worked)
> 
> 
> Diffs
> -----
> 
>   /trunk/main/parking.c 395653 
>   /trunk/main/features.c 395653 
>   /trunk/main/bridge_channel.c 395653 
>   /trunk/main/bridge.c 395653 
>   /trunk/include/asterisk/parking.h 395653 
>   /trunk/include/asterisk/features.h 395653 
>   /trunk/channels/sig_analog.c 395653 
>   /trunk/channels/chan_skinny.c 395653 
>   /trunk/channels/chan_mgcp.c 395653 
>   /trunk/channels/chan_iax2.c 395653 
>   /trunk/channels/chan_dahdi.c 395653 
>   /trunk/res/parking/parking_bridge_features.c 395653 
>   /trunk/res/res_parking.c 395653 
> 
> Diff: https://reviewboard.asterisk.org/r/2710/diff/
> 
> 
> Testing
> -------
> 
> Tested basic call parking:
> 
> SIP/Alice calls SIP/Bob. Both can park each other (kK)
> SIP/Alice calls SIP/Bob. SIP/Alice brings in SIP/Charlie (multi-party). All can park the other two.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130731/20290372/attachment.htm>