[Home]

Summary:ASTERISK-14196: [patch] ReadExten returns TIMEOUT in cases where it should return OK or INVALID
Reporter:John Covert (jcovert)Labels:
Date Opened:2009-05-23 14:07:00Date Closed:2010-08-11 10:22:48
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_readexten
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20100811__issue15188.diff.txt
( 1) app_readexten.c.patch
( 2) app_readexten.c.patch-1.6.2.6
( 3) app_readexten.c.patch-1.6.2.8-rc1
Description:The documentation for ReadExten states:

TIMEOUT   No extension was entered in the specified time

but ReadExten is returning TIMEOUT also in the case of a partial (INVALID) extension or a completed (OK) variable length extension.

The behaviour of asterisk dialplan processing has always been that "timeout" (i.e. continuation at the "t" extension) would only occur if "no extension" (i.e. nothing, not any digits, not even an invalid extension) was entered.  A "TIMEOUT" was always nothing entered in the Response Timeout. Once a single digit was entered, and processing switched to the Digit Timeout, the result could only be "INVALID" (continuation at the "i" extension) or "OK" continuation at an actual extension.

ReadExten is not doing this correctly.  Consider use of ReadExten specifying a context containing ONLY the following two extensions:

exten => 12,1,Noop()

exten => 345.,1,Noop()

The expected asterisk behaviour is that (1) entering just a "1" and waiting for a timeout would end up at the "i" extension.  Similarly, (2) entering 345 would also end up at the "i" extension, but (3) entering "3456" would be processed.

ReadExten returns "TIMEOUT" in all three cases, and the digits entered are lost.

The attached patch corrects this behaviour.  Cases (1) and (2) will return "INVALID" with the digits dialled returned in ${INVALID_EXTEN} and case (3) will return "OK" with the result in the return argument variable.

The patch also includes my suggestion concerning allowing tonelists, issue ASTERISK-1500185.

It also allows a "#" as the FIRST character of an extension (if the dialplan contains such an extension, frequently used for #xx feature codes).  Entry of a "#" when the dialplan does not contain such an extension is processed as an INVALID (with the "#" returned in INVALID_EXTEN).

Regards/john
Comments:By: John Covert (jcovert) 2010-05-06 14:06:29

I am now running 1.6.2.6 and have tested the work which was checked in.

It fixes the error associated with TIMEOUT/OK/INVALID.

However, the feature change -- to make the indication the same as the indication argument to Playtones (indication or tonelist) didn't happen.

I'm submitting another patch, relative to 1.2.6.2, which both simplifies the code and implements the feature.  It makes sense, if an indication is allowed, that it is the SAME interface to playing an indication that the user has in "Playtones."

I remember there being a comment made earlier about a potential conflict between a filename and a numeric tonelist.  Such a conflict really doesn't exist.  A filename (audio file) is the default, and an indication (either a named indication or a tonelist) is only applicable with the "i" option, in which case there are no filenames, only indications from indications.conf and tonelists.  The same considerations exist as for Playtones -- in fact, we use the "Playtones" call in order to preserve better modularity and consistency, rather than "rolling more of our own" in another place.

Please consider the feature request: it's very useful -- it avoids having to edit indications.conf to add various tones used for special ReadExten usage.

/john
By: John Covert (jcovert) 2010-05-22 08:53:56

After further testing, I find that I was wrong that the TIMEOUT error had been fixed (I thought I saw that the right code had been committed, but maybe there was a regression since then).

Given "exten => 12,1,..." and "exten => 123,1,..." it DOES return timeout instead of the "12" extension.  To properly implement TIMEOUT the test for x == 0 before setting the variable is required.  I've uploaded another version of the patch.  This is against 1.6.2.8-rc1.

As long as I'm adding a note for the bug fix above, I'm also lobbying again for the feature change, noting that it removes 11 lines of code (and a variable) and inserts only 2.  It also makes the argument containing the indication or tonelist consistent with PlayTones, the only other application which allows an indication to be specified in the dialplan.

Less code and more consistency.

/john
By: John Covert (jcovert) 2010-06-04 12:41:52

Tickle.  This patch applies against 1.6.2.8.
By: John Covert (jcovert) 2010-06-30 08:43:06

Tickle.
By: Leif Madsen (lmadsen) 2010-07-05 08:42:23

I've now imported this into our internal tracking system so that it can get evaluated and assigned to a developer at some point in the future. Thanks!
By: John Covert (jcovert) 2010-07-05 12:55:29

Note that there is both a BUG and a FEATURE in this patch.

The bugfix makes TIMEOUT work correctly as documented.

The FEATURE makes the file / tonelist argument take a file if no "-i" is given, but behave exactly like Playtones if "-i" is given, and saves a bit of unecessary code duplication in the process.
By: Leif Madsen (lmadsen) 2010-07-06 11:15:47

Perhaps the bug and the feature should be broken out into separate features so this can be better tested and merged (features only in trunk)
By: John Covert (jcovert) 2010-07-07 02:17:39

Based on the checkin comments in issue 001585 it looks like there was an attempt to check in the feature before:

-------
r234776 | qwell | 2009-12-14 15:32:03 -0600 (Mon, 14 Dec 2009) | 12 lines
 
 Allow tonelist as argument to ReadExten.
 
 ReadExten already supported playing a tonezone from indications.conf.
 It now has the ability to use a tonelist like 440+480/2000|0/4000
-------

So this is all a bugfix. :-)

However, the TIMEOUT bug is the bit with the "if (x == 0) {" and "}" around existing code.

The tonelist is all of the rest.  All of the simplification of the code is to make ReadExten have "the ability to use a tonelist like 440+480/2000|0/4000".
By: Digium Subversion (svnbot) 2010-08-11 10:17:18

Repository: asterisk
Revision: 281722

U   branches/1.6.2/apps/app_readexten.c

------------------------------------------------------------------------
r281722 | tilghman | 2010-08-11 10:17:18 -0500 (Wed, 11 Aug 2010) | 7 lines

Only set status TIMEOUT, if we have no digits.

(closes issue ASTERISK-14196)
Reported by: jcovert
Patches:
      app_readexten.c.patch-1.6.2.8-rc1 uploaded by jcovert (license 551)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=281722
By: Digium Subversion (svnbot) 2010-08-11 10:18:39

Repository: asterisk
Revision: 281723

_U  branches/1.8/
U   branches/1.8/apps/app_readexten.c

------------------------------------------------------------------------
r281723 | tilghman | 2010-08-11 10:18:38 -0500 (Wed, 11 Aug 2010) | 14 lines

Merged revisions 281722 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.6.2

........
 r281722 | tilghman | 2010-08-11 10:17:20 -0500 (Wed, 11 Aug 2010) | 7 lines
 
 Only set status TIMEOUT, if we have no digits.
 
 (closes issue ASTERISK-14196)
  Reported by: jcovert
  Patches:
        app_readexten.c.patch-1.6.2.8-rc1 uploaded by jcovert (license 551)
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=281723
By: Digium Subversion (svnbot) 2010-08-11 10:20:28

Repository: asterisk
Revision: 281726

_U  trunk/
U   trunk/apps/app_readexten.c

------------------------------------------------------------------------
r281726 | tilghman | 2010-08-11 10:20:28 -0500 (Wed, 11 Aug 2010) | 21 lines

Merged revisions 281723 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

................
 r281723 | tilghman | 2010-08-11 10:18:40 -0500 (Wed, 11 Aug 2010) | 14 lines
 
 Merged revisions 281722 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.6.2
 
 ........
   r281722 | tilghman | 2010-08-11 10:17:20 -0500 (Wed, 11 Aug 2010) | 7 lines
   
   Only set status TIMEOUT, if we have no digits.
   
   (closes issue ASTERISK-14196)
    Reported by: jcovert
    Patches:
          app_readexten.c.patch-1.6.2.8-rc1 uploaded by jcovert (license 551)
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=281726
By: Tilghman Lesher (tilghman) 2010-08-11 10:22:48

Your patches do not apply cleanly to an SVN checkout, which is why only the bugfix was applied.  If you want to resubmit your feature, please do so with a file that is correctly formatted and spaced.  In particular, use Unix-style linefeeds and do NOT convert tabs to spaces (Emacs sometimes does this without you realizing it).