[asterisk-dev] [Code Review] Bridging API for Conference Bridge purposes

[Joshua Colp]

> On 2008-12-16 07:59:58, Russell Bryant wrote:
> > /trunk/channels/chan_bridge.c, line 163
> > <http://reviewboard.digium.com/r/93/diff/1/?file=1915#file1915line163>
> >
> >     return NULL;

Done per Sean.


> On 2008-12-16 07:59:58, Russell Bryant wrote:
> > /trunk/include/asterisk/channel.h, line 409
> > <http://reviewboard.digium.com/r/93/diff/1/?file=1917#file1917line409>
> >
> >     I'm not saying you need to change this, but it's important for us to remember that merging this will break the ast_channel ABI.
> >     
> >     If you want to avoid it, you can use some of the space free'd up, but still available from the "old_unused_dtmfq"

Agreed. I was pondering the thought of just making this a datastore actually so that ABI compatibility would not be an issue.


> On 2008-12-16 07:59:58, Russell Bryant wrote:
> > /trunk/include/asterisk/bridging.h, line 20
> > <http://reviewboard.digium.com/r/93/diff/1/?file=1916#file1916line20>
> >
> >     As a general comment on this header, I think it would make sense to break it up into 2 files:
> >     
> >     1) For users of the API
> >     
> >     2) For implementations of the API
> >     
> >     because I don't think both cases need all of these definitions.

The actual modules that implement the API use a very small subset of the existing header file and will almost always have to also include the additional header file to get at the internals. The only benefit would be to the users of the API... and with the subset being rather small I dunno if it is worth it. I'll ponder this.


> On 2008-12-16 07:59:58, Russell Bryant wrote:
> > /trunk/channels/chan_bridge.c, lines 81-92
> > <http://reviewboard.digium.com/r/93/diff/1/?file=1915#file1915line81>
> >
> >     There is a potential for deadlock in this function.  This function is called with ast locked.  The function locks a 2nd channel in ast_queue_frame().  If another code path tries to lock both channels in the opposite order, such as in a write in the opposite direction, deadlock ensues.

Fixed.


> On 2008-12-16 07:59:58, Russell Bryant wrote:
> > /trunk/channels/chan_bridge.c, lines 117-128
> > <http://reviewboard.digium.com/r/93/diff/1/?file=1915#file1915line117>
> >
> >     The same deadlock potential applies here, as well.

Fixed.


- Joshua


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/93/#review231
-----------------------------------------------------------


On 2008-12-16 07:44:19, Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/93/
> -----------------------------------------------------------
> 
> (Updated 2008-12-16 07:44:19)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> This patch implements the new bridging API and brings with it a module for conference bridges. It does *not* replace existing internal bridging or features yet and will not cause any regressions when put in. It will essentially be introduced as a first test phase to work out any unforeseen critical issues. The bridging core itself is fully implemented besides the following: jitterbuffer support, native bridging, and interval hooks (hooks that are time based versus action based). If you would like an explanation of what the bridging API is made up of and how it works that can be found in the bridging.h header file.
> 
> 
> Diffs
> -----
> 
>   /trunk/Makefile 164597 
>   /trunk/apps/app_confbridge.c PRE-CREATION 
>   /trunk/bridges/Makefile PRE-CREATION 
>   /trunk/bridges/bridge_multiplexed.c PRE-CREATION 
>   /trunk/bridges/bridge_simple.c PRE-CREATION 
>   /trunk/bridges/bridge_softmix.c PRE-CREATION 
>   /trunk/channels/chan_bridge.c PRE-CREATION 
>   /trunk/include/asterisk/bridging.h PRE-CREATION 
>   /trunk/include/asterisk/channel.h 164597 
>   /trunk/main/Makefile 164597 
>   /trunk/main/bridging.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/93/diff
> 
> 
> Testing
> -------
> 
> Conference bridge testing using app_confbridge with features. Joining two channels with simple frame exchange and joining three channels to move it to a true conference bridge. IVR capability of app_confbridge was also tested.
> 
> 
> Thanks,
> 
> Joshua
> 
>