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

[Joshua Colp]

> On 2009-01-26 22:50:09, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, line 114
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2251#file2251line114>
> >
> >     constify

Done.


> On 2009-01-26 22:50:09, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 149-150
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2251#file2251line149>
> >
> >     To be pedantic, this isn't a maximum.  It is _the_ number of buckets.  It's not dynamic.  :-)

Done.


> On 2009-01-26 22:50:09, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, line 191
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2251#file2251line191>
> >
> >     You'll want to add CMP_STOP here, too

Done.


> On 2009-01-26 22:50:09, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 790-793
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2251#file2251line790>
> >
> >     You need to unregister your application before destroying module data.  Otherwise, it is theoretically possible that code could enter the application at a point where the data has become invalid.
> >     
> >     (Okay, well, I know this is in the module unload handler.  This is actually more of an actually real issue in load_module code, and in this case, that code is fine.  However, I think it's best to do things in the opposite order in unload_module anyway as good practice.)

Done.


> On 2009-01-26 22:50:09, Russell Bryant wrote:
> > /trunk/bridges/bridge_multiplexed.c, lines 45-46
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2253#file2253line45>
> >
> >     Again, it's not actually a max number of buckets

Done.


> On 2009-01-26 22:50:09, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 765-766
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2251#file2251line765>
> >
> >     I would do something here to make it clear that after calling this function, the "conference_bridge" pointer is no longer valid.  You could handle the unref outside of that function, or at least set it to NULL immediately after calling it.

Done.


> On 2009-01-26 22:50:09, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 440-458
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2251#file2251line440>
> >
> >     It appears that you have a reference counting error here.
> >     
> >     Above, you have the code where you allocate a new conference bridge.  It is linked in to the container, and then you release your reference.  At that point, you're not allowed to touch it anymore.  However, later, you proceed to access that memory anyway and even return it from this function.

Done.


- Joshua


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


On 2008-12-31 15:45:07, Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/93/
> -----------------------------------------------------------
> 
> (Updated 2008-12-31 15:45:07)
> 
> 
> 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
> 
>