[Asterisk-code-review] chan mobile: support handling of caller-id names ("cnam"). (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8801 )

Change subject: chan_mobile: support handling of caller-id names ("cnam").
......................................................................


Patch Set 1: Code-Review-1

(14 comments)

The patch looks good other than formatting and some typos.

https://gerrit.asterisk.org/#/c/8801/1//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/8801/1//COMMIT_MSG@9
PS1, Line 9: ASTERISK-27726
This should be near the bottom of the commit message.
https://wiki.asterisk.org/wiki/display/AST/Commit+Messages


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c
File addons/chan_mobile.c:

https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@708
PS1, Line 708: 	
Stray blank line added.


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@864
PS1, Line 864: 	if (cidinfo == NULL) {
             : 		/* Null pointer.  Dummy up CID info.  */
             : 		localcid.cnam = '\0';
             : 		localcid.cnum = '\0';
             : 	}
             : 	else
             : 	{
             : 		/* Valid CID info */
             : 		localcid.cnum = cidinfo->cnum;
             : 		localcid.cnam = cidinfo->cnam;
             : 	}
             : 
             : 	chn = ast_channel_alloc(1, state, localcid.cnum, localcid.cnam, 0, 0, pvt->context,
You can simplify the code by doing this:

if (!cidinfo) {
   static struct cidinfo localcid;

   cidinfo = &localcid;
}

chn = ast_channel_alloc(..., cidinfo->cnum, cidinfo->cnam, ...);

Or you can completely eliminate the localcid and just do this:

chn = ast_channel_alloc(..., cidinfo ? cidinfo->cnum : NULL, cidinfo ? cidinfo->cnam : NULL, ...);


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2226
PS1, Line 2226:  * \return a cidinfo structure pointing to the cnam and cnum
              :  * \data in buf.  On parse errors, either or both pointers 
              :  * \will point to null strings
s/\data/data/
s/\will/will/
The \ at the beginning of the lines identifies a doxygen header not something that is required for every line.

Red blob at the end of line 2227.  Red blobs indicate trailing whitespace that needs to be removed.


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2237
PS1, Line 2237: 	struct cidinfo cidinfo = {NULL,NULL};
You need to examine the coding guidelines [1].  Most of the problems you have are with missing white space around operators.

x = 5;
instead of
x=5;

[1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2253
PS1, Line 2253: 	cidinfo.cnum = &buf[tokens[1]];
              : 	ast_strip_quoted(cidinfo.cnum, "\"", "\"");
              : 	if (*cidinfo.cnum == '"') {cidinfo.cnum++;};	/* ast_strip_quoted leaves leading quote symbol */
You are ignoring the return value of ast_strip_quoted() which returns a pointer to a string that does not have the quotes.
So the marked code is best replaced with the below line:

cidinfo.cnum = ast_strip_quoted(&buf[tokens[1]], "\"", "\"");


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2256
PS1, Line 2256: 	if (!(ast_isphonenumber(cidinfo.cnum))) {
Extra parentheses.

if (!ast_isphonenumber(cidinfo.cnum)) {
}


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2261
PS1, Line 2261: 	/* Some docs say tokens 2 and 3 including the commas are optional.  If absent, that would move CNAM 
red blob


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2267
PS1, Line 2267: 		while (buf[i] == ' ') {i++;};	/* Find the first non-blank */
Guidelines: curlie braces

while () {
}


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2275
PS1, Line 2275: 	ast_strip_quoted(cidinfo.cnam, "\"", "\"");
              : 	if (*cidinfo.cnam == '"') {cidinfo.cnam++;};	/* ast_strip_quoted leaves leading quote symbol */
Same here about ast_strip_quoted() use.

cidinfo.cnam = ast_strip_quoted(cidinfo.cnam, "\"", "\"");


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2278
PS1, Line 2278:                 if (!strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ 0123456789-,abcdefghijklmnopqrstuvwxyz_", *cnamtmp)) {
Guidelines: tabs not spaces to indent.


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2293
PS1, Line 2293:  * \param string - the null-terminated string being parsed (will be altered!)
              :  * \param start - where the current token starts
The dashes should not be there.

\param string What is string
\param start What is start


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@2295
PS1, Line 2295:  * \delip - the token termination delimiter.  \0 is also considered a terminator.
s/\delip -/\param delim/


https://gerrit.asterisk.org/#/c/8801/1/addons/chan_mobile.c@3662
PS1, Line 3662: 		if (!cidinfo.cnum) {
              : 			ast_debug(1, "[%s] error parsing CLIP: %s\n", pvt->id, buf);
              : 		}
cidinfo.cnum is never going to be NULL.  It will be an empty string.

if (ast_strlen_zero(cidinfo.cnum)) {
}

Actually this test is not needed at all anymore since you have already done an ast_debug() log in hfp_parse_clip().



-- 
To view, visit https://gerrit.asterisk.org/8801
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I89490d85fa406c36261879c50ae5e65595538ba5
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 1
Gerrit-Owner: B. Martin <asterisk-forum at silverflash.net>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:07:50 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180427/4835b673/attachment.html>