BIND 10 #327: early work on recursion

[BIND 10 Development]

#327: early work on recursion
---------------------------+------------------------------------------------
      Reporter:  each      |        Owner:  each                 
          Type:  defect    |       Status:  reviewing            
      Priority:  major     |    Milestone:  y2 12 month milestone
     Component:  recurser  |   Resolution:                       
      Keywords:            |    Sensitive:  0                    
Estimatedhours:  0.0       |        Hours:  0                    
      Billable:  1         |   Totalhours:  0                    
      Internal:  0         |  
---------------------------+------------------------------------------------

Comment(by each):

 Thank you again for the review.  Most of these comments I'm working on
 addressing now.  A few remarks below on the others:

 > class IOSocket: adding a protected default constructor is unnecessary.

 Do you want that changed?  I believe Jinmei wrote that code originally.
 Seems harmless to leave it as it is.

 > "Server created." and other messages.  These goes to stdout - what
 happens when the recursor is started by BoB?  Shouldn't this message be
 logged?

 A C++ logging system is still work in progress (unless things have
 happened and I missed it).

 > Class !ConfigChecker: Not sure about processing configuration messages
 as part of a query; wouldn't it better to set up a separate thread to do
 that?

 Very possibly, but we're avoiding threads at this stage of development, I
 think.  The approach I was planning to take was to use a timeout and check
 for configuration updates periodically (every 5-10 seconds perhaps).  But
 I haven't written any timeout code at all yet.

 > class UDPEndpoint: The code for functions such as getAddress(),
 getPort(), getProtocol() and getFamily() is the same as that for the
 TCPEndpoint class.  As both classes derive from IOAddress

 ...from IOEndpoint, actually.  The problem is that these members all
 reference asio_endpoint_, which doesn't exist in the base class (can't, as
 it has a different type in each of the derived classes).

 (Is there a way to do this with templates, maybe?)

 > The same goes for methods in other classes, e.g. TCPServer::resume() is
 identical to UDPServer::resume().

 resume() calls io_service::post(), but the io_service reference is only
 initialized in the derived-class constructors.  Again, not sure how to
 work around this.

 > Should the UDPServer::operator() start with "if (ec) return;" (as does
 the similar TCPServer method)?

 Hmm.  I think I'd better get rid of it in the TCP version, actually.

 > UDPServer::operator(): To avoid the overhead of constantly allocating
 the data buffers, we may need to think about creating a "lookaside" list
 of them.  The same goes for the IOMessage objects.

 Agreed, I've been working on that.

 > Both the UDPServer and TCPServer need some timeout mechanism on the
 asynchronous reads.

 Agree with that too.  I think it's beyond the scope of my current work,
 though; I won't have time to do this before handing it off.

 > There should be some logging of errors; at the moment they are just
 dismissed.

 I will try to note places throughout the code where I think there should
 be logging, but I don't think we have a real logging system yet.

 > class TCPEndpoint: is this realistically ever going to be overridden and
 the protocol returned something other than IPPROTO_TCP?  If not, there is
 no readon to make the getXxx() functions virtual.  (And they could be
 implemented inline in the header file for speed.)

 Hm.  We had a discussion a while ago about whether we should define
 functions as virtual in classes that we don't expect to be overridden, and
 I think we ended up deciding it was okay to do so.  I'm happy either way
 though.  (Anyway in this case I don't think I was thinking about it
 particularly; I probably just copied the headers from the base class and
 forgot to remove the "virtual" keywords).

 > In the definition of !RecursorImpl, is there any reason why the member
 variables are public?

 Recursor accesses several of them.  (Also, I was just imitating
 AuthSrvImpl here.  It's been an ongoing point of disagreement between
 Jinmei and myself whether the PIMPL design pattern is even appropriate
 here--he thinks it is, I don't.  But I don't feel all that strongly about
 it.)

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