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

Comment(by vorner):

 (This is a partial response, how I get trough the comments. The ones not
 mentioned are still to be done.)

 Replying to [comment:6 stephen]:
 > '''src/lib/nsas/hash_table.h'''[[BR]]
 > ''HashTable::getOrAdd()''[[BR]]
 > Comment: the suggestion in the "to do" comment about doing part of the
 lookup using a shareable lock seems a good idea.  But I would use an
 upgradeable lock, allowing the conversion between a read lock to an
 exclusive lock without releasing it.  That would make the code simpler:
 acquire read lock and look up entry, if not found upgrade to exclusive
 lock and create entry.

 Reading documentation, it seems it would not work. It suggests only one
 can have upgradable lock at the time, which is the same as exclusive in
 our case.

 > ''newNS()''[[BR]]
 > As this function is purely local to this module, suggest that it is
 declared "static"

 It is in an anonymous namespace, which has equivalent effect to being
 static. It is unreachable from other modules and compiler knows it is not
 called from outside, allowing it to optimise more.

 > ''class  ZoneEntry::!ResolverCallback''[[BR]]
 > Needs a header describing the purpose of the class and full header
 documentation of its methods.

 Done.

 > ''ZoneEntry::ResolverCallback::success''[[BR]]
 > Needs better documentation.  It's manipulating pointers to nameservers,
 but I'm not clear what is the difference between "old" and "new"
 nameservers, what manipulation is taking place, or why.

 Done

 > ''ZoneEntry::randIndex()''[[BR]]
 > When this branch is merged with #356, check the random number generation
 code there.  It also might be worth splitting random number generating
 functions off into a separate library module.

 This one will not be needed at all after it is merged properly, it does
 the choosing of address Ocean already wrote.

 > ''class ZoneEntry::!ProcessGuard''[[BR]]
 > Needs full header documentation.

 Done

 > ''class ZoneEntry::!NameserverCallback''[[BR]]
 > Needs full header documentation.

 Done

 > ''ZoneEntry::dispatchFailures()''[[BR]]
 > Can't the arguments be passed by reference to save a copy operation?

 With family, there's no gain (it's an enum, which might be even smaller
 than the reference/pointer). There might be with the shared pointer,
 but the function is private, so the compiler can (and as it is small,
 probably will) inline it and eliminate all the stuff there. So I think
 this does not really matter at all.

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