Summary:	ASTERISK-03962: [patch] Add support for using labels in SET PRIORITY command
Reporter:	Matthew Nicholson (mnicholson)	Labels:
Date Opened:	2005-04-20 19:12:50	Date Closed:	2008-01-15 15:35:28.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) agi_set_priority_label.diff.txt ( 1) agi_set_priority_label.final.diff.txt
Description:	This patch adds support for using labels in the SET PRIORITY command, in addition to numbers. ****** ADDITIONAL INFORMATION ****** Disclaimer on file.  Patch attached. WARNING: This patch does compile, but I have not had a chance to test it...  Also this patch does not apply to STABLE as I do not recall stable having support for labels.
Comments:	By: Clod Patry (junky) 2005-04-20 20:41:47 why it's %i and not %d ? i expect some problem with that (see ASTERISK-3794). By: Kevin P. Fleming (kpfleming) 2005-04-21 00:11:47 I agree, usage of "%i" should be highly discouraged unless there is a good reason for it. Otherwise this looks fine; can you test it out and make sure it works as expected? Thanks. By: Matthew Nicholson (mnicholson) 2005-04-21 09:45:35 I will let you guys know once I have tested this.  There are several other places where %i is used in pbx.c (in the Goto application) and in res_agi.c (sevral of the date and time functions).  Just wondering if those are problematic as well? By: Clod Patry (junky) 2005-04-21 11:29:12 See ASTERISK-3794, you'll understand a direct impact of %i vs %d in res_agi.c By: Kevin P. Fleming (kpfleming) 2005-04-21 11:30:59 Yes, if you want to make a clean-up patch to convert %i to %d throughout the source base and post that in a separate bug that would be most welcome. Basically, we should _never_ use %i except in cases where we intentionally want to support non-base-10 numbers, and I don't know of any place in Asterisk where that is the case right now. By: James Golovich (jamesgolovich) 2005-04-21 14:46:02 Another thing I noticed with this patch is the if nesting in it + if (sscanf(argv[2], "%i", &pri) != 1) + if ((pri = ast_findlabel_extension(chan, chan->context, chan->exten, argv[2], chan->cid.cid_num)) < 1) + return RESULT_SHOWUSAGE; The first if should use braces By: Matthew Nicholson (mnicholson) 2005-04-21 18:10:37 The first "if" does in fact, not need braces.  Write some test code.  If will apply to the statement directly after it so this will work fine. Note:  I still have not tested this... but I did test the if. By: Kevin P. Fleming (kpfleming) 2005-04-21 20:46:17 You are correct, the C standard allows for chained if statements this way, but the Asterisk coding guidelines document strongly discourages this usage. It's too easy for people to misinterpret, and it's too easy to add new lines of code thinking they will fall inside the if-block when they don't. By: Clod Patry (junky) 2005-04-21 22:05:08 then why not just use the braces and it would be fine for everyone no? about switching the %i for %d, i can do it for res_agi.c (where there's a lot of changes to make), i'll submit a separate bug related to this soon. By: James Golovich (jamesgolovich) 2005-04-22 13:19:12 Absolutely the code will work, but it doesn't meet the asterisk coding guidelines (check the file in the doc/ dir). By: Matthew Nicholson (mnicholson) 2005-04-22 13:49:27 :) For a sec there I thought you had forgotten how to write C...  Once I test this I will submit an updated patch. By: Kevin P. Fleming (kpfleming) 2005-04-29 12:55:09 Any update here? By: Matthew Nicholson (mnicholson) 2005-04-29 14:24:28 Not yet.  Sorry... I am working on it. By: Matthew Nicholson (mnicholson) 2005-05-18 22:30:12 Ok here is a tested working version aganst the latest cvs head.  Sorry it took so long. By: Kevin P. Fleming (kpfleming) 2005-05-18 22:52:51 Committed to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:35:28.000-0600 Repository: asterisk Revision: 5727 U   trunk/res/res_agi.c ------------------------------------------------------------------------ r5727 | kpfleming | 2008-01-15 15:35:28 -0600 (Tue, 15 Jan 2008) | 2 lines support labels as targets of SET PRIORITY command (bug ASTERISK-3962) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5727