Bug 1139082 - Refactor

Bug 1139082 - Refactor

[ekr-rietveld, 2015-03-03 22:41:45]

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
File external_tests/ssl_gtest/databuffer.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:22: explicit DataBuffer(const DataBuffer&
other) : data_(nullptr), len_(0) {
If you are going to write a copy ctor, please either write operator= or delete
it.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:41: if (index + count > len()) {
prefer len_

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:52: bool WriteVariable(size_t index, const
DataBuffer& buf, size_t len_size) {
I suggest that these fxns go in tls_parser.cc since they encode TLS semantics.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:56: // Write an integer, also performing
host-to-network order conversion.
Whitespace before this function please.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:63: return Write(index, addr +
sizeof(uint32_t) - count, count);
This code seems kind of confusing. Why not just do a for loop instead?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:71: void Splice(const uint8_t* ins, size_t
ins_len, size_t index, size_t remove = 0) {
Space between functions.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:72: if (index + remove > len()) {
len_

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:73: return; // ERROR!
Suggest assert here or have this return error.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:76: uint8_t* old_value = data_;
This seems like it can silently leave things in a very message up situation,
e.g., if ins_len > len. I would suggest more error checking.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:93: bool empty() const { return len_ == 0;
}
oops.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:109: stream << std::hex <<
std::setfill('0') << std::setw(2)
Is there a reason not to put these inserters outside the main loop?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/test...
File external_tests/ssl_gtest/test_io.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/test...
external_tests/ssl_gtest/test_io.cc:13: #include "prlog.h"
Why did you reorder these? I generally try to maintain alphabetical order per
Google style guidelines.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/test...
external_tests/ssl_gtest/test_io.cc:36: PR_ASSERT(offset() + delta <= len());
Let's make this do something sensible even if assert is off, just on general
principles.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/test...
external_tests/ssl_gtest/test_io.cc:39: size_t offset() const { return offset_;
}
Whitespace before.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/test...
external_tests/ssl_gtest/test_io.cc:336: static_cast<size_t>(length));
Fix indent

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/test...
File external_tests/ssl_gtest/test_io.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/test...
external_tests/ssl_gtest/test_io.h:41: ~DummyPrSocket() { delete filter_; }
Old bug, but let's fix the leak here of input_

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:1: /* -*- Mode: C++; tab-width: 8;
indent-tabs-mode: nil; c-basic-offset: 2 -*- */
Need to review.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:54: CERT_DestroyCertificate(cert);
Why not move these abouve the SNI hook?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:135: &chosen_len, sizeof(chosen_len));
BUG: This should be sizeof(chosen), I believe.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:146: ASSERT_EQ(SECSuccess,
SSL_SetSRTPCiphers(ssl_fd_, ciphers, sizeof(ciphers)));
BUG: PR_ARRAY_SIZE(ciphers)

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:190: void
TlsAgent::ConfigureSessionCache(SessionResumptionMode mode) {
Do we need both this and the individual settings?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:84: State state() const { return state_; }
Since we're in here, let's move the inlines to all be consecutive.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:118: std::vector<uint8_t> GetSessionId() {
What was I thinking? This should be called session_id() and also be const.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:105: void
TlsConnectTestBase::ConnectAndFail() {
Suggest we name "ConnectExpectFail()"

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:127: int resume_ct = expected != 0;
This seems like it would be better with a ?: rather than relying on true == 1.

Yes, I assume I wrote it.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_connect.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.h:27: void Reset();
Now that these are not inline, let's have some comments.

The former two are magic gtest stuff. The later ones are internal.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.h:36: SessionResumptionMode server);
Fix comment.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_filter.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_filter.cc:13: bool TlsRecordFilter::Filter(const
DataBuffer& input, DataBuffer* output) {
I think this code would be a lot cleaner if it copied as it went rather than
rewriting effectively in place.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_filter.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_filter.h:18: // Filter that operates on entire
(D)TLS records.
Please comment that this is an abstract base class.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_filter.h:35: // record how many packets were
altered
Filtered or altered?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_filter.h:68: const DataBuffer& buffer() { return
buffer_; }
Is this const?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_filter.h:76: // fatal alert will be saved over a
warning.
I am not following this comment. Please rephrase.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_filter.h:77: class TlsAlertRecorder : public
TlsRecordFilter {
Ugh. Recorder/Record. Not sure what to do about that.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_parser.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_parser.cc:20: bool TlsParser::Read(uint32_t* val,
size_t len) {
s/len/size/

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_parser.cc:45: memcpy(val->data(), ptr(), len);
What are the semantics here? Merge the buffers? Perhaps comment in the header?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_parser.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_parser.h:23: const uint8_t kTlsHandshakeServerHello
= 0x04;
RFC2246 says: server_hello(2)

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_parser.h:48: inline uint16_t
NormalizeTlsVersion(uint16_t version) {
Space please.

[mt, 2015-03-03 23:45:48]

I fixed all the things you pointed out aside from one ugh regarding names
(recorder vs record).

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
File external_tests/ssl_gtest/databuffer.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:63: return Write(index, addr +
sizeof(uint32_t) - count, count);
On 2015/03/03 22:41:43, ekr-webrtc wrote:
> This code seems kind of confusing. Why not just do a for loop instead?

Now that I'm expanding the allocation on a write that runs off the end, the for
loop would have to take that into account.  This isn't so bad, is it?

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:73: return; // ERROR!
On 2015/03/03 22:41:43, ekr-webrtc wrote:
> Suggest assert here or have this return error.

In line with the changes to Write, I'm going to have this expand the buffer as
needed.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:76: uint8_t* old_value = data_;
On 2015/03/03 22:41:43, ekr-webrtc wrote:
> This seems like it can silently leave things in a very message up situation,
> e.g., if ins_len > len. I would suggest more error checking.

Maybe you should look at the new code.  I've done all the checking on paper and
the checks here were sufficient.  ins_len > len is perfectly OK (and I've used
that before).

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:109: stream << std::hex <<
std::setfill('0') << std::setw(2)
On 2015/03/03 22:41:43, ekr-webrtc wrote:
> Is there a reason not to put these inserters outside the main loop?

Probably, but I tried the obvious method and at least setw fails badly, so I
left them here.  Let the compiler worry about niceties like performance.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_filter.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_filter.cc:13: bool TlsRecordFilter::Filter(const
DataBuffer& input, DataBuffer* output) {
On 2015/03/03 22:41:44, ekr-webrtc wrote:
> I think this code would be a lot cleaner if it copied as it went rather than
> rewriting effectively in place.

Done.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_parser.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_parser.cc:45: memcpy(val->data(), ptr(), len);
On 2015/03/03 22:41:45, ekr-webrtc wrote:
> What are the semantics here? Merge the buffers? Perhaps comment in the header?

Actually, I should just be using Assign here.

[ekr-rietveld, 2015-03-04 01:07:04]

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
File external_tests/ssl_gtest/databuffer.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:41: if (index + count > len()) {
On 2015/03/03 22:41:43, ekr-webrtc wrote:
> prefer len_

Still outstanding.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:1: /* -*- Mode: C++; tab-width: 8;
indent-tabs-mode: nil; c-basic-offset: 2 -*- */
On 2015/03/03 22:41:44, ekr-webrtc wrote:
> Need to review.

Done.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_connect.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.h:27: void Reset();
On 2015/03/03 22:41:44, ekr-webrtc wrote:
> Now that these are not inline, let's have some comments.
> 
> The former two are magic gtest stuff. The later ones are internal.

Still pending

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.h:36: SessionResumptionMode server);
On 2015/03/03 22:41:44, ekr-webrtc wrote:
> Fix comment.

This should say "fix indent" and it should be fixed.

https://codereview.appspot.com/210160043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_filter.cc (right):

https://codereview.appspot.com/210160043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.cc:36: output->Write(output_offset,
input.data() + start, header_len);
suggest instead parser.consumed() - start.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/databuffer.h (right):

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:25: Assign(other.data(), other.len());
Aren't you supposed to be checking that &other != this?

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:31: return *this;
Check that &other != this

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:57: memset(static_cast<void*>(tmp + len_),
0, index - len_);
If it were me, I would forbid writing when index > len_. Let people write 0s if
that's what they want.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:88: size_t tail_len = old_len -
std::min(old_len, index + remove);
So I can remove |remove| > old_len?

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:95: }
Again, I would forbid this.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/test_io.cc (right):

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:37: PR_ASSERT(offset_ + delta <= len());
len_

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:38: offset_ = std::min(len(), offset_ +
delta);
len_

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:356: LOG("Wrote: " << packet);
I actually logged logging the Filtered/Wrote

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_filter.cc (right):

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.cc:36: output->Write(output_offset,
input.data() + start, header_len);
suggest header_len = parser.consumed()

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.cc:107: size_t header_len = IsDtls(version)
? 12 : 4;
suggest header_len = parser.consumed()

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.cc:164: if (IsDtls(version)) {
Potential BUG: You don't seem to be copying the message sequence #.

Also, if you take my advice about not writing implicit 0s, you need to write a 0
fragment offset.

[mt, 2015-03-04 19:18:18]

All fixed up now.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:135: &chosen_len, sizeof(chosen_len));
On 2015/03/03 22:41:44, ekr-webrtc wrote:
> BUG: This should be sizeof(chosen), I believe.

Done.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:146: ASSERT_EQ(SECSuccess,
SSL_SetSRTPCiphers(ssl_fd_, ciphers, sizeof(ciphers)));
On 2015/03/03 22:41:44, ekr-webrtc wrote:
> BUG: PR_ARRAY_SIZE(ciphers)

Done.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:190: void
TlsAgent::ConfigureSessionCache(SessionResumptionMode mode) {
On 2015/03/03 22:41:44, ekr-webrtc wrote:
> Do we need both this and the individual settings?

That's your code, your call.

[mt, 2015-03-04 20:06:49]

I think that I got the last batch.  I'm not sure that we'll ever be happy with
the databuffer stuff.  If I wanted to do this properly, the API would look quite
different to what you see there: basic buffers would be immutable to start with,
but that's hard work to integrate everywhere.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
File external_tests/ssl_gtest/databuffer.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/data...
external_tests/ssl_gtest/databuffer.h:41: if (index + count > len()) {
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> On 2015/03/03 22:41:43, ekr-webrtc wrote:
> > prefer len_
> 
> Still outstanding.

Done.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_connect.h (right):

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.h:27: void Reset();
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> On 2015/03/03 22:41:44, ekr-webrtc wrote:
> > Now that these are not inline, let's have some comments.
> > 
> > The former two are magic gtest stuff. The later ones are internal.
> 
> Still pending

Done.

https://codereview.appspot.com/210160043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.h:36: SessionResumptionMode server);
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> On 2015/03/03 22:41:44, ekr-webrtc wrote:
> > Fix comment.
> 
> This should say "fix indent" and it should be fixed.

Done.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/databuffer.h (right):

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:57: memset(static_cast<void*>(tmp + len_),
0, index - len_);
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> If it were me, I would forbid writing when index > len_. Let people write 0s
if
> that's what they want.

See below regarding where you take the hit.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:88: size_t tail_len = old_len -
std::min(old_len, index + remove);
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> So I can remove |remove| > old_len?

This allows removal of stuff that doesn't exist, yes.  That's idempotent.

I'm not 100% happy with this, particularly since it is a little hard to follow,
but I'd rather address that with refactoring.  Right now, I'll address it with
comments.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:95: }
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> Again, I would forbid this.

Forbidding has the consequence of forcing a check on the return code. 

I figured that taking a hit here was easier than taking a hit everywhere.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/test_io.cc (right):

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:38: offset_ = std::min(len(), offset_ +
delta);
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> len_

...is private.  len() is all that can be seen.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:356: LOG("Wrote: " << packet);
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> I actually logged logging the Filtered/Wrote

Done.

[ekr-rietveld, 2015-03-04 20:20:01]

LGTM

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/databuffer.h (right):

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:25: Assign(other.data(), other.len());
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> Aren't you supposed to be checking that &other != this?

Done.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/databuffer.h:31: return *this;
On 2015/03/04 01:07:03, ekr-webrtc wrote:
> Check that &other != this

Done.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/test_io.cc (right):

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:38: offset_ = std::min(len(), offset_ +
delta);
On 2015/03/04 20:06:49, mt wrote:
> On 2015/03/04 01:07:03, ekr-webrtc wrote:
> > len_
> 
> ...is private.  len() is all that can be seen.

Acknowledged.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:356: LOG("Wrote: " << packet);
On 2015/03/04 20:06:49, mt wrote:
> On 2015/03/04 01:07:03, ekr-webrtc wrote:
> > I actually logged logging the Filtered/Wrote
> 
> Done.

Acknowledged.

https://codereview.appspot.com/210160043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/test_io.cc:356: LOG("Wrote: " << packet);
On 2015/03/04 20:06:49, mt wrote:
> On 2015/03/04 01:07:03, ekr-webrtc wrote:
> > I actually logged logging the Filtered/Wrote
> 
> Done.

What I was suggesting was logging the before and after both at the LOG level.
But I can live with this.

https://codereview.appspot.com/210160043/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/tls_filter.cc (right):

https://codereview.appspot.com/210160043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_filter.cc:162: // Back up and overwrite the (two)
length field(s) that appear: the handshake
s/that appear/that may have changed/?