[asterisk-dev] [Code Review] 2642: app_bridgewait: Add a name argument so that multiple holding bridges may be used

[rmudgett]

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



/trunk/apps/app_bridgewait.c
<https://reviewboard.asterisk.org/r/2642/#comment17950>

    How about to changing the name to wait_bridges?  Or if you prefer wait_wrappers.



/trunk/apps/app_bridgewait.c
<https://reviewboard.asterisk.org/r/2642/#comment17954>

    You might be better served if these strings were immutable.  As it is you have reentrency problems when the bridge_id changes.  Also you don't want to create a lock relationship between the wrappers-container/wrapper and the bridges-container/bridge.



/trunk/apps/app_bridgewait.c
<https://reviewboard.asterisk.org/r/2642/#comment17951>

    Reference inconsistency.  The find returns a ref while the alloc does not return a ref.



/trunk/apps/app_bridgewait.c
<https://reviewboard.asterisk.org/r/2642/#comment17952>

    Use a for (;;) loop instead of while (1).



/trunk/apps/app_bridgewait.c
<https://reviewboard.asterisk.org/r/2642/#comment17953>

    RAII_VAR and the continue seem to be incompatible here.  I think you will leak a ref to holding_bridge here when you restart the loop.
    
    With the change of ast_bridge_join() to return -1 if could not join bridge, you can then check if the bridge is dissolved (using new ast_bridge_is_dissolved() call) and try again with a new bridge.



/trunk/include/asterisk/bridging.h
<https://reviewboard.asterisk.org/r/2642/#comment17945>

    I really dislike this state.  It can be done better another way.



/trunk/main/bridging.c
<https://reviewboard.asterisk.org/r/2642/#comment17949>

    Revert the changes to this function.



/trunk/main/bridging.c
<https://reviewboard.asterisk.org/r/2642/#comment17946>

    Change this to return -1 if it failed to join the bridge for whatever reason.



/trunk/main/bridging.c
<https://reviewboard.asterisk.org/r/2642/#comment17948>

    Revert this and change it to cause bridge_channel_join() to return -1 because the channel failed to get pushed into the bridge for whatever reason.



/trunk/main/bridging.c
<https://reviewboard.asterisk.org/r/2642/#comment17947>

    Since nobody currently cares about or checks this return value, it can be redefined.  Change it to return -1 if it could not join the bridge for whatever reason and return 0 if the channel was able to join.


- rmudgett


On July 8, 2013, 6:12 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2642/
> -----------------------------------------------------------
> 
> (Updated July 8, 2013, 6:12 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21922
>     https://issues.asterisk.org/jira/browse/ASTERISK-21922
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Previously the holding bridge used for bridgewait was a single bridge that once created would stick around until the application was unloaded.
> 
> Now things are a little more complicated. Each holding bridge has a name and will be destroyed when it is empty. Since it's theoretically possible to have channels enter a holding bridge without going through the bridge wait application, it isn't always possible to deliberately control when the bridge will be destroyed, so the bridge can effectively be abandoned and set to be destroyed once it empties out.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_bridgewait.c 393824 
>   /trunk/include/asterisk/bridging.h 393824 
>   /trunk/main/bridging.c 393824 
> 
> Diff: https://reviewboard.asterisk.org/r/2642/diff/
> 
> 
> Testing
> -------
> 
> Tested multiple simultaneous holding bridges, multiple channels in the same holding bridge, what would happen if one channel entered as another one was leaving but before the bridge was destroyed, tested bridges emptying out, tested module unload and module load after unload.
> 
> 
> Thanks,
> 
> jrose
> 
>

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