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

[Matt Jordan]

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



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

    Add doxygen.
    
    Change these to stringfields.



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

    Use a consistent naming scheme for the holding bridge wrapper.
    
    It should be either:
    
    holding_bridge_wrapper
    
    OR
    
    bridge_wrapper
    
    Since you've named the object "holding_bridge_wrapper", I'd go with "holding_bridge_wrapper" as the prefix to the function name as well. If you prefer brevity, rename the object as well.



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

    If you give the container the sole reference, make the reference check here '1'.



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

    Functions should use a consistent nomenclature.
    
    In general, I'd tend towards [namespace]_[object]_verb_[extra], where [namespace] is only used on public functions outside this translation unit.
    
    This means this should be:
    
    holding_bridge_wrapper_alloc



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

    Initialize string fields here.
    
    If you don't want to go with string fields that's fine; however, you then need to check the return value of every ast_strdup. Since the bridge_id isn't assigned until later, a memory allocation failure at that point in time is harder to handle then setting a string field.



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

    You can simplify this somewhat by giving the container the only reference. If you successfully link the object in, drop the allocation reference.



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

    This should be a WARNING. You're about to kill the channel, and it's indicative of a memory allocation failure or something else critical.


- Matt Jordan


On July 3, 2013, 4:40 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2642/
> -----------------------------------------------------------
> 
> (Updated July 3, 2013, 4:40 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 393517 
>   /trunk/include/asterisk/bridging.h 393517 
>   /trunk/main/bridging.c 393517 
> 
> 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/20130706/c0c423de/attachment-0001.htm>