Opened 9 years ago

Closed 9 years ago

#327 closed defect (complete)

early work on recursion

Reported by: each Owned by: each
Priority: medium Milestone: y2 12 month milestone
Component: recurser Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

At last week's face to face meeting we agreed to work on a basic recursive resolver, with no authoritative support, validation, acl/tsig, or cache. This ticket is for the early work on this. It will include a "Dispatch" data structure for rapidly associating incoming responses to outstanding fetch contexts, and a first pass at the resolution algorithm with some things stubbed out for future work.

Subtickets

Attachments (1)

trac327_review.txt (6.7 KB) - added by stephen 9 years ago.
List of files to be reviewed before branch is merged back into trunk

Download all attachments as: .zip

Change History (28)

comment:1 Changed 9 years ago by each

One of the first things that will need to be done to implement a resolver (or any other server type other than authoritative) is to split off the asio_link module from AuthSrv.

In current code, the asio_link object is constructed with a pointer to the auth server object; when packets are received, it calls auth_srv->processMessage() and auth_srv->configSession(). I've replaced that approach with one in which the caller provides asio_link with a pair of callback objects, which do the same thing but can be provided by any server class (AuthSrv, Resolver, Forwarder, or whatever.)

While I was at it, in hopes of making it less painful to write the ASIO handlers for the more complicated state machine we'll need to have when we implement a full validating resolver, I changed the TCPServer and UDPServer classes to use the "stackless coroutine" pattern described at: http://blog.think-async.com/2010/03/potted-guide-to-stackless-coroutines.html

The resulting code should be functionally identical to the previous code, but it is shorter and (IMHO) easier to read: instead of several different asynchronous response handlers, there's a single function for TCP and another for UDP, and the I/O operations are all laid out in logical order.

The next step will be to move asio_link out of bin/auth and into src/lib somewhere, but I'm leaving it where it is for now to make it easier to read the diff.

I'd very much appreciate another pair of eyes looking at what I've done so far...

comment:2 Changed 9 years ago by each

Oops, I forgot to say where to look for the diff...

svn diff -c 2934 svn://bind10.isc.org/svn/bind10/branches/trac327

comment:3 follow-up: Changed 9 years ago by stephen

Branch created: r2876
Code reviewed: r2941

As the request is for a look at what has been done so far, I've concentrated on the restructing using the stackless coroutines rather than checking the code is functionally correct.

src/bin/auth/yield.h
src/bin/auth/unyield.h
src/bin/auth/coroutine.h
These are files written by Christopher M. Kohlhoff and released under the Boost Software License V1.0.

Regarding the use of the macros, the code generated by them is convoluted - jumps into the middle of "if" blocks and "for" loops. I only got to understand it by writing a simple program, expanding it with the preprocessor, formatting the output, then stepping through it with gdb. However, they do appear to work.

src/bin/auth/asio_link.h
As the various constructors for DNSProvider and CheckinProvider are empty, you could declare the body of the function in the header file - it would allow for some optimisation.

Also, are default definitions for the operator() methods in these classes needed? If not (and I guess not as the documentation suggests that the classes are supposed to be abstract) and if this method overridden by a derived class, just declare the method abstract (along the lines of "void operator() const = 0" in the class definition). It will save a few bytes of generated code.

src/bin/auth/asio_link.cc

Class TCPServer
Should the checkin_callback_ and dns_callback_ object also be encapsulated in a boost::shared_ptr, as they must persist across calls to the operator() method.

Regarding the "fork" call in operator(), I'm not clear how the scheduling works, especially if multiple TCPServer children are created. Does a child run to completion before the parent? If not, what happens when a second child is created - is the previous object now unreferencable? Some detailed comments are required.

Class UDPServer
I didn't find the "yield continue" construct in the "Thinking Asynchronously in C++" article. What does it do? (Given the nested loops within the created by the macro expansions, is there a danger that it will cause control flow to transfer to an unexpected point in the code if used in the wrong place?)

src/bin/auth/auth_srv.h
OK

src/bin/auth/auth_srv.cc
The getCheckinProvider() and getDNSProvider() methods are sufficiently trivial that they could be placed in the header file.

src/bin/auth/tests/asio_link_unittest.cc
I've not really checked this in detail - most of the changes seem to do with the new signature for the IOService constructor.

src/bin/auth/main.cc
OK

Summary
The use of reenter/yield/throw does seem to produce code that is more easy to follow that splitting the processing sequence into multiple functions, in that the processing sequence is all in one place. But since this is something new (and, as noted above, the generated code is convoluted and the scheduling of coroutine calls possibly quite complex), there is the question as to whether this comes in the area of "too complex" (as we discussed at the last BIND-10 meeting). This is perhaps something that should be raised at the next conference call.

Regardless of the I think that there should be some significant detailed of what the macros do, either in in the code or in an associated README file. (A reference to a web site is insufficient as it may disappear at any time in the future.)

comment:4 in reply to: ↑ 3 Changed 9 years ago by each

Replying to stephen:

Also, are default definitions for the operator() methods in these classes needed? If not (and I guess not as the documentation suggests that the classes are supposed to be abstract) and if this method overridden by a derived class, just declare the method abstract (along the lines of "void operator() const = 0" in the class definition).

Thank you, that "= 0" was the part I'd forgotten. I kept getting compiler and linker errors, and putting the empty declarations into asio_link.cc was the only way I found to make them go away; declaring "void operator()() const = 0;" solves the problem much more elegantly.

Class TCPServer
Should the checkin_callback_ and dns_callback_ object also be encapsulated in a boost::shared_ptr, as they must persist across calls to the operator() method.

I'll clarify the comment on this point. The issue is that we need to use pointers for most things because when coroutines fork, the member variables in the coroutine class are copied not referenced. In my first attempt, I ended up with an async_receive_from() call copying data into a buffer and size variable, then yielding to the next operator() call, which tried to read the data from a *different* buffer and size variable. Turning those into shared pointers fixes the problem neatly.

The checkin pointers are pointers (and even if they weren't, they're never changed by a coroutine, so copying wouldn't cause problems). I don't think they need to be shared because they're allocated on the stack in auth_srv, but they can be if it turns out to be a good idea.

Regarding the "fork" call in operator(), I'm not clear how the scheduling works, especially if multiple TCPServer children are created. Does a child run to completion before the parent? If not, what happens when a second child is created - is the previous object now unreferencable? Some detailed comments are required.

Noted, I'll add comments about this. (To answer here, though, the child continues from the fork until it either completes or reaches a "yield". The parent is enqueued so that it can resume afterward--though if any ASIO events had been posted prior to the fork, they would be serviced before the parent.)

Class UDPServer
I didn't find the "yield continue" construct in the "Thinking Asynchronously in C++" article. What does it do?

Good catch. I was thinking it would yield to another coroutine, and then eventually resume at the top of the for(;;) loop... but now that you point it out, I'm not certain when, if ever, it would resume since it's not pending on an event--and if it did, it would unnecessarily reinitialize data_ and sender_. I'll fix this.

Regardless of the I think that there should be some significant detailed of what the macros do, either in in the code or in an associated README file. (A reference to a web site is insufficient as it may disappear at any time in the future.)

Okay. Thank you for your comments.

comment:5 Changed 9 years ago by each

  • Owner changed from each to stephen
  • Status changed from new to reviewing

In hopes of getting this merged fairly soon (given that I'm leaving to work on bind9 again soon) I'd like to get this interim-reviewed even though I don't think it quite meets the full definition of "done" (it needs a few more unit tests).

You previously reviewed changeset r2941. Between there and r2953, I moved the asio_link.cc code into a new library, src/lib/asiolink. From there to r2958, I created a new server binary, b10-recurse, in src/bin/recurse. And from there to r3125 I fiddled, refactored and refined until I had a working forwarder and a reasonable amount of inline documentation. It might simplify things to examine the work at each of those interim stopping-places.

Thanks for your help...

comment:6 Changed 9 years ago by stephen

  • Owner changed from stephen to each

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 "-46 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.

comment:7 Changed 9 years ago 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.)

comment:8 Changed 9 years ago by stephen

...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?)

Uh, good point. As to templates, although you'd want to keep the abstraction of IOEndpoint, the code for TCPEndpoint and UDPEndpoint could be combined in one templated class, with the type of endpoint as the template parameter.

comment:9 follow-up: Changed 9 years ago by stephen

The following is a review of the changes between r3136 and r3292, and lists outstanding issues (pulled down from earlier reviews where needed). Excluded are items that have been put down as tasks in the "R-Team" sprint of 20 Oct - 2 Nov 2010.

r3136 -> r3292

src/lib/asiolink/tests/asiolink_unittest.cc
TODO: (not covered in this review) Logic of the tests need to be checked.

src/lib/asiolink/ioaddress.h
OK, but shouldn't the toText() method be declared virtual (given that getFamily() is)?

src/bin/auth/auth_srv.h and src/bin/recurse/recursor.h
Methods have been documented, but processMessage() header needs arguments documenting.

Even though setVerbose() and getVerbose() methods are trivial, shouldn't we document them individually (and their parameters) for doxygen?

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

src/bin/auth/tests/mockups.h
Thanks for extracting the MockSession and related classes into a separate file.

Class MockSession: it would be useful to identify the methods (by means of comments) that have been added over and above the AbstractSession.

Class MockSession: Why are the "sent_msg" and "msg_destination" elements public?

src/bin/auth/main.cc and src/bin/recurse/main.cc
The example command line in the usage message should include "[-u]" as an option (although it is listed as an option in the text below).

Default port is still 5300 (and not 53). If this is only because it is an early version for testing, a comment should be added to that effect.

src/bin/recurse/recursor.cc
Member variables are still public.

src/lib/asiolink/README
TODO: Diagram has been added although text has not been greatly expanded. Would like to get opinion from someone else as to how useful the file is.

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

comment:10 in reply to: ↑ 9 ; follow-up: Changed 9 years ago by each

Replying to stephen:

src/lib/asiolink/ioaddress.h
OK, but shouldn't the toText() method be declared virtual (given that getFamily() is)?

OK.

src/bin/auth/auth_srv.h and src/bin/recurse/recursor.h
Methods have been documented, but processMessage() header needs arguments documenting.

Even though setVerbose() and getVerbose() methods are trivial, shouldn't we document them individually (and their parameters) for doxygen?

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

Agreed, but out of scope for this ticket.

src/bin/auth/tests/mockups.h
Class MockSession: it would be useful to identify the methods (by means of comments) that have been added over and above the AbstractSession.

I'm not actually sure, this part isn't my code, but I'll attempt it.

Class MockSession: Why are the "sent_msg" and "msg_destination" elements public?

Because they're used outside MockSession?, see for example auth_srv_unittest.cc line 391. But I can add "getter" functions.

src/bin/auth/main.cc and src/bin/recurse/main.cc
The example command line in the usage message should include "[-u]" as an option (although it is listed as an option in the text below).

Done.

Default port is still 5300 (and not 53). If this is only because it is an early version for testing, a comment should be added to that effect.

Okay. Added the comment above the definition of DNSPORT and the usage message now prints DNSPORT rather than having 5300 hardcoded.

src/bin/recurse/recursor.cc
Member variables are still public.

RecursorImpl? members can be seen by Recursor, yes. That doesn't seem wrong for a pimpl architecture, but we can add getter/setter functions if you like. Or define Recursor as a friend class (but we've been avoiding friend, as I understand it).

For the time being I just made all the members private that aren't currently being used outside the RecursorImpl? class. Ditto for AuthSrvImpl?.

Committed changes to r3310.

comment:11 in reply to: ↑ 10 Changed 9 years ago by each

Whoops, I skipped a few by mistake.

Replying to each:

src/bin/auth/auth_srv.h and src/bin/recurse/recursor.h
Methods have been documented, but processMessage() header needs arguments documenting.

Even though setVerbose() and getVerbose() methods are trivial, shouldn't we document them individually (and their parameters) for doxygen?

Done, see r3311.

comment:12 Changed 9 years ago by each

Note: I corrected a comment, and Jelte fixed some makefile problems so that "make distcheck" would run cleanly. We're up to r3325 now.

comment:13 follow-up: Changed 9 years ago by vorner

I synced the thing with trunk and tried to solve the conflicts (there were quite many), to easy up the later merging and so other branches originating from this one can sync too.

It would be nice if someone had a look, it was quite quite a big merge. It is in r3501.

comment:14 in reply to: ↑ 13 Changed 9 years ago by jreed

After merge it is missing
src/bin/recurse/tests/testdata/Makefile.am

comment:15 Changed 9 years ago by jreed

I also now need: (I didn't need last week)

Index: src/bin/recurse/Makefile.am
===================================================================
--- src/bin/recurse/Makefile.am (revision 3504)
+++ src/bin/recurse/Makefile.am (working copy)
@@ -6,6 +6,7 @@
 AM_CPPFLAGS += -I$(top_srcdir)/src/lib/cc -I$(top_builddir)/src/lib/cc
 AM_CPPFLAGS += -I$(top_srcdir)/src/lib/asiolink
 AM_CPPFLAGS += -I$(top_builddir)/src/lib/asiolink
+AM_CPPFLAGS += $(BOOST_INCLUDES)
 
 AM_CXXFLAGS = $(B10_CXXFLAGS)
 

comment:16 follow-up: Changed 9 years ago by jinmei

A couple of incomplete comments:

  • in asiolink_unittest.cc, we use the public interface of asio::io_service. basically "asiolink" is designed so that we don't have to refer to the original asio definitions. not sure how difficult it is, but we should replace it with IOService.
  • for a similar reason, including internal/{tcp,udp}dns.h in asiolink_unittest.cc is not good. btw, if we don't use asio::io_service it compiles without these header files, so at least including these doesn't make sense.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 9 years ago by each

  • for a similar reason, including internal/{tcp,udp}dns.h in asiolink_unittest.cc is not good. btw, if we don't use asio::io_service it compiles without these header files, so at least including these doesn't make sense.

IIRC my intention was to add more specific tests of tcpdns.cc and udpdns.cc routines, but I didn't make as much progress on testing as I had planned (unfortunately TDD went in the tank almost immediately after I started this work).

Probably those tests should still be written and they should include tcpdns.h and udpdns.h, but they should live in some other file than asiolink_unittest.cc.

Regarding the asio::io_service interface, I think that came out of the original asio_link_unittest.cc from trunk. I had assumed this was by design--the IOService class included a get_io_service() member to give access to the underlying implementation. I don't expect it would be very difficult to change this in the test code though.

comment:18 in reply to: ↑ 17 Changed 9 years ago by jinmei

Replying to each:

Regarding the asio::io_service interface, I think that came out of the original asio_link_unittest.cc from trunk. I had assumed this was by design--the IOService class included a get_io_service() member to give access to the underlying implementation.

No, it's not by design but a short term workaround. In fact, it's clearly documented at least in the latest trunk version:

    /// \brief Return the native \c io_service object used in this wrapper.
    ///
    /// This is a short term work around to support other BIND 10 modules
    /// that share the same \c io_service with the authoritative server.
    /// It will eventually be removed once the wrapper interface is
    /// generalized.
    asio::io_service& get_io_service() { return io_service_.get_io_service(); };

The only reason we needed get_io_service() is the cc session module still required the row asio::io_service.

For that matter, I also don't think it's by design that the naming of this function doesn't follow our guideline (it would have been getIOService or getASIOIOService or something). I noticed that when Jelte first introduced it, but since the idea was to obsolete it soon anyway I didn't touch that.

Changed 9 years ago by stephen

List of files to be reviewed before branch is merged back into trunk

comment:19 Changed 9 years ago by jelte

Files in my list are ok.

comment:20 follow-up: Changed 9 years ago by stephen

I've reviewed the files allocated to me in the attachment trac327_review.txt in r3791 of the #327 branch and have the following comments:

src/lib/asiolink/asiolink.cc
class RunningQuery
Don't understand the FIXME comment.

src/lib/asiolink/asiolink.h
class DNSService
This is a fairly important class, so a bit more detail in the header would be useful, as well as a stress on its importance. For example, the DNSService also handles configuration channel messages and is the core of the recursor.

class DNSServer
The header should note that the reason for the self_ pointer is that within the Boost library an object of this class is sometimes passed by reference and so the derived part of the class is lost. self_ allows the subclass components to still be accessed in this case.

class AsyncLookup
Although the calling object is copied into the handler, there appears to be no reason why it is passed by value to the constructor rather than by reference. The copy takes place when the AsyncLookup object is constructed and the argument is copied into the caller_ member, not during the argument passing.

class SimpleCallback
The header comment seems to be incomplete - it leads up to describing a function signature then omits the description.

src/lib/asiolink/internal/udpdns.h
class UDPServer
Needs a better class header (although there is good information within the class).

class UDPQuery
Comment style should be consistent through the file - the rest of the file uses C++ comment style, this uses C-style.

src/lib/datasrc/static_datasrc.cc
struct StaticDaraSrcImpl
We should include Ocean and Aharen in the list of authors.

src/lib/datasrc/tests/static_unittest.cc
Need to update the test to include Ocean and Aharen as authors.

comment:21 Changed 9 years ago by stephen

To clarify my comment about the DNSServer class - it should read:

class DNSServer
The header should note that the reason for the self_ pointer is that an object derived from this class is sometimes passed by reference and can be the subject of class slicing; in this case the derived part of the class is lost. self_ allows the subclass components to be accessed in this case.

comment:22 in reply to: ↑ 20 Changed 9 years ago by jreed

Replying to stephen:

src/lib/datasrc/static_datasrc.cc
struct StaticDaraSrcImpl
We should include Ocean and Aharen in the list of authors.

src/lib/datasrc/tests/static_unittest.cc
Need to update the test to include Ocean and Aharen as authors.

I don't see them mentioned in the logs for this branch. Did we lose some commit history for trac327? Or was there a neglect to mention them in the commit logs?

comment:23 Changed 9 years ago by zhanglikun

I have finished the review for the files of my part. it's ok.
Two monor suggestion which are not related with my review task.

  1. folder "src/lib/python/isc/util" should be removed now.
  2. I would like to suggest seperate the test case of RecursorConfig?(/bin/recurse/tests/recursor_unittest.cc) to another file.

comment:24 Changed 9 years ago by jelte

I found two duplicate lines (probably re-added due to a merge mismatch) and removed those in r3817

Addressed Stephen's comments in r3818, though some documentation descriptions may still be a bit on the sparse side, it's certainly better than nothing, and you are welcome to improve :)

Addressed Likun's comments in r3819, but only the second one. The directory src/lib/python/isc/util contains code that is heavily used. Did you really mean that directory?

Oh and I fixed a memleak reported by Jeremy that did not have a ticket (an object was clone()d twice) in r3820

comment:25 Changed 9 years ago by jelte

Ah, Jeremy pointed out to me it was utils, not util. Removed in r3821

comment:26 Changed 9 years ago by zhanglikun

Jelet, you are right, it should be utils, sorry for the misinput.

comment:27 Changed 9 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged, closing ticket.

Note: See TracTickets for help on using tickets.