[asterisk-dev] [Code Review] Use new ao2_global_obj API to clean up udptl.c config parsing/reloading

[Mark Michelson]

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



/trunk/main/config.c
<https://reviewboard.asterisk.org/r/1840/#comment10873>

    This is making the assumption that, given a possible range of values, the range bounds correspond to the default values. Is this a safe assumption to be making? For instance, with RTP port range settings, the range boundaries are from 1-65535. However, the default settings for RTP port range is 10000-20000. When using PARSE_RANGE_DEFAULTS, you need to specify both the bounds for the range AND separate default low and high values.



/trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1840/#comment10871>

    s/Dispaly/Display/
    s/udptl/UDPTL/



/trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1840/#comment10872>

    You should invert this if statement here so that the failure case can duck out early. This way the actual meat of the function can be backed off one level of indentation.



/trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1840/#comment10874>

    In the case that an out-of-range value is given to ast_parse_arg, it will return -1 instead of 0. You can check the return here and emit a warning message if the value had to be adjusted.



/trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1840/#comment10876>

    Amen, bro



/trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1840/#comment10875>

    This feels wrong. During a reload, if the config file is broken in some way, shouldn't the old options be left alone instead of being replaced with the defaults?


- Mark


On April 2, 2012, 2:06 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1840/
> -----------------------------------------------------------
> 
> (Updated April 2, 2012, 2:06 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This patch builds off of https://reviewboard.asterisk.org/r/1455/.
> 
> The differences are:
> 1) Uses the global ao2 object API to store config values and to make reloads thread-safe
> 2) Uses ast_parse_arg() where appropriate
> 3) Updates ast_parse_arg to be able to set defaults to the min/max range value on range errors
> 4) Adds CLI command 'udptl show config'
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/config.h 360980 
>   /trunk/main/config.c 360980 
>   /trunk/main/udptl.c 360980 
> 
> Diff: https://reviewboard.asterisk.org/r/1840/diff
> 
> 
> Testing
> -------
> 
> 1) Used the new 'udptl show config' command to verify that settings looked right on load/reload.
> 2) Did testing with 'memory show summary config.c/udptl.c' with loads/reloads to verify there are no leaks
> 3) Loaded with no config file to verify that default values were used
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120403/2db663f7/attachment-0001.htm>