[asterisk-dev] [Code Review] HD ConfBridge application

[jrose]

> On 2011-04-06 16:58:47, Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, line 1176
> > <https://reviewboard.asterisk.org/r/1147/diff/3/?file=15999#file15999line1176>
> >
> >     = { '\0', }; would be a bit more explicit, but I would just do = "";
> >     
> >     Also, what is MAX_PIN supposed to be?  I would read that as the maximum length of a pin, when in reality, the max length of a pin is MAX_PIN - 1 (because of the nul terminator).
> 
> jrose wrote:
>     Actually, MAX_PIN would still be the maximum length of the pin.  The null terminator is in the MAX_PINth + 1 position since the array starts at 0.  But you know that :P
>     
>     I'm not sure I'm working that properly, so I'll just say... if MAX_PIN is 4 and the array length is 8...
>     0 - char 1
>     1 - char 2
>     2 - char 3
>     3 - char 4
>     4 - '\0'
> 
> jrose wrote:
>     Oh wait, this is a declaration.  Disregard everything I just said.
> 
> jrose wrote:
>     Actually though, wouldn't that terminator be at the beginning of the line resulting in the possibility of the array having no terminator after it is set?  It might seem like bad form for a string, but the code might always be using comparison functions that take the length into account, which would mean that this could still be the maximum pin length.

Been looking at this, and here are my thoughts...

= { 0, }; (and by extension = { '\0', }) is actually unnecessary.  I think.  ast_app_getdata is going to take tmp (currently pointing at pin_guess) and set the first value to \0 at the beginning of that function anyway, so the assignment ends up being repeated.  Not a big deal of course, it only happens at the beginning of the function.

Since \0 is continually put onto the end of the array and len = MAX_PIN - 1 and since it isn't using something specifically defined to limit comparison to MAX_PIN characters, Russell is right, pin_guess[MAX_PIN+1] would be more appropriate and len = MAX_PIN.  Since MAX_PIN is hard defined at 80 right now, this is pretty much trivial, but I don't know... maybe the intention will be to let users set the max pin length in confbridge.conf later.


- jrose


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


On 2011-03-28 15:47:43, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1147/
> -----------------------------------------------------------
> 
> (Updated 2011-03-28 15:47:43)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The new ConfBridge application.  It's kind of a big deal.
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/confbridge.conf.sample PRE-CREATION 
>   /trunk/include/asterisk/bridging.h 311748 
>   /trunk/include/asterisk/bridging_features.h 311748 
>   /trunk/include/asterisk/bridging_technology.h 311748 
>   /trunk/include/asterisk/channel.h 311748 
>   /trunk/include/asterisk/dsp.h 311748 
>   /trunk/main/bridging.c 311748 
>   /trunk/main/channel.c 311748 
>   /trunk/main/dsp.c 311748 
>   /trunk/apps/confbridge/include/confbridge.h PRE-CREATION 
>   /trunk/bridges/bridge_builtin_features.c 311748 
>   /trunk/bridges/bridge_softmix.c 311748 
>   /trunk/apps/confbridge/conf_config_parser.c PRE-CREATION 
>   /trunk/apps/app_confbridge.c 311748 
>   /trunk/CHANGES 311748 
>   /trunk/UPGRADE.txt 311748 
>   /trunk/apps/Makefile 311748 
>   /trunk/res/res_musiconhold.c 311748 
> 
> Diff: https://reviewboard.asterisk.org/r/1147/diff
> 
> 
> Testing
> -------
> 
> All confbridge.conf features have been tested.
> Load tested at sample rates ranging from 8-48khz.
> AMI actions/events tested
> CLI commands tested
> 
> 
> Thanks,
> 
> David
> 
>

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