BIND 10 #458: Additional MX processing

[BIND 10 Development]

#458: Additional MX processing
-------------------------------+--------------------------------------------
      Reporter:  vorner        |        Owner:  vorner   
          Type:  task          |       Status:  reviewing
      Priority:  major         |    Milestone:           
     Component:  Unclassified  |   Resolution:           
      Keywords:                |    Sensitive:  0        
Estimatedhours:  0.0           |        Hours:  0        
      Billable:  1             |   Totalhours:  0        
      Internal:  0             |  
-------------------------------+--------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => vorner


Comment:

 Replying to [comment:3 vorner]:
 > Crap, my bad, I forgot to send the two commits into the repository. They
 should be there as r4072 and r4073 now.
 >
 Okay.  Here are review comments.

 '''Query::process() modification'''
  - the Zone::SUCCESS case block is getting large, and since it's quite
 likely to support more additional cases, we'll need to move it to a
 separate method.  I don't necessarily request it for this particular
 ticket for now, though.
  - constructing a vector of additional types for every processing seems to
 be a waste as it's mostly a constant setting.  but again, I don't
 necessarily request it change for now.
  - why do you perform recursive query calls to look up the additional
 RRsets?  From a quick look at the code with some local experiments, it
 could simply be something like this:
 {{{
 response_.addRRset(Message::SECTION_ADDITIONAL,
 boost::const_pointer_cast<RRset>(
 additional_result.rrset));
 }}}
  at least this code passes the test.

 '''query_unittest.cc modification'''
  - I'd also test the case where an MX name is under a zone cut.  e.g.
 there's a delegation at child.example.com. and one of the MX names is
 mail.child.example.com.  Such a name should *not* be added to the
 additional section.  (but right now we cannot effectively implement this
 case.  we need the zone cut handling support and also glue OK/NG modes)
  - do you mean SECTION_ADDITIONAL here?
 {{{
 +    for (SectionIterator<RRsetPtr> ai(response.beginSection(
 +        Message::SECTION_ANSWER)); ai != response.endSection(
 +        Message::SECTION_ANSWER); ++ ai)
 +    {
 +        additional_count ++;
 +    }
 }}}
  beside, if we only care about the number of RR(set)s, I'd simply use
 Message::getRRCount().
  - a minor style(-type) point: I think "unsigned int" would be better than
 "size_t" for this type of purpose (although we don't need the additional
 variable if we use getRRCount()):
 {{{
 +    size_t additional_count(0);
 }}}
  - another minor style issue: to be consistent with other part of the
 code, it would be better to remove a space before/after '++':
 {{{
 +        Message::SECTION_ANSWER); ++ ai)
 +    {
 +        additional_count ++;
 }}}
   same point applies to some other part of the diff, including query.cc.
 Also for consistency, we might want to use the prefix version of '++' (for
 additional_count), although, again, we don't need this variable if we use
 getRRCount() in the first place.

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