Add QuicFramer and friends.

net/quic/crypto/crypto_framer.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/quic/crypto/crypto_framer.h"
+
+#include "net/quic/quic_data_reader.h"
+#include "net/quic/quic_data_writer.h"
+#include "net/quic/quic_protocol.h"
+
+using base::StringPiece;
+
+namespace net {
+
+CryptoFramer::CryptoFramer()
+    : visitor_(NULL) {
+  Clear();
+}
+
+CryptoFramer::~CryptoFramer() {}
+
+bool CryptoFramer::ProcessInput(StringPiece input) {
+  DCHECK_EQ(QUIC_NO_ERROR, error_);
+  if (error_ != QUIC_NO_ERROR) {
+    return false;
+  }

[jar (doing other things), 2012/10/14 23:04:38]

I can live with (and think the network stack may have originally had this
policy)...  Do you want all if statements to have curlies?  even one-liners??

[Ryan Hamilton, 2012/10/15 21:22:08]

This is required Server-side style.  We have the same issue with the shared SPDY
code.  Minimizing diffs between Chrome and Server-side code is more important
than preserving this style in the network stack, at least in my experience
maintaining this shared code.

+  // Add this data to the buffer.
+  buffer_.append(input.data(), input.length());
+  reader_.reset(new QuicDataReader(buffer_.data(), buffer_.length()));
+
+  switch (state_) {
+    case STATE_READING_TAG:
+      if (reader_->BytesRemaining() < sizeof(uint32)) {

[jar (doing other things), 2012/10/14 23:04:38]

nit: better would be sizeof(message_tag_)

[Ryan Hamilton, 2012/10/15 21:22:08]

As per my C++ readability review, these sizeofs will change when I land
cl/35411506. 

+        break;

[jar (doing other things), 2012/10/14 23:04:38]

Should this check for BytesRemaining == 0, and return false if not? .... or do
we have a stream with an unaligned end?

[Ryan Hamilton, 2012/10/15 21:22:08]

I don't understand what this check would be doing?  BytesRemaing() == 0 does not
mean that we're at the end of the message, it just means we have not received
any more data.  But in order to have a valid method, more data better show up
later.

+      }
+      reader_->ReadUInt32(&message_tag_);
+      state_ = STATE_READING_NUM_ENTRIES;
+    case STATE_READING_NUM_ENTRIES:
+      if (reader_->BytesRemaining() < sizeof(uint16)) {

[jar (doing other things), 2012/10/14 23:04:38]

nit: sizeof(num_entries_)

+        break;
+      }
+      reader_->ReadUInt16(&num_entries_);
+      if (num_entries_ > kMaxEntries) {
+        error_ = QUIC_CRYPTO_TOO_MANY_ENTRIES;
+        return false;
+      }
+      state_ = STATE_READING_KEY_TAGS;
+    case STATE_READING_KEY_TAGS:
+      if (reader_->BytesRemaining() < num_entries_ * sizeof(uint32)) {
+        break;
+      }
+      for (int i = 0; i < num_entries_; ++i) {
+        CryptoTag tag;
+        reader_->ReadUInt32(&tag);
+        if (i > 0 && tag <= tags_.back()) {
+          error_ = QUIC_CRYPTO_TAGS_OUT_OF_ORDER;
+          return false;
+        }
+        tags_.push_back(tag);
+      }
+      state_ = STATE_READING_LENGTHS;
+    case STATE_READING_LENGTHS:
+      if (reader_->BytesRemaining() < num_entries_ * sizeof(uint16)) {

[jar (doing other things), 2012/10/14 23:04:38]

nit: it is always suggested that we use sizeof(variable) rather than
sizeof(typename).  

Could you consider moving line 68 out of the loop, and in front of line 63, so
that we can do sizeof(len)?

Same thing with line 53 moving up for use in line 49?

+        break;
+      }
+      values_len_ = 0;
+      for (int i = 0; i < num_entries_; ++i) {
+        uint16 len;
+        reader_->ReadUInt16(&len);
+        tag_length_map_[tags_[i]] = len;
+        values_len_ += len;
+        if (len == 0 && i != num_entries_ - 1) {
+          error_ = QUIC_CRYPTO_INVALID_VALUE_LENGTH;
+          return false;
+        }
+      }
+      state_ = STATE_READING_VALUES;
+    case STATE_READING_VALUES:
+      if (reader_->BytesRemaining() < values_len_) {
+        break;
+      }
+      for (int i = 0; i < num_entries_; ++i) {
+        StringPiece value;
+        reader_->ReadStringPiece(&value, tag_length_map_[tags_[i]]);
+        tag_value_map_[tags_[i]] = value;

[jar (doing other things), 2012/10/14 23:04:38]

Since StringPiece has a length, why do we want to keep a separate
tag_length_map?

[Ryan Hamilton, 2012/10/15 21:22:08]

I'm not sure the best way to make use of that.   (And I think wtc is going to
change the wire format so this might change).  First we have a list of uint16
(lengths).  We need to store these so that when we process the value, we know
how much to read.  Are you proposing that when we read the lengths, we create a
string with length "N" and then we later overwrite that with a new string whose
values were found in the message?  That feels worse.  

I think wtc is going to change this to <len><value>* which will make the whole
issue largely moot.

+      }
+      CryptoHandshakeMessage message;
+      message.tag = message_tag_;
+      message.tag_value_map.swap(tag_value_map_);
+      visitor_->OnHandshakeMessage(message);
+      Clear();
+      state_ = STATE_READING_TAG;
+      break;
+  }
+  // Save any left over data
+  buffer_ = reader_->PeekRemainingPayload().as_string();

[jar (doing other things), 2012/10/14 23:04:38]

Is it valuable to preserve the reader_ here?  It almost seems like it could have
been a local variable (or pointed at via a scoped_ptr), and been discarded.

[Ryan Hamilton, 2012/10/15 21:22:08]

Changed to a local variable.

+  return true;
+}
+
+bool CryptoFramer::ConstructHandshakeMessage(
+    const CryptoHandshakeMessage& message,
+    QuicData** packet) {
+  size_t len = sizeof(uint32);  // message tag

[jar (doing other things), 2012/10/14 23:04:38]

nit: Comments are sentences with Capital letters starting, and period at the
end.

+  len += sizeof(uint16);  // number of map entries

[jar (doing other things), 2012/10/14 23:04:38]

nit: declare/init as close to use as possible.  Move lines 103-104 to after 107.

[Ryan Hamilton, 2012/10/15 21:22:08]

Done.

+  if (message.tag_value_map.size() > kMaxEntries) {
+    return false;
+  }
+  CryptoTagValueMap::const_iterator it = message.tag_value_map.begin();
+  while (it != message.tag_value_map.end()) {
+    len += sizeof(uint32);  // tag
+    len += sizeof(uint16);  // value len

[jar (doing other things), 2012/10/14 23:04:38]

nit: how about using members, rather than types in most of these sizeof() calls?

+    len += it->second.length(); // value
+    if (it->second.length() == 0) {
+      return false;
+    }
+    ++it;
+  }
+  if (message.tag_value_map.size() % 2 == 1) {
+    len += sizeof(uint16);  // padding
+  }
+
+  QuicDataWriter writer(len);
+  CHECK(writer.WriteUInt32(message.tag));
+  CHECK(writer.WriteUInt16(message.tag_value_map.size()));
+  // Tags
+  for (it = message.tag_value_map.begin();
+       it != message.tag_value_map.end(); ++it) {
+    CHECK(writer.WriteUInt32(it->first));
+  }
+  // Lengths
+  for (it = message.tag_value_map.begin();
+       it != message.tag_value_map.end(); ++it) {
+    CHECK(writer.WriteUInt16(it->second.length()));
+  }
+  // Possible padding
+  if (message.tag_value_map.size() % 2 == 1) {
+    CHECK(writer.WriteUInt16(0xABAB));
+  }
+  // Values
+  for (it = message.tag_value_map.begin();
+       it != message.tag_value_map.end(); ++it) {
+    CHECK(writer.WriteBytes(it->second.data(), it->second.length()));
+  }
+  *packet = new QuicData(writer.take(), len, true);
+  return true;
+}
+
+void CryptoFramer::Clear() {
+  tag_value_map_.clear();
+  tag_length_map_.clear();
+  tags_.clear();
+  error_ = QUIC_NO_ERROR;
+  state_ = STATE_READING_TAG;

[jar (doing other things), 2012/10/14 23:04:38]

Why is reader_ not discarded here?  ...but perhaps it shouldn't be saved ever as
mentioned above.

+}
+
+}  // namespace net