Summary:	ASTERISK-05705: [patch] constification of ast_var_name(), ast_var_value(), pbx_builtin_getvar_helper() and friends.
Reporter:	Luigi Rizzo (rizzo)	Labels:
Date Opened:	2005-11-25 11:17:35.000-0600	Date Closed:	2005-12-03 13:26:30.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) patch.const ( 1) patch-aa ( 2) patch-constify
Description:	In continuing my attempt to move towards safer and more readable code, i have added a const qualifier to the return of ast_var_value(), ast_var_name(), ast_var_full_name(), pbx_builtin_getvar_helper() and a small number of additional functions which return a pointer to their private storage hence would not like others to overwrite it. In the process i discovered a true bug in    res_features.c::check_goto_on_transfer() (see the relevant part in the attached patch) where the function would actually modify a channel variable, and cleaned up the remaining code quite a bit. I managed to get a clean build with -Wall -Wer ror on my system. I am attacching a mostly complete patch, just omitting a few files (app_dial.c, channel.[ch], say.c and maybe a few more) for which i have a number of changes already in mantis waiting for commit, and for which it would be to difficult to separate one from the other. In any case the patch below should cover only miss a very small number of instances that need to be fixed. I will follow up with more fixes as things get committed. ****** ADDITIONAL INFORMATION ****** It is perfectly safe to apply the chunks of the attached patch separately - by putting "const" here and there you don't make the code any buggier than it was before, you only tell the compiler more about variable usage, and enable it to generate stricter warnings. As usual in these cases, if you start constifying things, you have a domino effect which might increase the number of warnings until you have cleaned up all the code. But there is nothing new to worry about.
Comments:	By: Tilghman Lesher (tilghman) 2005-11-28 13:09:56.000-0600 There is way too much intermixing of other patches in that patchset.  We either need to suspend this bug until all other dependant patches are merged or rejected or else you need to resubmit a patch against a clean HEAD, which only includes the changes suggested in this bug. By: Luigi Rizzo (rizzo) 2005-11-29 01:36:22.000-0600 try patch-aa, i have removed the app_voicemail and app_sayunixtime changes, this one applies and compiles cleanly to a fresh head. if you have concerns with specific parts of the patch please let me know which ones. consider that the res_features.c chunk fixes a true bug. i can file a separate bug report for that if considered useful. By: Tilghman Lesher (tilghman) 2005-11-29 10:17:42.000-0600 Unrelated changes: 1.  int casting in app_chanspy.c 2.  safe_strdup changes in app_macro.c 3.  int casting in app_queue.c 4.  all changes in app_sms.c 5.  all changes in asterisk.c 6.  sip_addheader changes in chan_sip.c 7.  removal of initialization code in chan_zap.c 8.  all changes in func_strings.c 9.  pbx_extension_helper changes in pbx.c 10.  int casting in res_monitor.c Note that I'm not debating the value of these changes, only that they are probably in some other patchset and do not need to be in this one. By: Luigi Rizzo (rizzo) 2005-11-29 11:12:05.000-0600 i think we should try to be pragmatic. yes there are changes that are possibly unrelated, they just happen to be in the same file or at times in the same chunk as others, or harmless. yes the patch touches 32 different files. as i said, it happens when you const-ify things. It would take a huge amount of time for the reviewer and the committer to hand these things as 10-20 separate patchfiles (especially not having control on the order in which things get committed if at all). This is why i am sending the patch as a comprehensive one. If you feel a change is inappropriate, i am all ears. If you feel a change is appropriate, i think it would be in the interest of the project to just commit it rather than debate on issues of principle, especially since you have already spent the time to look at it in such a fine detail - note, i am not asking for you or anyone to trust my changes, but just to trust your judgment on the appropriateness of a change. Let me just comment on what you consider an unrelated change, "7. removal of initialization code from chan_zap.c". The relevant chunk is at the end of the comment. The initialization was removed, yes. It was unnecessary because the vars are initialized on their first use, as shown compiling with -Werror. Now would you want to submit two patches, one moving to const -     char *cic = NULL, *ozz = NULL; +     const char *cic = NULL, *ozz = NULL; and the next one removing the initializers ? -      const char *cic = NULL, *ozz = NULL; +      const char *cic, *ozz; Would it help anyone to do so ? --- chan_zap.c  Fri Nov 11 18:00 :07 2005 +++ chan_zap.c   Fri Nov 25 17:31:30 2005 @@ -1932,7 +1932,7 @@ static int zt_call(struct ast_channel *a                        break;                case SIG_FEATDMF_TA:                { -                       char *cic = NULL, *ozz = NULL; +                       const char *cic, *ozz;                        /* If you have to go through a Tandem Access point you n eed to use this */                        ozz = pbx_builtin_getvar_helper(p->owner, "FEATDMF_OZZ") ; By: Tilghman Lesher (tilghman) 2005-11-29 11:55:34.000-0600 Please understand that when I look through these changes, I'm doing so as a bug marshal.  I am trying to help make patches 'clean' enough for a committer to commit them without a whole lot of trouble, and intermixing different patch sets is one of those things that a committer looks at and decides "this patch isn't clean enough; I'm going to postpone committing it until I have time to check every case." The criticism is meant to be constructive; it's to ensure that a patch goes in quickly, so you, as the reporter, don't have to maintain it for long periods of time before it is committed. That said, if you're willing to keep this patchset maintained until a committer can go through the entire patch, then you need do nothing.  If, on the other hand, you'd like the patch to go in quickly, I think you see what needs to be done. By: Luigi Rizzo (rizzo) 2005-11-29 12:19:15.000-0600 ok, understood - i thought you had commit privs, that's why i did not understand the approach. all clear now. By: Tilghman Lesher (tilghman) 2005-11-30 23:55:13.000-0600 You can now re-evaluate my comments in the context that I now have commit privileges.  I agree with your changes to chan_zap.c, but all of the other criticisms stand, for the time being. By: Luigi Rizzo (rizzo) 2005-12-03 08:59:21.000-0600 patch updated to today's sources and your comments. By: Tilghman Lesher (tilghman) 2005-12-03 13:26:13.000-0600 Fixed in the trunk.