Summary:	ASTERISK-11933: [patch] Janitor for getvar_helper threadsafe
Reporter:	snuffy (snuffy)	Labels:
Date Opened:	2008-04-28 10:01:49	Date Closed:	2008-04-30 14:16:30
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) bug_12156_apps.diff
Description:	I've done a few patches following the code outlined in the janitor-projects.txt document. Most apps have been done aside from app_dial at the moment. All still compiled/ran after change no other testing has taken place. Note for some multiple get of vars i hold lock for the 3 or so variables then unlock.
Comments:	By: Tilghman Lesher (tilghman) 2008-04-28 10:41:13 I see a basic problem with at least one of these patches, in that you are duplicating a variable that will be immediately scanned into an integer.  Why not simply delay the unlock until after the scan? Also, you're missing a space between your control structure and your opening parenthesis for the conditional:  "if (" not "if(". By: snuffy (snuffy) 2008-04-28 16:45:24 no problem.. (oops on the 'if (') i was just following exactly what was in the janitor project. if they all need to be scrapped and redone with more 'thought' and just extend channel locks instead of strdupa'ing all the vars.. i'm listening.. By: Tilghman Lesher (tilghman) 2008-04-28 17:40:45 I wouldn't say they need to be scrapped, but where it makes sense, fix the problems, instead of pasting simplistic code.  The two main cases I see are: cases where the value is immediately scanned into another variable, like scanning an integer (as is the case in app_morsecode) or where the value is immediately copied into another buffer (as is the case in app_stack).  Neither of these requires scrapping the patch, only in re-reading the patched version and making some very simple changes, to remove unnecessary operations. The only time you really need to ast_strdupa() the variable is if the use of that variable is relatively long-term.  If the use is just very quick, then extending the time period that the channel is locked is enough. By: snuffy (snuffy) 2008-04-28 19:08:44 no problems will redo all again.. pls delete current files i uploaded By: Tilghman Lesher (tilghman) 2008-04-29 16:56:46 Deleted, as requested. Also, please upload a single file containing all of the changes for this janitor.  There is no need (and in fact, it makes it more difficult on the committer) to have multiple patch files, unless they apply to different branches (which is not applicable in this case). By: snuffy (snuffy) 2008-04-30 11:42:15 These now have the less 'cut n paste' version of the changes No app_dial / app_queue included in the changes above. Fixed the coding-guidelines issue with 'if (' By: Digium Subversion (svnbot) 2008-04-30 14:15:58 Repository: asterisk Revision: 114904 U   trunk/apps/app_chanspy.c U   trunk/apps/app_externalivr.c U   trunk/apps/app_macro.c U   trunk/apps/app_meetme.c U   trunk/apps/app_minivm.c U   trunk/apps/app_morsecode.c U   trunk/apps/app_speech_utils.c U   trunk/apps/app_stack.c U   trunk/apps/app_voicemail.c U   trunk/apps/app_while.c ------------------------------------------------------------------------ r114904 | tilghman | 2008-04-30 14:15:43 -0500 (Wed, 30 Apr 2008) | 8 lines Lock around variables retrieved, and copy the values, if they stay persistent, since another thread could remove them. (closes issue ASTERISK-11933) Reported by: snuffy Patches:       bug_12156_apps.diff uploaded by snuffy (license 35)       Several additional changes by me ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=114904 By: Digium Subversion (svnbot) 2008-04-30 14:16:30 Repository: asterisk Revision: 114905 _U  branches/1.6.0/ ------------------------------------------------------------------------ r114905 | tilghman | 2008-04-30 14:16:25 -0500 (Wed, 30 Apr 2008) | 15 lines Blocked revisions 114904 via svnmerge ........ r114904 | tilghman | 2008-04-30 14:21:04 -0500 (Wed, 30 Apr 2008) | 8 lines Lock around variables retrieved, and copy the values, if they stay persistent, since another thread could remove them. (closes issue ASTERISK-11933) Reported by: snuffy Patches:       bug_12156_apps.diff uploaded by snuffy (license 35)       Several additional changes by me ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=114905