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

[Joshua Colp]

> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 116-132
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2907#file2907line116>
> >
> >     You have declared two instances of these two nameless enums.  You can just remove "option_flags" and "option_args".

Done.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 156-157
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2907#file2907line156>
> >
> >     These could be unsigned

Done.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 224-225
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2907#file2907line224>
> >
> >     superfluous return

Done.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, line 244
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2907#file2907line244>
> >
> >     superfluous return

Done.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, lines 310-311
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2907#file2907line310>
> >
> >     superfluous return
> >     
> >     (I'll stop marking these, now, you get the point.)  :-)

Done.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/main/bridging.c, lines 162-163
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2919#file2919line162>
> >
> >     missing space "ast_channel*"
> >     
> >     Also, check for allocation failure here

Done.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/main/bridging.c, line 178
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2919#file2919line178>
> >
> >     It's kind of interesting that usleep(1) is used everywhere in asterisk, when what we really mean is that we want to yield to other threads.
> >     
> >     I think for new code, we should start using sched_yield() instead.  At some point, we should make fixing all of the other code a janitor project.

Done in all the new code here.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/main/bridging.c, lines 181-189
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2919#file2919line181>
> >
> >     I presume this function assumes the bridge is locked.  I would document that fact.

Done.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/main/bridging.c, line 213
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2919#file2919line213>
> >
> >     I wish all of this code was wrapped at 90 columns, but we can deal with that later ...

I'll make a note to modify my emacs configuration to make sure that at least all my future code personally adheres to this.


> On 2009-03-05 09:40:22, Russell Bryant wrote:
> > /trunk/main/bridging.c, lines 318-322
> > <http://reviewboard.digium.com/r/93/diff/5/?file=2919#file2919line318>
> >
> >     It seems like the handling of waiting should be done with the lock held to synchronize with threads that want to check this value.

You are right in that bridge->waiting should be set before we release the lock to ensure anything waiting on waiting won't be allowed to execute until it has been set. The setting back to 0 though has to be done without the lock held because anything waiting on it has the lock held. I've made the change and think it should work fine.


- Joshua


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


On 2009-03-02 10:49:59, Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/93/
> -----------------------------------------------------------
> 
> (Updated 2009-03-02 10:49:59)
> 
> 
> 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 179359 
>   /trunk/apps/app_confbridge.c PRE-CREATION 
>   /trunk/bridges/Makefile PRE-CREATION 
>   /trunk/bridges/bridge_builtin_features.c 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/bridging_features.h PRE-CREATION 
>   /trunk/include/asterisk/bridging_technology.h PRE-CREATION 
>   /trunk/include/asterisk/channel.h 179359 
>   /trunk/main/Makefile 179359 
>   /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
> 
>