<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/1868/">https://reviewboard.asterisk.org/r/1868/</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;">Everything in this review is good aside from the changes in chan_sip.c
The approach you have taken is to temporarily associate a call ID with the monitor thread when processing a new INVITE request. This is a nifty idea but it falls short a bit. As you pointed out, one shortcoming is that the initial unauthenticated INVITE appears to be a different call than the next INVITE that arrives with authentication. Another drawback you will find is that any incoming SIP requests and responses beyond the initial INVITE will not have a call ID logged. So for instance, when Asterisk has a call up and going, if a SIP endpoint sends a reINVITE or BYE to Asterisk, then the handling of these messages will not have the call ID logged.
My suggestion is as follows:
Firstly, remove what you have added in handle_request_invite().
Much like you have done with the ast_channel, have an ast_callid in the sip_pvt struct. The find_call() function in chan_sip is where chan_sip attempts to find a sip_pvt associated with the incoming request or response.
If there currently is no sip_pvt associated with the request, sip_alloc() is used to create a new sip_pvt. If there is a sip_pvt associated with the call, then the sip_pvt is found.
In the case that sip_alloc() is used to create a new sip_pvt, you should create an ast_callid and place a reference to it in the sip_pvt if the incoming SIP request is an INVITE. If the incoming request is a reINVITE, then a new sip_pvt will not need to be created here, so you don't need to worry about trying to distinguish between types of INVITEs.
In the case that a pre-existing sip_pvt is found, then you can just get the ast_callid already associated with this sip_pvt to use in log messages. If the sip_pvt does not have an ast_callid, then that would mean that the sip_pvt is not associated with a call.
Now that you have a call ID, you can do your trick of temporarily associating the ast_callid with the monitor thread. I suggest keeping this association until the end of the handle_request_do() function.
The other change you would have to make is to associate the PBX thread's call ID with the sip_pvt that gets allocated in sip_request_call(). This way SIP messages on both the inbound and outbound call legs will have call IDs logged properly.
-----
As a final note, you mentioned an inconsistency with regards to blind and attended transfers. The nature of attended transfers in SIP is that you won't be able to help that a new call ID will get associated with the "consultation" portion of the attended transfer. However, arguments could be made either way regarding which of the two call IDs should be used once the transfer is completed. The problem here is that I think depending on the nature of the transfer (i.e. the number of channels involved) you may find that sometimes the first call ID is the one used for the remainder of the call and other times the second call ID is the one used for the remainder. Experiment some by transferring bridged calls to unbridged calls and transferring unbridged calls to bridged calls and see what occurs. Also experiment by changing whether the calling party or called party is the one doing the transferring.</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/1868/diff/1/?file=27275#file27275line844" style="color: black; font-weight: bold; text-decoration: underline;">/team/jrose/call_identifiers/main/channel_internal_api.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; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void ast_channel_callid_set(struct ast_channel *chan, struct ast_callid *callid)</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">844</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">//unbind if already set</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;">Don't use a C++ style comment.</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/1868/diff/1/?file=27277#file27277line1309" style="color: black; font-weight: bold; text-decoration: underline;">/team/jrose/call_identifiers/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; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_callid_threadassoc_remove()</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">1309</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_DEBUG</span><span class="p">,</span> <span class="s">"Call_ID [C-%08x] being removed from thread.</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="p">(</span><span class="o">*</span><span class="n">pointing</span><span class="p">)</span><span class="o">-></span><span class="n">call_identifier</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;">Use ast_debug()</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On April 16th, 2012, 11:06 a.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, rmudgett, and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated April 16, 2012, 11:06 a.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;">This patch adds a number of new functions involving channels and call identifiers to allow the creation of callids within specific channel drivers so that log messages can be bound to a call ID before going into the PBX. The SIP channel driver is used to demonstrate the methodology involved. Also adds display of callid involved with a channel in core show channel if it is a channel that has been deliberately bound to call id.</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;">Normal calls, calls being transfered, calls being rejected from Chan SIP.
EDIT:
I have a few scenarios to detail, and the behavior of one of them might be less than awesome.
1 - peer A calls extension to dial peer B (single invite, no reauthorization takes place)
A call ID is generated and bound to the channel for peer A making the call. The PBX thread is started and also takes a reference to the Call ID copied from the channel. Peer is dialed and when that channel is created, it gets a reference to the call ID as well. This is pretty ideal.
2 - peer C calls extension to dial peer B (reauthorization takes place, so a second invite happens before C connects)
In this case, the first invite gets a call ID which is bound to the SIP thread momentarily before the call is confirmed as being unable to complete. Peer C sends a second invite with authorization, which causes a second call ID to be created which gets bound to the call. This is a bit of a problem since the call ID should ideally be the same for both the preauthorized and post authorized portions of the call, so it needs some work.
As for transfer scenarios, it was tested with both blind and attended transfers. In the case of a blind transfer, the call ID remains the same throughout the whole process. For attended transfers however, a new call ID is created which covers the transferring channel as well as the channel receiving the transfer. Once the transfer is completed, the transferred channel is also on the new call ID. This behavior might not be a problem, though it is perhaps a little inconsistent.</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/jrose/call_identifiers/channels/chan_sip.c <span style="color: grey">(362131)</span></li>
<li>/team/jrose/call_identifiers/include/asterisk/channel.h <span style="color: grey">(362131)</span></li>
<li>/team/jrose/call_identifiers/include/asterisk/logger.h <span style="color: grey">(362131)</span></li>
<li>/team/jrose/call_identifiers/main/channel.c <span style="color: grey">(362131)</span></li>
<li>/team/jrose/call_identifiers/main/channel_internal_api.c <span style="color: grey">(362131)</span></li>
<li>/team/jrose/call_identifiers/main/cli.c <span style="color: grey">(362131)</span></li>
<li>/team/jrose/call_identifiers/main/logger.c <span style="color: grey">(362131)</span></li>
<li>/team/jrose/call_identifiers/main/pbx.c <span style="color: grey">(362131)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1868/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>