[asterisk-dev] [Code Review] app_alarmreceiver new features and fixes

[Pedro Kiefer]

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


The patch looks good. Added a few comments. 


/trunk/apps/app_alarmreceiver.c
<https://reviewboard.asterisk.org/r/2088/#comment13671>

    I think we are adding too many parameters to most functions.
    We could extract those variables to a struct, most of them are config constants, such as the timeout values.



/trunk/apps/app_alarmreceiver.c
<https://reviewboard.asterisk.org/r/2088/#comment13672>

    I would prefer to have the auto-detect block in its own function.



/trunk/apps/app_alarmreceiver.c
<https://reviewboard.asterisk.org/r/2088/#comment13673>

    Redefining the value of a variable to two different limits seems a little bit awkward. I think these limits could be on the same struct with the other config constants. 



/trunk/apps/app_alarmreceiver.c
<https://reviewboard.asterisk.org/r/2088/#comment13674>

    This if is getting too many OR's and AND's... makes it harder to read.



/trunk/apps/app_alarmreceiver.c
<https://reviewboard.asterisk.org/r/2088/#comment13675>

    I never liked this list implementation, i think we should change it and use AST_LIST macros.


- Pedro


On Sept. 17, 2012, 3:50 a.m., KNK wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2088/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2012, 3:50 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Adds new formats to app_alarmreceiver, ALAW calls support and protection from broken panels and malicious callers
> 
> 
> This addresses bug ASTERISK-20289.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20289
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_alarmreceiver.c 373116 
>   /trunk/configs/alarmreceiver.conf.sample 373116 
> 
> Diff: https://reviewboard.asterisk.org/r/2088/diff
> 
> 
> Testing
> -------
> 
> All new functionality was tested
> 
> 
> Thanks,
> 
> KNK
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120917/dd81e317/attachment.htm>