[Home]

Summary:ASTERISK-15975: [patch] Dial()'s do_forward() breaks Local/ channel frame forwarding
Reporter:Steve Davies (one47)Labels:
Date Opened:2010-04-19 12:07:06Date Closed:2010-06-01 16:12:51
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_dial
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) app_dial_fwdfix
Description:This was spotted due to an issue that is caused with CDR records of calls redirected via a SIP 302 message. The dstchannel is recorded incorrectly.


In chan_local:local_queue_frame(), there is a test to see whether
there is a channel or app to forward data to before frames are passed
on:

              if (other->pbx || other->_bridge ||
!ast_strlen_zero(other->appl)) {
                      ast_queue_frame(other, f);
              }

For a call that is being dialled, there is no pbx or bridge object, so
chan_local will only forward data if 'appl' is correctly set.

In app_dial:do_forward(), the original channel is replaced with a new
"forward" channel, but 'appl' is never set on this channel - The
effectively kills frame passing on the new Local/ channel created by
do_forward, and it stays that way until it is optimized away.
Comments:By: Leif Madsen (lmadsen) 2010-04-19 13:30:16

Issue confirmed until a developer can review and make sure this is the right approach, but the reporter seems to have figured out what needs to be done here.
By: Terry Wilson (twilson) 2010-05-03 15:33:29

Personally, I don't like the idea of *ever* fixing CDRs in a release branch because each change is by definition a change in behavior that may have unintended side-effects. It's hard to know when someone has already built workarounds for an issue and then we go and change the behavior in the middle of the release and break it.

Also, I'm not the CDR guru. :-p


By: Terry Wilson (twilson) 2010-05-03 15:42:13

As for this particular patch, I don't see anything obviously wrong with it. But, then again, I wouldn't have expected frame passing to rely on the application being set, either.
By: Terry Wilson (twilson) 2010-05-07 14:05:08

Turns out, tilghman fixed this 3 months ago by changing the code to just "if (other)". Please check out 1.6.2 from svn and test to make sure that the problem is indeed gone.
By: Steve Davies (one47) 2010-05-13 11:39:09

I did some very brief tests on this, and tilghman's change does seem to fix this issue. On the other hand, it is nice to have the consistent CDR columns for lastapp and lastdata, so perhaps it is worth keeping this patch regardless?
By: Terry Wilson (twilson) 2010-05-13 13:23:44

I might be willing to add it to trunk, but I don't like changing any behavior of CDRs within a release if I can help it.
By: Steve Davies (one47) 2010-05-14 08:35:39

@twilson, I will maintain a local patch for myself, so yes, trunk would be a fine place for this, or it could just be agreed that the indirect-dial caused by a SIP "Moved" response results in a blank lastapp.
By: Paul Belanger (pabelanger) 2010-06-01 13:12:11

@twilson: ping, do we want to move this into trunk or simply close out the issue?
By: Digium Subversion (svnbot) 2010-06-01 16:12:49

Repository: asterisk
Revision: 266786

U   trunk/UPGRADE.txt
U   trunk/apps/app_dial.c

------------------------------------------------------------------------
r266786 | twilson | 2010-06-01 16:12:48 -0500 (Tue, 01 Jun 2010) | 6 lines

Set app and appdata fields when a Dial is redirected

(closes issue ASTERISK-15975)
Reported by: one47
Tested by: twilson, one47

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

http://svn.digium.com/view/asterisk?view=rev&revision=266786