<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/1823/">https://reviewboard.asterisk.org/r/1823/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Lookin' good to me. I just have one final nitpick and I'm done here.</pre>
<br />
<div>
<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/1823/diff/3/?file=26947#file26947line277" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/include/asterisk/logger.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct ast_callid *ast_read_threadstorage_callid(void);</pre></td>
</tr>
</tbody>
<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">277</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \retval 0 - success</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">278</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \retval 1 - failure due to thread already being bound to a callid</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">279</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \note possibly other retvals</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I had a look and found that this function can actually return -1 as well. Probably best to change the second retval to be something like "non-zero" since callers probably will not care about the details of why the function failed.</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On March 29th, 2012, 1:52 p.m., jrose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/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, Mark Michelson, junky, and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated March 29, 2012, 1:52 p.m.</i></p>
<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;">https://wiki.asterisk.org/wiki/display/AST/Unique+Call-ID+Logging
Since none of this patch has appeared on the public reviewboard yet, I'll go ahead and fill everyone in on phase I's objectives which have already been met.
Phase I
Implement <call-id> structure and factory. Bind <call-id> pointers to newly created channel threads.
Modify logging to read thread storage so that channel thread log messages include the <call-id> stamp.
As for the current objective...
Phase II
1. Spread references to ast_callid structs to threads created by a pbx/channel thread already bound to a <call-id>.
The primary focus of this review is a sanity check for some of the thread associations that are being worked on for this feature and also to get some community feedback on the feature in general since it hasn't seen a great deal of that yet.
What I'm looking to learn from this review is if I missed any obvious places where this callid should be passed to a new thread and if the various places that I am passing it around right now are all appropriate. It could also be that there is a better approach to handling this passing of callids that I'm just not aware of right now.
This code can be found in the following repository:
https://origsvn.digium.com/svn/asterisk/team/jrose/call_identifiers</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;">Honestly, I'm not sure if all of these thread associations are appropriate or not. The one I understand best right now is mixmonitor, which handles thread associations like you would expect... it inherits them from the parent thread if the parent thread is associated with a call-id. In other words, if it's activated from the dialplan, it will be a part of the call. If it's activated from AMI or CLI, then the mixmonitor won't have a callid associated with it. This doesn't necessarily seem inappropriate, though it wouldn't necessarily be inappropriate to go ahead and bind mixmonitor to the callid when created in those ways either... but nothing can be done about that until the additon to handle callids in channels goes through in phase III.
For a little demonstration of this feature in action, the following is a call where the following happens:
1. SIP/gold dials an extension which mixmonitors a dial to SIP/101
2. SIP/gold holds on SIP/101 and then unholds
3. SIP/gold blind transfers SIP/101 to SIP/123
4. SIP/101 hangs up the line
[2012-03-2015:01:30] VERBOSE[28696] netsock2.c: == Using SIP RTP CoS mark 5
[2012-03-2015:01:30] DEBUG[28729] logger.c: CALL_ID [C000000] created by thread.
[2012-03-2015:01:30] DEBUG[28729][C000000] logger.c: CALL_ID [C000000] bound to thread.
[2012-03-2015:01:30] VERBOSE[28729][C000000] pbx.c: -- Executing [006@sipphones:1] MixMonitor("SIP/gold-00000000", "recordingx.wav,m(1111@default)") in new stack
[2012-03-2015:01:30] VERBOSE[28729][C000000] pbx.c: -- Executing [006@sipphones:2] Dial("SIP/gold-00000000", "SIP/101") in new stack
[2012-03-2015:01:30] DEBUG[28730][C000000] logger.c: CALL_ID [C000000] bound to thread.
[2012-03-2015:01:30] VERBOSE[28730][C000000] app_mixmonitor.c: == Begin MixMonitor Recording SIP/gold-00000000
[2012-03-2015:01:30] VERBOSE[28729][C000000] netsock2.c: == Using SIP RTP CoS mark 5
[2012-03-2015:01:30] VERBOSE[28729][C000000] app_dial.c: -- Called SIP/101
[2012-03-2015:01:30] VERBOSE[28729][C000000] app_dial.c: -- SIP/101-00000001 is ringing
[2012-03-2015:01:33] VERBOSE[28729][C000000] app_dial.c: -- SIP/101-00000001 answered SIP/gold-00000000
[2012-03-2015:02:17] VERBOSE[28729][C000000] res_musiconhold.c: -- Started music on hold, class 'default', on SIP/101-00000001
[2012-03-2015:02:22] VERBOSE[28729][C000000] res_musiconhold.c: -- Stopped music on hold on SIP/101-00000001
[2012-03-2015:02:22] DEBUG[28733] logger.c: CALL_ID [C000001] created by thread.
[2012-03-2015:02:22] DEBUG[28733][C000001] logger.c: CALL_ID [C000001] bound to thread.
[2012-03-2015:02:22] VERBOSE[28733][C000001] pbx.c: -- Executing [123*@sipphones:1] Dial("SIP/101-00000001", "SIP/123") in new stack
[2012-03-2015:02:22] VERBOSE[28733][C000001] netsock2.c: == Using SIP RTP CoS mark 5
[2012-03-2015:02:22] VERBOSE[28733][C000001] app_dial.c: -- Called SIP/123
[2012-03-2015:02:22] NOTICE[28696] chan_sip.c: Got OK on REFER Notify message
[2012-03-2015:02:22] VERBOSE[28729][C000000] pbx.c: == Spawn extension (sipphones, 006, 2) exited non-zero on 'SIP/gold-00000000'
[2012-03-2015:02:22] VERBOSE[28730][C000000] app_mixmonitor.c: == MixMonitor close filestream (mixed)
[2012-03-2015:02:22] VERBOSE[28730][C000000] app_mixmonitor.c: == End MixMonitor Recording SIP/gold-00000000
[2012-03-2015:02:22] VERBOSE[28733][C000001] app_dial.c: -- SIP/123-00000002 is ringing
[2012-03-2015:02:23] VERBOSE[28733][C000001] app_dial.c: -- SIP/123-00000002 answered SIP/101-00000001
[2012-03-2015:02:23] VERBOSE[28733][C000001] rtp_engine.c: -- Locally bridging SIP/101-00000001 and SIP/123-00000002
[2012-03-2015:02:35] VERBOSE[28733][C000001] pbx.c: == Spawn extension (sipphones, 123*, 1) exited non-zero on 'SIP/101-00000001'
I'm still working on test cases for the other associations I'm performing.</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/features.c <span style="color: grey">(360780)</span></li>
<li>/trunk/main/dial.c <span style="color: grey">(360780)</span></li>
<li>/trunk/main/bridging.c <span style="color: grey">(360780)</span></li>
<li>/trunk/configs/logger.conf.sample <span style="color: grey">(360780)</span></li>
<li>/trunk/include/asterisk/bridging.h <span style="color: grey">(360780)</span></li>
<li>/trunk/include/asterisk/logger.h <span style="color: grey">(360780)</span></li>
<li>/trunk/CHANGES <span style="color: grey">(360780)</span></li>
<li>/trunk/apps/app_mixmonitor.c <span style="color: grey">(360780)</span></li>
<li>/trunk/main/logger.c <span style="color: grey">(360780)</span></li>
<li>/trunk/main/pbx.c <span style="color: grey">(360780)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1823/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>