[asterisk-dev] [Code Review] Add app_v110 to accept v.110 data calls

Mon Aug 29 09:37:58 CDT 2011

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1394/#review4155
-----------------------------------------------------------

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
<https://reviewboard.asterisk.org/r/1394/#comment8146>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8157>

    Attribute here should be "required"

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8153>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8155>

    Don't use C++ style comments (2.1)

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8154>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8156>

    Don't use C++ style comments (2.1)

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8147>

    Consider using an enum for these rather than a series of #defines per the coding standard (2.13)

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8158>

    While not necessary, it is probably preferable to specify the full type here, i.e., unsigned int

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8203>

    For clarity, define each of these on a separate line

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8192>

    These methods can probably all be made static.

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8173>

    Brackets on the if statement

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8172>

    Log a warning here that a failure to write occurred.  Use brackets on the if statement.

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8148>

    Use sscanf here - see coding standard section 2.15

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8150>

    Use brackets and indent the subsequent line

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8149>

    Use sscanf here - see coding standard section 2.15

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8151>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8152>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8193>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8186>

    Why is the number of buffer warnings that will be emitted 5?

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8169>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8194>

    Brackets.  Use ast_debug instead of if (0) - although this should be refactored into a unit test.

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8195>

    Use ast_debug with an appropriate level of debug associate with it

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8196>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8197>

    Check for failure on the system calls here

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8177>

    Instead of if (0), make this an ast_debug statement at an appropriately high level of debug.

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8178>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8180>

    Change this syntax so that the order of operations is a little bit clearer.

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8181>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8184>

    Per the coding standard, variable declarations should be at the beginning of the method (2.3)

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8182>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8183>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8185>

    Per the coding standard, variable declarations should be at the beginning of the method (2.3).  Provide the type 'int' as well.

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8187>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8190>

    Does this need to be a caveat documented in the XML application description?

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8188>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8189>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8199>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8200>

    Brackets

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8161>

    chan can probably be const in this method

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8160>

    Use ast_realloc

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8159>

    Use ast_malloc

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8163>

    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
<https://reviewboard.asterisk.org/r/1394/#comment8164>

    You should test for ast_opt_high_priority before setting this

/trunk/apps/app_v110.c
<https://reviewboard.asterisk.org/r/1394/#comment8201>

    Brackets

/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/1394/#comment8198>

    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.

- mjordan

On Aug. 26, 2011, 7:19 p.m., dwmw2 wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1394/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2011, 7:19 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This adds a new application app_v110 which accepts data calls from a GSM mobile phone.
> 
> 
> This addresses bug ASTERISK-14185.
>     https://issues.asterisk.org/jira/browse/ASTERISK-14185
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 333428 
>   /trunk/apps/app_v110.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1394/diff
> 
> 
> Testing
> -------
> 
> 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 at obelisk ~]$ 
> 
> 
> Thanks,
> 
> dwmw2
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110829/587cacde/attachment-0001.htm>