BIND 10 #893: TSIG verify, part 2

[BIND 10 Development]

#893: TSIG verify, part 2
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  DNSPacket API                      |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  3.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:13 stephen]:
 > > So, are you suggesting we should make the buffer local to helper
 methods, or are you okaying the current code? I'm okay with either way.
 > I was suggesting using a local variable but like you, I'm OK with how it
 is at the moment.

 On looking at the code again, I found we can use local buffers with a
 precomputated expected (resulting) buffer size (actually we could do
 this for the code with the shared buffer, but the precomputation would
 be more complicated).  That way the performance penality wouldn't be
 much different because buffer.clear() internally involves free() and
 subsequent write operations require malloc() and realloc() (this is
 another drawback of our in-house buffer replacement, but that's
 another topic).

 So I made that change (345b598).  It's probably a bit beyond the very
 trivial level, but I've merged it to the master for the moment.  It
 would be nice if you could take a look at the change to see if there's
 something wrong.

 > > Hmm, I see. But if we use the free function version of writeUint16 we
 need to update the intermediate 8 bytes separately, which would make the
 code look rather complicated (relatively).
 > If std::copy is used, the copying would still be a one-line operation.
 >
 > I think that the performance gain would be very minor so I suggest that
 we leave the code as it is now, but bear this discussion in mind if we
 have to come back and do some low-level optimisation.

 Okay (although I'm still not sure how std::copy helps here).  I've
 added some more comments about performance consideration with the
 above change (I needed to update the comment anyway because we don't
 share a single buffer any more as was previously commented).

 > > Hmm, actually I also previously (not for this task) thought about
 having a separate header file to define numeric protocol constants such as
 Name::MAX_WIRE/MAX_LABELS, Message::DEFAULT_MAX_UDPSIZE. I was not sure if
 such a separate header file was really useful and still am not so sure,
 but if you like I'll create a separate ticket for that (at least for
 considering it). It's a different question whether we want to publicly
 define constants for things like the size of DNS header section or offsets
 to particular fields such as QID or ARCOUNT...I personally think they are
 too minor to be defined publicly.
 > I think I would argue for both.  I disagree that the constants are too
 trivial - if nothing else, using these constants in expressions (instead
 of "magic" numbers) would help the documentation of the code.

 I've created a ticket as a placeholder for this topic.  #919.

 I've also created tickets for remaining work I mentioned in my review
 request.  They are #920 and #921.

 > > I added some more explanation (143b2c6). Hopefully this addressed your
 concern.
 > It does, thank-you.
 >
 > All fine (including the !ChangeLog entry suggested earlier), please
 merge.

 Okay, thanks.  Merge done, closing ticket.

-- 
Ticket URL: <http://bind10.isc.org/ticket/893#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development