Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7008021:
Added DnsQuery class (Closed)
|
Created:
9 years, 7 months ago by agayev Modified:
9 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded DnsQuery class
BUG=60149
TEST=dns_query_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87809
Patch Set 1 #
Total comments: 12
Patch Set 2 : Added DnsResponse class #
Total comments: 43
Patch Set 3 : fix issues mentioned by agl #
Total comments: 30
Patch Set 4 : fix issues mentioned by cbentzel #Patch Set 5 : make dnsquery const #Patch Set 6 : make dnsquery const post-creation #Patch Set 7 : make dnsquery const post-creation #Patch Set 8 : get rid of unnecessary include #
Total comments: 18
Patch Set 9 : fixed issues cbentzel mentioned' #
Total comments: 2
Patch Set 10 : fix nit #Patch Set 11 : trying to fix the mess #Patch Set 12 : got rid of iostream #Patch Set 13 : fixed comments, made DnsQuery::io_buffer() const #Patch Set 14 : fix comments #Patch Set 15 : merge with trunk #
Messages
Total messages: 22 (0 generated)
Please see if this makes sense. I will add DnsResponse soon. Ignore the AddressList change that is duplicated here.
Sorry for being so slow. This generally looks like what I would expect. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode42 net/base/dns_query.cc:42: qtype_(kDNS_A), I'd add a function such as QtypeFromAddressFamily() so qtype_ is only initialized once. This can just be in the anonymous namespace. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode54 net/base/dns_query.cc:54: std::vector<char> query(query_size); Why don't you do this directly into the IOBuffer? This is causing additional memory copies [from the vector<char> to the io_buffer_->data()], additional memory allocations/frees, and default constructors. I'm also a bit worried about memcpy'ing directly into the vector's data. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode79 net/base/dns_query.cc:79: id_ = base::RandUint64() & 0xffff; Is this random enough? I haven't looked. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode80 net/base/dns_query.cc:80: PackUint16BE(io_buffer_->data(), id_); You should probably use htons here. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.h#newcode17 net/base/dns_query.h:17: // A class that encapsulates bits and pieces related to DNS request processing. Right now this is A and AAAA only. Do you want to expand this to support arbitrary queries? To allow more than one query? To specify whether recursion is allowed/denied? I'm fine with leaving it simple now, but it might be nice to have general framers and parsers API for any DNS message. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query_unittest.cc File net/base/dns_query_unittest.cc (right): http://codereview.chromium.org/7008021/diff/1/net/base/dns_query_unittest.cc#... net/base/dns_query_unittest.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. Wrong year
Addressed all the issues, also added DnsResponse. Will add more thorough unit tests once we agree on the interface. Thanks. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode42 net/base/dns_query.cc:42: qtype_(kDNS_A), On 2011/05/26 17:32:40, cbentzel wrote: > I'd add a function such as QtypeFromAddressFamily() so qtype_ is only > initialized once. This can just be in the anonymous namespace. Done. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode54 net/base/dns_query.cc:54: std::vector<char> query(query_size); On 2011/05/26 17:32:40, cbentzel wrote: > Why don't you do this directly into the IOBuffer? This is causing additional > memory copies [from the vector<char> to the io_buffer_->data()], additional > memory allocations/frees, and default constructors. I'm also a bit worried about > memcpy'ing directly into the vector's data. Done. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode79 net/base/dns_query.cc:79: id_ = base::RandUint64() & 0xffff; On 2011/05/26 17:32:40, cbentzel wrote: > Is this random enough? I haven't looked. This is the primitive used by all other randomness functions. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.cc#newcode80 net/base/dns_query.cc:80: PackUint16BE(io_buffer_->data(), id_); On 2011/05/26 17:32:40, cbentzel wrote: > You should probably use htons here. No, since I'm doing it big-endian anyway. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/1/net/base/dns_query.h#newcode17 net/base/dns_query.h:17: // A class that encapsulates bits and pieces related to DNS request processing. On 2011/05/26 17:32:40, cbentzel wrote: > Right now this is A and AAAA only. Do you want to expand this to support > arbitrary queries? To allow more than one query? To specify whether recursion is > allowed/denied? We will probably want most of that, but I'd rather do it incrementally. > > I'm fine with leaving it simple now, but it might be nice to have general > framers and parsers API for any DNS message. http://codereview.chromium.org/7008021/diff/1/net/base/dns_query_unittest.cc File net/base/dns_query_unittest.cc (right): http://codereview.chromium.org/7008021/diff/1/net/base/dns_query_unittest.cc#... net/base/dns_query_unittest.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2011/05/26 17:32:40, cbentzel wrote: > Wrong year Done.
Would it make more sense to just have a DnsMessage class instead of separate DnsQuery and DnsResponse? http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode78 net/base/dns_query.cc:78: DCHECK(iterator <= kMaxQuerySize); This shouldn't be a DCHECK - unless there are checks on hostname length from all callers of this function. I also think it would make more sense to do this earlier - like when query_size is calculated and before the io_buffer_ is created, so IsValid() will return false.
+agl I'd expect the following classes/structs for something like this. It's worth looking at dnsrr_resolver to see what we already have. - An RR class. - A bunch of different RDATA classes. - A question class - A message header class. - A DNSMessage class which wraps all of these up. I think it's then OK to have helper functions to generate a query, but I wouldn't have separate query and response classes.
On Fri, May 27, 2011 at 1:33 PM, <cbentzel@chromium.org> wrote: > +agl (note: I'm on vacation until Monday.)
On 2011/05/27 17:33:49, cbentzel wrote: > +agl > > I'd expect the following classes/structs for something like this. It's worth > looking at dnsrr_resolver to see what we already have. > > - An RR class. > - A bunch of different RDATA classes. > - A question class > - A message header class. > - A DNSMessage class which wraps all of these up. > > I think it's then OK to have helper functions to generate a query, but I > wouldn't have separate query and response classes. I took almost everything regarding generation and parsing from dnsrr_resolver, nothing left to take from there. I'm not fond of current design either, but can't think of a nicer interface. DNSMessage sounds a little better, but not terribly nice. Will think about it some more.
http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode20 net/base/dns_query.cc:20: buf[1] = v & 0xff; please flip these two lines. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode25 net/base/dns_query.cc:25: uint16 qtype = kDNS_A; switch (address_family) { case ADDRESS_FAMILY_IPV4: return kDNS_A; case ADDRESS_FAMILY_IPV6: return kDNS_AAAA; default: NOTREACHED() << "Bad address family"; return kDNS_A; } http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode35 net/base/dns_query.cc:35: // DNS query consists of a 12-byte header followed by a question section, s/section,/section./ (and s/for/For/ on the next line) http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode44 net/base/dns_query.cc:44: static const int kHeaderLen = arraysize(kHeader); s/int/size_t/ http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode75 net/base/dns_query.cc:75: static const int kMaxQuerySize = kHeaderLen + 255 + DNSDomainFromDot should fail if the name is too long, rendering this test superfluous. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode34 net/base/dns_query.h:34: uint16 qclass() const; Classes are a really obscure corner of DNS. I'd tend to keep this private unless there's a need for it that I haven't got to in the code yet. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode38 net/base/dns_query.h:38: int size() const; size_t for sizes? http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode42 net/base/dns_query.h:42: IOBuffer* io_buffer(); IOBuffers are refcounted, but returning a raw pointer allows callers to forget this. I'd suggest: void io_buffer(scoped_refptr<IOBuffer>* out); As a way to ensure that nobody ever makes that mistake while still minimizing copy constructors and atomic ops. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode54 net/base/dns_query.h:54: char* data() const; const char*? http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode66 net/base/dns_query.h:66: // Class of query, currently fixed to IN class. This is so unlikely to ever change I wouldn't make it a variable even. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode69 net/base/dns_query.h:69: // Hostname for we are trying to resolve. s/for/that/ http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc File net/base/dns_response.cc (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:14: // RFC 1035, section 4.2.1: Messages carried by UDP are restricted to 512 This is no longer true. The current recommendation is that clients SHOULD be able to cope with 4K replies I believe. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:60: !response.U16(&additional_count)) { } around the body if the conditional is multi-line. (I'm not sure if it's a style rule, so you can ignore this if you like.) http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:74: return false; ditto http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:76: if (answer_count < 1) This isn't a parse error of course. I feel that you would need to distinguish error codes from this function into multiple families: * Invalid packet (dispatcher can stop processing) * Not for me (dispatcher can try to match against other outstanding requests) * For me, but I couldn't get a useful answer (SERVFAIL, NXDOMAIN, no records etc) http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:88: return false; ditto http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h File net/base/dns_response.h (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h#newc... net/base/dns_response.h:26: IOBuffer* io_buffer() { return io_buffer_.get(); } see previous comment about IOBuffer and scoped_refptr. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h#newc... net/base/dns_response.h:33: bool Parse(int nbytes, AddressList* results); s/int/size_t/ http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h#newc... net/base/dns_response.h:42: char* data() { return io_buffer_->data(); } const char*? http://codereview.chromium.org/7008021/diff/5002/net/base/dns_util.h File net/base/dns_util.h (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_util.h#newcode35 net/base/dns_util.h:35: static const uint16 kClassIN = 1; Since this has graduated to a header file I worry that it's too generic. Maybe leave it as is for now, but I think that I should have put all this into a `dns' namespace now.
Adam, thanks for a detailed review. I've replied to your comments, but have written will do, since I will create a new CL due to some structural changes in classes. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode20 net/base/dns_query.cc:20: buf[1] = v & 0xff; On 2011/05/30 18:35:30, agl wrote: > please flip these two lines. Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode25 net/base/dns_query.cc:25: uint16 qtype = kDNS_A; On 2011/05/30 18:35:30, agl wrote: > switch (address_family) { > case ADDRESS_FAMILY_IPV4: > return kDNS_A; > case ADDRESS_FAMILY_IPV6: > return kDNS_AAAA; > default: > NOTREACHED() << "Bad address family"; > return kDNS_A; > } Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode35 net/base/dns_query.cc:35: // DNS query consists of a 12-byte header followed by a question section, On 2011/05/30 18:35:30, agl wrote: > s/section,/section./ (and s/for/For/ on the next line) Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode44 net/base/dns_query.cc:44: static const int kHeaderLen = arraysize(kHeader); On 2011/05/30 18:35:30, agl wrote: > s/int/size_t/ Same argument as previous one regarding int vs size_t, but will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode75 net/base/dns_query.cc:75: static const int kMaxQuerySize = kHeaderLen + 255 + On 2011/05/30 18:35:30, agl wrote: > DNSDomainFromDot should fail if the name is too long, rendering this test > superfluous. Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.cc#newcode78 net/base/dns_query.cc:78: DCHECK(iterator <= kMaxQuerySize); On 2011/05/27 15:15:54, cbentzel wrote: > This shouldn't be a DCHECK - unless there are checks on hostname length from all > callers of this function. > > I also think it would make more sense to do this earlier - like when query_size > is calculated and before the io_buffer_ is created, so IsValid() will return > false. Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode34 net/base/dns_query.h:34: uint16 qclass() const; On 2011/05/30 18:35:30, agl wrote: > Classes are a really obscure corner of DNS. I'd tend to keep this private unless > there's a need for it that I haven't got to in the code yet. DnsResponse was using it to match the Question part, but this can be hardcoded to IN for the foreseeable future. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode38 net/base/dns_query.h:38: int size() const; On 2011/05/30 18:35:30, agl wrote: > size_t for sizes? Originally, I had it size_t, but eventually, I needed to compare these to number of bytes read/written, which is a result from read/write calls, hence ints. I try to avoid casts, so decided to change it to int instead, but if the convention is to use size_t and do casting, I can change that. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode42 net/base/dns_query.h:42: IOBuffer* io_buffer(); On 2011/05/30 18:35:30, agl wrote: > IOBuffers are refcounted, but returning a raw pointer allows callers to forget > this. I'd suggest: > void io_buffer(scoped_refptr<IOBuffer>* out); > As a way to ensure that nobody ever makes that mistake while still minimizing > copy constructors and atomic ops. I agree that it's an error-prone API, however, the way I use it is udp_socket->Write(dns_query->io_buffer(), dns_query->size(), callback) and Write expects a raw pointer. Would it be better to return a scoped_refptr and do an explicit get on it? http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode54 net/base/dns_query.h:54: char* data() const; On 2011/05/30 18:35:30, agl wrote: > const char*? Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode66 net/base/dns_query.h:66: // Class of query, currently fixed to IN class. On 2011/05/30 18:35:30, agl wrote: > This is so unlikely to ever change I wouldn't make it a variable even. Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode69 net/base/dns_query.h:69: // Hostname for we are trying to resolve. On 2011/05/30 18:35:30, agl wrote: > s/for/that/ Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc File net/base/dns_response.cc (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:14: // RFC 1035, section 4.2.1: Messages carried by UDP are restricted to 512 On 2011/05/30 18:35:30, agl wrote: > This is no longer true. The current recommendation is that clients SHOULD be > able to cope with 4K replies I believe. Right. I was thinking of adding EDNS0 support in another iteration to keep this CL limited to RFC1035. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:60: !response.U16(&additional_count)) On 2011/05/30 18:35:30, agl wrote: > { } around the body if the conditional is multi-line. (I'm not sure if it's a > style rule, so you can ignore this if you like.) Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:74: return false; On 2011/05/30 18:35:30, agl wrote: > ditto Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:76: if (answer_count < 1) On 2011/05/30 18:35:30, agl wrote: > This isn't a parse error of course. I feel that you would need to distinguish > error codes from this function into multiple families: > * Invalid packet (dispatcher can stop processing) > * Not for me (dispatcher can try to match against other outstanding requests) > * For me, but I couldn't get a useful answer (SERVFAIL, NXDOMAIN, no records > etc) Thanks for these. As I've written in the comment on line 18, I need to decide whether I should add more ERR_* codes or continue with the existing host_resolver_proc method of reflecting back getaddrinfo errors, so will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.cc#new... net/base/dns_response.cc:88: return false; On 2011/05/30 18:35:30, agl wrote: > ditto Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h File net/base/dns_response.h (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h#newc... net/base/dns_response.h:26: IOBuffer* io_buffer() { return io_buffer_.get(); } On 2011/05/30 18:35:30, agl wrote: > see previous comment about IOBuffer and scoped_refptr. Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h#newc... net/base/dns_response.h:33: bool Parse(int nbytes, AddressList* results); On 2011/05/30 18:35:30, agl wrote: > s/int/size_t/ This is how it is being used, I don't think I can change the type here due to CompletionCallback signature, can I? void OnRecvComplete(int result) { ... response_.Parse(result) ... } http://codereview.chromium.org/7008021/diff/5002/net/base/dns_response.h#newc... net/base/dns_response.h:42: char* data() { return io_buffer_->data(); } On 2011/05/30 18:35:30, agl wrote: > const char*? Will do. http://codereview.chromium.org/7008021/diff/5002/net/base/dns_util.h File net/base/dns_util.h (right): http://codereview.chromium.org/7008021/diff/5002/net/base/dns_util.h#newcode35 net/base/dns_util.h:35: static const uint16 kClassIN = 1; On 2011/05/30 18:35:30, agl wrote: > Since this has graduated to a header file I worry that it's too generic. Maybe > leave it as is for now, but I think that I should have put all this into a `dns' > namespace now. I will leave it as is for now.
+ eroman, jar
Hi eroman, jar,
Please review the DnsQuery and DnsResponse classes in terms of the interface.
Right now, my async resolver uses them as below. Another thing I want your
guidance on is errors: should I get rid of os_error add more ERR_* codes or
should I imitate host_resolver_proc and try mapping errors to EAI_* and set
os_error accordingly? Currently, in case of success, I set error to OK,
os_error to 0, in case of any kind of failure, I set error to
ERR_NAME_NOT_RESOLVED and os_error to EAI_NONAME.
Please guide, thanks.
// members of DnsTransaction class which does socket creation, retransmission,
etc. (similar to Job in HostResolverImpl)
UdpSocket* socket_;
DnsQuery query_;
DnsResponse response_;
...
// sending out the query
socket_->Write(query_.io_buffer(), query_.size(), &send_callback_);
// reading response
int rv = socket_.Read(response_.data(), response_.size(), &recv_callback_);
// processing response
void OnRecvComplete(int result) {
...
if (response_.Parse(result, &results_))
// set errors..
Talib and I talked in person. Although I was initially a fan of a single DNSMessage class, I am comfortable with separate DNSQuery and DNSResponse classes, particularly if they end up sharing a DNSHeader class/struct. As far as errors, I'll let Eric Roman be the main decision-maker here. My preference is more error codes rather than the two error codes. This way, we'll have a better notion of why DNS is failing particularly for top-level pages. I'll take a look at the actual code details now.
I will try and commit this CL now that Chris is fine with two separate classes.
Adam: I've fixed all the issues you've mentioned, except IOBuffer API issue and
differentiating parse/resolution errors. Please review.
Eric: waiting for your review and your opinion on adding more ERR_* codes or
imitating error/os_error approach of host_resolver_proc.
A general question: once DnsQuery is constructed it needs to be written to a
socket; currently I do it by an IOBuffer getter which returns raw pointer to an
internal IOBuffer since that's what Write() expects [similar problem exists for
DnsResponse/Read()]. As Adam suggested, it's an error-prone API, but I can't
think of a cleaner solution for it, and have seen it used like that (see
net/base/{filter,upload_data_stream}.h) so any suggestions are welcome.
http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h
File net/base/dns_query.h (right):
http://codereview.chromium.org/7008021/diff/5002/net/base/dns_query.h#newcode34
net/base/dns_query.h:34: uint16 qclass() const;
On 2011/05/31 15:19:06, agayev wrote:
> On 2011/05/30 18:35:30, agl wrote:
> > Classes are a really obscure corner of DNS. I'd tend to keep this private
> unless
> > there's a need for it that I haven't got to in the code yet.
>
> DnsResponse was using it to match the Question part, but this can be hardcoded
> to IN for the foreseeable future.
Hardcoded class to IN.
LGTM. Just a random thought: when sending the DNS packets you know that you need to send from a different, random source port each time, right?
On 2011/06/01 16:29:42, agl wrote: > LGTM. > > Just a random thought: when sending the DNS packets you know that you need to > send from a different, random source port each time, right? Yes, I have it as a TODO in my DnsTransaction class which creates the UDP socket; will need to change net/udp for that.
http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... net/base/dns_query.cc:36: } Nit: // namespace to indicate end of anonymous namespace block. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... net/base/dns_query.cc:65: int iterator = 0; nit: byte_offset instead of iterator? Also, maybe retain a char* buffer_head = io_buffer_->data() variable? http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... net/base/dns_query.cc:75: RandomizeId(); Should you just do this in io_buffer() http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.h#newcode31 net/base/dns_query.h:31: int port() const; Should port be a uint16 rather than an int? http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.h#newcode53 net/base/dns_query.h:53: const char* data() const; I'd recommend removing data(), and just use io_buffer() by the tests so you can get rid of the FRIEND_TEST's. These can call id() after getting io_buffer() to construct the expected bytes. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest.cc File net/base/dns_query_unittest.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest... net/base/dns_query_unittest.cc:55: uint8 id_hi = q1.id() >> 8, id_lo = q1.id() & 0xff; I'm pretty sure this will do a logical shift instead of an arithmetic shift, but maybe mask id_hi with 0xff to make sure? http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest... net/base/dns_query_unittest.cc:125: EXPECT_FALSE(id_hi == q1.data()[0]); The bytes will occasionally match, so this test will fail about .8% of the time (1 - (255/256)^2) assuming pure randomness. I'd suggest removing it. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc File net/base/dns_response.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:27: io_buffer_(new IOBufferWithSize(size_)) { DCHECK(query_) in the constructor [and maybe query_->IsValid())] http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:85: if (!response.DNSName(NULL) || Why are you passing in NULL DNS name? Don't you want to validate that this matches? http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:85: if (!response.DNSName(NULL) || Does DNSResponseBuffer handle the compressed form for DNS names for you? http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:99: rdatas.push_back(IPAddressNumber(rdata.begin(), rdata.end())); At some point we'll want to preserve TTLs as well. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:100: } else if (!response.Skip(rdlength)) In the future, might be nice to keep track of CNAME's as well. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:105: return false; This should distinguish invalid DNS responses from invalid domains, and it doesn't do a good job. Either make this an enum return code, or make it a neterror code. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.h File net/base/dns_response.h (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.h#new... net/base/dns_response.h:17: class DnsResponse { DnsResponse is a bit different than I expected. I thought it was going to just parse the bytes over the network, determine if it's valid syntax, and add API's for extracting RR's etc. Then, a higher level function/class would determine whether it's a valid response for the query. However, I haven't seen this used yet. On it's own it seems fine, but I may ask for the class design to change once I see it in use. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.h#new... net/base/dns_response.h:22: DnsResponse(DnsQuery* query); Mark this constructor as explicit. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.h#new... net/base/dns_response.h:22: DnsResponse(DnsQuery* query); You should specify what the lifetime expectations are for |query| - does DNSResponse own it, should it last longer, etc.
http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... net/base/dns_query.cc:36: } On 2011/06/01 17:17:03, cbentzel wrote: > Nit: // namespace to indicate end of anonymous namespace block. Done. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... net/base/dns_query.cc:65: int iterator = 0; On 2011/06/01 17:17:03, cbentzel wrote: > nit: byte_offset instead of iterator? > > Also, maybe retain a char* buffer_head = io_buffer_->data() variable? Done. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... net/base/dns_query.cc:75: RandomizeId(); On 2011/06/01 17:17:03, cbentzel wrote: > Should you just do this in io_buffer() That would lead to a construction of a degenerate DnsQuery with 0 id. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest.cc File net/base/dns_query_unittest.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest... net/base/dns_query_unittest.cc:55: uint8 id_hi = q1.id() >> 8, id_lo = q1.id() & 0xff; On 2011/06/01 17:17:03, cbentzel wrote: > I'm pretty sure this will do a logical shift instead of an arithmetic shift, but > maybe mask id_hi with 0xff to make sure? id_hi is a single byte, masking it with 0xff is a no-op. Do you really want that? http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest... net/base/dns_query_unittest.cc:125: EXPECT_FALSE(id_hi == q1.data()[0]); On 2011/06/01 17:17:03, cbentzel wrote: > The bytes will occasionally match, so this test will fail about .8% of the time > (1 - (255/256)^2) assuming pure randomness. > > I'd suggest removing it. Decreased the probability of a failure to 1/2^16, is that good enough? http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc File net/base/dns_response.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:27: io_buffer_(new IOBufferWithSize(size_)) { On 2011/06/01 17:17:03, cbentzel wrote: > DCHECK(query_) in the constructor [and maybe query_->IsValid())] Done. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:85: if (!response.DNSName(NULL) || On 2011/06/01 17:17:03, cbentzel wrote: > Does DNSResponseBuffer handle the compressed form for DNS names for you? It does. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:85: if (!response.DNSName(NULL) || On 2011/06/01 17:17:03, cbentzel wrote: > Why are you passing in NULL DNS name? Don't you want to validate that this > matches? djbdns skips names there; I assume legal name servers wouldn't change it, illegal ones would not gain anything by having bogus names there. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:99: rdatas.push_back(IPAddressNumber(rdata.begin(), rdata.end())); On 2011/06/01 17:17:03, cbentzel wrote: > At some point we'll want to preserve TTLs as well. Yep, will amend it then. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:100: } else if (!response.Skip(rdlength)) On 2011/06/01 17:17:03, cbentzel wrote: > In the future, might be nice to keep track of CNAME's as well. Okay. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.cc#ne... net/base/dns_response.cc:105: return false; On 2011/06/01 17:17:03, cbentzel wrote: > This should distinguish invalid DNS responses from invalid domains, and it > doesn't do a good job. Either make this an enum return code, or make it a > neterror code. This is something agl pointed out too, I'm waiting for eroman's opinion. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.h File net/base/dns_response.h (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_response.h#new... net/base/dns_response.h:22: DnsResponse(DnsQuery* query); On 2011/06/01 17:17:03, cbentzel wrote: > You should specify what the lifetime expectations are for |query| - does > DNSResponse own it, should it last longer, etc. Done.
Mostly looks good. - Change the DnsQuery to be const-after-construction (in other words io_buffer() doesn't mutate the id) - I'd like to change the DnsResponse::Parse to return either a DnsResponse specific enum or a net error code instead of a bool. - Style nit Should it be Dns or DNS when camel-case? It looks like we are inconsistent in the code base, and this is true for other acronyms as well. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc File net/base/dns_query.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... net/base/dns_query.cc:75: RandomizeId(); On 2011/06/01 19:15:01, agayev wrote: > On 2011/06/01 17:17:03, cbentzel wrote: > > Should you just do this in io_buffer() > > That would lead to a construction of a degenerate DnsQuery with 0 id. We talked and decided to have io_buffer() mutate the id. That way the DnsQuery is constant post-construction, and we don't have to worry about how clients order calls to id or io_buffer, or be surprised that the io_buffer bytes change underneath them. http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest.cc File net/base/dns_query_unittest.cc (right): http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest... net/base/dns_query_unittest.cc:55: uint8 id_hi = q1.id() >> 8, id_lo = q1.id() & 0xff; On 2011/06/01 19:15:01, agayev wrote: > On 2011/06/01 17:17:03, cbentzel wrote: > > I'm pretty sure this will do a logical shift instead of an arithmetic shift, > but > > maybe mask id_hi with 0xff to make sure? > > id_hi is a single byte, masking it with 0xff is a no-op. Do you really want > that? Oops, I was thinking of it as uint16. No, the mask is not necessary.
Please review, I've added error codes. On 2011/06/01 19:41:21, cbentzel wrote: > Mostly looks good. > > - Change the DnsQuery to be const-after-construction (in other words io_buffer() > doesn't mutate the id) > > - I'd like to change the DnsResponse::Parse to return either a DnsResponse > specific enum or a net error code instead of a bool. Added error codes for it. > - Style nit Should it be Dns or DNS when camel-case? It looks like we are > inconsistent in the code base, and this is true for other acronyms as well. I followed what Http does. It's also easier to type. > http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc > File net/base/dns_query.cc (right): > > http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query.cc#newco... > net/base/dns_query.cc:75: RandomizeId(); > On 2011/06/01 19:15:01, agayev wrote: > > On 2011/06/01 17:17:03, cbentzel wrote: > > > Should you just do this in io_buffer() > > > > That would lead to a construction of a degenerate DnsQuery with 0 id. > > We talked and decided to have io_buffer() mutate the id. That way the DnsQuery You mean *NOT* to mutate id. > is constant post-construction, and we don't have to worry about how clients > order calls to id or io_buffer, or be surprised that the io_buffer bytes change > underneath them. > > http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest.cc > File net/base/dns_query_unittest.cc (right): > > http://codereview.chromium.org/7008021/diff/15001/net/base/dns_query_unittest... > net/base/dns_query_unittest.cc:55: uint8 id_hi = q1.id() >> 8, id_lo = q1.id() & > 0xff; > On 2011/06/01 19:15:01, agayev wrote: > > On 2011/06/01 17:17:03, cbentzel wrote: > > > I'm pretty sure this will do a logical shift instead of an arithmetic shift, > > but > > > maybe mask id_hi with 0xff to make sure? > > > > id_hi is a single byte, masking it with 0xff is a no-op. Do you really want > > that? > > Oops, I was thinking of it as uint16. No, the mask is not necessary.
This looks good. Most of the issues I raised are questions or comment nits. However, I think you really want more test coverage on the parse portion. Also make sure to get a solid answer on the numerical error codes to use for the new DNS errors. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h#newcode26 net/base/dns_query.h:26: // Every generated object has a different ID, hence two objects generated Thanks for this comment. You may want to clarify and say "are not guaranteed to be equal" instead of "are not equal" as there is a chance that they are equal. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h#newcode26 net/base/dns_query.h:26: // Every generated object has a different ID, hence two objects generated Why did you skip the factory approach? That way you don't need IsValid(). http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h#newcode33 net/base/dns_query.h:33: // DnsQuery field accessors. You should comment that these can only be called if IsValid() is true. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query_unittest.cc File net/base/dns_query_unittest.cc (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query_unittest... net/base/dns_query_unittest.cc:109: TEST(DnsQueryTest, IOBufferAccessRandomizesIdTest) { Remove this test. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response.h File net/base/dns_response.h (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response.h#new... net/base/dns_response.h:24: // Internal buffer accessor into which actual bytes of response to be bytes of response are to be read. Why don't callers just pass in an IOBuffer* and count-in-bytes to Parse? http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response.h#new... net/base/dns_response.h:36: DnsQuery* const query_; This can be const DnsQuery* const query_; http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response_unitt... File net/base/dns_response_unittest.cc (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response_unitt... net/base/dns_response_unittest.cc:70: TEST(DnsResponseTest, ResponseWithCnameA) { Thanks for including CNAME and making sure that the skip-over-it-logic works. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response_unitt... net/base/dns_response_unittest.cc:70: TEST(DnsResponseTest, ResponseWithCnameA) { You should add a lot more tests for the invalid cases, like non-3 rdata or non-echoed-back question, or invalid id, etc. If necessary you can just do a TODO, but it's pretty easy to set up a unit test for parsing. http://codereview.chromium.org/7008021/diff/26005/net/base/net_error_list.h File net/base/net_error_list.h (right): http://codereview.chromium.org/7008021/diff/26005/net/base/net_error_list.h#n... net/base/net_error_list.h:235: NET_ERROR(DNS_MALFORMED_RESPONSE, -143) I'm not sure if we want to create all DNS errors at -800, -801, -802 etc block to separate from others. Get Eric's feedback on this. Once we introduce a code, it's pretty hard to change.
http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h#newcode26 net/base/dns_query.h:26: // Every generated object has a different ID, hence two objects generated On 2011/06/02 21:27:13, cbentzel wrote: > Thanks for this comment. > > You may want to clarify and say "are not guaranteed to be equal" instead of "are > not equal" as there is a chance that they are equal. Done. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h#newcode26 net/base/dns_query.h:26: // Every generated object has a different ID, hence two objects generated On 2011/06/02 21:27:13, cbentzel wrote: > Why did you skip the factory approach? That way you don't need IsValid(). It looked ugly, with much redundancy. Also, I do not want to restrict creation of objects to heap. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h#newcode33 net/base/dns_query.h:33: // DnsQuery field accessors. On 2011/06/02 21:27:13, cbentzel wrote: > You should comment that these can only be called if IsValid() is true. Done. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query_unittest.cc File net/base/dns_query_unittest.cc (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query_unittest... net/base/dns_query_unittest.cc:109: TEST(DnsQueryTest, IOBufferAccessRandomizesIdTest) { On 2011/06/02 21:27:13, cbentzel wrote: > Remove this test. Done. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response.h File net/base/dns_response.h (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response.h#new... net/base/dns_response.h:24: // Internal buffer accessor into which actual bytes of response to be On 2011/06/02 21:27:13, cbentzel wrote: > bytes of response are to be read. > > Why don't callers just pass in an IOBuffer* and count-in-bytes to Parse? Moving it outside this class will break the symmetry with DnsRequest which manages its own buffer. I can move it out if you think it's definitely better. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response.h#new... net/base/dns_response.h:36: DnsQuery* const query_; On 2011/06/02 21:27:13, cbentzel wrote: > This can be > > const DnsQuery* const query_; Done. http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response_unitt... File net/base/dns_response_unittest.cc (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_response_unitt... net/base/dns_response_unittest.cc:70: TEST(DnsResponseTest, ResponseWithCnameA) { On 2011/06/02 21:27:13, cbentzel wrote: > You should add a lot more tests for the invalid cases, like non-3 rdata or > non-echoed-back question, or invalid id, etc. > > If necessary you can just do a TODO, but it's pretty easy to set up a unit test > for parsing. I will add more tests, but I need to stabilize the API first. Otherwise I will keep modifying the tests. http://codereview.chromium.org/7008021/diff/26005/net/base/net_error_list.h File net/base/net_error_list.h (right): http://codereview.chromium.org/7008021/diff/26005/net/base/net_error_list.h#n... net/base/net_error_list.h:235: NET_ERROR(DNS_MALFORMED_RESPONSE, -143) On 2011/06/02 21:27:13, cbentzel wrote: > I'm not sure if we want to create all DNS errors at -800, -801, -802 etc block > to separate from others. Get Eric's feedback on this. Once we introduce a code, > it's pretty hard to change. I'll email Eric and ask.
LGTM http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h File net/base/dns_query.h (right): http://codereview.chromium.org/7008021/diff/26005/net/base/dns_query.h#newcode26 net/base/dns_query.h:26: // Every generated object has a different ID, hence two objects generated On 2011/06/02 22:33:26, agayev wrote: > On 2011/06/02 21:27:13, cbentzel wrote: > > Why did you skip the factory approach? That way you don't need IsValid(). > > It looked ugly, with much redundancy. Also, I do not want to restrict creation > of objects to heap. OK, that's reasonable. http://codereview.chromium.org/7008021/diff/29004/net/base/net_error_list.h File net/base/net_error_list.h (right): http://codereview.chromium.org/7008021/diff/29004/net/base/net_error_list.h#n... net/base/net_error_list.h:240: // DNS server failed. This error is returned for all ofthe following error Nit: of the instead of ofthe
http://codereview.chromium.org/7008021/diff/29004/net/base/net_error_list.h File net/base/net_error_list.h (right): http://codereview.chromium.org/7008021/diff/29004/net/base/net_error_list.h#n... net/base/net_error_list.h:240: // DNS server failed. This error is returned for all ofthe following error On 2011/06/03 01:58:46, cbentzel wrote: > Nit: of the instead of ofthe Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
