<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/1993/">https://reviewboard.asterisk.org/r/1993/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 22nd, 2012, 5:08 p.m., <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/1993/diff/1/?file=28966#file28966line11098" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/channels/chan_sip.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; ">static int transmit_response_with_sip_etag(struct sip_pvt *p, const char *msg, const struct sip_request *req, struct sip_esc_entry *esc_entry, int need_new_etag)</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">11079</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">p</span><span class="o">-></span><span class="n">expiry</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">11080</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">add_expires</span><span class="p">(</span><span class="o">&</span><span class="n">resp</span><span class="p">,</span> <span class="n">p</span><span class="o">-></span><span class="n">expiry</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">11081</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="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Would I be correct in saying this is a bug fix rather than a piece of code cleanup? If so, this should probably be committed to other branches of Asterisk besides just trunk.</pre>
</blockquote>
<p>On June 24th, 2012, 10:21 p.m., <b>gareth</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;">The code was modified because Cisco phones require an Expires header to be present.
Looking at RFC3903 section 6.6 says "The response MUST contain an Expires header field indicating the expiration interval chosen by the ESC". So I would say that is a bug.</pre>
</blockquote>
<p>On June 25th, 2012, 8:04 a.m., <b>Leif Madsen</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;">Could you break out the bug fixes into separate reviews? Code cleanup I don't think should be making any behaviour changes like bug fixes. Those should be reviewed and committed independently.</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 Expires header addition and the not walking off the back of referred_by_name are then only actual bug fixes and the latter is probably harmless.
I will do a new review for the expires header fix.</pre>
<br />
<p>- gareth</p>
<br />
<p>On June 24th, 2012, 10:27 p.m., gareth 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.</div>
<div>By gareth.</div>
<p style="color: grey;"><i>Updated June 24, 2012, 10:27 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;">This is the first part split out from the much larger patch at https://reviewboard.asterisk.org/r/1976/ - it contains just the various chan_sip code cleanups.
- struct sip_refer converted to use the stringfields API.
- sip_{refer|notify}_allocate -> sip_{notify|refer}_alloc to match other *alloc functions.
- Replace get_msg_text, get_msg_text2 and get_pidf_body -> No, not get_pidf_msg_text_body3 but get_content, to match add_content.
- get_body doesn't get the request body, renamed to get_content_line.
- get_body_by_line doesn't get the body line, and is just a simple if test. Moved code inline and removed function.
- Remove camelCase in struct sip_peer peer state variables, onHold -> onhold, inUse -> inuse, inRinging -> ringing.
- Remove camelCase in struct sip_request rlPart1 -> rlpart1, rlPart2 -> rlpart2.
- Rename instances of pvt->randdata to pvt->nonce because that is what it is, no need to update struct sip_pvt because _it already has a nonce field_.
- Removed struct sip_pvt randdata stringfield.
- Remove useless (and inconsistent) 'header' suffix on variables in handle_request_subscribe.
- Use ast_strdupa on Event header in handle_request_subscribe to avoid overly complicated strncmp calls to find the event package.
- Move get_destination check in handle_request_subscribe to avoid duplicate checking for packages that don't need it.
- Move extension state callback management in handle_request_subscribe to avoid duplicate checking for packages that don't need it.
- Remove duplicate append_date prototype.
- Rename append_date -> add_date to match other add_xxx functions.
- Added add_expires helper function, removed code that manually added expires header.
- Remove _header suffix on add_diversion_header (no other header adding functions have this).
- Don't pass req->debug to request handle_request_XXXXX handlers if req is also being passed.
- Don't pass req->ignore to check_auth as req is already being passed.
- Don't create a subscription in handle_request_subscribe if p->expiry == 0.
- Don't walk of the back of referred_by_name when splitting string in get_refer_info
- Remove duplicate check for no dialog in handle_incoming when sipmethod == SIP_REFER, handle_request_refer checks for that.
</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;">It compiles.</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/channels/chan_sip.c <span style="color: grey">(369065)</span></li>
<li>/trunk/channels/sip/include/sip.h <span style="color: grey">(369065)</span></li>
<li>/trunk/channels/sip/security_events.c <span style="color: grey">(369065)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1993/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>