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

talk/media/sctp/sctpdataengine.cc

Index: talk/media/sctp/sctpdataengine.cc
diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc
index 8c8a6a192fa25833c3de66c0587b9ed7ed3bccb3..8164d1fd187d57f6a7c982e4ce1def68dd11cbcd 100644
--- a/talk/media/sctp/sctpdataengine.cc
+++ b/talk/media/sctp/sctpdataengine.cc
@@ -109,6 +109,8 @@ typedef rtc::ScopedMessageData<rtc::Buffer> OutboundPacketMessage;
 // 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
@@ -177,11 +179,11 @@ static bool GetDataMediaType(
 }
 
 // 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);
     }
@@ -258,6 +260,11 @@ SctpDataEngine::SctpDataEngine() {
     // 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
@@ -315,6 +322,39 @@ DataMediaChannel* SctpDataEngine::CreateChannel(
   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),
@@ -347,8 +387,16 @@ bool SctpDataMediaChannel::OpenSctpSocket() {
                     << "->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;
@@ -393,7 +441,8 @@ bool SctpDataMediaChannel::OpenSctpSocket() {
   }
 
   // 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;
@@ -904,10 +953,17 @@ bool SctpDataMediaChannel::SetRecvCodecs(const std::vector<DataCodec>& codecs) {
 
 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);
 }