<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/4502/">https://reviewboard.asterisk.org/r/4502/</a>
</td>
</tr>
</table>
<br />
<div>
<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/4502/diff/2/?file=72787#file72787line1331" style="color: black; font-weight: bold; text-decoration: underline;">/branches/13/main/logger.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 void logger_print_normal(struct logmsg *logmsg)</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">1331</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/* Block ast_log_safe from this thread. */</span></pre></td>
</tr>
<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">1332</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_threadstorage_set_ptr</span><span class="p">(</span><span class="o">&</span><span class="n">in_safe_log</span><span class="p">,</span> <span class="p">(</span><span class="kt">void</span><span class="o">*</span><span class="p">)</span><span class="mi">1</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; 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 this thread needs to be treated differently than any other thread. The recursion check is thread specific to prevent double OOM messages on the same thread.</pre>
</div>
<br />
<div>
<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/4502/diff/2/?file=72787#file72787line1784" style="color: black; font-weight: bold; text-decoration: underline;">/branches/13/main/logger.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">1784</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="cm">/* We were already dealing with an allocation failure,</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This comment isn't always true. It is likely to be an allocation failure, but it could have been an invalid ao2 object error compounded by an allocation failure for the thread local storage.</pre>
</div>
<br />
<p>- rmudgett</p>
<br />
<p>On March 24th, 2015, 3:31 p.m. CDT, Corey Farrell 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 Corey Farrell.</div>
<p style="color: grey;"><i>Updated March 24, 2015, 3: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-24155">ASTERISK-24155</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;">This introduces a new logger routine ast_log_safe. This routine should be used for all error messages in code that can be run as a result of ast_log. ast_log_safe does nothing if run recursively or from the logger thread. All error logging in astobj2.c, strings.c and utils.h have been switched to ast_log_safe. One ast_log from stringfields code in utils.c was also changed.
I've also added support for raw threadstorage. This provides direct access to the void* pointer in threadstorage. In ast_log_safe I use NULL to signify that this thread is not already running ast_log_safe, (void*)1 when it is already running. This was done since it's critical that ast_log_safe do nothing that could log during recursion checking.
This review shows the version 13 patch. Version 11 didn't have the backtrace check for MALLOC_FAILURE_MSG, and trunk uses 'ast_callid' instead of 'struct ast_callid *'. Patches for each version are on JIRA.
The idea to use threadstorage to protect certain error logging came from the patch posted by Timo Teräs.</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;">Verified with 'nm -g main/astobj2.o' that ast_log_safe was being used.
Tested by further modifying Asterisk with added calls to ast_log_safe().
* In main() after fully booted.
* In ast_log_safe() after setting in_safe_log.
* From the logger thread.
Only the message after fully booted was shown in the logs, all others were ignored.</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/utils.c <span style="color: grey">(433360)</span></li>
<li>/branches/13/main/strings.c <span style="color: grey">(433360)</span></li>
<li>/branches/13/main/logger.c <span style="color: grey">(433360)</span></li>
<li>/branches/13/main/hashtab.c <span style="color: grey">(433360)</span></li>
<li>/branches/13/main/astobj2.c <span style="color: grey">(433360)</span></li>
<li>/branches/13/include/asterisk/utils.h <span style="color: grey">(433360)</span></li>
<li>/branches/13/include/asterisk/threadstorage.h <span style="color: grey">(433360)</span></li>
<li>/branches/13/include/asterisk/logger.h <span style="color: grey">(433360)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4502/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>