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         |  
---------------------------+------------------------------------------------
Changes (by stephen):

  * owner:  stephen => each


Comment:

 I took your advice and reviewed in in three passes corresponding to the
 changes you made.  As a result, some of the comments may have been
 addressed by later revisions of the code.

 '''General'''
 A lot more documentation is required, especially high-level overview of
 the architecture; the current README file is insufficient.  A class
 diagram, showing how all the classes fit together, would be useful as
 would something like a UML sequence diagram showing the passage of a query
 (and associated response) through the recursive server.

 Although I have put down comments for the tests, in the case of the
 asiolink tests the comments just refer to local things: I spent a long
 time looking at the code in that module trying to follow the logic, but I
 don't think I completely succeeded.  Rather than spend more time on it, I
 would like to look at them again when the high-level documentation has
 been written.

 Should the coroutine header files (coroutine.h, yield.h, unyield.h) be
 placed in the "ext" directory tree as they are external code?


 '''r2941 -> r2953'''
 '''src/lib/asiolink/asiolink.h'''
 Comment: From the comments in the code, I presume that the use of
 asio::ip::address is OK, although it does mean including one of the asio
 header files in user code.  If this were to prove to be a problem, one
 could store the address as the target of a "void*" pointer and cast it to
 the appropriate type in the asiolink.cc file. (Of course, this couldn't be
 as type-safe.)

 Definitions of DNSProvider and !CheckinProvider; it is not really
 explained what these terms mean.

 '''src/lib/asiolink/unyield.h'''
 class IOSocket: adding a protected default constructor is unnecessary.
 The comment suggests that this is because the class should not be
 instantiated; however, this is guaranteed because the class is abstract
 (by virtue of the "virtual getXxxx() const = 0" declarations.

 '''src/lib/asiolink/asiolink.cc'''
 IOAddress::IOAddress(): BIND 10 standards suggest that opening brace
 should be on same line as method declaration.

 TCPServer::operator(): a comment to indicate that the read of the message
 is in two parts, the length and the message body would be apposite here.

 "yield continue" is still present in this version of the code.

 Although the logic is similar, the TCPServer uses a do...while loop for
 the "parent" routine while the child processes code ultimately returns.
 However the UDPServer has the code enclosed in a for (;;) loop with "yield
 return" to get out.  It would be clearer to adopt one structure for both
 servers.

 There is special code to cope with !SunStudio's inability to cope with a
 particular lexical_cast.  It would be useful to think about extracting
 this code into a separate utility function for use elsewhere.



 '''r2953 -> r2958'''
 '''src/bin/recurse/tests/recursor_unittest.cc'''
 From just the point of view of code reuse, it would be useful to put
 !MockSession into a separate file.

 Similarly, isn't there already code that creates packets from the data
 files somewhere in the repository (i.e. is there a need for the
 RecursorTest::createXxxx() methods)?

 EXPECT_EQ(true/false, xxxx) can be more succinctly written as
 EXPECT_TRUE/FALSE(xxx).

 A number of these tests (where malformed packets are rejected) can be
 shared with the authoritative server tests.

 Not sure where the stand-along function updateConfig is called from.

 '''src/bin/recurse/b10-recurse.xml'''
 Not examined in detail, need to look at the output instead.

 '''src/bin/recurse/main.cc'''
 This section contains aggregated comments on all versions of main.cc in
 this review.

 The usage message is a bit terse - explanation of the options should be
 given.

 The usage message does not include the "-u" switch (for setting the UID).

 The default port used (if "-p" is not given) is 5300, not 53.

 "-4 and -6 can't coexist" is perhaps better put as "you may not specify
 both -4 and -6 at the same time".  Similar commands apply to the message
 "-4|-6 and -a can't coeexist".

 Suggest that the module-specific variables (e.g. verbose_mode, io_service
 etc.) be declared "static" to restrict them to the module.

 Line 175: "recursor ->setVerbose(verbose_mode);".  The space between
 "recursor" and "->", although ignored by the compiler, should not be
 there.

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

 '''src/bin/recurse/recursor.cc'''
 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?

 Headers needed for classes defined in this file.

 Errors and the like ought to be output via a logging function or object
 and not routed directly to cerr.

 Recursor::processMessage(): I think that the parsing of the message and
 creation of a suitable error response if incorrect could be split off into
 a separate object and used by both the authoritative and recursive
 servers.

 '''src/bin/recurse/recursor.h'''
 Header for constructor refers to two parameters that aren't present as
 constructor arguments.

 Other methods need method headers.

 '''src/bin/recurse/b10-recurse.8'''
 Not checked - need to check the output.

 '''r2958 -> r3136'''
 (r3136 (the latest revision at the time of review) only had three changes
 over r3125 so I elected to review the latest version instead of the one
 mention in the previous comment by Evan.

 '''src/lib/asiolink/asiolink.h'''
 The header for DNSServer::operator() needs a bit more text to say that the
 reason that the indirection has to take place is to cope with the
 "slicing" that takes place when a derived class is passed as a reference
 to a base class.

 asiolink was defined as a way to insulate code from the vagaries of the
 asio header files.  However, with the introduction of DNSServer, DNSLookup
 and other classes, its scope seems to have expanded.  I suggest that the
 DNSXxx classses be moved into a seprate file.

 Class !RecursiveQuery: given that an IOAddress class has been defined to
 hide asio stuff from other classes, and that the members of this class
 include an IOService, it seems logical for the ns_addr_ member to be of
 type IOAddress.


 '''src/lib/asiolink/tcpdns.cc and src/lib/asiolink/udpdns.cc'''
 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, and as the code in these
 methods depend on members of IOAddress, these methods could be inherited
 from the base class.  (The base class can remain abstract by giving
 IOAddress a pure virtual destructor (i.e. "virtual ~IOAddress() = 0") -
 although remember to provide a definition for the destructor in the .cc
 file! (See item 7 in "Effective C++, Scott Meyers, Addison Wesley, ISBN
 978-0-321-33487-9").  The same goes for methods in other classes, e.g.
 TCPServer::resume() is identical to UDPServer::resume().

 UDPQuery::operator(): Comment above the call to "async_receive_from" says
 "When the send completes" - it should be "when the receive completes".

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

 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.

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

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

 '''src/lib/asiolink/tests/asio_link_unittest.cc'''
 Shouldn't this be renamed asiolink_unittest.cc?

 In the tests for bad ports, bad addresses etc., there is no explicit tests
 that the method succeeds for a good port/good address.  Although this is
 implicitly tested elsewhere, a specific test would simplify fault-finding
 if there were a problem.

 Tests for bad V4 addresses should check an address with an octet out of
 range (e.g. 257.1.2.3).  The same goes for tests on bad IPV6 addresses.

 In the test for an unavailable address, there is no check that binding to
 a good IPv4-mapped IPV6 address (e.g. ::ffff:1.2.3.4) works.

 The destructor for ASIOLinkTest has code along the lines of "if (x !=
 NULL) delete x;".  There is no need for the check; delete is a no-op if
 the pointer is NULL.

 Helper functions and classes should have header documentation.

 As noted earlier, I have looked at the individual classes in this file,
 but have not checked in detail that the logic is correct.  This should be
 revisited when additional documentation has been written.

 '''src/lib/asiolink/internal/tcpdns.h'''
 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.)

 '''src/lib/asiolink/README'''
 OK, but this needs to be greatly expanded and we definitely need that
 diagram showing flow control somewhere.

 '''src/bin/auth/auth_srv.cc'''
 Messages are being written to cerr; a proper logging system is required.

 '''src/bin/auth/auth_srv.h'''
 Not all methods in the !AuthSrv class have been properly documented.

 '''src/bin/recurse/tests/recursor_unittest.cc'''
 The class !MockSession should be properly documented.  (Also, perhaps put
 it in an external file to avoid cluttering up the unit test file?)

 The tests are incomplete - the only check failures caused by packet
 errors; they do not check that it works.


 '''src/bin/recurse/recursor.cc'''
 In the definition of !RecursorImpl, is there any reason why the member
 variables are public?

 ''' src/bin/recurse/recursor.h'''
 Methods need documentation.

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