[asterisk-dev] [Code Review] 4354: Bridge core: Pass a ref with the swap channel when joining a bridge.

[rmudgett]

> On Jan. 19, 2015, 4:24 p.m., Matt Jordan wrote:
> > /branches/13/main/bridge.c, line 1601
> > <https://reviewboard.asterisk.org/r/4354/diff/1/?file=70760#file70760line1601>
> >
> >     I don't think the join actually failed here. This should just get called when bridge_channel_internal_join returns, which could occur when the channel was released from the bridge. If I'm wrong, feel free to disregard :-)
> 
> rmudgett wrote:
>     If bridge_channel->swap is non-NULL here then the join really did fail and the unref is logged to REF_DEBUG as a result of join failure.  If it is NULL then there is no unref and no corresponding REF_DEBUG log entry.  ao2_cleanup() is a conditional unref.  bridge_channel_internal_join() is blocking so bridge_channel->swap cannot still have a ref if the channel successfully joined.
> 
> Matt Jordan wrote:
>     That's actually fairly confusing. I would not infer from bridge_channel_internal_join that it consumes the data on the bridge_channel and will set the swap pointer to NULL on exit. If it does do that on a successful call, then I'd expect we would only bother to call of the various cleanups if bridge_channel_internal_join really does fail, that is:
>     
>     if (res) {
>       /* Do cleanups */
>     }
>     
>     Mixing the nominal and off-nominal cleanups feels wrong - or at the various least, very confusing.

This is not mixing of off-nominal and nominal cleanups.  This is just cleanup after use.  You're just confused because of what the REF_DEBUG tag says.  Would you be confused if the tag weren't there?

bridge_channel_internal_join() must consume the swap channel ref for the reasons I've pointed out earlier for ast_bridge_join().  bridge_channel_internal_join() must set the bridge_channel->swap pointer to NULL when it does consume the ref so callers can know if they have to do the unref.

I'll add and update comments to try making it clearer.


- rmudgett


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


On Jan. 19, 2015, 2:31 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4354/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2015, 2:31 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24649
>     https://issues.asterisk.org/jira/browse/ASTERISK-24649
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> When code imparts a channel into a bridge to swap with another channel, a ref needs to be held on the swap channel to ensure that it cannot dissapear before finding it in the bridge.
> 
> * The ast_bridge_join() swap channel parameter now always steals a ref for the swap channel.  This is the only change to the bridge framework's public API semantics.
> 
> * bridge_channel_internal_join() now requires the bridge_channel->swap channel to pass in a ref.
> 
> 
> Diffs
> -----
> 
>   /branches/13/main/bridge_channel.c 430794 
>   /branches/13/main/bridge.c 430794 
>   /branches/13/include/asterisk/bridge_internal.h 430794 
>   /branches/13/include/asterisk/bridge_channel_internal.h 430794 
>   /branches/13/include/asterisk/bridge.h 430794 
> 
> Diff: https://reviewboard.asterisk.org/r/4354/diff/
> 
> 
> Testing
> -------
> 
> The testsutite masquerade super test and the --tags=transfer tests still pass.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150120/e42917ff/attachment-0001.html>