<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/2447/">https://reviewboard.asterisk.org/r/2447/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 2nd, 2013, 7:33 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;">I suggest adding XXX, BUGBUG, TODO, or similar comments to areas where you know that functionality is missing. Your review description has a list of todo items, but I ran across a thing or two on here that I wasn't sure corresponded to the listed items. For instance, the Park application has a 'c' option that currently doesn't do anything. I didn't comment on these individual items I came across because I assume you already know that they're unimplemented, but seeing a comment acknowledging that functionality is unaddressed helps me to know for sure whether something is purposely missing or if there is an oversight.</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;">Actually I had the 'c' option working for a really long time and then changed some stuff there a bit and just ended up forgetting to put it in the park common datastore. I'll fix it though.
Added everything from the list as TODO comments.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 2nd, 2013, 7:33 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2447/diff/4/?file=36813#file36813line56" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/res/parking/res_parking.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">56</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">PARKINGLOT_DISABLED</span><span class="p">,</span> <span class="cm">/*! The parking lot is no longer linked to a parking lot in configuration. It can no longer be parked to.</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">57</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span> * and it can not be parked to. Terminal Mode doest. */</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think this parking lot mode can be removed.
If I understand correctly, the way a parking lot gets marked as disabled is if a parking lot existed in configuration, had users in it, and then got removed from configuration.
At this point, the parking lot is disabled. Care has to be taken to ensure that users do not get parked to this lot, and each time a user is removed from a parking lot, a check gets made to see if the disabled lot can be removed from the container of parking lots. When the parking lot is disabled and finally becomes empty, the parking lot can be removed from the container and thus be destroyed.
One thing I noticed, though, was that parked users have a reference to the lot they're parked in. Because of this, you can instead change the behavior so that when a parking lot is removed from configuration, the lot is removed from the container of parking lots immediately. Then, as users are removed from the lot, their references to the lot are removed. When the final user is removed from the lot, the final reference to the lot is removed, and the lot can die. And of course if there were no users in the lot when it was removed from configuration, then removing the lot from the container immediately results in the lot's destruction.
With this construct, you don't have to worry about potentially parking a user in a disabled lot, and you don't have to manually perform any destruction checks when users exit the parking lot.
Also, what does "Terminal Mode doest" mean?</pre>
</blockquote>
<p>On May 2nd, 2013, 7:49 p.m. UTC, <b>rmudgett</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;">Removing the parking lot immediately from the parking lots container is not going to work. You need to find the parking lot name in the container to pickup a parked call.</pre>
</blockquote>
<p>On May 2nd, 2013, 7:54 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;">Okay, that makes sense. I don't think reviewboard will let me drop the issue, unfortunately...</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Also you may still want to look at the parking lot using cli commands and such in spite of it being disabled. The 'doest' thing... it was supposed to be "Terminal mode, doesn't transition to other modes". I'm changing that a big because it sounds a little awkward.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 2nd, 2013, 7:33 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2447/diff/4/?file=36813#file36813line125" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/res/parking/res_parking.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">125</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">struct</span> <span class="n">parking_lot</span> <span class="o">*</span><span class="nf">parking_lot_build_or_update</span><span class="p">(</span><span class="k">struct</span> <span class="n">parking_lot_cfg</span> <span class="o">*</span><span class="n">cfg</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why do the structures and functions in res_parking.h not have ast_ prefixes on them?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">None of these structures or functions are ever intended to be accessed outside of the parking module. Public-facing functionality provided by the parking module is provided through installable functions in parking.h</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 2nd, 2013, 7:33 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2447/diff/4/?file=36814#file36814line219" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/res/res_parking.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">219</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">void</span> <span class="o">*</span><span class="nf">named_item_find</span><span class="p">(</span><span class="k">struct</span> <span class="n">ao2_container</span> <span class="o">*</span><span class="n">container</span><span class="p">,</span> <span class="k">const</span> <span class="kt">char</span> <span class="o">*</span><span class="n">name</span><span class="p">);</span> <span class="cm">/* </span><span class="cs">XXX</span><span class="cm"> This is really just a generic string find. Move to astobj2.c? */</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If this were to go into astobj2's core, I'd suggest adding a flags parameter to the function. That way, people could specify flags like OBJ_NODATA or OBJ_MULTIPLE if desired.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For now it isn't to keep the commit from touching too many core files, but I'll keep that in mind.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 2nd, 2013, 7:33 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2447/diff/4/?file=36814#file36814line582" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/res/res_parking.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">582</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">((</span><span class="n">lot</span> <span class="o">=</span> <span class="n">named_item_find</span><span class="p">(</span><span class="n">parking_lot_container</span><span class="p">,</span> <span class="n">lot_cfg</span><span class="o">-></span><span class="n">name</span><span class="p">)))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Kind of nitpicky, but you have a function called parking_lot_find_by_name() that wraps around named_item_find(), so why not call it instead?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The parking lot container is only accessible in res_parking.c while this function is called from other places. Doing this keeps other stuff from needing to obtain the parking lot container directly.</pre>
<br />
<p>- jrose</p>
<br />
<p>On April 30th, 2013, 6:44 p.m. UTC, jrose 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, David Lee, kmoore, Matt Jordan, Mark Michelson, and rmudgett.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated April 30, 2013, 6:44 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-21059">ASTERISK-21059</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21272">ASTERISK-21272</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21353">ASTERISK-21353</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;">As a result of all the new bridging changes, certain applications such as parking, queues, confbridge, and a few others require significant reworking within Asterisk to make usable.
Parking was less than salvagable, so I've rebuilt it from the ground up to use a lot of new stuff including config hooks, the new bridging architecture, stasis, and just generally to
be an independent module with its own scope that doesn't get tangled into a giant morass within features.c
Parking configuration works somewhat differently now and extensions aren't necessarily automatically generated (it's optional and hasn't been implemented), but the intention with the final result is that if you were using parking before without too much additional dialplan manipulation, that should all just work once your parking lots are migrated to the new configuration file.
Currently supported:
parking from the PBX with the Park application (all arguments are supported)
parking within a call using the one touch parking feature
parking within a call using DTMF blind transfers (attended transfers work, but it's basically just like they called the application itself).
Picking up parked calls using the PBX
Multiple parking lots
All options available to parking lots are currently configurable. Some aren't currently doing anything, namely parkext and hints.
To do list:
* Dialplan generation for parking lots with parkext (included 'Park' and 'ParkedCall' applications)
* Dialplan generation for comebacktoorigin (park-dial extensions)
* Hints
* Implement 'Park' manager action
* Dynamic parking lots and the default parking lot
* Scraping the greasy remnants of parking out of features.c
* CEL events
* Unit tests and testsuite tests</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;">This is currently being tested as it's developed and breaks in minor ways frequently. Unit tests and testsuite tests are on the to do list.</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>/team/group/bridge_construction/include/asterisk/parking.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/main/bridging.c <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/main/bridging_roles.c <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/main/config_options.c <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/main/features.c <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/main/parking.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/res/Makefile <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/res/parking/parking_applications.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/include/asterisk/config_options.h <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/include/asterisk/bridging.h <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/configs/res_parking.conf.sample <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/bridges/bridge_builtin_features.c <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/CHANGES <span style="color: grey">(387012)</span></li>
<li>/team/group/bridge_construction/res/parking/parking_bridge.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/res/parking/parking_bridge_features.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/res/parking/parking_controller.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/res/parking/parking_manager.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/res/parking/parking_ui.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/res/parking/res_parking.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/res/res_parking.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2447/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>