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

[wdoekes]

> On Aug. 6, 2014, 6: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?

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;


- wdoekes


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


On Aug. 6, 2014, 2:21 a.m., ebroad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3882/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2014, 2:21 a.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/47999882/attachment.html>