<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/3248/">https://reviewboard.asterisk.org/r/3248/</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;">Minor stylistic issues left:</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/3248/diff/2/?file=55032#file55032line316" style="color: black; font-weight: bold; text-decoration: underline;">/branches/11/res/res_http_websocket.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">316</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="mi">0</span> <span class="o">==</span> <span class="n">rlen</span> <span class="o">&&</span> <span class="n">ferror</span><span class="p">(</span><span class="n">session</span><span class="o">-></span><span class="n">f</span><span class="p">)</span> <span class="o">&&</span> <span class="n">errno</span> <span class="o">!=</span> <span class="n">EAGAIN</span><span class="p">)</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;">Generally we use (rlen == 0), not (0 == rlen), and you do too, on the same line with errno != EAGAIN.
But I'm not sure there is a strong consensus there.
And below I see a bunch of excess parentheses around pointer dereferences:
(*opcode) => *opcode
(*payload) => *payload
(*payload_len) => *payload_len
Here it makes sense (the first reference):
if ((*payload_len) && (new_payload = ast_realloc(session->payload, *payload_len))) {
Here it doesn't:
if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
Consistency is nice.</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/3248/diff/2/?file=55032#file55032line732" style="color: black; font-weight: bold; text-decoration: underline;">/branches/11/res/res_http_websocket.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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 void websocket_echo_callback(struct ast_websocket *session, struct ast_variable *parameters, struct ast_variable *headers)</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">730</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_debug</span><span class="p">(</span><span class="mi">1</span><span class="p">,</span> <span class="s">"Exitting WebSocket echo loop</span><span class="se">\n</span><span class="s">"</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;">*Exiting</pre>
</div>
<br />
<p>- wdoekes</p>
<br />
<p>On March 3rd, 2014, 1:19 a.m. UTC, Moises Silva 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and rnewton.</div>
<div>By Moises Silva.</div>
<p style="color: grey;"><i>Updated March 3, 2014, 1:19 a.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-21930">ASTERISK-21930</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23099">ASTERISK-23099</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;">Several fixes for the WebSockets implementation in res/res_http_websocket.c
* Flush the websocket session FILE* as fwrite() may not actually guarantee sending
the data to the network. If we do not flush, it seems that buffering on the SSL
socket for outbound messages causes issues
* Refactored ast_websocket_read to take into account that SSL file descriptors
may be ready to read via fread() but poll() will not actually say so because
the data was already read from the network buffers and is now in the libc buffers
This should fix an issue that I have experienced and other users may have reported [1][2][3], where
secure websockets wouldn't work, messages seem to not make it into Asterisk
[1] http://lists.digium.com/pipermail/asterisk-users/2013-August/280175.html
[2] https://issues.asterisk.org/jira/browse/ASTERISK-21930
[3] https://issues.asterisk.org/jira/browse/ASTERISK-23099</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;">See ASTERISK-21930 for details on other users testing these changes. I did both WS and WSS calls, confirmed audio works with chrome. This patch is for Asterisk 11 as the issue is reported on Asterisk 11, but I tested a few months ago and same issue existed on 12 and trunk. I created my own team branches for those too (/team/moy/webrtc-11, /team/moy/webrtc-12, /team/moy/webrtc-trunk)
Confirmed working by Sean Bright on Jan 20, 2014 on Asterisk 11 (see ASTERISK-21930 comment)</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>/branches/11/res/res_http_websocket.c <span style="color: grey">(409360)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3248/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>