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        |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Replying to [comment:18 jinmei]:
 >  - (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.

 OK, I'm going to create a 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.

 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.

 >  - 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.

 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.

 > '''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()

 Well, that's generalization of the rule that if function header exceeds
 one line, the brace is on the line by itself. Then it's visible where the
 condition ends and the body starts.

 But if there's a guideline for it this way, I don't mind.

 >  - 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.

 >  - why was this comment removed?
 > {{{
 > -// This tests that when we need to look up Zone's apex NS records for
 > }}}

 It was an accident.

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