<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/3882/">https://reviewboard.asterisk.org/r/3882/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 6th, 2014, 1:58 a.m. CDT, <b>Alexander Traud</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/3882/diff/2/?file=66262#file66262line3042" style="color: black; font-weight: bold; text-decoration: underline;">trunk/channels/chan_sip.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int sip_tcp_read(struct sip_request *req, struct ast_tcptls_session_instance *tcptls_session,</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int sip_tcptls_read(struct sip_request *req, struct ast_tcptls_session_instance *tcptls_session,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3042</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span>ast_debug(2, "SIP TCP server error when receiving data\n");</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2892</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span>if ((errno == EAGAIN) | (errno == EINTR)) {</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;">Because the rest of the Asterisk code does it this way:
instead of bitwiseOr |
please, a logicalOr ||
see http://stackoverflow.com/questions/3154132/what-is-the-difference-between-logical-and-conditional-and-or-in-c
One reason, I would use an "else if" for the EINTR case. However, I am not sure if the coding-style guides have a rule for this.
@Richard
I am just curious after reading the code: When is EINTR possible? Or is that just a coding convention?</pre>
</blockquote>
<p>On August 6th, 2014, 4:48 a.m. CDT, <b>wdoekes</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;">If the call is interrupted by a signal handler, it may return EINTR. Example:
#include <errno.h>
#include <signal.h>
#include <stdio.h>
#include <unistd.h>
void handler(int signum) {
fprintf(stderr, "got sigalarm\n");
}
int main() {
char buf[1];
struct sigaction act = {0,}; // ignore https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
act.sa_handler = handler;
act.sa_flags = 0; //SA_RESTART;
sigaction(SIGALRM, &act, NULL);
//signal(SIGALRM, handler);
alarm(1);
read(0, buf, 1);
if (errno == EINTR) {
fprintf(stderr, "got EINTR\n");
}
return 0;
}
I'm not sure SA_RESTART is used consistently in asterisk, seeing that both
signal(2) and sigaction(2) are used. So EINTR might happen on systems that
do not default to SA_RESTART. In any case, it's convention and smart to
check for both.
As for the code, I'd prefer this, without the extra else block.
The continue already makes sure that we don't get to the next statements.
if (errno == EAGAIN || errno == EINTR) {
continue;
}
ast_debug(2, "SIP TCP/TLS server error when receiving data\n");
return -1;
</pre>
</blockquote>
<p>On August 6th, 2014, 5:03 a.m. CDT, <b>Alexander Traud</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;">Walter, I am about ast_tcptls_server_read() which handles EINTR already. There, I did not find the condition which returns EINTR to us here in this code.</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 ast_tcptls_server_read() call will not handle the EINTR in this case because the previous call to ast_tcptls_stream_set_exclusive_input() in _sip_tcp_helper_thread() tells it not to wait.
Also please change the if statement as walter mentioned. Use of the bitwise or in that manner is unusual. Where did you see this done elsewhere?</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On August 5th, 2014, 9:21 p.m. CDT, ebroad 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 Alexander Traud.</div>
<div>By ebroad.</div>
<p style="color: grey;"><i>Updated Aug. 5, 2014, 9:21 p.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-18345">ASTERISK-18345</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;">Replace sip_tls_read() and sip_tcp_read() with a single function and resolve the poll/wait issue with large SDP payloads. See https://reviewboard.asterisk.org/r/3653/ for the discussion on this.</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;">Made and received calls successfully with CSipSimple with full SIP headers over TLS, SRTP and multiple codecs enabled ensuring a large SDP payload.</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">(419821)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3882/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>