[asterisk-dev] [Code Review] 3882: Replace sip_tls_read() and resolve the large SDP poll issue

[rmudgett]

> On Aug. 6, 2014, 1:58 a.m., Alexander Traud wrote:
> > trunk/channels/chan_sip.c, line 3042
> > <https://reviewboard.asterisk.org/r/3882/diff/2/?file=66262#file66262line3042>
> >
> >     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?
> 
> wdoekes wrote:
>     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;
>     
>
> 
> Alexander Traud wrote:
>     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.

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?


- rmudgett


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3882/#review13016
-----------------------------------------------------------


On Aug. 5, 2014, 9:21 p.m., ebroad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3882/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 9:21 p.m.)
> 
> 
> Review request for Asterisk Developers and Alexander Traud.
> 
> 
> Bugs: ASTERISK-18345
>     https://issues.asterisk.org/jira/browse/ASTERISK-18345
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   trunk/channels/chan_sip.c 419821 
> 
> Diff: https://reviewboard.asterisk.org/r/3882/diff/
> 
> 
> Testing
> -------
> 
> Made and received calls successfully with CSipSimple with full SIP headers over TLS, SRTP and multiple codecs enabled ensuring a large SDP payload.
> 
> 
> Thanks,
> 
> ebroad
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140806/bae6024f/attachment.html>