<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8801">View Change</a></p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p style="white-space: pre-wrap; word-wrap: break-word;">The patch looks good other than formatting and some typos.</p><p>(14 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8801/1//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1//COMMIT_MSG@9">Patch Set #1, Line 9:</a> <code style="font-family:monospace,monospace">ASTERISK-27726</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be near the bottom of the commit message.<br>https://wiki.asterisk.org/wiki/display/AST/Commit+Messages</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c">File addons/chan_mobile.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@708">Patch Set #1, Line 708:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Stray blank line added.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@864">Patch Set #1, Line 864:</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 (cidinfo == NULL) {<br> /* Null pointer. Dummy up CID info. */<br> localcid.cnam = '\0';<br> localcid.cnum = '\0';<br> }<br> else<br> {<br> /* Valid CID info */<br> localcid.cnum = cidinfo->cnum;<br> localcid.cnam = cidinfo->cnam;<br> }<br><br> chn = ast_channel_alloc(1, state, localcid.cnum, localcid.cnam, 0, 0, pvt->context,<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You can simplify the code by doing this:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!cidinfo) {<br> static struct cidinfo localcid;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> cidinfo = &localcid;<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">chn = ast_channel_alloc(..., cidinfo->cnum, cidinfo->cnam, ...);</p><p style="white-space: pre-wrap; word-wrap: break-word;">Or you can completely eliminate the localcid and just do this:</p><p style="white-space: pre-wrap; word-wrap: break-word;">chn = ast_channel_alloc(..., cidinfo ? cidinfo->cnum : NULL, cidinfo ? cidinfo->cnam : NULL, ...);</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2226">Patch Set #1, Line 2226:</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;"> * \return a cidinfo structure pointing to the cnam and cnum<br> * \data in buf. On parse errors, either or both pointers <br> * \will point to null strings<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/\data/data/<br>s/\will/will/<br>The \ at the beginning of the lines identifies a doxygen header not something that is required for every line.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Red blob at the end of line 2227. Red blobs indicate trailing whitespace that needs to be removed.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2237">Patch Set #1, Line 2237:</a> <code style="font-family:monospace,monospace"> struct cidinfo cidinfo = {NULL,NULL};</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to examine the coding guidelines [1]. Most of the problems you have are with missing white space around operators.</p><p style="white-space: pre-wrap; word-wrap: break-word;">x = 5;<br>instead of<br>x=5;</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2253">Patch Set #1, Line 2253:</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;"> cidinfo.cnum = &buf[tokens[1]];<br> ast_strip_quoted(cidinfo.cnum, "\"", "\"");<br> if (*cidinfo.cnum == '"') {cidinfo.cnum++;}; /* ast_strip_quoted leaves leading quote symbol */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You are ignoring the return value of ast_strip_quoted() which returns a pointer to a string that does not have the quotes.<br>So the marked code is best replaced with the below line:</p><p style="white-space: pre-wrap; word-wrap: break-word;">cidinfo.cnum = ast_strip_quoted(&buf[tokens[1]], "\"", "\"");</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2256">Patch Set #1, Line 2256:</a> <code style="font-family:monospace,monospace"> if (!(ast_isphonenumber(cidinfo.cnum))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Extra parentheses.</p><p style="white-space: pre-wrap; word-wrap: break-word;">if (!ast_isphonenumber(cidinfo.cnum)) {<br>}</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2261">Patch Set #1, Line 2261:</a> <code style="font-family:monospace,monospace"> /* Some docs say tokens 2 and 3 including the commas are optional. If absent, that would move CNAM </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">red blob</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2267">Patch Set #1, Line 2267:</a> <code style="font-family:monospace,monospace"> while (buf[i] == ' ') {i++;}; /* Find the first non-blank */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Guidelines: curlie braces</p><p style="white-space: pre-wrap; word-wrap: break-word;">while () {<br>}</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2275">Patch Set #1, Line 2275:</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;"> ast_strip_quoted(cidinfo.cnam, "\"", "\"");<br> if (*cidinfo.cnam == '"') {cidinfo.cnam++;}; /* ast_strip_quoted leaves leading quote symbol */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here about ast_strip_quoted() use.</p><p style="white-space: pre-wrap; word-wrap: break-word;">cidinfo.cnam = ast_strip_quoted(cidinfo.cnam, "\"", "\"");</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2278">Patch Set #1, Line 2278:</a> <code style="font-family:monospace,monospace"> if (!strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ 0123456789-,abcdefghijklmnopqrstuvwxyz_", *cnamtmp)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Guidelines: tabs not spaces to indent.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2293">Patch Set #1, Line 2293:</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 string - the null-terminated string being parsed (will be altered!)<br> * \param start - where the current token starts<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The dashes should not be there.</p><p style="white-space: pre-wrap; word-wrap: break-word;">\param string What is string<br>\param start What is start</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2295">Patch Set #1, Line 2295:</a> <code style="font-family:monospace,monospace"> * \delip - the token termination delimiter. \0 is also considered a terminator.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/\delip -/\param delim/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@3662">Patch Set #1, Line 3662:</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 (!cidinfo.cnum) {<br> ast_debug(1, "[%s] error parsing CLIP: %s\n", pvt->id, buf);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">cidinfo.cnum is never going to be NULL. It will be an empty string.</p><p style="white-space: pre-wrap; word-wrap: break-word;">if (ast_strlen_zero(cidinfo.cnum)) {<br>}</p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually this test is not needed at all anymore since you have already done an ast_debug() log in hfp_parse_clip().</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8801">change 8801</a>. To unsubscribe, 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/8801"/><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: I89490d85fa406c36261879c50ae5e65595538ba5 </div>
<div style="display:none"> Gerrit-Change-Number: 8801 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: B. Martin <asterisk-forum@silverflash.net> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 27 Apr 2018 19:07:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>