[Home]

Summary:ASTERISK-08446: [patch] Fix bad handling of #include directives from manager GetConfig/UpdateConfig
Reporter:Steven Sokol (ssokol)Labels:
Date Opened:2006-12-28 10:28:51.000-0600Date Closed:2007-08-29 15:41:15
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/Configuration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) config_patches.tar.gz
( 1) config_patches2.tar.gz
( 2) diffs.patch
Description:Currently the configuration parser used to process GetConfig/UpdateConfig manager commands follows the standard rules for reading configuration files: it reads until it hits a directive (#include or #exec).  When it hits a directive, it loads the included or executed file, then continues with the original.  Unfortunately, it does not retain any knowledge of the directive (i.e. it has no idea that a portion of the config data was added due to a #include).

This makes no difference when the config file is being read for the sake of the actual running configuration, but it presents a problem for the GetConfig and UpdateConfig manager commands.  When the UpdateConfig command goes to save the altered version of the configuration, it has no idea that a portion of the config data was actually inserted from another file.  Thus all of the data gets written back to the original file and the #include command disappears.

The attached patches avoid this issue by forcing the configuration parser to ignore any #include directives.  The load command simply treats them as comments so that they are retained and written back out during the save step.

****** ADDITIONAL INFORMATION ******

This change has both pros and cons.  On the pro side, you can use #include along with the GUI and things continue to work.  On the con side, GUI applications have to separately load each file you wish to manipulate.  The net gain is well worth the down side aspects as it allows you to store hundreds of lines of boilerplate (i.e. features, LCR tables, etc.) in files which are included.  This makes the reading of your "dynamic" configuration files much faster.

NOTE: THIS HAS BEEN IMPLEMENTED IN SUCH A WAY THAT WHEN ASTERISK LOADS A CONFIGURATION FILE IT WILL FOLLOW #INCLUDE AND #EXEC DIRECTIVES.  THE CHANGE ONLY APPLIES TO FILES LOADED BY THE MANAGER USING GetConfig AND UpdateConfig.

Note on the method of implementation:  there were several ways I could have approached this.  One would have been to alter the ast_config, ast_category, ast_variable and ast_comment structures to store the source file.  That seemed like too much work for too little pay-out.

The second was to add a new parameter to the config handling functions to pass an "IngnoreDirectives" parameter, but this would have created havoc with any external configuration engines, so I decided to go a third and more "hackish" route.

The configuration functions for 1.4 already include a "withcomments" integer flag which, if set to anything other than zero, forces the comments to be retained in the config structure.  I hijacked this argument.  If the argument is 0, comments are ignored and directives are processed as before.  If the argument is 1, comments are retained and directives are processed.  If the argument is 2, comments are retained and directives are treated as comments.

Note To Maintainers:  While you might consider this a feature, I think it does fix a bug (i.e. the miss-handling of #included files) and thus should be considered for inclusion in the next 1.4 release.
Comments:By: Steven Sokol (ssokol) 2006-12-28 10:55:15.000-0600

New version of patches ignores both #include and #exec for the manager UpdateConfig and GetConfig commands.
By: Anthony LaMantia (alamantia) 2006-12-28 11:22:28.000-0600

what would you think about doing something such as the following. just keeping track of the include level of the ast_category  structures by adding a new include_level variable to the ast_category structure itself. which can be inherited from the ast_config structure when ast_category_append() is called.

if we do that we can then just not write anything from a include level greater then 1 during config_text_file_save().


By: Anthony LaMantia (alamantia) 2007-01-03 16:14:53.000-0600

part of my patch for 0008678 includes include_level inheritance durring ast_catagory_append().

we can then use that new information from ast_catagory to begin to resolve this issue.
By: Anthony LaMantia-2 (anthonyl) 2007-01-21 15:03:13.000-0600

my patch for 0008678  was included into asterisk , someone should be able to write a new patch to resolve this....
By: Steven Sokol (ssokol) 2007-01-25 08:20:09.000-0600

Before anyone puts in effort in adapting the config structures or otherwise modifying the parser, let me put in a plug for handling directives (#include/#exec) by NOT handling them...

The GetConfig/UpdateConfig manager commands were created primarily for the purposes of the GUI and for that reason I really think that when GetConfig/UpdateConfig parses a configuration file, the directives should be passed through (just like comments).  I suggest this for the following reasons:

1) When doing AJAM calls it is best to work with the minimum necessary data set.  Rather than pulling across a massive set of structures comprised (in our example) of the core extensions file, the boilerplate macros, features, national numbering plans, etc., it is best to just pull the part you need to view or alter.  This steals less cycles from the CPU, puts less traffic across the wire, and makes the client more responsive.

2) Even if you track the include level when reading in a configuration, we have no way of specifying the include level of new config data added to the configuration.  Rather than trying to come up with a way to specify that, just open the #included file you want to edit.

3) There is no realistic way to handle #exec directives -- they MUST be passed through.

Thanks,

Steve
By: Anthony LaMantia-2 (anthonyl) 2007-01-25 15:20:17.000-0600

I think not processing includes from the manager interface it self is just plain wrong, if you want to edit extensions.conf for example, you want to edit it and any files include by it, as asterisk loads extensions.conf and the files included in it as your dialplan...

i just can't see the huge advantage of taking a shortcut to fix this bug, all i can see is that working in a few situations and causing a great deal of problems in others.


By: Steve Murphy (murf) 2007-03-23 13:12:57

ssokol-- Looks like I inherited this issue, and I have to agree with alamantia; by default, none of the digium example configs use #include; But where a #include is used, the contents of the included file is usually very relevant to the configuration! If the users at the other end know this will happen, and can pull any file they want out for edit, this could be OK...

So, please, help me to understand why skipping/storing these directives and regurgitating them is good... I'm no expert at the manager end of things!
By: Steve Murphy (murf) 2007-03-23 13:16:42

Pari--

Could you take a look at this bug, 8684, on what the manager interface should do with #include and #exec directives in config files? Is ssokol's proposal good or bad?
By: Pari Nannapaneni (pari) 2007-03-27 15:02:33

I vote for ssokol's proposal but anthonyl has a strong point here.

Getconfig should give some kind of feedback/clue to the user if it is
ignoring output from included files.

That means getconfig(file1) should output only content of file1 -
but the output should also give some clues to the user that file2 and file3's
were included but omitted. This way if the user needs those files, he can make
separate getconfig requests on them.

Here is a sample request and the corresponding OUTPUT
** Request **
http://$IP/rawman?action=getconfig&filename=extensions.conf

** Current Output **
Response: Success
Category-000000: general
Line-000000-000000: static=yes
Line-000000-000001: writeprotect=no
Line-000000-000002: autofallthrough=yes
Line-000000-000003: clearglobalvars=no
Line-000000-000004: priorityjumping=no

** Recommended Output if there are any included files **
Response: Success
Category-000000: general
Line-000000-000000: static=yes
Line-000000-000001: writeprotect=no
Line-000000-000002: autofallthrough=yes
Line-000000-000003: clearglobalvars=no
Line-000000-000004: priorityjumping=no
Include-000000-000001: file2
Include-000000-000002: file3

This way the user gets a clean output and he also gets a clue if there are any files included and what those files are. If he needs to read those files
he can issue separate getconfig requests.

-Pari
By: Steve Murphy (murf) 2007-08-27 09:25:22

I personally think we could upgrade the config reader to remember #include directives, and we can mark each entry as to its origin.

We can then regenerate the entire include tree when we write the config files.

This issue is kinda old; is it worth my time to make these enhancements?
By: Steve Murphy (murf) 2007-08-29 15:19:53

OK, I've uploaded the diffs.patch, which applies to trunk.

I will momentarily commit these changes to trunk, and see what the reaction is.

If this fix is borked, or not what y'all are looking for, it can be reverted.
By: Steve Murphy (murf) 2007-08-29 15:41:14

I merged the fix into 81361 in trunk; I could do it to 1.4, if this is generally considered a bug fix vs. an enhancement.  What I said:

"This code was in team/murf/bug8684-trunk; it should fix bug 8684 in trunk. I didn't add it to 1.4 yet, because it's not entirely clear to me if this is a bug fix or an enhancement. A lot of files were affected by small changes like ast_variable_new getting an added arg, for the file name the var was defined in; ast_category_new gets added args of filename and lineno; ast_category and ast_variable structures now record file and lineno for each entry; a list of all #include and #execs in a config file (or any of its inclusions are now kept in the ast_config struct; at save time, each entry is put back into its proper file of origin, in order. #include and #exec directives are folded in properly.  Headers indicating that the file was generated, are generated also for each included file. Some changes to main/manager.c to take care of file renaming, via the UpdateConfig command. Multiple inclusions of the same file are handled by exploding these into multiple include files, uniquely named. There's probably more, but I can't remember it right now."

Re-open this issue if there are problems, or open a new one... it's up to you.

murf