Review Board 1.7.16


Add app_v110 to accept v.110 data calls

Review Request #1394 - Created Aug. 26, 2011 and updated

dwmw2
ASTERISK-14185
Reviewers
asterisk-dev
Asterisk
This adds a new application app_v110 which accepts data calls from a GSM mobile phone.
Asterisk 1.8.5.0+app_v110, ISDN BRI on British Telecom, Nokia N97 GSM phone...

AT+CBST=71,0,1
OK
ATD01799xxxxxx
CONNECT
Fedora release 15 (Lovelock)
Kernel 2.6.40.3-0.fc15.i686 on an i686 (/dev/pts/34)

obelisk.infradead.org login: dwmw2
Password: 
Last login: Sat Aug 27 01:02:52 2011 from 07976xxxxxx
[dwmw2@obelisk ~]$ 

Posted (Aug. 29, 2011, 4:37 a.m.)
I can't really comment or review whether or not the application functions as its intended, so I stuck to coding guidelines and other tangential areas.  Someone else will have to weigh in on the domain specific areas.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Most application names are a bit more verbose then this, and typically self describe what it is that they're doing, e.g., Answer, Dial, etc.  I'm not sure what would be appropriate for this, but it would be nice if this were more self describing then just "V110"
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Attribute here should be "required"
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Rather than there being a precompiled testing mode, this should use the TEST_FRAMEWORK compilation flag, and all loopback testing functionality should be implemented as unit tests.

Really, this application would benefit greatly from having some unit tests written for it, particularly the inboud / outbound frame manipulation routines.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Don't use C++ style comments (2.1)
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Then why have it?  If its for self-testing, then it should only be available in the guise of the unit tests.  If it has application beyond that but has security risks, it should probably be a parameter to the application, and be documented how it can expose you to security problems.

Compilation flags means changes should be made to menuselect, which introduces additional complexity.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Don't use C++ style comments (2.1)
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Consider using an enum for these rather than a series of #defines per the coding standard (2.13)
/trunk/apps/app_v110.c (Diff revision 1)
 
 
While not necessary, it is probably preferable to specify the full type here, i.e., unsigned int
/trunk/apps/app_v110.c (Diff revision 1)
 
 
For clarity, define each of these on a separate line
/trunk/apps/app_v110.c (Diff revision 1)
 
 
These methods can probably all be made static.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets on the if statement
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Log a warning here that a failure to write occurred.  Use brackets on the if statement.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Use sscanf here - see coding standard section 2.15
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Use brackets and indent the subsequent line
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Use sscanf here - see coding standard section 2.15
/trunk/apps/app_v110.c (Diff revision 1)
 
 
What purpose does this comment serve?  If this comment is useful, rephrase this so that it's not in the form of a question.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
It may be better to check if the channel is DAHDI or mISDN, and if not, reject it in the application.  Since your its unlikely the other channels would behave well with the application, allowing them to proceed could lead to unintended consequences.

At a minimum, the channel limitations should be documented in the XML description of the application.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Why 200?  If this value has some meaning, other than its less than 4000, it should be a constant or macro'd to convey its meaning
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Why is the number of buffer warnings that will be emitted 5?
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Is it necessary to change the priority of the parent process here to something higher than the previously spawned child?
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets.  Use ast_debug instead of if (0) - although this should be refactored into a unit test.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Use ast_debug with an appropriate level of debug associate with it
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Should we hang up the channel here, or let the dialplan hangup after the application exits?

Either way, AST_SOFTHANGUP_SHUTDOWN is used on system shutdown, which isn't the case here.  I would imagine this would probably be AST_SOFTHANGUP_DEV or AST_SOFTHANGUP_EXPLICIT.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Check for failure on the system calls here
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Instead of if (0), make this an ast_debug statement at an appropriately high level of debug.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Change this syntax so that the order of operations is a little bit clearer.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Per the coding standard, variable declarations should be at the beginning of the method (2.3)
  1. Actually, this is allowed by the coding guidelines, since the declaration is at the beginning of the for loop's scope.
    
    The guideline you are thinking of states that variables shouldn't be declared "mid-block." In other words, no mixed declarations and code. Since this declaration is at the top of the scope, it's fine.
    
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Per the coding standard, variable declarations should be at the beginning of the method (2.3).  Provide the type 'int' as well.
  1. Actually, this is allowed by the coding guidelines, since the declaration is at the beginning of the for loop's scope.
    
    The guideline you are thinking of states that variables shouldn't be declared "mid-block." In other words, no mixed declarations and code. Since this declaration is at the top of the scope, it's fine.
    
    
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Does this need to be a caveat documented in the XML application description?
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/apps/app_v110.c (Diff revision 1)
 
 
chan can probably be const in this method
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Use ast_realloc
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Use ast_malloc
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Although its highly unlikely, the fcntl calls can still fail.  Check the return codes and fail gracefully if there is a problem.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
You should test for ast_opt_high_priority before setting this
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Brackets
/trunk/channels/chan_dahdi.c (Diff revision 1)
 
 
Is this change related to your application?  If so, since its in chan_dahdi, there should probably be some related testing to verify that no regressions were introduced.
Posted (Aug. 30, 2011, 4:14 a.m.)

   

  
  1. Ignore these since I've moved them up so they are replies to Matt's comments now.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Actually, this is allowed by the coding guidelines, since the declaration is at the beginning of the for loop's scope.

The guideline you are thinking of states that variables shouldn't be declared "mid-block." In other words, no mixed declarations and code. Since this declaration is at the top of the scope, it's fine.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
Same comment here as well.
Posted (Aug. 30, 2011, 4:30 a.m.)
Simple peer review
/trunk/apps/app_v110.c (Diff revision 1)
 
 
 
Comments should be doxygen style when ever possible
/trunk/apps/app_v110.c (Diff revision 1)
 
 
It would be nice to see doxygen comments for all functions in your application.
/trunk/apps/app_v110.c (Diff revision 1)
 
 
ast_debug()
/trunk/apps/app_v110.c (Diff revision 1)
 
 
ast_debug()
/trunk/apps/app_v110.c (Diff revision 1)
 
 
ast_debug()
/trunk/apps/app_v110.c (Diff revision 1)
 
 
ast_debug()
/trunk/apps/app_v110.c (Diff revision 1)
 
 
 
braces {}
/trunk/apps/app_v110.c (Diff revision 1)
 
 
remove space between ( (
/trunk/apps/app_v110.c (Diff revision 1)
 
 
remove space between ( (

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.