[asterisk-dev] [Code Review] 3575: config: Fix config files not reloading when only an included file changes.

[rmudgett]

> On May 30, 2014, 9:53 a.m., Matt Jordan wrote:
> > /branches/1.8/main/config.c, lines 1058-1062
> > <https://reviewboard.asterisk.org/r/3575/diff/2/?file=59028#file59028line1058>
> >
> >     Just for readability, I'd put a space between the end of the statement and the comment:
> >     
> >     strcpy(dst, filename); /* Safe */
> >     
> >     There's a few other places this applies to as well.

done


> On May 30, 2014, 9:53 a.m., Matt Jordan wrote:
> > /branches/1.8/main/config.c, lines 1552-1570
> > <https://reviewboard.asterisk.org/r/3575/diff/2/?file=59028#file59028line1552>
> >
> >     The indentation here is weird. Is it intentional that the following do loop is included with the else of the glob statement?
> >     
> >     If so, a comment at least indicating that would be nice.

Yes it is intentional and the do/while loop is actually in the body of the for loop within the else clause.  It has been this way from before revision -r6000.  We're talking about ancient code here where readability was the next persons problem.  This routine badly needs refactoring.  At the least it should be indented correctly rather than having a chunk of it outdented/unindented by a few levels.

The AST_INCLUDE_GLOB code could always be included because it is always defined.

Just adding a comment that the indentation is screwey rather than actually fixing the indentation doesn't seem right.


> On May 30, 2014, 9:53 a.m., Matt Jordan wrote:
> > /branches/1.8/main/config.c, lines 1655-1657
> > <https://reviewboard.asterisk.org/r/3575/diff/2/?file=59028#file59028line1655>
> >
> >     The globbuf could be free'd earlier than this now (in the first #ifdef block testing for AST_INCLUDE_GLOB), which would remove the need for this #ifdef.

No.  The globbuf cannot be freed before now because an active for loop is iterating over it.  I think the screwed up indentation got you. :)


> On May 30, 2014, 9:53 a.m., Matt Jordan wrote:
> > /branches/1.8/main/config.c, lines 1617-1623
> > <https://reviewboard.asterisk.org/r/3575/diff/2/?file=59028#file59028line1617>
> >
> >     Just to confirm, we no longer need to glob here due to always reloading in the case of a wildcard/directory include, correct?
> >     
> >

Yes that is correct.  The removed comment about needing to glob here to detect an added file was confusing because it didn't state why it was needed very well.

The code also had a bug here because fn2[] didn't get ast_config_AST_CONFIG_DIR prepended if the include didn't have a '/' as the first character.  As a result the glob would not expand any wildcards unless the include explicitly had the full path.


- rmudgett


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


On May 29, 2014, 5:11 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3575/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 5:11 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23683
>     https://issues.asterisk.org/jira/browse/ASTERISK-23683
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The twisted logic determining if a config file should be reloaded was mostly broken and disabled.  The incorrect test that ASTERISK-23383 fixed actually reenabled the broken logic.  The incorrect test was causing the timestamp to always be cleared which caused config files with includes to always be reloaded.
> 
> * Made wildcard includes always cause a reload.  Determining if a file was deleted cannot be determined without restructuring the cache to determine if any files are missing from the last files actually loaded.  Also without refactoring config_text_file_load(), the glob loop couldn't check more than one file for changes anyway.
> 
> * Made remove the cache entry if the file no longer exists when trying to get its timestamp or it is no longer a regular file.  This fixes the corner case where the file was loaded, then deleted, then the config reloaded, then the file restored with the same timestamp, and then the config reloaded again.
> 
> * Made remove the cache entry include list when actually loading the file.  This gets rid of any stale includes the file had from the last time the file was loaded.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/config.c 414853 
> 
> Diff: https://reviewboard.asterisk.org/r/3575/diff/
> 
> 
> Testing
> -------
> 
> Made sip.conf include a wildcard pattern.  The config was always reloaded even if no files were touched.
> 
> Made sip.conf include several specific files.  The config was reloaded when any file was touched.
> 
> Edited sip.conf to change which files the file included.  Verified with debug statements added to determine that the old include files were no longer checked on subsequent reload attempts.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140530/af77491d/attachment-0001.html>