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

[rmudgett]

> On Oct. 20, 2011, 11:14 a.m., mjordan wrote:
> > trunk/main/http.c, line 1010
> > <https://reviewboard.asterisk.org/r/1460/diff/1/?file=20887#file20887line1010>
> >
> >     I'm not sure I like this portion moved below the resetting of the http_tls_cfg.  Imagine the following scenario:
> >     
> >     1. An existing http session exists where TLS is enabled
> >     2. The admin edits http.conf, and puts a typo somewhere in there
> >     3. A reload occurs over the CLI.  On one thread, we perform the http_load, while at the same time, the existing http session performs a request which calls httpstatus_callback
> >     
> >     Since the config file is invalid, we would reset the TLS information (potentially at the same time as an existing http session is attempting to use it), and then fail to load the rest of the http configuration information.
> >     
> >     In general, I would think we would want something like the following for configuration loading that supports reloading:
> >     1. Load all information into a temp object
> >     2. Verify that all information is correct and valid
> >     3. Lock the configuration objects
> >     4. Update configuration objects from the temp
> >     5. Unlock
> >

Yes, moving the check from where it was is definitely bad.  If the config file did not change, we would always reset the active config to defaults.


> On Oct. 20, 2011, 11:14 a.m., mjordan wrote:
> > trunk/main/http.c, line 1015
> > <https://reviewboard.asterisk.org/r/1460/diff/1/?file=20887#file20887line1015>
> >
> >     None of this is thread safe.  This is a general problem throughout this load if the server is started - we blow out the previous configuration at the the same time httpstatus_callback (and other things in tcptls) could be accessing it.
> >     
> >     We could get around some of those problems using a ref counted object.  We'd still have to lock the portion where we switch the previous ref counted object with a new one, but it'd prevent a lot of weird scenarios that could occur if someone did a reload in the middle of an http request.

A RWLOCK could be used to block users while the config is updated.


- rmudgett


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


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

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