<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/4354/">https://reviewboard.asterisk.org/r/4354/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 19th, 2015, 4:24 p.m. CST, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/4354/diff/1/?file=70760#file70760line1601" style="color: black; font-weight: bold; text-decoration: underline;">/branches/13/main/bridge.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_bridge_join(struct ast_bridge *bridge,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1601</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_t_cleanup</span><span class="p">(</span><span class="n">bridge_channel</span><span class="o">-></span><span class="n">swap</span><span class="p">,</span> <span class="s">"Bridge complete: join failed"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :-)</pre>
</blockquote>
<p>On January 19th, 2015, 6:45 p.m. CST, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On January 19th, 2015, 7:53 p.m. CST, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On January 19th, 2015, 2:31 p.m. CST, rmudgett wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated Jan. 19, 2015, 2:31 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24649">ASTERISK-24649</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The testsutite masquerade super test and the --tags=transfer tests still pass.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/13/main/bridge_channel.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/main/bridge.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/include/asterisk/bridge_internal.h <span style="color: grey">(430794)</span></li>
<li>/branches/13/include/asterisk/bridge_channel_internal.h <span style="color: grey">(430794)</span></li>
<li>/branches/13/include/asterisk/bridge.h <span style="color: grey">(430794)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4354/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>