<p>Patch set 6:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10602">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c">File main/backtrace.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@33">Patch Set #6, Line 33:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">#include "asterisk/utils.h"<br>#include "asterisk/strings.h"<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These includes are unused and should be removed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@58">Patch Set #6, Line 58:</a> <code style="font-family:monospace,monospace">#include "asterisk/vector.h"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">My vote is to prevent vector.h from including asterisk/lock.h.  We should avoid as many Asterisk headers as possible.<br>/* Block include of asterisk/lock.h */<br>#define _ASTERISK_LOCK_H</p><p style="white-space: pre-wrap; word-wrap: break-word;">On the other hand maybe vector.h shouldn't include asterisk/lock.h at all.  Any source which uses locking versions of vectors should directly include asterisk/lock.h.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@111">Patch Set #6, Line 111:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/*<br> * We CANNOT call the ast_ equivalents for pthread_<br> * functions here so we need to undo the redirections<br> * created by lock.h so we can call the pthread_<br> * functions directly.<br> */<br>#undef pthread_mutex_lock<br>#undef pthread_mutex_unlock<br>#undef pthread_mutex_t<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd prefer this be moved to the top after including headers.  If we remove the include of lock.h from vector.h this will become unnecessary.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@163">Patch Set #6, Line 163:</a> <code style="font-family:monospace,monospace">             snprintf(data->msg, MSG_BUFF_LEN, "%s %s %s:%u %s()",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I added a 'void *address' to data, set it to addresses[stackfr], prepended "[%p] " to the format with address.  The following was printed by ast_log_backtrace:<br># 0: [0x51afdb] [0x7fcb609b8af4]   chan_sip.so chan_sip.c:35545 load_module()<br># 1: [0x51bc0c] [0x51afdb]   asterisk loader.c:1126 start_resource()<br># 2: [0x51db7e] [0x51bc0c]   asterisk loader.c:1119 start_resource()<br># 3: [0x4366fd] [0x51bc0c] i asterisk loader.c:1374 load_resource_list()<br># 4: [0x7fcbcf44911b] [0x51db7e]   asterisk loader.c:1512 load_modules()<br># 5: [0x43813a] [0x4366fd]   asterisk asterisk.c:4038 check_init()<br># 6: [(nil)] [0x4366fd] i asterisk asterisk.c:4279 asterisk_daemon()<br># 7: [(nil)] [0x4366fd] i asterisk asterisk.c:4032 main()<br># 8: [(nil)] [0x7fcbcf44911b]   libc.so.6 :0 __libc_start_main()<br># 9: [(nil)] [0x43813a]   asterisk :0 _start()</p><p style="white-space: pre-wrap; word-wrap: break-word;"><br>In this example output the first [%p] of each line is from ast_log_backtrace, the second [%p] is from right here.  Probably should just print the address from here instead of ast_log_backtrace, I don't think ast_log_backtrace can compensate for the inline frames that do not have corresponding entries in the backtrace addresses.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@305">Patch Set #6, Line 305:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct ast_vector_string *__ast_bt_get_symbols(void **addresses, size_t num_frames)<br>{<br> return __ast_bt_get_symbols_impl(addresses, num_frames);<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not just rename __ast_bt_get_symbols_impl in the two sections?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10602">change 10602</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/10602"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I79d02862ddaa2423a0809caa4b3b85c128131621 </div>
<div style="display:none"> Gerrit-Change-Number: 10602 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 12 Nov 2018 20:51:26 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>