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

[KNK]

> On Sept. 17, 2012, 9:09 a.m., Pedro Kiefer wrote:
> > /trunk/apps/app_alarmreceiver.c, line 247
> > <https://reviewboard.asterisk.org/r/2088/diff/3/?file=31250#file31250line247>
> >
> >     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.

I have moved the timeouts and loudness to global constants, as they do not change.


> On Sept. 17, 2012, 9:09 a.m., Pedro Kiefer wrote:
> > /trunk/apps/app_alarmreceiver.c, line 687
> > <https://reviewboard.asterisk.org/r/2088/diff/3/?file=31250#file31250line687>
> >
> >     This if is getting too many OR's and AND's... makes it harder to read.

Moved to its own function.


> On Sept. 17, 2012, 9:09 a.m., Pedro Kiefer wrote:
> > /trunk/apps/app_alarmreceiver.c, line 590
> > <https://reviewboard.asterisk.org/r/2088/diff/3/?file=31250#file31250line590>
> >
> >     I would prefer to have the auto-detect block in its own function.

Done.


> On Sept. 17, 2012, 9:09 a.m., Pedro Kiefer wrote:
> > /trunk/apps/app_alarmreceiver.c, line 640
> > <https://reviewboard.asterisk.org/r/2088/diff/3/?file=31250#file31250line640>
> >
> >     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.

I have made them channel variables on purpose - you can change them mid call and they are checked after each iteration.


> On Sept. 17, 2012, 9:09 a.m., Pedro Kiefer wrote:
> > /trunk/apps/app_alarmreceiver.c, line 704
> > <https://reviewboard.asterisk.org/r/2088/diff/3/?file=31250#file31250line704>
> >
> >     I never liked this list implementation, i think we should change it and use AST_LIST macros.

I have not used those macros so far, but will take a look.
I wonder does any one use log_individual_events at all, because this delays the event processing, also event_app should probably be executed on each event and then ... there is just no need of a list


- KNK


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


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/20120918/8d702bc6/attachment-0001.htm>