<p>Patch set 7:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/10930">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/10930/7/channels/chan_iax2.c">File channels/chan_iax2.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/10930/7/channels/chan_iax2.c@3875">Patch Set #7, Line 3875:</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;"> for (v = peer->vars; v; v = v->next)<br> ast_cli(a->fd, " %s = %s\n", v->name, v->value);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">'for' loop needs braces.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10930/7/include/asterisk/config.h">File include/asterisk/config.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/10930/7/include/asterisk/config.h@372">Patch Set #7, Line 372:</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;">* \param variable list to update<br> * \param variable name to search<br> * \param variable new value<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to replace 'variable' with the name of the actual parameter.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10930/7/include/asterisk/config.h@377">Patch Set #7, Line 377:</a> <code style="font-family:monospace,monospace"> * Goes through a lst of variables and updates it's value</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">lst -> list<br>it's -> its</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10930/7/include/asterisk/config.h@382">Patch Set #7, Line 382:</a> <code style="font-family:monospace,monospace">int ast_var_update(struct ast_variable *vars, char *varname, char *varval);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Needs to take a 'struct ast_variable **' or return a 'struct variable *' - see note in config.c.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, make the arguments 'name' and 'value' for consistency with 'ast_variable_new'</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10930/7/main/config.c">File main/config.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/10930/7/main/config.c@839">Patch Set #7, Line 839:</a> <code style="font-family:monospace,monospace">int ast_var_update(struct ast_variable *vars, char *varname, char *varval)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Because you could be adding or replacing the head of this list, this needs to either take a struct ast_variable **, or return a struct ast_variable *.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10930/7/main/config.c@841">Patch Set #7, Line 841:</a> <code style="font-family:monospace,monospace"> struct ast_variable *cur = NULL, *prev = NULL, *tmpvar = NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Defaulting cur and tmpvar is unnecessary as they will be initialized before use.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10930/7/main/config.c@843">Patch Set #7, Line 843:</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;">if(!(tmpvar = ast_variable_new(varname, varval, ""))) {<br> return -1;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Split this into two lines. The assignment to tmpvar, and then the NULL check.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10930/7/main/config.c@848">Patch Set #7, Line 848:</a> <code style="font-family:monospace,monospace"> if(!strcmp(varname, cur->name)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Space after 'if'</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10930/7/main/config.c@850">Patch Set #7, Line 850:</a> <code style="font-family:monospace,monospace"> if(prev == NULL)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Space after 'if' and this if/else block needs braces.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10930/7/main/config.c@861">Patch Set #7, Line 861:</a> <code style="font-family:monospace,monospace"> vars = tmpvar;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">See comment above, this is not going to update the caller's variable list.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/10930">change 10930</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/c/asterisk/+/10930"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ie1897a96c82b8945e752733612ee963686f32839 </div>
<div style="display:none"> Gerrit-Change-Number: 10930 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: Guido Falsi <madpilot@freebsd.org> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Guido Falsi <madpilot@freebsd.org> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 19 Mar 2019 14:05:57 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>