Summary:	ASTERISK-10398: [patch]Empty vars in AEL2 switches lead to dropped calls
Reporter:	Chris Tracy (adiemus)	Labels:
Date Opened:	2007-09-27 17:59:14	Date Closed:	2007-10-03 09:18:45
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	PBX/pbx_ael
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) aelnullswitch.patch
Description:	Occasionally my telco doesn't signal CID data properly, leaving me empty vars for CALLERID(number) and CALLERID(name).  In my dialplan, I have the following structure: switch(${CALLERID(number)}) {    case 1234567890:       NoOp(Call from 1234567890);       break;    default:       NoOp(No special processing); } In the case of a blank CALLERID(number) I'd expect the default path to run.  However, this AEL gets translated to: exten => s,16,Goto(sw-12-${CALLERID(number)}|10) exten => s,17,NoOp(Finish switch-pstn-inbound-12) exten => _sw-12-.,10,NoOp(No special processing); exten => _sw-12-.,11,Goto(s|17) exten => sw-12-1234567890,10,NoOp(Call from 1234567890) exten => sw-12-1234567890,11,Goto(s|17) So when asterisk processes it, I end up with:    -- Executing [s@pstn-inbound:12] Goto("Zap/4-1", "sw-12-|10") in new stack    -- Goto (pstn-inbound,sw-12-,10) [Sep 27 17:58:13] WARNING[32414]: pbx.c:2450 __ast_pbx_run: Channel 'Zap/4-1' sent into invalid extension 'sw-12-' in context 'pstn-inbound', but no invalid handler    -- Hungup 'Zap/4-1' Not quite sure where the "bug" is here since obviously "sw-12-" won't be matched by "_sw-12-."  Maybe this is the desired behavior and I'm wrong. For now I'm just checking if CALLERID(number) is empty first, and populating it with "0000000000" before the switch() which works around the problem.  -Chris
Comments:	By: Steve Murphy (murf) 2007-10-02 11:20:12 Well, my first reaction was to modify the code so, for switch cases, it puts a '1' (or any other equally suited character) in front of every case (except the default), and a '1' at the front of the label in the Goto. This solved the problem, and I tested it and it works fine for the empty case, but it's not transparent. The 1 will show up in ${EXTEN} and that's not good. So, the only other thing I can think of, is to make a special 'empty' case, and have that jump to the default case.That would be slightly more transparent... I think.... By: Steve Murphy (murf) 2007-10-02 22:16:44 OK, I've attached a patch that will add one more extension to the generated code: an extension for just the empty-string case. All it has is one priority to jump to the default extension. All the regression tests are also updated after making sure no side problems have been created. Give it a try and let me know if solves your problem. Otherwise, based on my own testing, I will commit this to 1.4. (and trunk). By: Dmitry Andrianov (dimas) 2007-10-03 03:59:10 My first thougt was that this approach will break cases where user explicitly handles null values with case "": but then I discovered there is no way to specify case with empty string because empty quotes are just literally copied to label :) Hovewer, your special null branch to me looks like less transparent then your original idea of adding extra character to labesl. Amd of empty-string cases will ever be supported, your patch will make them non-working anyway. Isn't it possible to just add another dash to label names so labels will look like sw-15--1234567 where the default one will be _sw-15-. and GoTo will be going to sw-%d--%s ? I'm not quite sure how this extra dash shows up in ${EXTEN} and if it does, why is it so bad. By: Steve Murphy (murf) 2007-10-03 07:54:03 You are correct about not being specify an empty case. It'd have to be done grammatically, like with a new keyword or somesuch; and right now, it's not worth it. I think you have it correct; it should end up in the default, and in that case, you can detect and handle it. If I ever do handle null cases grammatically, I'd just remove the generation of the null pattern; actually, I'd just modify it slightly. No sweat. Whether I use a - or a 1 or whatever, ${EXTEN} will end up being "-" or "1" or whatever, which would be an unpleasant surprise for those expecting to see nothing. And for everyone else, everywhere else, they would see the extra - or 1 or whatever either at the beginning or end of the string they expected. It would possibly break existing implementations. Not good! True, not everyone uses ${EXTEN} in switch cases, or even know they can use it. But if I don't muck with it, I don't have to document it. By: Steve Murphy (murf) 2007-10-03 08:10:34 Hmmm. After thinking more about this, ${EXTEN} in switch cases is probably something that needs to be explained in the wiki and documentation ANYWAY. So, either way would work. And if we do figure out someday that a separate "nullcase" is desirable, then the structure of the generated code won't change much from what I have now. I think I'll commit this; if you have serious problems with this, or this introduces bugs, re-open this or file a new issue... By: Digium Subversion (svnbot) 2007-10-03 09:03:21 Repository: asterisk Revision: 84511 U   branches/1.4/pbx/ael/ael-test/ref.ael-ntest10 U   branches/1.4/pbx/ael/ael-test/ref.ael-test1 U   branches/1.4/pbx/ael/ael-test/ref.ael-test18 U   branches/1.4/pbx/ael/ael-test/ref.ael-test3 U   branches/1.4/pbx/ael/ael-test/ref.ael-test5 U   branches/1.4/pbx/ael/ael-test/ref.ael-test8 U   branches/1.4/pbx/ael/ael-test/ref.ael-vtest13 U   branches/1.4/pbx/ael/ael-test/ref.ael-vtest17 U   branches/1.4/pbx/pbx_ael.c ------------------------------------------------------------------------ r84511 | murf | 2007-10-03 09:03:20 -0500 (Wed, 03 Oct 2007) | 1 line closes issue ASTERISK-10398 ; where a null input to a switch statement results in a hangup; since switch is implemented with extensions, and the default case is implemented with a '.', and the '.' matches 1 or more remaining characters, the case where 0 characters exist isn't matched, and the extension isn't matched, and the goto fails, and a hangup occurs. Now, when a default case is generated, it also generates a single fixed extension that will match a null input. That extension just does a goto to the default extension for that switch. I played with an alternate solution, where I just tack an extra char onto all the patterns and the goto, but not the default case's pattern. Then even a null input will still have at least one char in it. But it made me nervous, having that extra char in , even if that's a pretty secret and low-level issue. ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-10-03 09:18:45 Repository: asterisk Revision: 84512 _U  trunk/ U   trunk/pbx/ael/ael-test/ref.ael-ntest10 U   trunk/pbx/ael/ael-test/ref.ael-test1 U   trunk/pbx/ael/ael-test/ref.ael-test18 U   trunk/pbx/ael/ael-test/ref.ael-test19 U   trunk/pbx/ael/ael-test/ref.ael-test3 U   trunk/pbx/ael/ael-test/ref.ael-test5 U   trunk/pbx/ael/ael-test/ref.ael-test8 U   trunk/pbx/ael/ael-test/ref.ael-vtest13 U   trunk/pbx/ael/ael-test/ref.ael-vtest17 U   trunk/res/ael/pval.c ------------------------------------------------------------------------ r84512 | murf | 2007-10-03 09:18:44 -0500 (Wed, 03 Oct 2007) | 9 lines Merged revisions 84511 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r84511 | murf | 2007-10-03 08:23:00 -0600 (Wed, 03 Oct 2007) | 1 line closes issue ASTERISK-10398 ; where a null input to a switch statement results in a hangup; since switch is implemented with extensions, and the default case is implemented with a '.', and the '.' matches 1 or more remaining characters, the case where 0 characters exist isn't matched, and the extension isn't matched, and the goto fails, and a hangup occurs. Now, when a default case is generated, it also generates a single fixed extension that will match a null input. That extension just does a goto to the default extension for that switch. I played with an alternate solution, where I just tack an extra char onto all the patterns and the goto, but not the default case's pattern. Then even a null input will still have at least one char in it. But it made me nervous, having that extra char in , even if that's a pretty secret and low-level issue. ........ ------------------------------------------------------------------------