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

[Terry Wilson]

> On April 3, 2012, 1:51 p.m., Mark Michelson wrote:
> > /trunk/main/udptl.c, lines 1441-1443
> > <https://reviewboard.asterisk.org/r/1840/diff/1/?file=26989#file26989line1441>
> >
> >     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.

Will do.


> On April 3, 2012, 1:51 p.m., Mark Michelson wrote:
> > /trunk/main/udptl.c, line 1438
> > <https://reviewboard.asterisk.org/r/1840/diff/1/?file=26989#file26989line1438>
> >
> >     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.

I originally tried something kind of like that and backed out of it. I think I thought I'd have to duplicate some code or something...who knows. I was probably sleepy.  Looks like a goto will make things work.


> On April 3, 2012, 1:51 p.m., Mark Michelson wrote:
> > /trunk/main/udptl.c, line 1350
> > <https://reviewboard.asterisk.org/r/1840/diff/1/?file=26989#file26989line1350>
> >
> >     s/Dispaly/Display/
> >     s/udptl/UDPTL/

Will do.


> On April 3, 2012, 1:51 p.m., Mark Michelson wrote:
> > /trunk/main/config.c, lines 2641-2647
> > <https://reviewboard.asterisk.org/r/1840/diff/1/?file=26988#file26988line2641>
> >
> >     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.

This option is only for setting a default value when there is a parsing error due to something being out of range. It isn't for defining a default range. rtp.conf has rtpstart/rtpend options. Unless I am misunderstanding, I don't think having bounds for both the range and high/low defaults makes sense since the high/low settings are separate options. The idea behind this options is that you set the default value initially when creating the config object, then if someone comes in and specifically overrides it with an out of range value, it gets set to the lowest/highest value allowable depending on which side of the range they are on. If someone just wants to set it to the default value on error, they would use the PARSE_DEFAULT flag instead.

It should be theoretically possible to use PARSE_DEFAULT and PARSE_RANGE_DEFAULTS for setting a different default if there was a range error or parse error, but I'm not sure that is useful. It also looks like strtoul isn't being checked for an error (0 return with errno set) and additional range checking could be done.


> On April 3, 2012, 1:51 p.m., Mark Michelson wrote:
> > /trunk/main/udptl.c, lines 1498-1500
> > <https://reviewboard.asterisk.org/r/1840/diff/1/?file=26989#file26989line1498>
> >
> >     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?

Yes. Defaults should really only be set on initial load instead of load/reload. I'll fix. Thanks.


- Terry


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


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/c9b5942a/attachment-0001.htm>