<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/1900/">https://reviewboard.asterisk.org/r/1900/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 7th, 2012, 10:41 a.m., <b>rmudgett</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/1900/diff/2/?file=27729#file27729line6297" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/channel.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">static const char *oldest_linkedid(const char *a, const char *b)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">6289</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"> </span><span class="k"><span class="hl">if</span></span><span class="hl"> </span><span class="p">(</span><span class="o">!</span><span class="n">ast_strlen_zero</span><span class="p">(</span><span class="n">ast_channel_linkedid</span><span class="p">(</span><span class="n">chan</span><span class="p">))</span> <span class="o">&&</span> <span class="mi"><span class="hl">0</span></span><span class="hl"> </span><span class="o"><span class="hl">!=</span></span><span class="hl"> </span><span class="n"><span class="hl">strcmp</span></span><span class="p"><span class="hl">(</span></span><span class="n">ast_<span class="hl">channel_linkedid</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">chan</span></span><span class="p"><span class="hl">),</span></span><span class="hl"> </span><span class="n">linkedid</span><span class="p">))</span><span class="hl"> </span><span class="p"><span class="hl">{</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">6297</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"> </span><span class="n"><span class="hl">ast_assert</span></span><span class="p">(</span><span class="o">!</span><span class="n">ast_strlen_zero</span><span class="p">(</span><span class="n">ast_channel_linkedid</span><span class="p">(</span><span class="n">chan</span><span class="p">))</span> <span class="o">&&</span> <span class="o"><span class="hl">!</span></span><span class="n">ast_<span class="hl">strlen_zero</span></span><span class="p"><span class="hl">(</span></span><span class="n">linkedid</span><span class="p">))<span class="hl">;</span></span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">6290</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_cel_check_retire_linkedid</span><span class="p">(</span><span class="n">chan</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">6298</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">ast_strlen_zero</span><span class="p">(</span><span class="n">ast_channel_linkedid</span><span class="p">(</span><span class="n">chan</span><span class="p">))</span> <span class="o">||</span> <span class="n">ast_strlen_zero</span><span class="p">(</span><span class="n">linkedid</span><span class="p">))</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;">You do not need to check if the channel has a linkedid. If it does not have one it has already been complained about when it failed to get the initial linkedid.
With this code, you cannot give a channel a new linkedid if the channel does not already have one. Therefor, the problem of a channel not having a linkedid cannot be corrected.
</pre>
</blockquote>
<p>On May 7th, 2012, 10:59 a.m., <b>Terry Wilson</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;">> With this code, you cannot give a channel a new linkedid if the channel does not already have one. Therefor, the problem of a channel not having a linkedid cannot be corrected.
It isn't about correcting anything. It is just about warning in case something that *should* be impossible happens. For instance, if someone made a change to the alloc() function where all of the sudden it becomes possible to have a channel without a linkedid, then we don't crash when we hit the strcmp. Yes, bad things will probably still happen with the call, but at least we wouldn't crash. Or if someone calls the function with a NULL/empty linkedid, then fail the assertion or at least warn if it somehow makes it out of development .
Chances are it is a condition that will never be hit. When I can, I like to avoid passing NULL to strcmp and crashing.</pre>
</blockquote>
<p>On May 7th, 2012, 11:25 a.m., <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;">ast_channel_linkedid() cannot return NULL. It is a string field. String fields can never be NULL unless the string fields were never initialized. For an ast_channel this is not going to happen unless someone really brakes channel allocation.
I did not say remove the check for the passed in linkedid. I said the check for the channels existing linkedid is not needed. It is also potentially harmful because you cannot correct a missing linkedid on a channel by giving it one later.</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;">It cannot *currently* return NULL. Its actual internal type is not known to us as that information is only available in channel_internal.c. All we know is that it is a const char * that we will be passing to strcmp which can't handle NULLs.
How about a compromise: I'll check ast_channel_linkedid() for NULL instead of using ast_strlen_zero. I could also make ast_channel_linkedid_set() ignore empty linkedids and also assert that they are non-empty so that we are guaranteed to never have an empty linkedid, but even if we do then change_linkedid() will still let you set it to something else.</pre>
<br />
<p>- Terry</p>
<br />
<p>On May 7th, 2012, 9:10 a.m., Terry Wilson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Mark Michelson and rmudgett.</div>
<div>By Terry Wilson.</div>
<p style="color: grey;"><i>Updated May 7, 2012, 9:10 a.m.</i></p>
<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;">This patch fixes to situations that could cause the CEL LINKEDID_END event to be missed.
1) During a core stop gracefully, modules are unloaded when ast_active_channels == 0. The LINKDEDID_END event fires during the channel destructor. This means that occasionally, the cel_* module will be unloaded before the channel is destroyed. It seemed generally useful to wait until the refcount of all channel == 0 before unloading, so I added a channel counter and used it in the shutdown code.
2) During a masquerade, ast_channel_change_linkedid is called. It calls ast_cel_check_retire_linkedid which unrefs the linkedid in the linkedids container in cel.c. It didn't ref the new linkedid. Now it does. I also changed the logic a little, since it used to call ast_channel_linkedid_set() even when it hadn't changed (or was blank, which should be verboten).
(this patch is against trunk, but it will go in 1.8+)</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;">My CEL test passed 100 times or so and transferred calls no longer show an error.</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>/trunk/include/asterisk/cel.h <span style="color: grey">(365451)</span></li>
<li>/trunk/include/asterisk/channel.h <span style="color: grey">(365451)</span></li>
<li>/trunk/main/asterisk.c <span style="color: grey">(365451)</span></li>
<li>/trunk/main/cel.c <span style="color: grey">(365451)</span></li>
<li>/trunk/main/channel.c <span style="color: grey">(365451)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1900/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>