Summary:	ASTERISK-02776: [patch] Allow globbing in #include on config files
Reporter:	Tzafrir Cohen (tzafrir)	Labels:
Date Opened:	2004-11-10 07:47:50.000-0600	Date Closed:	2008-01-15 15:16:21.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 2825_rev2.diff ( 1) includeglob.diff.txt ( 2) includeglob.diff.txt ( 3) includeglob-cvs.diff.txt
Description:	The attached patch adds glob exansion support (using glob(3) ) to the directive #include . That is, '#include "sip-*.conf"' will include all files of the pattern /etc/asterisk/sip-*.conf .
Comments:	By: twisted (twisted) 2004-11-10 14:51:46.000-0600 Do you have a disclaimer on file?  Is this for stable?  If so, it won't make it, as stable is not accepting any new features.  Please fix for cvs-head if it's built on stable. By: Brian West (bkw918) 2004-11-10 18:59:51.000-0600 No this is not how I would like to see it.. How about how apache does the config directory.  Where it will parse everything in a given directory? ie #include /etc/asterisk/sip <- if its a directory then it will parse all .conf files in that directory. bkw By: Tzafrir Cohen (tzafrir) 2004-11-11 02:38:15.000-0600 twisted: No disclaimer yet. Will be soon. It is not yet throughly tested. bkw918: It would be simpler to add a separate include directive #includedir , or make include try to include a directory if the last character of the file name is a '/'. This can save "magic". But there are too many "bad" cases which make me not want to include everything in a directory: * One may want to to manage configuration by RCS * a .bak file from certain editors * #file# from other editors * .file.swp from yet another editor I also want to make it easy to "rem-out" a config file without moving it to a different directory. I must be fair and mention a problem with globbing vs. a whole directory inclusion: if you don't trust all of Asterisk's configuration files, one can easily add an include pattern that will take much CPU time to expand, and thus cause some sort of DoS on Asterisk. Though I figure that some other problems may be caused by games with the config files, so it's not a big issue. By: Mark Spencer (markster) 2004-11-11 10:10:11.000-0600 I like globbing more than the dir.  This may have bizarre interactions with the realtime engine though *shrugs* By: Mark Spencer (markster) 2004-11-17 00:50:05.000-0600 Any update on the disclaimer and on a version for CVS head? By: twisted (twisted) 2004-12-01 01:15:29.000-0600 Reminder sent to tzafrir Please respond :) By: Tzafrir Cohen (tzafrir) 2004-12-05 10:02:47.000-0600 Updated patch (version for 1.0.2). Changes: better documented, less mess This version is tested and seems to work well. By: Tzafrir Cohen (tzafrir) 2004-12-05 10:05:42.000-0600 A patch vs. CVS version. Compiles. Seems to work. Not tested. (But that code hasn't changed much) By: Kevin P. Fleming (kpfleming) 2004-12-05 15:57:39.000-0600 You forgot to actually upload those last two patches... (you probably thought you could just point to the files and they would be uploaded when you clicked "Add Bugnote"... unfortunately it doesn't work that way, they are two separate processes). By: Tzafrir Cohen (tzafrir) 2004-12-06 01:48:44.000-0600 Disclaimer should be faxed today (local time). Anyway, as I said, the "cvs" version is more of an educated guess: You get it by basically adding anything inside the #ifdef blocks in the "right" place. The right place should be after the full file name is generated to 'fn' but before the fopen. The last bit just closes the scope. If it is not in the right place config.c simply won't compile. By: Russell Bryant (russell) 2004-12-07 13:50:42.000-0600 Just so you know, no new features will be going in to the stable branch.  Testing this against CVS head is what matters to get this in. > This may have bizarre interactions with the realtime engine though *shrugs* I suppose this needs to be looked into ... Thanks. By: Kevin P. Fleming (kpfleming) 2004-12-11 00:00:11.000-0600 Here is an updated version of the original patch (against current CVS), with the following improvements: - the proper filename is passed to cfg_process, so error messages during parsing show the correct name (not the wildcard) - the wildcard loop is now exited if any cfg_process return an error - the "parsing" and "found" messages are now properly repeated for each file found during the expansion - the unnecessary log messages have been removed By: Kevin P. Fleming (kpfleming) 2004-12-11 00:00:28.000-0600 Oh, and I have a disclaimer on file. By: Mark Spencer (markster) 2004-12-11 00:19:28.000-0600 Added to CVS head, thanks! By: Tzafrir Cohen (tzafrir) 2004-12-11 13:58:26.000-0600 Sorry to re-open this, but there is something U don't understand. That is: it seems like changing the way asterisk currently behaves. config file has "#include "a/*.conf" All of the files under a/ are OK, except a/bad.conf which is a dandling link or has some bad config. What will this cause (besides the obligatory warning)? 1. a/bad.conf is ignored. Rest of the config is read as usual 2. Everything up to a/bad.conf is read. Everything past it is ignored. That is: a/a.conf will be read, but not a/c.conf 3. The whole reading of the configuration file will fail. The module/app will fail to load . MY original patch takes (1). The current patch chooses (2) if I understand it right. The current behaviour on Asterisk is (1) (again, to the best of my humble understanding). Another test case: what happens with a config file that has the following snippet: #include "nonexistant.conf" where "nonexistant.conf" doesn't exist. Note that this involves no globing. Thanks for your time. By: Mark Spencer (markster) 2004-12-11 15:14:32.000-0600 Should be fixed in CVS now.  Feel free to reopen if it isn't.  Thanks! :) By: Russell Bryant (russell) 2004-12-11 15:55:32.000-0600 not in 1.0 By: Tzafrir Cohen (tzafrir) 2004-12-11 16:36:03.000-0600 Re-open it I will. But this time it is per kpfleming's request, who should post his note shortly. By: Kevin P. Fleming (kpfleming) 2004-12-11 16:48:26.000-0600 OK, the requested change (to handle non-existent files, or files that cannot be read) was not needed. Look at lines 604-609 of config.c; if the file could not be opened, it sends some messages about it, then continues around the file opening loop to the next file. The only time that it exits the loop is if there are no more files to open, or if any of the calls to cfg_process have returned an error. This last point is crucial; if cfg_process has returned an error, it has already called ast_destroy() on the "struct ast_config" that tmp points to, and it is GONE. tmp is set to NULL, and the loop in __ast_load must be exited. If it is not exited, the next time around the loop, __ast_load will allocate a new "struct ast_config" and store the pointer, continue loading files, then return that pointer to its caller, which will then throw it away (since none of __ast_load's callers actually use the return value for anything other than an error indicator). I added that line after fighting with very strange crashes and core dumps; I suspect it doesn't happen that often in the "real world", but I bet if you make a configuration file that is included and contains an incomplete context marker (no closing bracket), you'll be able to see the problem. So, someone needs to decide how fatal parsing errors are supposed to be: in my mind, a parsing error means that everything after that is unreliable, and I'd rather have the reload/restart fail, intead of Asterisk attempting to continue on with unreliable configuration data. If, however, we want parsing errors to be non-fatal, that can be arranged, but it will require some major reworking of config.c to make it work right. By: Mark Spencer (markster) 2004-12-11 17:53:52.000-0600 Sorry left off the comment... I reverted the patch.  My reading of the code is that if it can't open the file it should return whatever was passed to it. By: Mark Spencer (markster) 2004-12-11 17:54:18.000-0600 Anyway I think that parsing errors being fatal are fine so long as errors related to opening files are not themselves fatal. By: Kevin P. Fleming (kpfleming) 2004-12-11 17:55:47.000-0600 I agree; file open failures (for any reason) are just skipped, with the appropriate message sent to the console/log. All other failures are more serious and cause processing of that configuration file to stop (which would usually result in the module that asked for it failing to load). Thanks for the quick response Mark :-) By: Russell Bryant (russell) 2004-12-11 18:39:19.000-0600 This might set a record for number of times reopened.  :) By: Digium Subversion (svnbot) 2008-01-15 15:16:17.000-0600 Repository: asterisk Revision: 4426 U   trunk/config.c ------------------------------------------------------------------------ r4426 | markster | 2008-01-15 15:16:17 -0600 (Tue, 15 Jan 2008) | 2 lines Add support for globbing (bug ASTERISK-2776) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4426 By: Digium Subversion (svnbot) 2008-01-15 15:16:19.000-0600 Repository: asterisk Revision: 4428 U   trunk/config.c ------------------------------------------------------------------------ r4428 | markster | 2008-01-15 15:16:18 -0600 (Tue, 15 Jan 2008) | 2 lines Restore ignoring missing include files (bug ASTERISK-2776) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4428 By: Digium Subversion (svnbot) 2008-01-15 15:16:21.000-0600 Repository: asterisk Revision: 4430 U   trunk/config.c ------------------------------------------------------------------------ r4430 | markster | 2008-01-15 15:16:20 -0600 (Tue, 15 Jan 2008) | 2 lines Revert earlier patch (bug ASTERISK-2776) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4430