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-20110209
                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):

 It basically looks doing what it's expected to do functionality wise,
 but I have some points that would have to be discussed.

 '''query.cc'''
  - For the initial implementation, I'd rather suppress adding the
    auth NS/additional while we discuss it on bind10-dev, because
     - this is what other implementations (both BIND 9 and NSD) do
     - this way we can avoid "falling through" (and we can just break
       from the switch at the end of the DNAME case), thereby making
       the code easier to follow and more robust to changes.  This way
       we can also keep "qtype_is_any" const.
     - unlike CNAME chains, the target of a DNAME chain is always
       "external domain" by definition.  we are still not certain what
       we should do with CNAME chains, but for external chains it seems
       to be more likely to not provide chains according to discussions
       on bind10-dev so far.
   I don't preclude the possibility that we find a need to provide a
   chain after DNAME, at which point we can revisit the above
   points.  But for the initial implementation I believe it's better
   to not provide the NS and additional.
  - (comment) the switch block is becoming an unreadable fat monster.
    the need for defining a {} block inside 'case' also indicates the
    DNAME case is not concise enough.  I think we are getting to the
    point of refactoring.  I don't request it within this ticket,
    however, and would suggest creating a new backlog ticket for it.
  - for the case of too long name then YPDOMAIN, I'd rather check the
    length explicitly than relying on exception.  This is partly
    because of a philosophical reason: I'd use exceptions in our C++
    code basically for things that cannot happen in a normal condition
    (such as a broken DNS message).  This case can happen against a
    completely valid query (though it may be unnaturally long), for
    which I'd not use an exception.  Another reason is that we may
    revisit exception organization and switch to less grained
    exceptions as we discussed in a different context before, and if
    that happens we may only have a higher level exception than
    TooLongName, e.g, "NameError".  But this may be ultimately a matter
    of preference.  If the preferences vary, I wouldn't necessarily
    push my own one.
  - asserting "getRdataCount() > 0" seems to be a bit strong because
    there can be many underlying data sources, some of which may not
    even be implemented by us.  And, in fact, even our current
    implementation of in memory data source doesn't prohibit an empty
    RRset (if explicitly added via the add() interface).  [I think we
    should change that, and it should be possible once we merge #550,
    but that's a different topic].  We should probably make sure
    getRdataIterator() (or its getCurrent()) throws an exception if the
    RRset is empty and relies on it.
  - some comment lines are too long.  following the BIND 9 convention,
    I suggest lines to not exceed 79 characters.

 '''query_unittest.cc'''
  - brace positions are not consistent with other part of the code (and
    not follow the BIND 9's common guideline):
 {{{
             rrset->getType() == RRType::DNAME())
         {
             dname_rrset_ = rrset;
         } else if (rrset->getName() == longdname_name_ &&
             rrset->getType() == RRType::DNAME())
         {
             longdname_rrset_ = rrset;
         }
 }}}
   Same comment applies to the special case of MockZone::find()
  - I would add a note before the MockZone about the DNAME name.
  - We don't necessarily need to have dname_name_ and longdname_; we
    can test the latter with a long query name.  That way we can make
    the MockZone simpler.  (if we want to be paranoid we may want
    to test both cases though)
  - why was this comment removed?
 {{{
 -// This tests that when we need to look up Zone's apex NS records for
 }}}
  - I don't understand this comment.  What does "original version" means?
 {{{
  * This is added because the original version didn't include the CNAME at
  * all.
 }}}

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