[Home]

Summary:ASTERISK-12930: Indications are not passed from old peer to new peer during masquerade
Reporter:David Woolley (davidw)Labels:
Date Opened:2008-10-20 13:04:07Date Closed:2009-04-29 09:45:37
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/Channels
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 13747.patch
Description:We want to transfer a channel that is already indicating (typically ringing) to a parked channel, which we do with AMI Originate.  When we do this, the person on the parked channel has MOH changed to silence until the ringing channel answers, or otherwise changes indication.

It looks to me as though there may have been some attempt to support this, but it looks like it copies the indications at the wrong end of the masquerade.

****** ADDITIONAL INFORMATION ******

This was originally tagged onto ASTERISK-11939, but I'm repeating it as the symptom is rather different and we have worked round the primary symptom by adding a local channel, so that the SIP outgoing call is undisturbed by the redirect.

When Asterisk sends an indication down a channel, it also sets a variable, called visible_indication, in the channel data structure.  When it does a masquerade, it ends up doing the following, in main/channel.c:

  4028         if (original->visible_indication)
  4029                 ast_indicate(original, original->visible_indication);

If I have identified the channels correctly, this means that it is repeating the indication that already is present on the channel that is it just about to turn into a zombie, so even in terms of a local end action, it looks wrong.

Moreover, when one does a masquerade, one is typically going to change the other party and therefore almost certainly going to change the appropriate indication, so it seems of little value applied to either near end channel.

I wonder if what was really intended here was to copy the indication, that was currently applied to the old peer channel, to the new peer channel.  Unfortunately, that means accessing the visible_indication from the old peer.  Also, one would normally queue a frame to get the indication across.
Comments:By: David Woolley (davidw) 2008-10-20 13:07:15

Sorry, I think I meant AMI Redirect.
By: Leif Madsen (lmadsen) 2008-10-21 09:51:27

Hrmmm... well I've assigned this to putnopvut for some initial thoughts should he not be swamped by all the other bugs I've assigned to him recently. Please redirect the assignment to another bloke should you feel this is in error.
By: David Woolley (davidw) 2008-10-21 10:59:49

do_masquerade is confusing. original and clone have opposite meanings to what I find intuitive.  However, I still don't think it should be re-indicating on the same channel that it got visible_indication from, and I still think it needs to indicate on the other end of the bridge.
By: Joshua C. Colp (jcolp) 2008-10-21 12:20:44

During a masquerade a channel's contents are changed so it is not who it once was. For example you may have a channel with pointer 0x42 being SIP/bob but after a masquerade while the channel still is pointer 0x42 it may actually become SIP/dave. If there was ringing indication on SIP/bob you need to start it again on SIP/dave that took its place. The code you are talking about is what does that. So in your last note it is *not* the same channel.
By: David Woolley (davidw) 2008-10-21 12:51:02

OK, but note that the real problem here is the indication (not) being sent on the other side of the bridge. (I may come back on this one though!  For now I don't want to confuse the main issue.)
By: Mark Michelson (mmichelson) 2008-10-21 17:47:34

This seems interesting, because of all the times I've looked through the ast_do_masquerade function, I've never noticed how indications are handled. My interpretation of the section davidw cited in the additional information is that that piece of code doesn't look like it actually accomplishes anything. The visible_indication from the original channel hasn't changed (i.e. we haven't moved the clone->visible_indication over to the original), and so that would seem to mean that the visible_indication of the original channel should already be in the process of being indicated.

It seems to me as if the code should actually check the clone's visible_indication and then indicate this condition to the original channel. I can write up a quick patch in the meantime (it's just a one-line change), but I'm not sure if that's exactly what's wanted, nor do I know if it will work as desired.
By: Mark Michelson (mmichelson) 2008-10-21 17:50:08

I've uploaded the aforementioned one-line patch. I'm not sure if it will do what's desired, but it's worth a try. If it doesn't work, I can look more deeply into this to see exactly what's going on.
By: David Woolley (davidw) 2008-10-22 05:36:37

This patch is addressing the issue that I consider secondary.  Moreover, for it to work, I think that you need to make the same change on the next line.

(I did try something like this, but, unfortunately, changed the destination of the indicate, rather than the source, because I thought original referered to the original channel, when it is actually the channel that is replacing it!)

My argument would be that, in most/all cases, one hasn't started the replacement channel, so no indications will have been generated yet.

I'm going to look at proof of concept on propagating the indication between peers, but, if that fails, or takes too long, I'll try this patch, first with the next line changed, then without.
By: David Woolley (davidw) 2008-10-22 08:52:40

Meanwhile, here is some dialplan that simulates the AMI and full dialplan, sufficiently to reproduce the fault:

exten => 6185,1,Dial(Local/*6185@default&Local/ASTERISK-6028@default,,g)


exten => *6185,1,Set(GLOBAL(TESTCHAN)=${CHANNEL:0:${MATH(${LEN(${CHANNEL})}-1):0:2}}1)
exten => *6185,n,Dial(SIP/4222,15)

exten => ASTERISK-6028,1,Wait(3)
exten => ASTERISK-6028,n,ChannelRedirect(${TESTCHAN},parkedcalls,701,1)

To use, dial 700 from one phone, then dial 6185 from another phone.  In the fault condition, the first phone should not give any ringback indication, but will set up a call if the phone represented here as SIP/4222 is answered.

This depends on the 4222 phone not trying to send call progress in band (it didn't work using a Cisco to PSTN line - which I tried because to try and avoid getting someone else to help run the test).

The phones we actually used, were: Polycom Soundpoint IP 301 dialled 700, Polycom Soundpoint IP 320 was 4222, and, although I'm assuming it isn't critical, 3185 was entered using a Cisco phone, via CCM.

Step 6185:1 is changing the ;2 into ;1 in the local channel name.  The ,g on the Dial was the result of a wrong turning, but should be harmless.

The 000 in 0006185 was really a hash, although it ought to work even with the Mantis mangled values.


By: David Woolley (davidw) 2008-10-22 09:23:31

This is messier than I thought; I got false hopes from the channel.c code.

By the same token that there isn't a useful indication for the channel being masqueraded, because it will be re-starting in the PBX (not sure if the new bridging operations differ here), there won't actually be a new peer yet.  What one actually needs to do is to send the indication state of the old peer to the new peer when it exists, and is in a fit state to receive it. One consequence of this, I think, is that one needs to store visible indication states for both directions (or just the other one!) against a channel.
By: David Woolley (davidw) 2008-10-22 10:38:43

As expected, neither the patch, nor my suggested tweak to it, helped.
By: David Woolley (davidw) 2008-10-22 11:10:45

I think the code fragment is actually OK, although I'm not sure that the ast_indicate is really being done in the right layer, as it exists to support this fragment, for attended transfers, in features.c:

  1126                 xferchan->visible_indication = transferer->visible_indication;
  1127                 xferchan->readformat = transferee->readformat;
  1128                 xferchan->writeformat = transferee->writeformat;
  1129                 ast_channel_masquerade(xferchan, transferee);

I think the solution for my problem may lie in features.c.
By: David Woolley (davidw) 2008-10-23 11:45:51

Another observation about the code fragment is that visible_indication seems to assume that indications are states, but some, e.g. SRCUPDATE, look more like pure events.  Maybe it is safe in practice, but that's not immediately clear.
By: David Woolley (davidw) 2008-10-23 12:00:48

My thought for the day was that one might indicate any indication required, just after this code in ast_channel_bridge.

  4435         ast_indicate(c0, AST_CONTROL_SRCUPDATE);
  4436         ast_indicate(c1, AST_CONTROL_SRCUPDATE);

However:

1) AST_CONTROL_RINGING seems to cause the bridge to drop out, so one would need to queue it, or short cut to an exit.

2) I'm not sure if that would result in an unwanted hangup or dialplan continuation (I think I'm safe from the latter in the case I'm interested in).  More code reading needed.

3) We seem to have the choice of using the channel state, or a, new, analogue of visible_indication, in the reverse direction, to choose what to send.  State doesn't seem to directly correlate with indication, and, in particular, looks to be controlled by technology drivers, rather than the core.  An analogue of visible_indication suffers from the problem in the previous comment, namely that there seem to be pure events as well as state change indications.
By: David Woolley (davidw) 2008-10-24 07:56:47

Seems that one can't use state in our case, because there is a local channel (a workround for another problem), and local channels don't fully reflect the state of their bridged channels (e.g ASTERISK-1274825).
By: David Woolley (davidw) 2008-10-24 09:03:44

It also looks like a transition from bridged to ringing will cause the channel to be hungup, so there is no simple, localised change.
By: David Woolley (davidw) 2008-10-24 10:01:48

Would it be safe to treat RINGING like hold, here, in ast_generic_bridge??

  4264                         switch (f->subclass) {
  4265                         case AST_CONTROL_HOLD:
  4266                         case AST_CONTROL_UNHOLD:
  4267                         case AST_CONTROL_VIDUPDATE:
  4268                         case AST_CONTROL_SRCUPDATE:
  4269                                 ast_indicate_data(other, f->subclass, f-
By: Digium Subversion (svnbot) 2008-12-12 10:18:35.000-0600

Repository: asterisk
Revision: 163578

A   team/russell/issue_13747/

------------------------------------------------------------------------
r163578 | russell | 2008-12-12 10:18:35 -0600 (Fri, 12 Dec 2008) | 2 lines

Create a branch to work on issue ASTERISK-12930 related fixes (indications + masquerade madness)

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

http://svn.digium.com/view/asterisk?view=rev&revision=163578
By: Russell Bryant (russell) 2008-12-12 16:12:41.000-0600

http://reviewboard.digium.com/r/90/
By: Digium Subversion (svnbot) 2008-12-15 08:31:33.000-0600

Repository: asterisk
Revision: 164201

U   branches/1.4/main/channel.c
U   branches/1.4/res/res_features.c

------------------------------------------------------------------------
r164201 | russell | 2008-12-15 08:31:32 -0600 (Mon, 15 Dec 2008) | 31 lines

Handle a case where a call can be bridged to a channel that is still ringing.

The issue that was reported was about a case where a RINGING channel got
redirected to an extension to pick up a call from parking.  Once the parked
call got taken out of parking, it heard silence until the other side answered.  
Ideally, the caller that was parked would get a ringing indication.  This patch
fixes this case so that the caller receives ringback once it comes out of
parking until the other side answers.

The fixes are:

- Make sure we remember that a channel was an outgoing channel when doing
  a masquerade.  This prevents an erroneous ast_answer() call on the channel,
  which causes a bogus 200 OK to be sent in the case of SIP.

- Add some additional comments to explain related parts of code.

- Update the handling of the ast_channel visible_indication field.  Storing
  values that are not stateful is pointless.  Control frames that are events
  or commands should be ignored.

- When a bridge first starts, check to see if the peer channel needs to be
  given ringing indication because the calling side is still ringing.

- Rework ast_indicate_data() a bit for the sake of readability.

(closes issue ASTERISK-12930)
Reported by: davidw
Tested by: russell
Review: http://reviewboard.digium.com/r/90/

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

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