<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/2710/">https://reviewboard.asterisk.org/r/2710/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 31st, 2013, 6:38 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Nothing I've found here is particularly bad, so I've opened no issues.
The one thing I find a bit strange in this review is the presence of the parking provider function table that has to be retrieved. What's more common in Asterisk is to hide such a function table behind corresponding public API calls. So instead of having to retrieve the parking provider and call its parking_park_call function, you'd just have an ast_parking_park_call() function that would retrieve the parking provider and call the provider's function for you. The one merit you could chalk up to your approach is that you grab a reference to the parking provider and have that reference for as long as you require it. The problem is that res_parking currently does not permit unloading, so there's no way that you would run into an issue doing it the way I described.
I don't feel strongly enough to make you change from what works though.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That's actually why I took that approach - I was hoping that grabbing the ao2 object would also allow bumping the module reference count at some future date, which would then live for the lifetime of that ao2 object. That may be a bit optimistic however.</pre>
<br />
<p>- Matt</p>
<br />
<p>On July 29th, 2013, 3:52 p.m. UTC, Matt Jordan wrote:</p>
<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.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated July 29, 2013, 3:52 p.m.</i></p>
<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-22134">ASTERISK-22134</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;">This task started out as a goal to remove as much dead parking code from features.c as possible.
While doing so, we realized that we still had a few ways certain channel drivers (chan_skinny, chan_mgcp, and chan_dahdi) could initiate a parking request that we hadn't handled. So that got bundled in.
Once that got bundled in, it became useful to refactor (a little) the way in which the Bridging API - and external consumes of parking - interact with parking features. At that point, I decided I had fallen victim to the scope creep monster and cut it off.
So, this patch includes:
* A parking bridge features virtual table that provides access to the parking functionality that the Bridging API needs. This includes requests to park an entire 'call' (with little or no additional information, thank you chan_skinny), perform a blind transfer to a parking extension, determine if an extension is a parking extension, as well as the actual "do the parking" request from the Bridging API.
* Refactoring in chan_mgcp, chan_skinny, and chan_dahdi to make use of the new functions
* The removal of some - but not all - dead parking code from features.c
This also fixed blind transferring a multi-party bridge to a parking lot (which was implemented, but had at least one code path where using the parking features kK might not have worked)</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;">Tested basic call parking:
SIP/Alice calls SIP/Bob. Both can park each other (kK)
SIP/Alice calls SIP/Bob. SIP/Alice brings in SIP/Charlie (multi-party). All can park the other two.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/main/parking.c <span style="color: grey">(395653)</span></li>
<li>/trunk/main/features.c <span style="color: grey">(395653)</span></li>
<li>/trunk/main/bridge_channel.c <span style="color: grey">(395653)</span></li>
<li>/trunk/main/bridge.c <span style="color: grey">(395653)</span></li>
<li>/trunk/include/asterisk/parking.h <span style="color: grey">(395653)</span></li>
<li>/trunk/include/asterisk/features.h <span style="color: grey">(395653)</span></li>
<li>/trunk/channels/sig_analog.c <span style="color: grey">(395653)</span></li>
<li>/trunk/channels/chan_skinny.c <span style="color: grey">(395653)</span></li>
<li>/trunk/channels/chan_mgcp.c <span style="color: grey">(395653)</span></li>
<li>/trunk/channels/chan_iax2.c <span style="color: grey">(395653)</span></li>
<li>/trunk/channels/chan_dahdi.c <span style="color: grey">(395653)</span></li>
<li>/trunk/res/parking/parking_bridge_features.c <span style="color: grey">(395653)</span></li>
<li>/trunk/res/res_parking.c <span style="color: grey">(395653)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2710/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>