Voozh

👁 Image
Chromium Code Reviews

(125)

Issues Search

My Issues | Starred Open | Closed | All

Side by Side Diff: net/quic/crypto/crypto_framer.cc

👁 Image
Issue 11125002: Add QuicFramer and friends. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: narrowing in Created 8 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

OLD	NEW
(Empty)
// 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 h 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 s Show quoted text On 2012/10/14 23:04:38, jar wrote: > 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?? 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_) nit: better would be sizeof(message_tag_) Ryan Hamilton 2012/10/15 21:22:08 As per my C++ readability review, these sizeofs wi Show quoted text On 2012/10/14 23:04:38, jar wrote: > nit: better would be sizeof(message_tag_) 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 ret 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? Show quoted text On 2012/10/14 23:04:38, jar wrote: > Should this check for BytesRemaining == 0, and return false if not? .... or do > we have a stream with an unaligned end? 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_) 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(var 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 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. ( Show quoted text On 2012/10/14 23:04:38, jar wrote: > Since StringPiece has a length, why do we want to keep a separate > tag_length_map? 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 a 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. Show quoted text On 2012/10/14 23:04:38, jar wrote: > 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. 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 s 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. Mo 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. Show quoted text On 2012/10/14 23:04:38, jar wrote: > nit: declare/init as close to use as possible. Move lines 103-104 to after 107. 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 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 Why is reader_ not discarded here? ...but perhaps it shouldn't be saved ever as mentioned above.
}
} // namespace net
OLD	NEW

« net/quic/crypto/crypto_framer.h ('K') | « net/quic/crypto/crypto_framer.h ('k') | net/quic/crypto/crypto_framer_test.cc » ('j') | net/quic/crypto/crypto_framer_test.cc » ('J')

👁 Powered by Google App Engine

This is Rietveld 408576698

URL: https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_framer.cc

⇱ net/quic/crypto/crypto_framer.cc - Issue 11125002: Add QuicFramer and friends. - Code Review

Side by Side Diff: net/quic/crypto/crypto_framer.cc