Summary:	ASTERISK-03333: [PATCH] redesign config system interaction with res_config modules
Reporter:	Kevin P. Fleming (kpfleming)	Labels:
Date Opened:	2005-01-22 16:38:43.000-0600	Date Closed:	2008-01-15 15:23:11.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/Configuration
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) config_api_rework_rev3.diff.txt ( 1) config_mysql_fix_rev1.diff.txt ( 2) config_odbc_fix_rev2.diff.txt
Description:	The changes in ASTERISK-3203237 changed the config system and broke res_config_odbc (and probably all out-of-tree res_config modules). One of the changes made it difficult for res_config modules to add variables into a config, because they could not take advantage of a new function to do that (it was private to config.c). The attached patch is the result of conversation on IRC with Mark and Tony, and it re-builds the config system to simplify the API and make res_config modules somewhat simpler to build/maintain. There are major changes here: - config_pvt.h is gone, all private definitions moved to config.c - the custom config engine objects and functions have been renamed to better reflect what they actually do - some very widely used functions got renamed so that their names are more logical (ast_destroy -> ast_config_destroy, ast_destroy_realtime -> ast_variables_destroy) I have built and run this on my test box, and compiled against unixODBC-2.2.10 (but have no databases to use to actually test it). ****** ADDITIONAL INFORMATION ****** Disclaimer is on file.
Comments:	By: Anthony Minessale (anthm) 2005-01-23 09:52:37.000-0600 I have an issue when you change the names of long standing functions. Do you think you could leave the old ones in as legacy back compat and generate a depreciation warninig or something? You may not understand, but I have a great many out of tree  projects that are all going to be rendered useless by these changes and force me to spend very much time repairing them and it will be much harder to convert them all at once than it would be to slowly migrate them.  I dont recall making any agreement to rename the config READING functions.... I don't mind the change *if* there is backwards compatability if you are eliminating config_pvt.h that would be a good place to put the old protos and maybe a legacy_config.c that would be temporarily linked in for the old names. By: Kevin P. Fleming (kpfleming) 2005-01-23 10:08:05.000-0600 You are correct, I did not bring up renaming the config reading functions on IRC yesterday. However, this patch only renames two functions that are used by config readers, so I don't see that as a huge burden. This is, after all, the "development" branch of Asterisk, and if we aren't allowed to change it to make it better (because we have to maintain backwards compatibility) then development will certainly accomplish less than it could. I too maintain a number of out-of-tree patches, and I understand that since they are based on CVS HEAD that at any time I could have work on my hands to bring them up to date. The same issues occurred when the CID stuff was worked over a few months ago; all out of tree patches that dealt with channels and CID had to be updated. I'll let Mark make the decision on whether "long standing" is an adequate reason to keep function names that don't really describe what they do (or are too ambiguous, as is the case with ast_destroy). ast_destroy_realtime is not "long standing", it only exists in CVS HEAD, and it's already being used for purposes that have nothing to do with Realtime, so renaming it seems perfectly sensible to me. The "config engine" register functions are also new in HEAD, and are not "long standing". If any of this code had been released as "stable", then I would not have proposed such a radical change. I guess I won't bother to post my next rev of this patch, that actually cleans up all the config users and makes the ast_category_browse/ast_variable_browse APIs consistent and more efficient, because it's even more invasive. The results are very beneficial, which is usually worth some transition pain, but maybe for this project that is not true. By: Mark Spencer (markster) 2005-01-23 10:42:04.000-0600 I've discussed with kpfleming some methods for achieving the desired performance benefits (and even more actually) without breaking the API at all (binary and source). By: Anthony Minessale (anthm) 2005-01-23 10:47:55.000-0600 sigh, you mock my comments, why? you really need to lose the attitude, the first time we met on the moh bug you were the same way and I thought you agreed to chill? We all know what is wrong with the code you don't need to take that position.  Even when I say "Ok but..." you insist on this my way or the highway reply.  Go ahead and take the last word, I know you can't resist but unless it mentions how you plan to try and work together better, I give up. By: Kevin P. Fleming (kpfleming) 2005-01-23 17:07:40.000-0600 rev3 uploaded, changes are: - ast_load -> ast_config_load - ast_internal_load -> ast_config_internal_load - config CLI command has been renamed to "show config mappings" from "show config handles" - config API has been updated to use const arguments wherever possible - config API doxygen docs updated and corrected where needed - process_text_line no longer destroys the in-process "struct ast_config" when it finds a major problem - ast_config_load now destroys the config it had created if any ast_config_internal_load call returns failure - ast_category_browse now caches the last category returned, and can quickly get the "next" category when used in a normal looping mode (ast_variable_browse also uses this cached value when appropriate) All old APIs are still available, in config_old.h and config_old.c (automatically built and included). Use of any of them will generate a deprecation warning (only once for each function). Behavior changes to be aware of for res_config modules: - the config_load function you supply will _always_ be given a "struct ast_config *" to work with, and you must never call ast_config_destroy on it (even if you come to some major failure point and cannot continue)... leave that to the top-level ast_config_load to do after you exit with a non-zero result - all config_load functions should call ast_config_internal_load to handle "#include", rather than calling directly back to themselves, and if ast_config_internal_load returns a non-zero result, they should exit completely (stop processing) res_config_odbc.c has been updated as best I can tell it needs; it compiles here cleanly. Once this is in I will update res_config_mysql with these changes as well. By: Kevin P. Fleming (kpfleming) 2005-01-23 17:50:08.000-0600 Let me clarify that last note... all old "READING" APIs are still available. The APIs to register/deregister config engines have changed names (but not prototypes), and some of the APIs for inserting entries into a config have changed names for consistency's sake. By: Anthony Minessale (anthm) 2005-01-24 09:20:10.000-0600 Thank you!  This helps very much.  I have always been on board with this change, I was only concerned with the backlash such a broad stroke would have caused at this juncture.  Thank you for taking the time to work on this and see it through.  I'm sure it will open doors to even cooler config stuff down the road. I think in general we should really strive to always follow this approach if we are forced to alter core code. Thanks again. By: Kevin P. Fleming (kpfleming) 2005-01-24 09:52:23.000-0600 You are welcome... My first version was definitely a rush job, and didn't really take into account the compatibility aspects. With that said, though, we do need to have a plan in place to be able to make non-compatible changes at some point, otherwise we are going to be stuck with current design decisions we may later want to redo. I don't know if that means waiting for a new "major release" or what, but someone (or all of us) will have to get together and come up with a plan. It would also help if Asterisk was "released" more frequently, so that we had more opportunity for changes and people in the community didn't have to be making patches against CVS HEAD just to add their own features, but that's a far more major change to make :-) By: Mark Spencer (markster) 2005-01-25 00:08:18.000-0600 Okay, added to CVS, but we need res_config_mysql updated! By: Digium Subversion (svnbot) 2008-01-15 15:23:10.000-0600 Repository: asterisk Revision: 4889 U   trunk/.cleancount U   trunk/Makefile U   trunk/apps/app_alarmreceiver.c U   trunk/apps/app_directory.c U   trunk/apps/app_enumlookup.c U   trunk/apps/app_festival.c U   trunk/apps/app_meetme.c U   trunk/apps/app_privacy.c U   trunk/apps/app_queue.c U   trunk/apps/app_realtime.c U   trunk/apps/app_rpt.c U   trunk/apps/app_txtcidname.c U   trunk/apps/app_voicemail.c U   trunk/asterisk.c U   trunk/cdr/cdr_manager.c U   trunk/cdr/cdr_odbc.c U   trunk/cdr/cdr_pgsql.c U   trunk/cdr/cdr_tds.c U   trunk/channels/chan_agent.c U   trunk/channels/chan_alsa.c U   trunk/channels/chan_h323.c U   trunk/channels/chan_iax2.c U   trunk/channels/chan_mgcp.c U   trunk/channels/chan_modem.c U   trunk/channels/chan_oss.c U   trunk/channels/chan_phone.c U   trunk/channels/chan_sip.c U   trunk/channels/chan_skinny.c U   trunk/channels/chan_vpb.c U   trunk/channels/chan_zap.c U   trunk/channels/iax2-provision.c U   trunk/codecs/codec_speex.c U   trunk/config.c U   trunk/enum.c U   trunk/include/asterisk/config.h U   trunk/loader.c U   trunk/logger.c U   trunk/manager.c U   trunk/pbx/pbx_config.c U   trunk/pbx/pbx_dundi.c U   trunk/pbx/pbx_realtime.c U   trunk/res/res_adsi.c U   trunk/res/res_config_odbc.c U   trunk/res/res_features.c U   trunk/res/res_indications.c U   trunk/res/res_musiconhold.c U   trunk/res/res_odbc.c U   trunk/res/res_osp.c U   trunk/rtp.c ------------------------------------------------------------------------ r4889 | markster | 2008-01-15 15:23:09 -0600 (Tue, 15 Jan 2008) | 2 lines Merge config updates (bug ASTERISK-3333) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4889 By: Digium Subversion (svnbot) 2008-01-15 15:23:11.000-0600 Repository: asterisk Revision: 4890 A   trunk/config_old.c A   trunk/include/asterisk/config_old.h ------------------------------------------------------------------------ r4890 | markster | 2008-01-15 15:23:10 -0600 (Tue, 15 Jan 2008) | 2 lines Add old config files (bug ASTERISK-3333) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4890