BIND 10 #505: DNAME Implementation

[BIND 10 Development]

#505: DNAME Implementation
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  jinmei
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  A-Team-
                 Priority:  major    |  Sprint-20110223
                Component:           |           Resolution:
  b10-auth                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  2.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:20 vorner]:

 > >  - for the case of too long name then YXDOMAIN, I'd rather check the
 > >    length explicitly than relying on exception.
 >
 > I did it this way. I'm not sure if it looks simpler.
 >
 > My understanding of exceptions is that they are for „unusual“
 (exceptional) situation, not impossible ones. That's why they can be
 caught. Then it depends on how exceptional the too long name is considered
 to be.

 I admit it's sometimes subtle when to use exceptions and it's often a
 matter of opinion.

 As for the revised code itself, we should be able to skip the label
 count check; if the length of the resulting target is valid it should
 also have a valid number of labels.  Also, I think in this case we can
 simply use the magic number of "1" instead of Name(".").getLength();
 it should be obvious that the length of the root name is 1 (and we
 have a comment about this).  So I'd suggest revising the if condition
 as follows:
 {{{
                 if (prefix.getLength() + dname.getDname().getLength() - 1
 >
                     Name::MAX_WIRE) {
 }}}

 (If you really want to use the root name, I'd suggest using
 Name::ROOT_NAME().)

 > >  - asserting "getRdataCount() > 0" seems to be a bit strong
 >
 > What do you propose to do here in the meantime, then? The code can
 hardly do anything useful without the RR. Should I throw an exception?
 Send some server error back?
 >
 > I added a FIXME note there for now.

 I don't have a strong opinion, but if you are okay with the idea of
 having RdataIterator throw an exception, I'm okay with keeping the
 assert() for now while creating a separate ticket for it.  Once we
 revise RdataIterator, if a buggy data source passes an empty DNAME, it
 will result in an exception and result in SERVFAIL, which should be
 okay.

 > >  - I would add a note before the MockZone about the DNAME name.
 >
 > I'm not sure if I see the correct place for note or understand what
 note.

 I've added some and pushed it.

 A few more (minor) comments:

  - I still don't understand this comment (even after revised)
 {{{
  * This is added because the original implementation had a bug and didn't
  * include the CNAME at all.
 }}}
   What's the original implementation?  Does this mean this test would
   have failed before #505?  Or some earlier version of #505?  Maybe we
   can say something like: "Type ANY queries are sometimes handled as a
   special case, but they shouldn't be special in DNAME handling.  So
   we explicitly test it to see if it's handled like any other types of
   queries".
  - I'd add another type of test for "LongDNAME": test the case where
    the resulting target has the max length to check we don't
    accidentally reject the valid case.
  - I've made some minor (mostly editorial changes) to the branch.
    Please check the diff.

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