[asterisk-dev] [Code Review] Clean up udptl.conf parsing

[mjordan]

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



trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1455/#comment8717>

    Again, none of this is thread safe.  See other conf parsing reviews.
    
    This is particularly scary since we use udptlstart / udptlend in statements like this:
    
    x = (udptlstart == udptlend) ? udptlstart : (ast_random() % (udptlend - udptlstart)) + udptlstart;
    
    Highly unlikely you'd hit that and context switch in the middle of something like that during a reload, but still.  Another risk is that we set the value of these before validation - which means a configuration file with a udptlstart of 1 and udptlend of 2 (or something goofier) has a chance of being used.
    
    Also, it'd be nice if the default values for ports and limits were static consts that could be referenced at the top of the file.  That would apply to the port limits used to validate the udptlstart / udptlend values below.



trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1455/#comment8718>

    Again, I disagree with resetting configuration values used by a live system before checking if the config file loaded successfully.



trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1455/#comment8719>

    Don't use atoi, use sscanf instead



trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1455/#comment8720>

    sscanf



trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/1455/#comment8721>

    sscanf


- mjordan


On Sept. 23, 2011, 10:57 a.m., Paul Belanger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1455/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2011, 10:57 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Another patch to help clean up Asterisk configuration file parsing.  This time it is udptl.conf
> 
> 
> Diffs
> -----
> 
>   trunk/main/udptl.c 337894 
> 
> Diff: https://reviewboard.asterisk.org/r/1455/diff
> 
> 
> Testing
> -------
> 
> local development box
> 
> 
> Thanks,
> 
> Paul
> 
>

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