[asterisk-dev] [Code Review] Rework the Asterisk indications API

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/149/#review379
-----------------------------------------------------------



/trunk/include/asterisk/indications.h
<http://reviewboard.digium.com/r/149/#comment899>

    "param country"



/trunk/main/indications.c
<http://reviewboard.digium.com/r/149/#comment900>

    I'm not going to pretend to know what the heck is going on in this loop, but I notice that x has an upper bound of len/2 here. There is a variable called "samples" used earlier which is equal to len/2, so I wonder if this would read more clearly using samples instead of len/2.



/trunk/main/indications.c
<http://reviewboard.digium.com/r/149/#comment901>

    Before putting this odd, yet fully C99-compliant, syntax in the code, you may want to verify that there won't be problems on certain platforms or versions of gcc.
    
    It may be less hassle to just change this to an if statement since there is only one case after all.



/trunk/main/indications.c
<http://reviewboard.digium.com/r/149/#comment906>

    Looks like you never check for cfg to be NULL.



/trunk/res/snmp/agent.c
<http://reviewboard.digium.com/r/149/#comment903>

    I don't understand why either of these structs are static since they both get reset for every call to this function.



/trunk/res/snmp/agent.c
<http://reviewboard.digium.com/r/149/#comment904>

    ao2_container_count will make your life a lot easier here.



/trunk/res/snmp/agent.c
<http://reviewboard.digium.com/r/149/#comment905>

    Why not set var_len to be the strlen of ret_buf after doing the ast_copy_string operation?


I'll take a closer look at refcounts and such when you get the ast_tone_zone_sound portion complete.

- Mark


On 2009-02-10 14:59:43, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/149/
> -----------------------------------------------------------
> 
> (Updated 2009-02-10 14:59:43)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch includes a number of changes to the indications API.  The primary motivation for this work was to improve stability.  The object management in this API was significantly flawed, and a number of trivial situations could cause crashes.
> 
> The changes included are:
> 
> 1) Remove the module res_indications.  This included the critical functionality that actually loaded the indications configuration.  I have seen many people have Asterisk problems because they accidentally did not have an indications.conf present and loaded.  In fact, this came up again in #asterisk-dev today.  Now, this code is in the core, and Asterisk will fail to start without indications configuration.  
> 
> There was one part of res_indications, the dialplan applications, which did belong in a module, and have been moved to a new module, app_playtones.
> 
> 2) Object management has been significantly changed.  Tone zones are now managed using astobj2, and it is no longer possible to crash Asterisk by issuing a reload while tone zones are in use.
> 
> 3) The API documentation has been filled out.
> 
> 4) The API has been updated to follow our naming conventions.
> 
> 5) Various bits of code throughout the tree have been updated to account for the API update.
> 
> 6) Configuration parsing has been mostly re-written.
> 
> 7) "Code cleanup"
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_disa.c 174745 
>   /trunk/apps/app_playtones.c PRE-CREATION 
>   /trunk/apps/app_read.c 174745 
>   /trunk/apps/app_readexten.c 174745 
>   /trunk/channels/chan_skinny.c 174745 
>   /trunk/channels/chan_unistim.c 174745 
>   /trunk/configs/indications.conf.sample 174745 
>   /trunk/funcs/func_channel.c 174745 
>   /trunk/include/asterisk/_private.h 174745 
>   /trunk/include/asterisk/channel.h 174745 
>   /trunk/include/asterisk/indications.h 174745 
>   /trunk/main/app.c 174745 
>   /trunk/main/asterisk.c 174745 
>   /trunk/main/channel.c 174745 
>   /trunk/main/indications.c 174745 
>   /trunk/main/loader.c 174745 
>   /trunk/main/pbx.c 174745 
>   /trunk/res/res_indications.c 174745 
>   /trunk/res/snmp/agent.c 174745 
> 
> Diff: http://reviewboard.digium.com/r/149/diff
> 
> 
> Testing
> -------
> 
> Basic testing playing different tones using the Playtones() application works fine.
> 
> 
> Thanks,
> 
> Russell
> 
>