<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/2880/">https://reviewboard.asterisk.org/r/2880/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 7th, 2013, 12:28 p.m. UTC, <b>Corey Farrell</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/2880/diff/4/?file=47008#file47008line2663" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/cdr.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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_cdr_register(const char *name, const char *desc, ast_cdrbe be)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2625</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">AST_RWLIST_REMOVE_CURRENT</span><span class="p">(</span><span class="n">list</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2663</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">AST_RWLIST_REMOVE_CURRENT</span><span class="p">(</span><span class="n">list</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;">if (!active_count || i->suspended) {
AST_RWLIST_REMOVE_CURRENT(list);
}
I think active_count init should happen above the loop as it was in a previous diff, and that failure return should stay after the loop.</pre>
</blockquote>
<p>On October 14th, 2013, 2:45 p.m. UTC, <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;">I don't think the location of active_count makes a big difference here, although I don't mind moving it earlier. The real problem is the act of removing the backend from the list prior to determining whether or not we can safely unregister. Fixed that.
</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;">Now that I think about it a bit more, I'm actually going to keep it roughly where it is. If an invalid module name is provided, we lock the active_cdrs_by_channel container without actually needing to do so - we only need to lock it and get the active count if a valid module was provided.</pre>
<br />
<p>- Matt</p>
<br />
<p>On October 4th, 2013, 9:59 p.m. UTC, Matt Jordan 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.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Oct. 4, 2013, 9:59 p.m.</i></p>
<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;">During shutdown, Asterisk attempts to flush out any active CDRs by calling ast_cdr_engine_term. This is to try to get as much billing data recorded as possible before Asterisk shuts down/restarts and that data is lost. Unfortunately, module unloading occurs prior to calling ast_cdr_engine_term, and the CDR backend modules have already unregistered themselves. Thus, while the records are successfully flushed from the system, there are no registered backends left to handle the data.
This is technically a problem in prior versions of Asterisk as well; however, it is far less likely to occur in Asterisk 1.8/11 due to CDRs being build on an asynchronous message bus in Asterisk 12.
This patch modified ast_cdr_unregister such that if active_cdrs_by_channel has an entry, the unregistration fails. This would have the side effect of preventing CDR module unloading via the CLI on a busy system - but if you're doing that, you're already losing billing data, so that seems like a very bad idea anyway.</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 various dial tests that have a tendency to end very quickly while CDR data is in flight now pass consistently.</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/12/addons/cdr_mysql.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_adaptive_odbc.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_csv.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_custom.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_manager.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_odbc.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_pgsql.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_radius.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_sqlite.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_sqlite3_custom.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_syslog.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/cdr/cdr_tds.c <span style="color: grey">(400553)</span></li>
<li>/branches/12/include/asterisk/cdr.h <span style="color: grey">(400553)</span></li>
<li>/branches/12/main/cdr.c <span style="color: grey">(400553)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2880/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>