BIND 10 #356: Resolver address database

[BIND 10 Development]

#356: Resolver address database
-------------------------------+--------------------------------------------
      Reporter:  stephen       |        Owner:  stephen  
          Type:  enhancement   |       Status:  reviewing
      Priority:  major         |    Milestone:           
     Component:  Unclassified  |   Resolution:           
      Keywords:                |    Sensitive:  0        
Estimatedhours:  0.0           |        Hours:  0        
      Billable:  1             |   Totalhours:  0        
      Internal:  0             |  
-------------------------------+--------------------------------------------
Changes (by ocean):

  * owner:  ocean => stephen


Comment:

 Replying to [comment:15 stephen]:
 > Reviewed differences between latest version at time of writing (r3706)
 and the last-reviewed revision (r3533)
 >
 > '''src/lib/nsas/random_number_generator.h'''[[BR]]
 > Need to {{{#include <math.h>}}} (on Ubuntu at least) to get the
 declaration of {{{fabs()}}}.

 Done. Add {{{#include <cmath>}}}

 >
 > ''WeightedRandomIntegerGenerator::isProbabilitiesValid''[[BR]]
 > Although the compiler will do the appropriate conversions, I suggest
 using literals that are doubles (1.0, 0.0) instead of integers to
 emphasise the fact that we are comparing floating-point numbers.
 >
 Done.

 > ''WeightedRandomIntegerGenerator::isProbabilitiesValid''[[BR]]
 > It is clearer to combine the two range checks in one statement, i.e.
 > {{{
 > if ((*it < 0.0) || (*it > 1.0)) {
 >     return false;
 > }
 > }}}

 Done

 >
 > '''src/lib/nsas/tests/random_number_generator_unittest.cc'''[[BR]]
 > Some of the tests use the ASSERT_DEATH macro.  This relies on the
 assert() macro in the constructor to call isProbabilitiesValid() and, if
 it returns false, to call abort().  However, if the code is compiled with
 NDEBUG set, assert() is a no-op and the test fails.
 >

 Add {{{#ifndef NDEBUG .... #endif }}} macro on these tests.
 Since some validation will  incur performance penalty, so do the
 validation only on debug version.

 >
 > '''src/lib/nsas/tests/Makefile.am'''[[BR]]
 > Should include a dependency on random_number_generator.h
 >

 Done.

 > '''src/lib/nsas/nameserver_address.h'''[[BR]]
 > The constructor contains the line:
 > {{{
 >  if(!ns_.get()) isc_throw(NullNameserverEntryPointer, "NULL
 NameserverEntry pointer.");
 > }}}
 > The [wiki:CodingGuidelines coding guidelines] request that braces be
 used even for a single-line "if" block.

 Done.

 >
 > '''src/lib/nsas/nameserver_entry.h'''[[BR]]
 > '''src/lib/nsas/nameserver_entry.cc'''[[BR]]
 >
 > ''NameserverEntry::getAddress()''[[BR]]
 > The first argument  must be a shared_ptr pointing to "this".  As it is
 only used in constructing a !NameserverAddress object (which is then
 copied over the passed !NameserverAddress object), the argument seems
 redundant.  Suggest either:
 >  a. the passed-in !NameserverAddress object could be partially
 constructed using the current !NameserverEntry before calling this method.
 (The assert() call in this method would be kept to ensure that the
 argument is compatible with this object.)
 >  b. Derive !NamserverEntry (or perhaps its base class !NsasEntry, so
 avoiding multiple inheritance) from boost::enable_shared_from_this, which
 will allow the construction of a shared_ptr for "this" within the method.
 >

 Done. I choose method b, so the code will be more clear. Actually this has
 bothered me very much to figure out how to get {{{shared_ptr}}} that
 points to {{{this}}} object.

 > ''NameserverEntry::updateAddressRTTAtIndex()''[[BR]]
 > The [wiki:CodingGuidelines coding guidelines] request that braces be
 used for all "if" and "else" clauses.
 Done.

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