<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 />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Mostly looks good if you ask me. Remove the additions you have made in ccss.c. The recall that a call completion agent performs is a separate call from the original call being tracked, so it should get its own callid.</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/1/?file=26475#file26475line745" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/dial.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">enum ast_dial_result ast_dial_run(struct ast_dial *dial, struct ast_channel *chan, int async)</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">745</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">                <span class="k">if</span> <span class="p">((</span><span class="n">dial</span><span class="o">-></span><span class="n">callid</span> <span class="o">=</span> <span class="n">ast_read_threadstorage_callid</span><span class="p">()))</span> <span class="p">{</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">746</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">                        <span class="n">ast_callid_ref</span><span class="p">(</span><span class="n">dial</span><span class="o">-></span><span class="n">callid</span><span class="p">);</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">747</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">                        <span class="cm">/* reference be released at dial destruction */</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">748</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">                <span class="p">}</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;">This pattern is used a lot. You should alter ast_read_threadstorage_callid() to return a reference to the callid instead of a pointer that you then have to call ast_callid_ref() on.</pre>
</div>
<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/1/?file=26477#file26477line1360" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/logger.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">void ast_log(int level, const char *file, int line, const char *function, const char *fmt, ...)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static void __attribute__((format(printf, 6, 0))) ast_log_full(int level, const char *file, int line, const char *function, struct ast_callid *callid, const char *fmt, va_list ap)</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">1355</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">                <span class="n">ast_callid_ref</span><span class="p">(</span><span class="n">callid</span><span class="p">);</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">1356</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">                <span class="n">logmsg</span><span class="o">-></span><span class="n">callid</span> <span class="o">=</span> <span class="p">(</span><span class="n">callid</span><span class="p">);</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;">Combine these two lines into
logmsg->callid = ast_callid_ref(callid);
</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On March 20th, 2012, 3:14 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 20, 2012, 3:14 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>.
2. Add CLI command to display <call-id> with verbose messages on CLI.
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/pbx.c <span style="color: grey">(360031)</span></li>
<li>/trunk/apps/app_mixmonitor.c <span style="color: grey">(360031)</span></li>
<li>/trunk/include/asterisk/bridging.h <span style="color: grey">(360031)</span></li>
<li>/trunk/include/asterisk/ccss.h <span style="color: grey">(360031)</span></li>
<li>/trunk/include/asterisk/logger.h <span style="color: grey">(360031)</span></li>
<li>/trunk/main/bridging.c <span style="color: grey">(360031)</span></li>
<li>/trunk/main/ccss.c <span style="color: grey">(360031)</span></li>
<li>/trunk/main/dial.c <span style="color: grey">(360031)</span></li>
<li>/trunk/main/features.c <span style="color: grey">(360031)</span></li>
<li>/trunk/main/logger.c <span style="color: grey">(360031)</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>