Summary:	ASTERISK-10140: res_features CLI revamping and removal of default builtins
Reporter:	Caio Begotti (caio1982)	Labels:
Date Opened:	2007-08-22 07:36:56	Date Closed:	2007-10-29 15:42:43
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Resources/res_features
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) cli_default.txt ( 1) cli_new.txt ( 2) cli_old.txt ( 3) res_features_CLI_6.diff ( 4) res_features_CLI_7.diff
Description:	One problem already talked in previous reports about res_features (regarding park-dial context) is that wasn't possible to avoid default builtins for transfers and some parking options, even if you erase your features.conf. There were hardcoded stuff in res_features.c I changed it so it's now possible to have a clean installation where nobody will ever use those default builtins to possible bypass the PBX restrictions (my own case). Also, I've changed the way information are displayed in the CLI so now it has a bunch of new parameters shown. Random thoughts: 1. Having default builtins are wrong because the system may break its own PBX restrictions 2. And even putting blank options in features.conf to null the builtins is not very smart, IMHO. It's better have a way to configure them, I mean, at least the major parameters. 3. Some key information such as timeouts were not getting shown. 4. Also, I have changed the default value for DEFAULT_FEATURE_DIGIT_TIMEOUT from 500ms to 1000ms so it waits enough. It was really a short ammount of time to press something in a softphone keypad, for example. 5. I got rid of the middle column 'Default' in CLI's features show. ****** ADDITIONAL INFORMATION ****** These three cli_.txt files shows samples from how it was and how it's now with the attached patch. The .diff patch also updates the features.conf.sample file in configs/ to reflect the changes in 'defaults'.
Comments:	By: Caio Begotti (caio1982) 2007-08-22 13:11:50 Already replying a possible comment (made by Clod Patry, thanks!) is that changing default values might be a bad idea but I believe it's a even worse idea to keep default values builtin for actions like, for example, (both methods of) transfers and call disconnect. I just thought about it as a PBX admin would not want people pressing random keys and getting not allowed resposes from Asterisk "by default" :-) By: Michiel van Baak (mvanbaak) 2007-09-04 16:39:27 removing the defaults is always dangerous because people depend on it. Our production systems use the defaults as well and our customers use it. The CLI updates are nice tho. By: Paul Belanger (pabelanger) 2007-09-04 17:25:06 Not that it matters, but this patch gets a +1 from me.  I don't use call parking in my configuration of asterisk, yet there is hardcoded values.  Glad to see a possible solution to removing them. By: Caio Begotti (caio1982) 2007-09-04 21:14:15 Mvanbaak, I'm pretty sure that if this patch is ever accepted into trunk (not 1.4 as you might have thought) some bug marshall will add a few lines to UPGRADE.txt stating the changes and why they were proposed, as written above in the bug description :-) By: Caio Begotti (caio1982) 2007-09-06 09:32:21 Another issue just discovered related to this one: given that * and # are hardcoded values in features.conf right now, what would happen if someone enable the new func_volume.so which has also hardcoded * and # to increase and decrease the channel gain at the same time? According to file it's not certain what would happen, but maybe func_volume.so would get it first, disabling features.conf hardcoded values. By: Caio Begotti (caio1982) 2007-09-06 14:35:53 res_features_CLI_7.diff adds a small UPGRADE.txt paragraph stating the affected stuff with these changes and a check for the parking context and extension before registering empty a context/priority as before. I hope it's all okay... By: Michiel van Baak (mvanbaak) 2007-09-06 15:00:20 parkedcallstimeout is still the default? If not, you should remove it from the ast_log call. By: Caio Begotti (caio1982) 2007-09-06 15:08:05 parkedcallstimeout is related to a minor typo (actually case change) in a sentence already used in the original code, I did not remove any timeout value because I thought it would be too dangerous for someone (inexperienced) like me to do. By: Caio Begotti (caio1982) 2007-10-16 15:08:52 Any core developer around to comment on these changes for trunk? By: Russell Bryant (russell) 2007-10-16 17:48:15 First off, thank you very much for the patch.  :)  Here is my opinion on the changes: 1) I do not like the idea of removing the defaults.  It will break many systems, and I don't feel it really provides any benefit.  There really aren't any access control issues because:  - For transfers, you have to explicitly allow them using the tT options to Dial(), so there is no way that it is going to just magically work "by default".  - For call parking, you have to configure the dialplan in such a way that parking is allowed, so again, it's not possible by any defaults. 2) I do like the CLI updates to show additional information. 3) I also agree that 500ms is too short.  1000ms is more reasonable. By: Russell Bryant (russell) 2007-10-29 15:42:42 I'm going to close this out since I have decided not to make the changes suggested here.  However, a patch to improve the CLI output would be welcome.