[asterisk-dev] [Code Review] 2508: Create unreal channel framework for derivative channels like local channels.

rmudgett reviewboard at asterisk.org
Wed May 8 18:19:54 CDT 2013 


> On May 8, 2013, 9:55 p.m., Mark Michelson wrote:
> > There are a bunch of functions in core_local.h where it's not clear why they were made public. For instance, why are the channel technology callbacks public? Why is the unreal_pvt destructor public?
> > 
> > You need to standardize on "unreal" or "local" because right now it appears that which one is used in specific instances is arbitrary. For instance, I'd call ast_

There are two things here.  The unreal framework and local channels using the unreal framework.  The unreal framework is used to create channel types that are similar to local channels.  The unreal framework is the equivalent to an abstract base class.  chan_agent will use the unreal framework to create Agent;1--Agent;2 channels just like local channels thus the common channel technology callbacks need to be made public.


> On May 8, 2013, 9:55 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/include/asterisk/core_local.h, line 38
> > <https://reviewboard.asterisk.org/r/2508/diff/1/?file=37393#file37393line38>
> >
> >     What's this about?

Formatting flair.  :)  Used to separate boilerplate and includes.


> On May 8, 2013, 9:55 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/include/asterisk/core_local.h, line 70
> > <https://reviewboard.asterisk.org/r/2508/diff/1/?file=37393#file37393line70>
> >
> >     Vossel is leaking :)

Shall I remove his memory?


> On May 8, 2013, 9:55 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/include/asterisk/core_local.h, lines 171-182
> > <https://reviewboard.asterisk.org/r/2508/diff/1/?file=37393#file37393line171>
> >
> >     Is the joining of the bridge synchronous or asynchronous? In other words, will ast_call() return when the channel joins the bridge or when the channel leaves the bridge()?
> >     
> >     Also, this function should take an ast_bridge_features() struct as a parameter.

Since ast_call() is passed the ;1 channel, the ;2 channel must be imparted into the bridge.  So to answer the question, the ;2 channel joins asynchronously.

Added a features parameter to the function.


> On May 8, 2013, 9:55 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/main/core_local.c, lines 1263-1287
> > <https://reviewboard.asterisk.org/r/2508/diff/1/?file=37397#file37397line1263>
> >
> >     I think destructors for unreal channels are handled a bit backwards.
> >     
> >     It makes sense to have ast_unreal_alloc() take a custom destructor. However, why not have ast_unreal_destructor() always passed as the destructor to ao2_alloc() and then have a callback within ast_unreal_destructor() call the custom function that was passed into ast_unreal_alloc()?
> >     
> >     This way, destructors passed into ast_unreal_alloc() don't have to remember to call ast_unreal_destructor().

I don't think there is much benefit to gain by doing this.  Each instance of a derived unreal channel would get this callback pointer in the private.  I don't anticipate many unreal channel driver derivatives.


> On May 8, 2013, 9:55 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/main/core_local.c, line 1568
> > <https://reviewboard.asterisk.org/r/2508/diff/1/?file=37397#file37397line1568>
> >
> >     There have been three new operations added that result in searches through the container for a specific local_pvt. Do you think this would be grounds for using a more search-friendly container type?

I did consider changing the locals container to be keyed.  There just wasn't an obvious key.  There are two kinds of searches done on the container: exten at context by local_devicestate and pointer searches done by the others to validate the pointer.

Another factor is local channels optimize themselves out which would keep the number of container members small.


- rmudgett


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


On May 7, 2013, 10:55 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2508/
> -----------------------------------------------------------
> 
> (Updated May 7, 2013, 10:55 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21713
>     https://issues.asterisk.org/jira/browse/ASTERISK-21713
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Part of the bridging work being done needs the ability to manipulate local channels in new ways:
> 1) When one-touch parking a multi-party bridge, a local channel needs to be created between the bridge being parked and the park holding bridge.  In this case, the local;2 channel needs to be pushed into the parking bridge since there isn't a dialplan location to execute.
> 2) When attended transferring a multi-party bridge to an application like voicemail using channel driver protocol transfers.  A local channel needs to be created between the transferred bridge and the application.  In this case, the local;2 channel needs to masquerade into the channel running voicemail.
> 3) The ConfBridge announcer channels need to replace the chan_bridge channels with an unreal channel framework channel for implementation reasons. (The chan_bridge driver will be deleted as a result.)
> 4) The chan_agent channel driver will use the unreal channel framework to create Agent channels.
> 
> This patch refactors chan_local.c into core_local.c, extracts an unreal channel driver framework to create derivative channel types similar to local channels, and moves it into the system core.
> 
> The new API calls for custom local channel manipulation are:
> ast_local_get_peer() - Get the other local channel in the pair.  Useful for getting the local;2 channel after an ast_request().
> ast_local_setup_bridge() - After performing an ast_request(), using this call will make the subsequent ast_call() push the local;2 channel into the specified bridge.
> ast_local_setup_masquerade() - After performing an ast_request(), using this call will make the subsequent ast_call() masquerade the local;2 channel into the specified channel.
> 
> Changes/fixes in local/unreal channel behavior:
> * Made unreal COLP indicate handling always set the caller information on the other unreal channel in the pair.  Previously this happened only in the local;2 to local;1 direction.
> 
> * Fixed using the wrong callerid when checking if the exten exists in local_call().
> 
> * Renamed LOCAL_MOH_PASSTHRU to AST_UNREAL_MOH_INTERCEPT and invert the use of the flag to match the new name.  This makes the unreal channels default to not keeping state information in case they optimize.  Local channels still behave the same for the /m option.  I would love to make local channels not default to handling the HOLD/UNHOLD frames because this represents state that will disappear when the local channels optimize out.  Unfortunately, there is likely too much inertia expecting this behavior.
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/CHANGES 387867 
>   /team/group/bridge_construction/apps/app_dial.c 387867 
>   /team/group/bridge_construction/apps/app_followme.c 387867 
>   /team/group/bridge_construction/apps/app_queue.c 387867 
>   /team/group/bridge_construction/bridges/bridge_builtin_features.c 387867 
>   /team/group/bridge_construction/channels/chan_agent.c 387867 
>   /team/group/bridge_construction/channels/chan_local.c 387867 
>   /team/group/bridge_construction/channels/chan_sip.c 387867 
>   /team/group/bridge_construction/include/asterisk/_private.h 387867 
>   /team/group/bridge_construction/include/asterisk/bridging.h 387867 
>   /team/group/bridge_construction/include/asterisk/ccss.h 387867 
>   /team/group/bridge_construction/include/asterisk/core_local.h PRE-CREATION 
>   /team/group/bridge_construction/main/asterisk.c 387867 
>   /team/group/bridge_construction/main/bridging.c 387867 
>   /team/group/bridge_construction/main/channel.c 387867 
>   /team/group/bridge_construction/main/core_local.c PRE-CREATION 
>   /team/group/bridge_construction/main/pbx.c 387867 
> 
> Diff: https://reviewboard.asterisk.org/r/2508/diff/
> 
> 
> Testing
> -------
> 
> Local channels still optimize out.
> The /n option prevents optimization.
> The /m option still works.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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

More information about the asterisk-dev mailing list