[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] [Code Review] 2638: res_parking: Gut parker snapshots and replace them with Parke
From: "jrose" <reviewboard () asterisk ! org>
Date: 2013-06-27 20:51:56
Message-ID: 20130627205156.21320.40150 () hotblack ! digium ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2638/
-----------------------------------------------------------
(Updated June 27, 2013, 8:51 p.m.)
Review request for Asterisk Developers, Matt Jordan and rmudgett.
Changes
-------
Hit Matt's review. Updated the review to included changes that have been made to \
trunk since the review was last posted.
Bugs: ASTERISK-21877
https://issues.asterisk.org/jira/browse/ASTERISK-21877
Repository: Asterisk
Description
-------
We decided parker snapshots were misleading and unuseful for all but a few uses of \
parked call messages and events. As such, they've been replaced with just having \
parker dial strings for handling call control on timeout.
If a channel directly parks another channel, then setting the park dial string is \
fairly trivial. We just use the copy the name of the channel that parked the call \
and then flatten it while creating the parked user. If the call was transferred to \
the park application somehow, then it must be read from either the BLINDTRANSFER \
variable or from the newly added ATTENDEDTRANSFER variable which is basically just \
the BLINDTRANSFER variable for attended transfers.
This effectively wipes out one of the bug bug comments mentioned in ASTERISK-21877 \
regarding not having a snapshot of who parked the call on so-called 'deep parks' \
where a park application is deeper than the first priority of the extension a call is \
transferred to. The other bug bug comment remains at large, but mmichelson has a \
patch for bridge roles in the works which should make fixing that trivial.
Diffs (updated)
-----
/trunk/bridges/bridge_builtin_features.c 393053
/trunk/include/asterisk/channel.h 393053
/trunk/include/asterisk/parking.h 393053
/trunk/main/bridging.c 393053
/trunk/main/cel.c 393053
/trunk/main/channel.c 393053
/trunk/main/features.c 393053
/trunk/main/parking.c 393053
/trunk/res/parking/parking_applications.c 393053
/trunk/res/parking/parking_bridge.c 393053
/trunk/res/parking/parking_bridge_features.c 393053
/trunk/res/parking/parking_controller.c 393053
/trunk/res/parking/parking_manager.c 393053
/trunk/res/parking/parking_ui.c 393053
/trunk/res/parking/res_parking.h 393053
Diff: https://reviewboard.asterisk.org/r/2638/diff/
Testing
-------
All kinds of different transfers (DTMF blind, DTMF attended, SIP blind, SIP attended, \
SIP attended with hangups before park, etc).
The only noteworthy case where the parker was anything other than who transferred the \
call was the following:
SIP/A calls SIP/B
SIP/A performs attended transfer to extension f
f,1,NoOp
f,2,Wait(10)
f,3,Park()
SIP/A hangs up during the wait
This causes the park dial string for SIP/B's parked user to be SIP/B unfortunately, \
but this is more of a transfer related issue than a parking related issue. It's also \
one that Richard says was also the case in previous versions, so this isn't a change \
in behavior.
This could be addressed by setting the ATTENDEDTRANSFER variable when the masquerade \
occurs to put B in the PBX where A leaves off.
Thanks,
jrose
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2638/">https://reviewboard.asterisk.org/r/2638/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Asterisk Developers, Matt Jordan and rmudgett.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated June 27, 2013, 8:51 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Hit Matt's review. Updated the review to included changes that have \
been made to trunk since the review was last posted.</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21877">ASTERISK-21877</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">We decided parker snapshots were misleading and unuseful for all but a \
few uses of parked call messages and events. As such, they've been replaced with \
just having parker dial strings for handling call control on timeout.
If a channel directly parks another channel, then setting the park dial string is \
fairly trivial. We just use the copy the name of the channel that parked the call \
and then flatten it while creating the parked user. If the call was transferred to \
the park application somehow, then it must be read from either the BLINDTRANSFER \
variable or from the newly added ATTENDEDTRANSFER variable which is basically just \
the BLINDTRANSFER variable for attended transfers.
This effectively wipes out one of the bug bug comments mentioned in ASTERISK-21877 \
regarding not having a snapshot of who parked the call on so-called 'deep \
parks' where a park application is deeper than the first priority of the \
extension a call is transferred to. The other bug bug comment remains at large, but \
mmichelson has a patch for bridge roles in the works which should make fixing that \
trivial.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">All kinds of different transfers (DTMF blind, DTMF attended, SIP blind, \
SIP attended, SIP attended with hangups before park, etc).
The only noteworthy case where the parker was anything other than who transferred the \
call was the following:
SIP/A calls SIP/B
SIP/A performs attended transfer to extension f
f,1,NoOp
f,2,Wait(10)
f,3,Park()
SIP/A hangs up during the wait
This causes the park dial string for SIP/B's parked user to be SIP/B \
unfortunately, but this is more of a transfer related issue than a parking related \
issue. It's also one that Richard says was also the case in previous versions, \
so this isn't a change in behavior.
This could be addressed by setting the ATTENDEDTRANSFER variable when the masquerade \
occurs to put B in the PBX where A leaves off.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> \
(updated)</h1> <ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/bridges/bridge_builtin_features.c <span style="color: \
grey">(393053)</span></li>
<li>/trunk/include/asterisk/channel.h <span style="color: grey">(393053)</span></li>
<li>/trunk/include/asterisk/parking.h <span style="color: grey">(393053)</span></li>
<li>/trunk/main/bridging.c <span style="color: grey">(393053)</span></li>
<li>/trunk/main/cel.c <span style="color: grey">(393053)</span></li>
<li>/trunk/main/channel.c <span style="color: grey">(393053)</span></li>
<li>/trunk/main/features.c <span style="color: grey">(393053)</span></li>
<li>/trunk/main/parking.c <span style="color: grey">(393053)</span></li>
<li>/trunk/res/parking/parking_applications.c <span style="color: \
grey">(393053)</span></li>
<li>/trunk/res/parking/parking_bridge.c <span style="color: \
grey">(393053)</span></li>
<li>/trunk/res/parking/parking_bridge_features.c <span style="color: \
grey">(393053)</span></li>
<li>/trunk/res/parking/parking_controller.c <span style="color: \
grey">(393053)</span></li>
<li>/trunk/res/parking/parking_manager.c <span style="color: \
grey">(393053)</span></li>
<li>/trunk/res/parking/parking_ui.c <span style="color: grey">(393053)</span></li>
<li>/trunk/res/parking/res_parking.h <span style="color: grey">(393053)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2638/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic