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

Tue Feb 10 21:14:27 CST 2009

> On 2009-02-10 19:10:15, Mark Michelson wrote:
> > /trunk/include/asterisk/indications.h, line 144
> > <http://reviewboard.digium.com/r/149/diff/1/?file=2552#file2552line144>
> >
> >     "param country"

fixed

> On 2009-02-10 19:10:15, Mark Michelson wrote:
> > /trunk/main/indications.c, line 198
> > <http://reviewboard.digium.com/r/149/diff/1/?file=2556#file2556line198>
> >
> >     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.

Agreed, changed.

The loop is magic.  :-)  It's computing the next set of samples for the current tone.

> On 2009-02-10 19:10:15, Mark Michelson wrote:
> > /trunk/main/indications.c, line 380
> > <http://reviewboard.digium.com/r/149/diff/1/?file=2556#file2556line380>
> >
> >     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.

It's not actually C99, I don't think.  It's a gcc C extension.  I like it, and we use a number of other gcc C extensions, so it's probably okay.  But, if it causes you heartburn, I don't mind removing it.  :-)

> On 2009-02-10 19:10:15, Mark Michelson wrote:
> > /trunk/main/indications.c, lines 1093-1100
> > <http://reviewboard.digium.com/r/149/diff/1/?file=2556#file2556line1093>
> >
> >     Looks like you never check for cfg to be NULL.

CONFIG_STATUS_FILEMISSING is NULL

> On 2009-02-10 19:10:15, Mark Michelson wrote:
> > /trunk/res/snmp/agent.c, lines 646-647
> > <http://reviewboard.digium.com/r/149/diff/1/?file=2560#file2560line646>
> >
> >     I don't understand why either of these structs are static since they both get reset for every call to this function.

The snmp code is really bizarre.  It has to be static.  All of the API calls are that way.  They return a pointer to static data.  I brought this up before and someone said "trust me".  We'd have to go dig into libnetsnmp sample code to understand further.

> On 2009-02-10 19:10:15, Mark Michelson wrote:
> > /trunk/res/snmp/agent.c, lines 654-667
> > <http://reviewboard.digium.com/r/149/diff/1/?file=2560#file2560line654>
> >
> >     ao2_container_count will make your life a lot easier here.

Sure enough.  I'll add an API call to get the number of tone zones.

> On 2009-02-10 19:10:15, Mark Michelson wrote:
> > /trunk/res/snmp/agent.c, lines 671-677
> > <http://reviewboard.digium.com/r/149/diff/1/?file=2560#file2560line671>
> >
> >     Why not set var_len to be the strlen of ret_buf after doing the ast_copy_string operation?

Good idea, much nicer, thanks.

- Russell

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

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
> 
>