On Wed, Nov 21, 2018 at 9:25 AM Sara Dickinson <[email protected]> wrote:
> > > Begin forwarded message: > > *From: *Eric Rescorla <[email protected]> > *Subject: **Eric Rescorla's No Objection on > draft-ietf-dnsop-dns-capture-format-08: (with COMMENT)* > *Date: *21 November 2018 at 02:07:20 GMT > *To: *"The IESG" <[email protected]> > *Cc: *Tim Wicinski <[email protected]>, [email protected], > [email protected], [email protected], > [email protected] > *Resent-From: *<[email protected]> > *Resent-To: *[email protected], [email protected], [email protected], > [email protected], [email protected] > > Eric Rescorla has entered the following ballot position for > draft-ietf-dnsop-dns-capture-format-08: No Objection > > > Hi, > > Many thanks for the review > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > Rich version of this review at: > https://mozphab-ietf.devsvcdev.mozaws.net/D4670 > > > I support Ben's DISCUSS. > > > Ack - hopefully our response to him addresses your concerns. > I'll leave it with him. > Am I understanding the design correctly in that you can't have > literals in the individual per-query values, but instead references to > the block tables, so you can't stream inside a block? > > > Yes, that is correct - do you think we need to add text to clarify this? > Yes, as it seems like an unobvious tradeoff. It's worth noting that if you put the block tables at the end, you could save memory on the encoder. > > > IMPORTANT > S 7.5.1. > > | earliest-time | O | A | A timestamp (2 unsigned integers, | > | | | | "Timestamp") for the earliest record | > | | | | in the "Block" item. The first integer | > | | | | is the number of seconds since the | > | | | | Posix epoch ("time_t"). The second | > | | | | integer is the number of ticks since | > > > I don't know what a "tick" is, so you need some definitionf or this. > > > From 7.4.1.1: > > ticks-per-second | M | U | Sub-second timing is recorded in | > | | | | ticks. This specifies the number of | > | | | | ticks in a second. > > This was the solution to a request to allow flexibility in how granular > the sub-second timing increments are. > Ah I missed that. A reference to 7.4.1.1 would help. > S 7.4.1.1.1. > > > +------------------+---+---+----------------------------------------+ > | Field | O | T | Description | > +------------------+---+---+----------------------------------------+ > | query-response | M | U | Hints indicating which "QueryResponse" | > | -hints | | | fields are omitted, see section | > > > Nit: generally, you would say "indicating which fields are included" > if 1 means included. > > > The issue is that if the bit is set the field might still be missing > because although the configuration was set to collect it the data wasn’t > available to the encoder from some other reason. However when the bit is > not set it means that the data will definitely not be present because the > collector is configured not to collect it. > > We do discuss this problem in section 6.2.1 - perhaps a reference in the > table back to that discussion is what is needed? > Maybe, I just thought that the text was a bit odd. > > > S 7.5.3. > > +---------------------+---+---+-------------------------------------+ > > 7.5.3. "BlockTables" > > Arrays containing data referenced by individual "QueryResponse" or > "MalformedMessage" items in this "Block". Each element is an array > > > This is not a sentence. > > > Suggest: “Map of arrays containing data... " > Maybe. I'll let you sort this out with RFC Ed. S 7.5.3.2. > > +--------------------+---+---+--------------------------------------+ > | server-address | O | U | The index in the item in the "ip- | > | -index | | | address" array of the server IP | > | | | | address. See Section 7.5.3. | > | | | | | > | server-port | O | U | The server port. | > > > Isn't the server port generally constant? It seems like you might save > some bits by having this attached to the server and then indixed > abvoe. > > > Yes it is (or likely to be one of 3 values), and I think you are right > there is a small saving in what you are suggesting but we chose consistency > of representation (with client address) over that slight added saving... > OK. Just thought I would point it out. -Ekr > > > > S 7.5.3.2. > > | | | | used to service the query. | > | | | | Bit 0. IP version. 0 if IPv4, 1 if | > | | | | IPv6 | > | | | | Bit 1-4. Transport. 4 bit unsigned | > | | | | value where 0 = UDP, 1 = TCP, 2 = | > | | | | TLS, 3 = DTLS. Values 4-15 are | > > > You might want to specify DoH > > > > S 7.5.3.5. > > | | | | Bit 0. IP version. 0 if IPv4, 1 if | > | | | | IPv6 | > | | | | Bit 1-4. Transport. 4 bit unsigned | > | | | | value where 0 = UDP, 1 = TCP, 2 = | > | | | | TLS, 3 = DTLS. Values 4-15 are | > | | | | reserved for future use. | > > > Again, probably want to specify DoH. > > > Yes - we’ll add it. > > > > S 17.3. > > [18] https://github.com/dns-stats/draft-dns-capture- > format/blob/master/file-size-versus-block-size.svg > > Appendix A. CDDL > > This appendix gives a CDDL [I-D.ietf-cbor-cddl] specification for > > > Is this a normative appendix? > > > As picked up in other reviews, we will make the CDDL document a normative > reference. > > Best regards > > Sara. > >
_______________________________________________ DNSOP mailing list [email protected] https://www.ietf.org/mailman/listinfo/dnsop
