BIND 10 #408: General control logic of NSAS

[BIND 10 Development]

#408: General control logic of NSAS
---------------------------+------------------------------------------------
      Reporter:  vorner    |        Owner:  vorner   
          Type:  task      |       Status:  reviewing
      Priority:  major     |    Milestone:           
     Component:  recurser  |   Resolution:           
      Keywords:            |    Sensitive:  0        
Estimatedhours:  0.0       |        Hours:  0        
      Billable:  1         |   Totalhours:  0        
      Internal:  0         |  
---------------------------+------------------------------------------------
Changes (by stephen):

  * owner:  stephen => vorner


Comment:

 I've reviewed the source code but have only skimmed the tests - these need
 to be reviewed properly when this task is finished.

 '''src/lib/nsas/hash_table.h'''[[BR]]
 get() now locks the table entry with a scoped_lock.  The original code
 used a sharable_lock to allow concurrent gets.

 Comment: There needs to be a restriction on the generator function that it
 must return in a very short time, since the relevant slot in the hash
 table is locked while it is running.

 '''src/lib/nsas/zone_entry.h'''[[BR]]
 '''src/lib/nsas/zone_entry.cc'''[[BR]]
 Would prefer a more descriptive name such as !LocalLock instead of LLock.
 The same goes for Lock::Impl - something like "Lock::!LockList" or
 "Lock::!LockCollection".

 It would be more efficient to sort the nameservers once when they are
 added to the zone entry than to sort them every time a lock is requested.

 When locking the nameservers, the internal mutex of the nameserver entry
 is directly accessed (via ns->mutex_).  This breaks encapsulation.  Would
 it not be better to add a "getLock()" to the !NameserverEntry object and
 to lock them through that interface?

 Is there actually a need to lock all nameservers within a zone at once?


 '''src/lib/nsas/tests/fetchable_unittest.cc'''[[BR]]
 The accessMethods test ends with the creation of a new Fetchable object,
 but no associated test for it.


 '''src/lib/nsas/tests/nameserver_address_store_unittest.cc'''[[BR]]
 There are some non-ASCII characters in the file (the quotes around the
 word 'unreachable' in the zoneWithoutNameservers test).

 '''src/lib/nsas/resolver_interface.h'''
 Shouldn't the arguments to resolve() be passed by reference so as to save
 a copy?

 '''src/lib/nsas/nameserver_address_store.cc'''[[BR]]
 '''src/lib/nsas/nameserver_address_store.h'''[[BR]]
 Constructor.  The default value of 3001 is the first prime number above
 3000 (header comment is wrong, sorry!)

 lookup().  Further to a comment made in another review, should we be
 passing RRClass objects instead of uint16_t?

 processZone().  This accesses both the zones and the nameservers in them,
 which breaks the layering of !NameserverAddressStore -> !ZoneEntry ->
 !NameserverEntry.  I would suggest that this method should be part of the
 zone entry object.

 The same goes for newNS and newZone; pass the arguments to the !ZoneEntry
 object and have it create the internal !NameserverEntry objects.

 Comment: with respect to TTLs, we need to be aware of the possibility that
 records may have zero TTL.  This means that during an access to new
 records, the records may become expired.

 Comment: chooseAddress().  This will have to be tied into the RTT
 associated with an address.

 '''src/lib/nsas/nameserver_entry.h'''[[BR]]
 '''src/lib/nsas/nameserver_entry.cc'''[[BR]]
 The idea regarding the callbacks was that when the zone_entry receives a
 query, it adds the callback related to the query to its list, then queries
 the nameservers.  If the nameserver entry object does not have the
 information, a callback is added to its callback list and a query
 initiated to the resolver.  When the resolver returns with an answer, the
 nameserver entry executes all its callbacks.  These callbacks in turn
 execute all the callbacks associated with the zone, which cause the result
 to be returned to the caller.  If a second query for the zone comes in
 during this processing, the callback is added to the zone entry's callback
 queue; the fact that there is already a callback present in the queue
 means that there is no need to do any tests or checks - all these were
 done when the first callback was added.

 In askIP() then, is there a need check that a callback for a given zone
 exists? askIP() should never be called if there is already an address
 fetch outstanding on a nameserver (because there will be something in the
 callback queue).  Besides, if an additional callback for a particular zone
 is mistakenly added, when it is executed there are no callbacks in the
 zone entry's callback queue, so the effect is a no-op.

 In askIP, I suggest that we should always issue queries for both A and
 AAAA records.

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