(try 2) Added send-thresholding and fixed text packet dumping

talk/media/sctp/sctpdataengine.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc. and Robin Seggelmann
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 91 matching lines @@
 }  // namespace
 
 namespace cricket {
 typedef rtc::ScopedMessageData<SctpInboundPacket> InboundPacketMessage;
 typedef rtc::ScopedMessageData<rtc::Buffer> OutboundPacketMessage;
 
 // The biggest SCTP packet.  Starting from a 'safe' wire MTU value of 1280,
 // take off 80 bytes for DTLS/TURN/TCP/IP overhead.
 static const size_t kSctpMtu = 1200;
 
+// The size of the SCTP association send buffer.  256kB, the usrsctp default.
+static const int kSendBufferSize = 262144;
 enum {
   MSG_SCTPINBOUNDPACKET = 1,   // MessageData is SctpInboundPacket
   MSG_SCTPOUTBOUNDPACKET = 2,  // MessageData is rtc:Buffer
 };
 
 struct SctpInboundPacket {
   rtc::Buffer buffer;
   ReceiveDataParams params;
   // The |flags| parameter is used by SCTP to distinguish notification packets
   // from other types of packets.
@@ skipping 48 matching lines @@
     case SctpDataMediaChannel::PPID_NONE:
       *dest = cricket::DMT_NONE;
       return true;
 
     default:
       return false;
   }
 }
 
 // Log the packet in text2pcap format, if log level is at LS_VERBOSE.
-static void VerboseLogPacket(void *addr, size_t length, int direction) {
+static void VerboseLogPacket(void *data, size_t length, int direction) {
   if (LOG_CHECK_LEVEL(LS_VERBOSE) && length > 0) {
     char *dump_buf;
     if ((dump_buf = usrsctp_dumppacket(
-             addr, length, direction)) != NULL) {
+             data, length, direction)) != NULL) {
       LOG(LS_VERBOSE) << dump_buf;
       usrsctp_freedumpbuffer(dump_buf);
     }
   }
 }
 
 // This is the callback usrsctp uses when there's data to send on the network
 // that has been wrapped appropriatly for the SCTP protocol.
 static int OnSctpOutboundPacket(void* addr, void* data, size_t length,
                                 uint8_t tos, uint8_t set_df) {
@@ skipping 56 matching lines @@
     // AF_CONN use of sctp.
     usrsctp_init(0, cricket::OnSctpOutboundPacket, debug_sctp_printf);
 
     // To turn on/off detailed SCTP debugging. You will also need to have the
     // SCTP_DEBUG cpp defines flag.
     // usrsctp_sysctl_set_sctp_debug_on(SCTP_DEBUG_ALL);
 
     // TODO(ldixon): Consider turning this on/off.
     usrsctp_sysctl_set_sctp_ecn_enable(0);
 
+    int send_size = usrsctp_sysctl_get_sctp_sendspace();
+    if (send_size != kSendBufferSize) {
+      LOG(LS_ERROR) << "Got different send size than expected: " << send_size;

[tommi, 2015/08/28 11:44:04]

is it still ok to continue?  what are the implications?

[lally1, 2015/08/28 17:54:48]

Harmless check, it's been documented.

+    }
+
     // TODO(ldixon): Consider turning this on/off.
     // This is not needed right now (we don't do dynamic address changes):
     // If SCTP Auto-ASCONF is enabled, the peer is informed automatically
     // when a new address is added or removed. This feature is enabled by
     // default.
     // usrsctp_sysctl_set_sctp_auto_asconf(0);
 
     // TODO(ldixon): Consider turning this on/off.
     // Add a blackhole sysctl. Setting it to 1 results in no ABORTs
     // being sent in response to INITs, setting it to 2 results
@@ skipping 37 matching lines @@
 }
 
 DataMediaChannel* SctpDataEngine::CreateChannel(
     DataChannelType data_channel_type) {
   if (data_channel_type != DCT_SCTP) {
     return NULL;
   }
   return new SctpDataMediaChannel(rtc::Thread::Current());
 }
 
+// static
+SctpDataMediaChannel* SctpDataEngine::GetChannelFromSocket(
+    struct socket* sock) {
+  struct sockaddr* addrs;

[tommi, 2015/08/28 11:44:04]

= nullptr;

[lally1, 2015/08/28 17:54:48]

Done.

+  int naddrs;

[tommi, 2015/08/28 11:44:04]

declare where needed.

[lally1, 2015/08/28 17:54:48]

Done.

+  SctpDataMediaChannel* channel = 0;

[tommi, 2015/08/28 11:44:04]

= nullptr;

[lally1, 2015/08/28 17:54:48]

Done.

+  naddrs = usrsctp_getladdrs(sock, 0, &addrs);
+  if (naddrs >= 0) {

[tommi, 2015/08/28 11:44:04]

nit: what about bailing out in case of errors and declare the other variables
closer to where they're needed:

struct sockaddr* addrs = nullptr;
int naddrs = usrsctp_getladdrs(sock, 0, &addrs);
if (naddrs < 0)
  return nullptr;  // log an error?

SctpDataMediaChannel* channel = nullptr;
if (naddrs > 0) {
  ...
}

usrsctp_freeladdrs(addrs);

return channel;

[lally1, 2015/08/28 17:54:49]

Done.

+    if (naddrs > 0) {
+      if (addrs[0].sa_family == AF_CONN) {

[tommi, 2015/08/28 11:44:04]

I'm not familiar with what the returned order will always be, so just checking
if [0] is always the right addrs to check?

[lally1, 2015/08/28 17:54:48]

Documented.

+        struct sockaddr_conn* sconn = (struct sockaddr_conn*) &addrs[0];

[tommi, 2015/08/28 11:44:04]

here and elsewhere, use C++ casts
(reinterpret_cast or static_cast depending on what's appropriate)

[lally1, 2015/08/28 17:54:49]

Done.

+        channel = (SctpDataMediaChannel*) sconn->sconn_addr;
+      }
+    }
+    usrsctp_freeladdrs(addrs);
+  }
+
+  return channel;
+}
+
+
+// static
+int SctpDataEngine::SendThresholdCallback(struct socket* sock,
+                                          uint32_t sb_free) {
+  SctpDataMediaChannel* channel = GetChannelFromSocket(sock);

[tommi, 2015/08/28 11:44:04]

Add:

DCHECK(sock);

or better yet, do it in GetChannelFromSocket()

[lally1, 2015/08/28 17:54:48]

DCHECKs added to GetChannelFromSocket.

+  if (!channel) {
+    LOG(LS_ERROR) << "SendThresholdCallback: Failed to get channel for socket "

[tommi, 2015/08/28 11:44:04]

you might want to defer this logging to GetChannelFromSocket which will have a
better idea as to why it failed.

[lally1, 2015/08/28 17:54:48]

Acknowledged.

+                  << sock;
+  }
+  channel->SignalReadyToSend(true);

[tommi, 2015/08/28 11:44:04]

Another thing... here we're firing a signal for the channel.  On what thread are
we now?  Is it guaranteed to be always the same thread?

In order to know a bit more about the threading model of the class and be able
to have thread guarantees, what about this:

- Make SendThresholdCallback call a private, OnSendThresholdCallback() method
that is a non-static, private member function.
- In OnSendThresholdCallback(), we call SignalReadyToSend(true), but before
doing so, we add a DCHECK using a thread checker.  This way we'll know a little
bit more about the context in which we're running.

[tommi, 2015/08/28 11:44:04]

gaa.... here you might be dereferencing a nullptr

[lally1, 2015/08/28 17:54:48]

We have the appropriate rtc::Thread as a member, so I checked against that
directly instead of using a thread checker.

[lally1, 2015/08/28 17:54:48]

Good catch.

+  return 0;
+}
+
 SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread)
     : worker_thread_(thread),
       local_port_(kSctpDefaultPort),
       remote_port_(kSctpDefaultPort),
       sock_(NULL),
       sending_(false),
       receiving_(false),
       debug_name_("SctpDataMediaChannel") {
 }
 
 SctpDataMediaChannel::~SctpDataMediaChannel() {
   CloseSctpSocket();
 }
 
 sockaddr_conn SctpDataMediaChannel::GetSctpSockAddr(int port) {
   sockaddr_conn sconn = {0};
   sconn.sconn_family = AF_CONN;
 #ifdef HAVE_SCONN_LEN
   sconn.sconn_len = sizeof(sockaddr_conn);
 #endif
   // Note: conversion from int to uint16_t happens here.
   sconn.sconn_port = rtc::HostToNetwork16(port);
   sconn.sconn_addr = this;
   return sconn;
 }
 
 bool SctpDataMediaChannel::OpenSctpSocket() {

[tommi, 2015/08/28 11:44:04]

as a general comment, since these methods do not have locks to protect the
member variables, it would be good to have thread checks to protect us from
incorrect usage and/or regressions.

   if (sock_) {
     LOG(LS_VERBOSE) << debug_name_
                     << "->Ignoring attempt to re-create existing socket.";
     return false;
   }
+
+  // If kSendBufferSize isn't reflective of reality, we log an error, but we
+  // still have to do something reasonable here.  Look up what the buffer's
+  // real size is and set our threshold to something reasonable.
+  const static int kSendThreshold = usrsctp_sysctl_get_sctp_sendspace() / 2;
+
   sock_ = usrsctp_socket(AF_CONN, SOCK_STREAM, IPPROTO_SCTP,
-                         cricket::OnSctpInboundPacket, NULL, 0, this);
+                         cricket::OnSctpInboundPacket,
+                         &SctpDataEngine::SendThresholdCallback,
+                         kSendThreshold, this);
   if (!sock_) {
     LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to create SCTP socket.";
     return false;
   }
 
   // Make the socket non-blocking. Connect, close, shutdown etc will not block
   // the thread waiting for the socket operation to complete.
   if (usrsctp_set_non_blocking(sock_, 1) < 0) {
     LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to set SCTP to non blocking.";
     return false;
@@ skipping 24 matching lines @@
 
   // Nagle.
   uint32_t nodelay = 1;
   if (usrsctp_setsockopt(sock_, IPPROTO_SCTP, SCTP_NODELAY, &nodelay,
                          sizeof(nodelay))) {
     LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to set SCTP_NODELAY.";
     return false;
   }
 
   // Disable MTU discovery
-  struct sctp_paddrparams params = {{0}};
+  struct sctp_paddrparams params;

[tommi, 2015/08/28 11:44:03]

nit: omit 'struct'

[lally1, 2015/08/28 17:54:48]

Done.

+  memset(&params, 0, sizeof(params));

[tommi, 2015/08/28 11:44:04]

if memset is doing more than = {}, then that suggests a compiler bug.  I think
this should be done the way it was previously (it's a part of the C++
specification that the compiler must initialize the struct to zero in such
cases), so if you have more information about what exactly it was that went
wrong, it would be good to know.

[lally1, 2015/08/28 17:54:48]

Done.  I can't remember where it happened anymore, so I'll take it out.

   params.spp_assoc_id = 0;
   params.spp_flags = SPP_PMTUD_DISABLE;
   params.spp_pathmtu = kSctpMtu;
   if (usrsctp_setsockopt(sock_, IPPROTO_SCTP, SCTP_PEER_ADDR_PARAMS, &params,
       sizeof(params))) {
     LOG_ERRNO(LS_ERROR) << debug_name_
                         << "Failed to set SCTP_PEER_ADDR_PARAMS.";
     return false;
   }
 
@@ skipping 490 matching lines @@
 }
 
 bool SctpDataMediaChannel::SetRecvCodecs(const std::vector<DataCodec>& codecs) {
   return GetCodecIntParameter(
       codecs, kGoogleSctpDataCodecId, kGoogleSctpDataCodecName, kCodecParamPort,
       &local_port_);
 }
 
 void SctpDataMediaChannel::OnPacketFromSctpToNetwork(
     rtc::Buffer* buffer) {
-  if (buffer->size() > kSctpMtu) {
+  // usrsctp seems to interpret the MTU we give it strangely -- it seems to
+  // give us back packets bigger than that MTU, if only by a fixed amount.
+  // This is that amount that we've observed.
+  const int kSctpOverhead = 76;
+  if (buffer->size() > (kSctpOverhead + kSctpMtu)) {
     LOG(LS_ERROR) << debug_name_ << "->OnPacketFromSctpToNetwork(...): "
                   << "SCTP seems to have made a packet that is bigger "
-                     "than its official MTU.";
+                  << "than its official MTU: " << buffer->size()
+                  << " vs max of " << kSctpMtu
+                  << " even after adding " << kSctpOverhead
+                  << " extra SCTP overhead";
   }
   MediaChannel::SendPacket(buffer);
 }
 
 bool SctpDataMediaChannel::SendQueuedStreamResets() {
   if (!sent_reset_streams_.empty() || queued_reset_streams_.empty())
     return true;
 
   LOG(LS_VERBOSE) << "SendQueuedStreamResets[" << debug_name_ << "]: Sending ["
                   << ListStreams(queued_reset_streams_) << "], Open: ["
@@ skipping 41 matching lines @@
     }
     case MSG_SCTPOUTBOUNDPACKET: {
       rtc::scoped_ptr<OutboundPacketMessage> pdata(
           static_cast<OutboundPacketMessage*>(msg->pdata));
       OnPacketFromSctpToNetwork(pdata->data().get());
       break;
     }
   }
 }
 }  // namespace cricket