Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1304063006:
(try 2) Added send-thresholding and fixed text packet dumping (Closed)
DescriptionAdded send-thresholding and fixed text packet dumping. Also a little squelch for the over-max-MTU log spam we see in there.
BUG=https://code.google.com/p/webrtc/issues/detail?id=4468
R=pthatcher@chromium.org, tommi@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/e8386d21992b6683b6fadd315ea631875b4256fb
Patch Set 1 : Post-revert fixes. No more static vars. #
Total comments: 33
Patch Set 2 : Post tommi@ comments. #
Total comments: 17
Patch Set 3 : Simplification of GetChannelFromSocket, and a dcheck moved. #
Messages
Total messages: 14 (2 generated)
Patchset #1 (id:1) has been deleted
lally@webrtc.org changed reviewers: + bemasc@webrtc.org, pthatcher@chromium.org, pthatcher@webrtc.org, thakis@chromium.org, tommi@webrtc.org
Hi, moved from https://codereview.webrtc.org/1266033005/
Thanks Lally. Took a look and have some suggestions questions (and found one bug). https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:265: LOG(LS_ERROR) << "Got different send size than expected: " << send_size; is it still ok to continue? what are the implications? https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:328: struct sockaddr* addrs; = nullptr; https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:329: int naddrs; declare where needed. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:330: SctpDataMediaChannel* channel = 0; = nullptr; https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:332: if (naddrs >= 0) { 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; https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:334: if (addrs[0].sa_family == AF_CONN) { I'm not familiar with what the returned order will always be, so just checking if [0] is always the right addrs to check? https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:335: struct sockaddr_conn* sconn = (struct sockaddr_conn*) &addrs[0]; here and elsewhere, use C++ casts (reinterpret_cast or static_cast depending on what's appropriate) https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:349: SctpDataMediaChannel* channel = GetChannelFromSocket(sock); Add: DCHECK(sock); or better yet, do it in GetChannelFromSocket() https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:351: LOG(LS_ERROR) << "SendThresholdCallback: Failed to get channel for socket " you might want to defer this logging to GetChannelFromSocket which will have a better idea as to why it failed. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:354: channel->SignalReadyToSend(true); gaa.... here you might be dereferencing a nullptr https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:354: channel->SignalReadyToSend(true); 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. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:384: bool SctpDataMediaChannel::OpenSctpSocket() { 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. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:444: struct sctp_paddrparams params; nit: omit 'struct' https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:445: memset(¶ms, 0, sizeof(params)); 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. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:102: static int SendThresholdCallback(struct socket* sock, uint32_t sb_free); nit: lose "struct" https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:108: static SctpDataMediaChannel* GetChannelFromSocket(struct socket* sock); ditto https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:197: struct socket* socket() const { return sock_; } here too. This is a const method that returns a non-const pointer to an internal member variable. We don't do this usually, so if this method doesn't really need to be const, it shouldn't be.
https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:265: LOG(LS_ERROR) << "Got different send size than expected: " << send_size; On 2015/08/28 11:44:04, tommi (webrtc) wrote: > is it still ok to continue? what are the implications? Harmless check, it's been documented. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:328: struct sockaddr* addrs; On 2015/08/28 11:44:04, tommi (webrtc) wrote: > = nullptr; Done. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:329: int naddrs; On 2015/08/28 11:44:04, tommi (webrtc) wrote: > declare where needed. Done. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:330: SctpDataMediaChannel* channel = 0; On 2015/08/28 11:44:04, tommi (webrtc) wrote: > = nullptr; Done. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:332: if (naddrs >= 0) { On 2015/08/28 11:44:04, tommi (webrtc) wrote: > 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; Done. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:334: if (addrs[0].sa_family == AF_CONN) { On 2015/08/28 11:44:04, tommi (webrtc) wrote: > I'm not familiar with what the returned order will always be, so just checking > if [0] is always the right addrs to check? Documented. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:335: struct sockaddr_conn* sconn = (struct sockaddr_conn*) &addrs[0]; On 2015/08/28 11:44:04, tommi (webrtc) wrote: > here and elsewhere, use C++ casts > (reinterpret_cast or static_cast depending on what's appropriate) Done. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:349: SctpDataMediaChannel* channel = GetChannelFromSocket(sock); On 2015/08/28 11:44:04, tommi (webrtc) wrote: > Add: > > DCHECK(sock); > > or better yet, do it in GetChannelFromSocket() DCHECKs added to GetChannelFromSocket. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:351: LOG(LS_ERROR) << "SendThresholdCallback: Failed to get channel for socket " On 2015/08/28 11:44:04, tommi (webrtc) wrote: > you might want to defer this logging to GetChannelFromSocket which will have a > better idea as to why it failed. Acknowledged. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:354: channel->SignalReadyToSend(true); On 2015/08/28 11:44:04, tommi (webrtc) wrote: > 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. We have the appropriate rtc::Thread as a member, so I checked against that directly instead of using a thread checker. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:354: channel->SignalReadyToSend(true); On 2015/08/28 11:44:04, tommi (webrtc) wrote: > gaa.... here you might be dereferencing a nullptr Good catch. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:444: struct sctp_paddrparams params; On 2015/08/28 11:44:03, tommi (webrtc) wrote: > nit: omit 'struct' Done. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:445: memset(¶ms, 0, sizeof(params)); On 2015/08/28 11:44:04, tommi (webrtc) wrote: > 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. Done. I can't remember where it happened anymore, so I'll take it out. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:102: static int SendThresholdCallback(struct socket* sock, uint32_t sb_free); On 2015/08/28 11:44:04, tommi (webrtc) wrote: > nit: lose "struct" Compiler wouldn't let me. It says "must use 'struct' tag to refer to type 'socket' in this scope". https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:108: static SctpDataMediaChannel* GetChannelFromSocket(struct socket* sock); On 2015/08/28 11:44:04, tommi (webrtc) wrote: > ditto Acknowledged. https://codereview.webrtc.org/1304063006/diff/20001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:197: struct socket* socket() const { return sock_; } On 2015/08/28 11:44:04, tommi (webrtc) wrote: > here too. > > This is a const method that returns a non-const pointer to an internal member > variable. We don't do this usually, so if this method doesn't really need to be > const, it shouldn't be. Made const.
Thanks Lally. lgtm. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:263: // This is harmless, but we should find out when the library default move to error scope? https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:332: DCHECK(naddrs > 0); Generally in chromium (the guidelines webrtc follows), a DCHECK is used to assert and document expectations. The condition in the DCHECK is then not repeated in the production code. The reason for this is that the DCHECK should have caught unexpected situations and if we encounter those unexpected situations out in the field, the code should just crash. So, should we move this DCHECK down below the check in line 333 and remove the if (naddres > 0)? It's up to you, I'm not hung up on it. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:344: reinterpret_cast<struct sockaddr_conn*>(&addrs[0]); nit: 4 space indent https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:358: // and then back here. nice https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:384: DCHECK(rtc::Thread::Current() == worker_thread_); thanks!
https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:332: DCHECK(naddrs > 0); On 2015/08/28 18:12:32, tommi (webrtc) wrote: > Generally in chromium (the guidelines webrtc follows), a DCHECK is used to > assert and document expectations. The condition in the DCHECK is then not > repeated in the production code. The reason for this is that the DCHECK should > have caught unexpected situations and if we encounter those unexpected > situations out in the field, the code should just crash. > > So, should we move this DCHECK down below the check in line 333 and remove the > if (naddres > 0)? > > It's up to you, I'm not hung up on it. On the other hand, there is a ton of code in WebRTC that looks like this: if (naddrs < 0) { ASSERT(false); return nullptr; } And DCHECK is just the rename ASSERT, and so there is already much code that looks like this: if (naddrs < 0) { DCHECK(false); return nullptr; } Peronally, I like it that way.
https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:332: DCHECK(naddrs > 0); On 2015/08/28 18:36:59, pthatcher2 wrote: > On 2015/08/28 18:12:32, tommi (webrtc) wrote: > > Generally in chromium (the guidelines webrtc follows), a DCHECK is used to > > assert and document expectations. The condition in the DCHECK is then not > > repeated in the production code. The reason for this is that the DCHECK > should > > have caught unexpected situations and if we encounter those unexpected > > situations out in the field, the code should just crash. > > > > So, should we move this DCHECK down below the check in line 333 and remove the > > if (naddres > 0)? > > > > It's up to you, I'm not hung up on it. > > On the other hand, there is a ton of code in WebRTC that looks like this: > > if (naddrs < 0) { > ASSERT(false); > return nullptr; > } > > And DCHECK is just the rename ASSERT, and so there is already much code that > looks like this: > > > if (naddrs < 0) { > DCHECK(false); > return nullptr; > } > > Peronally, I like it that way. Actually, in this case, I'd rather not have GetChannelFromSocket have a DCHECK at all. Just return null and let the caller do a DCHECK against being non-null.
https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:337: if (naddrs > 0) { Huh? Why check naddrs so many times? if (naddrs < 0) { return nullptr; } SctpDataMediaChannel* channel = nullptr; if (naddrs > 0) { // ... } return channel; Then it's shorter and less indented. Why not do this? if (naddrs <= 0) { return nullptr; } SctpDataMediaChannel* channel = nullptr; // ... return channel;
lgtm, assuming you fix the nits. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:342: if (addrs[0].sa_family == AF_CONN) { In this case, I'm fine with a DCHECK with no if statement. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:347: usrsctp_freeladdrs(addrs); Can you put a little comment about what this does? https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:675: DCHECK(rtc::Thread::Current() == worker_thread_); Why not just put this at the top of the method?
Thanks, anything else? https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:263: // This is harmless, but we should find out when the library default On 2015/08/28 18:12:32, tommi (webrtc) wrote: > move to error scope? I changed the comment a little (s/happens/changes/), but don't know what you mean here. The log already LS_ERROR, is that what you meant? https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:332: DCHECK(naddrs > 0); On 2015/08/28 18:38:15, pthatcher2 wrote: > On 2015/08/28 18:36:59, pthatcher2 wrote: > > On 2015/08/28 18:12:32, tommi (webrtc) wrote: > > > Generally in chromium (the guidelines webrtc follows), a DCHECK is used to > > > assert and document expectations. The condition in the DCHECK is then not > > > repeated in the production code. The reason for this is that the DCHECK > > should > > > have caught unexpected situations and if we encounter those unexpected > > > situations out in the field, the code should just crash. > > > > > > So, should we move this DCHECK down below the check in line 333 and remove > the > > > if (naddres > 0)? > > > > > > It's up to you, I'm not hung up on it. > > > > On the other hand, there is a ton of code in WebRTC that looks like this: > > > > if (naddrs < 0) { > > ASSERT(false); > > return nullptr; > > } > > > > And DCHECK is just the rename ASSERT, and so there is already much code that > > looks like this: > > > > > > if (naddrs < 0) { > > DCHECK(false); > > return nullptr; > > } > > > > Peronally, I like it that way. > > > Actually, in this case, I'd rather not have GetChannelFromSocket have a DCHECK > at all. Just return null and let the caller do a DCHECK against being non-null. I took out the DCHECKs and merged the if()s a bit. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:337: if (naddrs > 0) { On 2015/08/28 18:40:44, pthatcher2 wrote: > Huh? Why check naddrs so many times? > > if (naddrs < 0) { > return nullptr; > } > SctpDataMediaChannel* channel = nullptr; > if (naddrs > 0) { > // ... > } > return channel; > > > Then it's shorter and less indented. > > Why not do this? > > > if (naddrs <= 0) { > return nullptr; > } > SctpDataMediaChannel* channel = nullptr; > // ... > return channel; Acknowledged. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:342: if (addrs[0].sa_family == AF_CONN) { On 2015/08/28 18:44:49, pthatcher2 wrote: > In this case, I'm fine with a DCHECK with no if statement. Acknowledged. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:344: reinterpret_cast<struct sockaddr_conn*>(&addrs[0]); On 2015/08/28 18:12:32, tommi (webrtc) wrote: > nit: 4 space indent Acknowledged. https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:675: DCHECK(rtc::Thread::Current() == worker_thread_); On 2015/08/28 18:44:49, pthatcher2 wrote: > Why not just put this at the top of the method? Done.
Ah, missed that second LGTM. Thanks everyone. Submitting.
Message was sent while issue was closed.
Committed patchset #3 (id:60001) manually as e8386d21992b6683b6fadd315ea631875b4256fb (presubmit successful).
Message was sent while issue was closed.
On 2015/08/28 18:36:59, pthatcher2 wrote: > https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... > File talk/media/sctp/sctpdataengine.cc (right): > > https://codereview.webrtc.org/1304063006/diff/40001/talk/media/sctp/sctpdatae... > talk/media/sctp/sctpdataengine.cc:332: DCHECK(naddrs > 0); > On 2015/08/28 18:12:32, tommi (webrtc) wrote: > > Generally in chromium (the guidelines webrtc follows), a DCHECK is used to > > assert and document expectations. The condition in the DCHECK is then not > > repeated in the production code. The reason for this is that the DCHECK > should > > have caught unexpected situations and if we encounter those unexpected > > situations out in the field, the code should just crash. > > > > So, should we move this DCHECK down below the check in line 333 and remove the > > if (naddres > 0)? > > > > It's up to you, I'm not hung up on it. > > On the other hand, there is a ton of code in WebRTC that looks like this: > > if (naddrs < 0) { > ASSERT(false); > return nullptr; > } > > And DCHECK is just the rename ASSERT, and so there is already much code that > looks like this: > > > if (naddrs < 0) { > DCHECK(false); > return nullptr; > } > > Peronally, I like it that way. Yes, that's why I'm fine with this as is. In Chromium it's less optional :) I didn't like it at first myself actually but have seen the benefit of that approach several times since then. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
