Limited NSEC support to RecordParsed

Limited NSEC support to RecordParsed

Add support for parsing the subset of nsec records specified for multicast DNS to RecordParsed, in order to allow support for domain negation in multicast DNS (see section 6.1 "Negative Responses" of http://www.rfc-editor.org/rfc/rfc6762.txt).

BUG=233821
TEST=RecordRdataTest.ParseNsecRecord

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208616

[gene, 2013-06-24 19:47:15]

lgtm,
just a few nits below:

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc
File net/dns/record_rdata.cc (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:248: bitmap_data.begin(),
indentation

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:259: if (other->Type() != Type()) return false;
move return to the next line

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h
File net/dns/record_rdata.h (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h#newcod...
net/dns/record_rdata.h:204: bool bit(unsigned i);
could you please add comment describing what bit() is doing?
also, consider naming it GetBit() since logic requires a few lines

[Noam Samuel, 2013-06-24 21:31:30]

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc
File net/dns/record_rdata.cc (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:248: bitmap_data.begin(),
On 2013/06/24 19:47:16, gene wrote:
> indentation

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:259: if (other->Type() != Type()) return false;
On 2013/06/24 19:47:16, gene wrote:
> move return to the next line

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h
File net/dns/record_rdata.h (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h#newcod...
net/dns/record_rdata.h:204: bool bit(unsigned i);
On 2013/06/24 19:47:16, gene wrote:
> could you please add comment describing what bit() is doing?
> also, consider naming it GetBit() since logic requires a few lines

Done.

[szym, 2013-06-24 21:50:28]

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc
File net/dns/record_rdata.cc (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:230: unsigned len_next_domain =
parser.ReadName(data.data(), NULL);
nit: |next_domain_len| would be better, |next_domain_length| even better.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:234: 
StringPiece::operator[] returns char, so I'd suggest something like:

struct BitmapHeader {
  uint8 block_number;
  uint8 length;
} header = reinterpret_cast<BitmapHeader*>(
    data.data() + next_domain_length);

if (header->block_number)
  return ...();  // Block number must be zero.

Another option is:
unsigned bitmap_length = static_cast<unsigned>(data[...]);

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:235: unsigned window_num = data[len_next_domain];
I suggest block_num or block_number
"window number" is used only once in RFC3845 and RFC6762 does not use "window"
at all.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:236: unsigned len = data[len_next_domain+1];
bitmap_length

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:238: // The window number must be zero in mDns-secific
nsec records. The bitmap
"specific"
"NSEC"

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:240: // data length must be exactl equal to the domain
length plus bitmap size.
"exactly"

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:242: data.length() != len_next_domain + 2 + len)
The last check would be easier to understand if you moved it after the
construction of the bitmap_data StringPiece.

if (bitmap_data.length() != bitmap_length)
  return ..();

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h
File net/dns/record_rdata.h (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h#newcod...
net/dns/record_rdata.h:188: // one bit-map:
The formatting suggests variable names, but they are not used like this in code
nor in the referenced RFCs. There is also no need for "one" in "one bit-map".

I suggest using:
"Next Domain Name"
and
"Type Bit Map"

or better yet copy the relevant part of RFC6762 here.

Another option is to leave this out (a reference to RFC seems sufficient) and
perhaps add the most useful remarks as comments in the implementation of Create.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h#newcod...
net/dns/record_rdata.h:203: const std::vector<uint8>& bitmap() { return bitmap_;
}
Do you need to expose both bitmap and bit?

https://codereview.chromium.org/17143006/diff/6001/net/dns/dns_protocol.h
File net/dns/dns_protocol.h (right):

https://codereview.chromium.org/17143006/diff/6001/net/dns/dns_protocol.h#new...
net/dns/dns_protocol.h:121: 
unnecessary blank

[Noam Samuel, 2013-06-25 01:15:35]

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h
File net/dns/record_rdata.h (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h#newcod...
net/dns/record_rdata.h:203: const std::vector<uint8>& bitmap() { return bitmap_;
}
On 2013/06/24 21:50:28, szym wrote:
> Do you need to expose both bitmap and bit?

The main use case for exposing bitmap is to allow users to copy the full bitmap
easily. I don't think this should be necessary for most uses of nsec (since nsec
should really only be used inside the mDNS layer itself, this is easier to say).
I think it makes more sense to expose only the size of the bitmap, in case the
user wants to do something for each bit.

[Noam Samuel, 2013-06-25 01:31:31]

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc
File net/dns/record_rdata.cc (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:230: unsigned len_next_domain =
parser.ReadName(data.data(), NULL);
On 2013/06/24 21:50:28, szym wrote:
> nit: |next_domain_len| would be better, |next_domain_length| even better.

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:234: 
On 2013/06/24 21:50:28, szym wrote:
> StringPiece::operator[] returns char, so I'd suggest something like:
> 
> struct BitmapHeader {
>   uint8 block_number;
>   uint8 length;
> } header = reinterpret_cast<BitmapHeader*>(
>     data.data() + next_domain_length);
> 
> if (header->block_number)
>   return ...();  // Block number must be zero.
> 
> Another option is:
> unsigned bitmap_length = static_cast<unsigned>(data[...]);
> 

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:235: unsigned window_num = data[len_next_domain];
On 2013/06/24 21:50:28, szym wrote:
> I suggest block_num or block_number
> "window number" is used only once in RFC3845 and RFC6762 does not use "window"
> at all.

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:236: unsigned len = data[len_next_domain+1];
On 2013/06/24 21:50:28, szym wrote:
> bitmap_length

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:238: // The window number must be zero in mDns-secific
nsec records. The bitmap
On 2013/06/24 21:50:28, szym wrote:
> "specific"
> "NSEC"

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:240: // data length must be exactl equal to the domain
length plus bitmap size.
On 2013/06/24 21:50:28, szym wrote:
> "exactly"

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.cc#newco...
net/dns/record_rdata.cc:242: data.length() != len_next_domain + 2 + len)
On 2013/06/24 21:50:28, szym wrote:
> The last check would be easier to understand if you moved it after the
> construction of the bitmap_data StringPiece.
> 
> if (bitmap_data.length() != bitmap_length)
>   return ..();

Done.

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h
File net/dns/record_rdata.h (right):

https://codereview.chromium.org/17143006/diff/1/net/dns/record_rdata.h#newcod...
net/dns/record_rdata.h:188: // one bit-map:
On 2013/06/24 21:50:28, szym wrote:
> The formatting suggests variable names, but they are not used like this in
code
> nor in the referenced RFCs. There is also no need for "one" in "one bit-map".
> 
> I suggest using:
> "Next Domain Name"
> and
> "Type Bit Map"
> 
> or better yet copy the relevant part of RFC6762 here.
> 
> Another option is to leave this out (a reference to RFC seems sufficient) and
> perhaps add the most useful remarks as comments in the implementation of
Create.

Done.

[szym, 2013-06-25 01:40:56]

LGTM with two nits.

Don't forget to update BUG=
Also mention the new test in TEST=

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.cc
File net/dns/record_rdata.cc (right):

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.cc#n...
net/dns/record_rdata.cc:247: // The window number must be zero in mDns-specific
NSEC records. The bitmap
"block number"

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.h
File net/dns/record_rdata.h (right):

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.h#ne...
net/dns/record_rdata.h:199: const unsigned bitmap_length() { return
bitmap_.size() * 8; }
I think you meant to put the const after ()

[Noam Samuel, 2013-06-25 01:58:41]

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.cc
File net/dns/record_rdata.cc (right):

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.cc#n...
net/dns/record_rdata.cc:247: // The window number must be zero in mDns-specific
NSEC records. The bitmap
On 2013/06/25 01:40:57, szym wrote:
> "block number"

Done.

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.h
File net/dns/record_rdata.h (right):

https://codereview.chromium.org/17143006/diff/18001/net/dns/record_rdata.h#ne...
net/dns/record_rdata.h:199: const unsigned bitmap_length() { return
bitmap_.size() * 8; }
On 2013/06/25 01:40:57, szym wrote:
> I think you meant to put the const after ()

Done.

[commit-bot: I haz the power, 2013-06-25 01:59:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/17143006/23001

[commit-bot: I haz the power, 2013-06-25 02:04:39]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-06-25 17:24:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/17143006/23001

[commit-bot: I haz the power, 2013-06-25 17:26:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/17143006/23001

[Noam Samuel, 2013-06-25 18:10:50]

On 2013/06/25 17:26:09, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/noamsml%40chromium.org/17143006/23001

Last-minute change: Added "const" qualifier to GetBit.

[commit-bot: I haz the power, 2013-06-25 20:51:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/17143006/46001

[commit-bot: I haz the power, 2013-06-26 04:12:32]

Change committed as 208616