BIND 10 #495: Hook up NSAS to iterator/cache

[BIND 10 Development]

#495: Hook up NSAS to iterator/cache
-------------------------------------+-------------------------------------
                 Reporter:  shane    |                Owner:  jelte
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110316
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  3.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jelte


Comment:

 > it's alive! ALIVE!!!
 Congratulations Dr Frankenstein.  Does this mean that instead of bug
 reports we'll get users carrying flaming torches hammering on the doors of
 950 Charter Street?

 '''src/bin/resolver/main.cc'''[[BR]]
 Why is the try/catch commented out?

 '''src/lib/asiolink/io_address.h'''[[BR]]
 Do we need "#include <iostream>" here?  There is nothing in the header
 file that requires it.

 '''src/lib/nsas/nameserver_address_store.{cc,h}'''[[BR]]
 In cancel(), the "callback" argument can be passed by reference.

 '''src/lib/nsas/zone_entry.{cc,h}'''[[BR]]
 removeCallback(): are we sure at this point that this is the only instance
 of this particular callback object in the list?  (addCallback just pushes
 the callback on the end of the callbacks_[family] vector without checking
 whether it already exists in the list.)

 The callback argument can be passed by reference.

 '''src/lib/resolve/recursive_query.{cc,h}'''[[BR]]
 Should rename the "#ifdef" variable in recursive_query.h to
 !__RECURSIVE_QUERY_H

 send(): should add a TODO to change the call to rand() to a call to a
 Boost PRNG (or a yet-to-be provided utilities "rand()" equivalent using
 Boost).

 handleRecursiveAnswer(): suggest a TODO be added to the effect that
 cur_zone_ should be a "Name" object instead of a string.  (I think you
 made a comment like this in another ticket and I agree - we should try to
 avoid conversions to strings if not required.)

 !RunningQuery constructor:  it should be possible to initialize
 nsas_callback_ with a plain "new ResolverNSASCallback(this)" and not use
 an intermediate shared_ptr.

 Comment.  The outstanding events pointer is fine, but is there a way we
 could handle the reference-counting semi-automatically, e.g. passing a
 shared_ptr to the !RunningQuery as an argument to the callbacks?  Then the
 object should be automatically deleted when all callbacks have fired
 without the need to maintain our own count of outstanding events.

 operator(): when calculating the elapsed time, do you need a test before
 doing the calculation?  (Also, the test is wrong - the AND should be an
 OR.)

 '''recursive_query_unittest.cc'''[[BR]]
 The test_data variable encodes the count in the first two bytes of the
 sent data.  As test_data is a byte array, the data sent should be in the
 order given.  However, the first two bytes of a TCP message are
 interpreted as a 16-bit value and converted from network byte order to
 host byte order.  Is there a chance that on some machines, the byte order
 will be flipped?  (However, in the rest of the test the first two bytes
 are not interpreted as a value, so perhaps it doesn't matter.)

 We should add some tests that check whether the cache was updated on
 successful queries (and that invalid replies were not added).

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