<p>Patch set 5:<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>10 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10602/5/include/asterisk/utils.h">File include/asterisk/utils.h:</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/5/include/asterisk/utils.h@517">Patch Set #5, Line 517:</a> <code style="font-family:monospace,monospace">#define ast_std_strdup strdup</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You don't appear to be using this at all.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10602/5/include/asterisk/vector.h">File include/asterisk/vector.h:</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/5/include/asterisk/vector.h@172">Patch Set #5, Line 172:</a> <code style="font-family:monospace,monospace">#define AST_VECTOR_STD_PTR_FREE(vec) do { \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You shouldn't need to have this since the vector is Asterisk managed.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10602/5/main/astmm.c">File main/astmm.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/5/main/astmm.c@178">Patch Set #5, Line 178:</a> <code style="font-family:monospace,monospace">char *ast_std_strdup(const char *ptr)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You don't appear to be using this at all.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10602/5/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/5/main/backtrace.c@36">Patch Set #5, Line 36:</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;">#undef malloc<br>#undef calloc<br>#undef strdup<br>#undef free<br>#undef ast_free<br>#undef ast_calloc<br>#define ast_free(x) free(x)<br>#define ast_calloc(n, x) calloc(n, x)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I do not think you need to do this. Since you are using Asterisk vector code you should let Asterisk handle memory for the vector itself. In fact since you either create or duplicate the strings put into the vector yourself you don't even need to use the ast_std_xxx() functions for anything put in the vector either.</p><p style="white-space: pre-wrap; word-wrap: break-word;">You only need to use standard allocation memory when dealing with the backtrace library functions themselves. You have completely encapsulated the access to those functions in this file so the rest of Asterisk can use the normal Asterisk allocation functions.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@155">Patch Set #5, Line 155:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_APPEND(data->return_strings, strdup(data->msg));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The APPEND can fail since inlined functions can add extra backtrace lines than initially allocated.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@174">Patch Set #5, Line 174:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_INIT(return_strings, num_frames);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The INIT can fail since you are passing in nonzero num_frames.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@198">Patch Set #5, Line 198:</a> <code style="font-family:monospace,monospace"> pthread_mutex_lock(&bfd_mutex);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It may be better/safer to lock around the for loop instead.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@224">Patch Set #5, Line 224:</a> <code style="font-family:monospace,monospace"> data.syms = malloc(allocsize);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Allocation can fail.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@266">Patch Set #5, Line 266:</a> <code style="font-family:monospace,monospace"> strings = backtrace_symbols(addresses, num_frames);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to check if strings is NULL before duping the strings in the array.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10602/5/tests/test_pbx.c">File tests/test_pbx.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/5/tests/test_pbx.c@354">Patch Set #5, Line 354:</a> <code style="font-family:monospace,monospace">ast_asert()K!!!</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What is K?</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: 5 </div>
<div style="display:none"> Gerrit-Owner: 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: Fri, 09 Nov 2018 01:14:32 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>