BIND 10 #117: NSEC3 RDATA needs more tests and has serious bugs

[BIND 10 Development]

#117: NSEC3 RDATA needs more tests and has serious bugs
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:           |            Milestone:  A-Team-
  critical                           |  Sprint-20110223
                Component:           |           Resolution:
  DNSPacket API                      |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  0.0      |          Total Hours:  5.0
                Billable?:  0        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:15 vorner]:
 > I had little problems with compiling it (missing boost:: in front of
 lexical_cast) and there were some problems with make distcheck (the
 nsec_bitmap.* can't be nodist, that would mean they are not included in
 the distribution tarball). I fixed both of these, could you have a look if
 it is OK?

 It looks okay, thanks for fixing it.

 > With the code itself, it is OK. I have just two minor points:
 >
 > This is in the nsec_bitmap.cc:
 > {{{
 > for (int i = 0; i < total_len; i += len)
 > }}}
 >
 > I do understand it and I like compressed code, but I noticed there's
 inclination to more wordy ways. As the len changes each iteration and the
 i moves by another 2 inside the body, use of for might be slightly
 misleading (it is usually used when the only one changing the i is the for
 cycle itself and with fixed length). While cycle might be more intuitive
 for that, but I personally don't mind this either.

 Changed in 73b2eae.  I'm not sure if the change from for to while
 improved the clarity of the code, but I agree using two mutable
 parameters for a single for loop is more error prone.  As a bonus side
 effect I can change 'len' to const, which is a clear win.  I also
 noticed 'block' can also be const so I did it.

 > As you note the NSEC3 tests are the same as with NSEC at one place ‒
 would it be possible to have them unified to have the code only once,
 let's say in some kind of template manner?

 I actually thought about that possibility, but didn't do it in the
 first push.  We still need to prepare test data separately, which have
 different file names, so I'm still not sure if it's an easy and
 reasonable way to share the same test code for both cases.  But I
 agree it would be better to centralize the two cases, so I created a
 separate test case in a separate .cc file, and moved bitmap related
 tests there.  If we add more test cases and find the duplicate too
 noisy in future, we can then consider sharing the code.  This change
 is aa32fd5.

 I also made one minor change: d0a29d0

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