Merge branch 'master' of git+ssh://git.bind10.isc.org/var/bind10/git/bind10

ChangeLog

@@ -1,3 +1,20 @@
+bind10-1.0.0beta2 released on May 3, 2013
+
+610.	[bug]		muks
+	When the sqlite3 program is not available on the system (in
+	PATH), we no longer attempt to run some tests which depend
+	on it.
+	(Trac #1909, git f85b274b85b57a094d33ca06dfbe12ae67bb47df)
+
+609.	[bug]		jinmei
+	Handled some rare error cases in DNS server classes correctly.
+	This fix specifically solves occasional crash of b10-auth due to
+	errors caused by TCP DNS clients.  Also, as a result of cleanups
+	with the fix, b10-auth should now be a little bit faster in
+	handling UDP queries: in some local experiments it ran about 5%
+	faster.
+	(Trac #2903, git 6d3e0f4b36a754248f8a03a29e2c36aef644cdcc)
+
 608.	[bug]		jinmei
 	b10-cmdctl: fixed a hangup problem on receiving the shutdown
 	command from bindctl.  Note, however, that cmdctl is defined as
@@ -52,7 +69,7 @@
 	(Trac #2770, git a622140d411b3f07a68a1451e19df36118a80650)
 
 602.	[bug]		tmark
-	Perdhcp will now exit gracefully if the command line argument for
+	Perfdhcp will now exit gracefully if the command line argument for
 	IP version (-4 or -6) does not match the command line argument
 	given for the server. Prior to this perfdhcp would core when given
 	an IP version of -6 but a valid IPv4 address for server.

Makefile.am

@@ -2,7 +2,7 @@ ACLOCAL_AMFLAGS = -I m4macros -I examples/m4 ${ACLOCAL_FLAGS}
 # ^^^^^^^^ This has to be the first line and cannot come later in this
 # Makefile.am due to some bork in some versions of autotools.
 
-SUBDIRS = compatcheck doc . src tests
+SUBDIRS = compatcheck doc . src tests m4macros
 USE_LCOV=@USE_LCOV@
 LCOV=@LCOV@
 GENHTML=@GENHTML@
@@ -46,7 +46,7 @@ endif
 clean-cpp-coverage:
 	@if [ $(USE_LCOV) = yes ] ; then \
 		$(LCOV) --directory . --zerocounters; \
-		rm -rf coverage/; \
+		rm -rf $(abs_top_srcdir)/coverage-cpp-html/; \
 	else \
 		echo "C++ code coverage not enabled at configuration time." ; \
 		echo "Use: ./configure --with-lcov" ; \

configure.ac

@@ -2,7 +2,7 @@
 # Process this file with autoconf to produce a configure script.
 
 AC_PREREQ([2.59])
-AC_INIT(bind10, 20130221, bind10-dev@isc.org)
+AC_INIT(bind10, 20130503, bind10-dev@isc.org)
 AC_CONFIG_SRCDIR(README)
 # serial-tests is not available in automake version before 1.13. In
 # automake 1.13 and higher, AM_PROG_INSTALL is undefined, so we'll check
@@ -388,8 +388,6 @@ In this case we will continue, but naming of python processes will not work.])
     fi
 fi
 
-# TODO: check for _sqlite3.py module
-
 # (g++ only check)
 # Python 3.2 has an unused parameter in one of its headers. This
 # has been reported, but not fixed as of yet, so we check if we need
@@ -1043,12 +1041,16 @@ AC_SUBST(GTEST_LDFLAGS)
 AC_SUBST(GTEST_LDADD)
 AC_SUBST(GTEST_SOURCE)
 
-dnl check for pkg-config itself so we don't try the m4 macro without pkg-config
+dnl check for pkg-config itself
 AC_CHECK_PROG(HAVE_PKG_CONFIG, pkg-config, yes, no)
 if test "x$HAVE_PKG_CONFIG" = "xno" ; then
   AC_MSG_ERROR(Please install pkg-config)
 fi
-PKG_CHECK_MODULES(SQLITE, sqlite3 >= 3.3.9, enable_features="$enable_features SQLite3")
+
+AX_SQLITE3_FOR_BIND10
+if test "x$have_sqlite" = "xyes" ; then
+  enable_features="$enable_features SQLite3"
+fi
 
 #
 # ASIO: we extensively use it as the C++ event management module.
@@ -1323,6 +1325,7 @@ AC_CONFIG_FILES([Makefile
                  tests/tools/perfdhcp/Makefile
                  tests/tools/perfdhcp/tests/Makefile
                  tests/tools/perfdhcp/tests/testdata/Makefile
+                 m4macros/Makefile
                  dns++.pc
                ])
 AC_OUTPUT([doc/version.ent

m4macros/Makefile.am

@@ -0,0 +1,2 @@
+EXTRA_DIST  = ax_boost_for_bind10.m4
+EXTRA_DIST += ax_sqlite3_for_bind10.m4

m4macros/ax_sqlite3_for_bind10.m4

@@ -0,0 +1,25 @@
+dnl @synopsis AX_SQLITE3_FOR_BIND10
+dnl
+dnl Test for the sqlite3 library and program, intended to be used within
+dnl BIND 10, and to test BIND 10.
+dnl
+dnl We use pkg-config to look for the sqlite3 library, so the sqlite3
+dnl development package with the .pc file must be installed.
+dnl
+dnl This macro sets SQLITE_CFLAGS and SQLITE_LIBS. It also sets
+dnl SQLITE3_PROGRAM to the path of the sqlite3 program, if it is found
+dnl in PATH.
+
+AC_DEFUN([AX_SQLITE3_FOR_BIND10], [
+
+PKG_CHECK_MODULES(SQLITE, sqlite3 >= 3.3.9,
+    have_sqlite="yes",
+    have_sqlite="no (sqlite3 not detected)")
+
+# Check for sqlite3 program
+AC_PATH_PROG(SQLITE3_PROGRAM, sqlite3, no)
+AM_CONDITIONAL(HAVE_SQLITE3_PROGRAM, test "x$SQLITE3_PROGRAM" != "xno")
+
+# TODO: check for _sqlite3.py module
+
+])dnl AX_SQLITE3_FOR_BIND10

src/bin/dbutil/tests/Makefile.am

@@ -5,5 +5,11 @@ SUBDIRS = . testdata
 noinst_SCRIPTS = dbutil_test.sh
 
 check-local:
+if HAVE_SQLITE3_PROGRAM
 	B10_LOCKFILE_DIR_FROM_BUILD=$(abs_top_builddir) \
 	$(SHELL) $(abs_builddir)/dbutil_test.sh
+else
+	@echo ""
+	@echo " **** The sqlite3 program is required to run dbutil tests **** "
+	@echo ""
+endif

src/lib/asiodns/asiodns_messages.mes

@@ -53,6 +53,78 @@ The asynchronous I/O code encountered an error when trying to send data to
 the specified address on the given protocol.  The number of the system
 error that caused the problem is given in the message.
 
+% ASIODNS_SYNC_UDP_CLOSE_FAIL failed to close a DNS/UDP socket: %1
+This is the same to ASIODNS_UDP_CLOSE_FAIL but happens on the
+"synchronous UDP server", mainly used for the authoritative DNS server
+daemon.
+
+% ASIODNS_TCP_ACCEPT_FAIL failed to accept TCP DNS connection: %1
+Accepting a TCP connection from a DNS client failed due to an error
+that could happen but should be rare.  The reason for the error is
+included in the log message.  The server still keeps accepting new
+connections, so unless it happens often it's probably okay to ignore
+this error.  If the shown error indicates something like "too many
+open files", it's probably because the run time environment is too
+restrictive on this limitation, so consider adjusing the limit using
+a tool such as ulimit.  If you see other types of errors too often,
+there may be something overlooked; please file a bug report in that case.
+
+% ASIODNS_TCP_CLEANUP_CLOSE_FAIL failed to close a DNS/TCP socket on port cleanup: %1
+A TCP DNS server tried to close a TCP socket (one created on accepting
+a new connection or is already unused) as a step of cleaning up the
+corresponding listening port, but it failed to do that.  This is
+generally an unexpected event and so is logged as an error.
+See also the description of ASIODNS_TCP_CLOSE_ACCEPTOR_FAIL.
+
+% ASIODNS_TCP_CLOSE_ACCEPTOR_FAIL failed to close listening TCP socket: %1
+A TCP DNS server tried to close a listening TCP socket (for accepting
+new connections) as a step of cleaning up the corresponding listening
+port (e.g., on server shutdown or updating port configuration), but it
+failed to do that.  This is generally an unexpected event and so is
+logged as an error.  See ASIODNS_TCP_CLOSE_FAIL on the implication of
+related system resources.
+
+% ASIODNS_TCP_CLOSE_FAIL failed to close DNS/TCP socket with a client: %1
+A TCP DNS server tried to close a TCP socket used to communicate with
+a client, but it failed to do that.  While closing a socket should
+normally be an error-free operation, there have been known cases where
+this happened with a "connection reset by peer" error.  This might be
+because of some odd client behavior, such as sending a TCP RST after
+establishing the connection and before the server closes the socket,
+but how exactly this could happen seems to be system dependent (i.e,
+it's not part of the standard socket API), so it's difficult to
+provide a general explanation.  In any case, it is believed that an
+error on closing a socket doesn't mean leaking system resources (the
+kernel should clean up any internal resource related to the socket,
+just reporting an error detected in the close call), but, again, it
+seems to be system dependent.  This message is logged at a debug level
+as it's known to happen and could be triggered by a remote node and it
+would be better to not be too verbose, but you might want to increase
+the log level and make sure there's no resource leak or other system
+level troubles when it's logged.
+
+% ASIODNS_TCP_GETREMOTE_FAIL failed to get remote address of a DNS TCP connection: %1
+A TCP DNS server tried to get the address and port of a remote client
+on a connected socket but failed.  It's expected to be rare but can
+still happen.  See also ASIODNS_TCP_READLEN_FAIL.
+
+% ASIODNS_TCP_READDATA_FAIL failed to get DNS data on a TCP socket: %1
+A TCP DNS server tried to read a DNS message (that follows a 2-byte
+length field) but failed.  It's expected to be rare but can still happen.
+See also ASIODNS_TCP_READLEN_FAIL.
+
+% ASIODNS_TCP_READLEN_FAIL failed to get DNS data length on a TCP socket: %1
+A TCP DNS server tried to get the length field of a DNS message (the first
+2 bytes of a new chunk of data) but failed.  This is generally expected to
+be rare but can still happen, e.g, due to an unexpected reset of the
+connection.  A specific reason for the failure is included in the log
+message.
+
+% ASIODNS_TCP_WRITE_FAIL failed to send DNS message over a TCP socket: %1
+A TCP DNS server tried to send a DNS message to a remote client but
+failed.  It's expected to be rare but can still happen.  See also
+ASIODNS_TCP_READLEN_FAIL.
+
 % ASIODNS_UDP_ASYNC_SEND_FAIL Error sending UDP packet to %1: %2
 The low-level ASIO library reported an error when trying to send a UDP
 packet in asynchronous UDP mode. This can be any error reported by
@@ -64,6 +136,23 @@ If you see a single occurrence of this message, it probably does not
 indicate any significant problem, but if it is logged often, it is probably
 a good idea to inspect your network traffic.
 
+% ASIODNS_UDP_CLOSE_FAIL failed to close a DNS/UDP socket: %1
+A UDP DNS server tried to close its UDP socket, but failed to do that.
+This is generally an unexpected event and so is logged as an error.
+
+% ASIODNS_UDP_RECEIVE_FAIL failed to receive UDP DNS packet: %1
+Receiving a UDP packet from a DNS client failed due to an error that
+could happen but should be very rare.  The server still keeps
+receiving UDP packets on this socket.  The reason for the error is
+included in the log message.  This log message is basically not
+expected to appear at all in practice; if it does, there may be some
+system level failure and other system logs may have to be checked.
+
+% ASIODNS_UDP_SYNC_RECEIVE_FAIL failed to receive UDP DNS packet: %1
+This is the same to ASIODNS_UDP_RECEIVE_FAIL but happens on the
+"synchronous UDP server", mainly used for the authoritative DNS server
+daemon.
+
 % ASIODNS_UDP_SYNC_SEND_FAIL Error sending UDP packet to %1: %2
 The low-level ASIO library reported an error when trying to send a UDP
 packet in synchronous UDP mode. See ASIODNS_UDP_ASYNC_SEND_FAIL for

src/lib/asiodns/dns_service.cc

@@ -58,9 +58,15 @@ public:
     template<class Ptr, class Server> void addServerFromFD(int fd, int af) {
         Ptr server(new Server(io_service_.get_io_service(), fd, af, checkin_,
                               lookup_, answer_));
-        server->setTCPRecvTimeout(tcp_recv_timeout_);
-        (*server)();
-        servers_.push_back(server);
+        startServer(server);
+    }
+
+    // SyncUDPServer has different constructor signature so it cannot be
+    // templated.
+    void addSyncUDPServerFromFD(int fd, int af) {
+        SyncUDPServerPtr server(new SyncUDPServer(io_service_.get_io_service(),
+                                                  fd, af, lookup_));
+        startServer(server);
     }
 
     void setTCPRecvTimeout(size_t timeout) {
@@ -72,6 +78,13 @@ public:
             (*it)->setTCPRecvTimeout(timeout);
         }
     }
+
+private:
+    void startServer(DNSServerPtr server) {
+        server->setTCPRecvTimeout(tcp_recv_timeout_);
+        (*server)();
+        servers_.push_back(server);
+    }
 };
 
 DNSService::DNSService(IOService& io_service, SimpleCallback* checkin,
@@ -95,8 +108,7 @@ void DNSService::addServerUDPFromFD(int fd, int af, ServerFlag options) {
                   << options);
     }
     if ((options & SERVER_SYNC_OK) != 0) {
-        impl_->addServerFromFD<DNSServiceImpl::SyncUDPServerPtr,
-            SyncUDPServer>(fd, af);
+        impl_->addSyncUDPServerFromFD(fd, af);
     } else {
         impl_->addServerFromFD<DNSServiceImpl::UDPServerPtr, UDPServer>(
             fd, af);

src/lib/asiodns/sync_udp_server.cc

@@ -39,18 +39,21 @@ namespace isc {
 namespace asiodns {
 
 SyncUDPServer::SyncUDPServer(asio::io_service& io_service, const int fd,
-                             const int af, asiolink::SimpleCallback* checkin,
-                             DNSLookup* lookup, DNSAnswer* answer) :
+                             const int af, DNSLookup* lookup) :
     output_buffer_(new isc::util::OutputBuffer(0)),
     query_(new isc::dns::Message(isc::dns::Message::PARSE)),
-    answer_(new isc::dns::Message(isc::dns::Message::RENDER)),
-    checkin_callback_(checkin), lookup_callback_(lookup),
-    answer_callback_(answer), stopped_(false)
+    udp_endpoint_(sender_), lookup_callback_(lookup),
+    resume_called_(false), done_(false), stopped_(false),
+    recv_callback_(boost::bind(&SyncUDPServer::handleRead, this, _1, _2))
 {
     if (af != AF_INET && af != AF_INET6) {
         isc_throw(InvalidParameter, "Address family must be either AF_INET "
                   "or AF_INET6, not " << af);
     }
+    if (!lookup) {
+        isc_throw(InvalidParameter, "null lookup callback given to "
+                  "SyncUDPServer");
+    }
     LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_FD_ADD_UDP).arg(fd);
     try {
         socket_.reset(new asio::ip::udp::socket(io_service));
@@ -61,59 +64,36 @@ SyncUDPServer::SyncUDPServer(asio::io_service& io_service, const int fd,
         // convert it
         isc_throw(IOError, exception.what());
     }
+    udp_socket_.reset(new UDPSocket<DummyIOCallback>(*socket_));
 }
 
 void
 SyncUDPServer::scheduleRead() {
-    socket_->async_receive_from(asio::buffer(data_, MAX_LENGTH), sender_,
-                                boost::bind(&SyncUDPServer::handleRead, this,
-                                            _1, _2));
+    socket_->async_receive_from(asio::mutable_buffers_1(data_, MAX_LENGTH),
+                                sender_, recv_callback_);
 }
 
 void
 SyncUDPServer::handleRead(const asio::error_code& ec, const size_t length) {
-    // Abort on fatal errors
     if (ec) {
         using namespace asio::error;
-        if (ec.value() != would_block && ec.value() != try_again &&
-            ec.value() != interrupted) {
+        const asio::error_code::value_type err_val = ec.value();
+
+        // See TCPServer::operator() for details on error handling.
+        if (err_val == operation_aborted || err_val == bad_descriptor) {
             return;
         }
+        if (err_val != would_block && err_val != try_again &&
+            err_val != interrupted) {
+            LOG_ERROR(logger, ASIODNS_UDP_SYNC_RECEIVE_FAIL).arg(ec.message());
+        }
     }
-    // Some kind of interrupt, spurious wakeup, or like that. Just try reading
-    // again.
     if (ec || length == 0) {
         scheduleRead();
         return;
     }
     // OK, we have a real packet of data. Let's dig into it!
 
-    // XXX: This is taken (and ported) from UDPSocket class. What the hell does
-    // it really mean?
-
-    // The UDP socket class has been extended with asynchronous functions
-    // and takes as a template parameter a completion callback class.  As
-    // UDPServer does not use these extended functions (only those defined
-    // in the IOSocket base class) - but needs a UDPSocket to get hold of
-    // the underlying Boost UDP socket - DummyIOCallback is used.  This
-    // provides the appropriate operator() but is otherwise functionless.
-    UDPSocket<DummyIOCallback> socket(*socket_);
-    UDPEndpoint endpoint(sender_);
-    IOMessage message(data_, length, socket, endpoint);
-    if (checkin_callback_ != NULL) {
-        (*checkin_callback_)(message);
-        if (stopped_) {
-            return;
-        }
-    }
-
-    // If we don't have a DNS Lookup provider, there's no point in
-    // continuing; we exit the coroutine permanently.
-    if (lookup_callback_ == NULL) {
-        scheduleRead();
-        return;
-    }
-
     // Make sure the buffers are fresh.  Note that we don't touch query_
     // because it's supposed to be cleared in lookup_callback_.  We should
     // eventually even remove this member variable (and remove it from
@@ -121,13 +101,13 @@ SyncUDPServer::handleRead(const asio::error_code& ec, const size_t length) {
     // implementation should be careful that it's the responsibility of
     // the callback implementation.  See also #2239).
     output_buffer_->clear();
-    answer_->clear(isc::dns::Message::RENDER);
 
     // Mark that we don't have an answer yet.
     done_ = false;
     resume_called_ = false;
 
     // Call the actual lookup
+    const IOMessage message(data_, length, *udp_socket_, udp_endpoint_);
     (*lookup_callback_)(message, query_, answer_, output_buffer_, this);
 
     if (!resume_called_) {
@@ -135,27 +115,14 @@ SyncUDPServer::handleRead(const asio::error_code& ec, const size_t length) {
                   "No resume called from the lookup callback");
     }
 
-    if (stopped_) {
-        return;
-    }
-
     if (done_) {
         // Good, there's an answer.
-        // Call the answer callback to render it.
-        (*answer_callback_)(message, query_, answer_, output_buffer_);
-
-        if (stopped_) {
-            return;
-        }
-
-        asio::error_code ec;
-        socket_->send_to(asio::buffer(output_buffer_->getData(),
-                                      output_buffer_->getLength()),
-                         sender_, 0, ec);
-        if (ec) {
+        socket_->send_to(asio::const_buffers_1(output_buffer_->getData(),
+                                               output_buffer_->getLength()),
+                         sender_, 0, ec_);
+        if (ec_) {
             LOG_ERROR(logger, ASIODNS_UDP_SYNC_SEND_FAIL).
-                      arg(sender_.address().to_string()).
-                      arg(ec.message());
+                      arg(sender_.address().to_string()).arg(ec_.message());
         }
     }
 
@@ -181,13 +148,13 @@ SyncUDPServer::stop() {
     /// for it won't be scheduled by io service not matter it is
     /// submit to io service before or after close call. And we will
     /// get bad_descriptor error.
-    socket_->close();
+    socket_->close(ec_);
     stopped_ = true;
+    if (ec_) {
+        LOG_ERROR(logger, ASIODNS_SYNC_UDP_CLOSE_FAIL).arg(ec_.message());
+    }
 }
 
-/// Post this coroutine on the ASIO service queue so that it will
-/// resume processing where it left off.  The 'done' parameter indicates
-/// whether there is an answer to return to the client.
 void
 SyncUDPServer::resume(const bool done) {
     resume_called_ = true;

src/lib/asiodns/sync_udp_server.h

@@ -25,10 +25,14 @@
 
 #include <dns/message.h>
 #include <asiolink/simple_callback.h>
+#include <asiolink/dummy_io_cb.h>
+#include <asiolink/udp_socket.h>
 #include <util/buffer.h>
 #include <exceptions/exceptions.h>
 
+#include <boost/function.hpp>
 #include <boost/noncopyable.hpp>
+#include <boost/scoped_ptr.hpp>
 
 #include <stdint.h>
 
@@ -39,29 +43,39 @@ namespace asiodns {
 ///
 /// That means, the lookup handler must provide the answer right away.
 /// This allows for implementation with less overhead, compared with
-/// the UDPClass.
+/// the \c UDPServer class.
 class SyncUDPServer : public DNSServer, public boost::noncopyable {
 public:
     /// \brief Constructor
+    ///
+    /// Due to the nature of this server, it's meaningless if the lookup
+    /// callback is NULL.  So the constructor explicitly rejects that case
+    /// with an exception.  Likewise, it doesn't take "checkin" or "answer"
+    /// callbacks.  In fact, calling "checkin" from receive callback does not
+    /// make sense for any of the DNSServer variants (see Trac #2935);
+    /// "answer" callback is simply unnecessary for this class because a
+    /// complete answer is built in the lookup callback (it's the user's
+    /// responsibility to guarantee that condition).
+    ///
     /// \param io_service the asio::io_service to work with
     /// \param fd the file descriptor of opened UDP socket
     /// \param af address family, either AF_INET or AF_INET6
-    /// \param checkin the callbackprovider for non-DNS events
-    /// \param lookup the callbackprovider for DNS lookup events
-    /// \param answer the callbackprovider for DNS answer events
+    /// \param lookup the callbackprovider for DNS lookup events (must not be
+    ///        NULL)
+    ///
     /// \throw isc::InvalidParameter if af is neither AF_INET nor AF_INET6
+    /// \throw isc::InvalidParameter lookup is NULL
     /// \throw isc::asiolink::IOError when a low-level error happens, like the
     ///     fd is not a valid descriptor.
     SyncUDPServer(asio::io_service& io_service, const int fd, const int af,
-                  isc::asiolink::SimpleCallback* checkin = NULL,
-                  DNSLookup* lookup = NULL, DNSAnswer* answer = NULL);
+                  DNSLookup* lookup);
 
     /// \brief Start the SyncUDPServer.
     ///
     /// This is the function operator to keep interface with other server
     /// classes. They need that because they're coroutines.
     virtual void operator()(asio::error_code ec = asio::error_code(),
-                    size_t length = 0);
+                            size_t length = 0);
 
     /// \brief Calls the lookup callback
     virtual void asyncLookup() {
@@ -114,22 +128,39 @@ private:
     // If it was OK to have just a buffer, not the wrapper class,
     // we could reuse the data_
     isc::util::OutputBufferPtr output_buffer_;
-    // Objects to hold the query message and the answer
+    // Objects to hold the query message and the answer.  The latter isn't
+    // used and only defined as a placeholder as the callback signature
+    // requires it.
     isc::dns::MessagePtr query_, answer_;
     // The socket used for the communication
     std::auto_ptr<asio::ip::udp::socket> socket_;
+    // Wrapper of socket_ in the form of asiolink::IOSocket.
+    // "DummyIOCallback" is not necessary for this class, but using the
+    // template is the easiest way to create a UDP instance of IOSocket.
+    boost::scoped_ptr<asiolink::UDPSocket<asiolink::DummyIOCallback> >
+    udp_socket_;
     // Place the socket puts the sender of a packet when it is received
     asio::ip::udp::endpoint sender_;
-    // Callbacks
-    const asiolink::SimpleCallback* checkin_callback_;
+    // Wrapper of sender_ in the form of asiolink::IOEndpoint.  It's set to
+    // refer to sender_ on initialization, and keeps the reference throughout
+    // this server class.
+    asiolink::UDPEndpoint udp_endpoint_;
+    // Callback
     const DNSLookup* lookup_callback_;
-    const DNSAnswer* answer_callback_;
     // Answers from the lookup callback (not sent directly, but signalled
     // through resume()
     bool resume_called_, done_;
     // This turns true when the server stops. Allows for not sending the
     // answer after we closed the socket.
     bool stopped_;
+    // Placeholder for error code object.  It will be passed to ASIO library
+    // to have it set in case of error.
+    asio::error_code ec_;
+    // The callback functor for internal asynchronous read event.  This is
+    // stateless (and it will be copied in the ASIO library anyway), so
+    // can be const
+    const boost::function<void(const asio::error_code&, size_t)>
+    recv_callback_;
 
     // Auxiliary functions
 
@@ -144,3 +175,7 @@ private:
 } // namespace asiodns
 } // namespace isc
 #endif // SYNC_UDP_SERVER_H
+
+// Local Variables:
+// mode: c++
+// End:

src/lib/asiodns/tcp_server.cc

@@ -14,13 +14,6 @@
 
 #include <config.h>
 
-#include <unistd.h>             // for some IPC/network system calls
-#include <netinet/in.h>
-#include <sys/socket.h>
-#include <errno.h>
-
-#include <boost/shared_array.hpp>
-
 #include <log/dummylog.h>
 
 #include <util/buffer.h>
@@ -32,6 +25,14 @@
 #include <asiodns/tcp_server.h>
 #include <asiodns/logger.h>
 
+#include <boost/shared_array.hpp>
+
+#include <cassert>
+#include <unistd.h>             // for some IPC/network system calls
+#include <netinet/in.h>
+#include <sys/socket.h>
+#include <errno.h>
+
 using namespace asio;
 using asio::ip::udp;
 using asio::ip::tcp;
@@ -100,41 +101,58 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
 
     CORO_REENTER (this) {
         do {
-            /// Create a socket to listen for connections
+            /// Create a socket to listen for connections (no-throw operation)
             socket_.reset(new tcp::socket(acceptor_->get_io_service()));
 
             /// Wait for new connections. In the event of non-fatal error,
             /// try again
             do {
                 CORO_YIELD acceptor_->async_accept(*socket_, *this);
-
-                // Abort on fatal errors
-                // TODO: Log error?
                 if (ec) {
                     using namespace asio::error;
-                    if (ec.value() != would_block && ec.value() != try_again &&
-                        ec.value() != connection_aborted &&
-                        ec.value() != interrupted) {
+                    const error_code::value_type err_val = ec.value();
+                    // The following two cases can happen when this server is
+                    // stopped: operation_aborted in case it's stopped after
+                    // starting accept().  bad_descriptor in case it's stopped
+                    // even before starting.  In these cases we should simply
+                    // stop handling events.
+                    if (err_val == operation_aborted ||
+                        err_val == bad_descriptor) {
                         return;
                     }
+                    // Other errors should generally be temporary and we should
+                    // keep waiting for new connections.  We log errors that
+                    // should really be rare and would only be caused by an
+                    // internal erroneous condition (not an odd remote
+                    // behavior).
+                    if (err_val != would_block && err_val != try_again &&
+                        err_val != connection_aborted &&
+                        err_val != interrupted) {
+                        LOG_ERROR(logger, ASIODNS_TCP_ACCEPT_FAIL).
+                            arg(ec.message());
+                    }
                 }
             } while (ec);
 
             /// Fork the coroutine by creating a copy of this one and
             /// scheduling it on the ASIO service queue.  The parent
-            /// will continue listening for DNS connections while the
+            /// will continue listening for DNS connections while the child
             /// handles the one that has just arrived.
             CORO_FORK io_.post(TCPServer(*this));
         } while (is_parent());
 
+        // From this point, we'll simply return on error, which will
+        // immediately trigger destroying this object, cleaning up all
+        // resources including any open sockets.
+
         /// Instantiate the data buffer that will be used by the
         /// asynchronous read call.
         data_.reset(new char[MAX_LENGTH]);
 
         /// Start a timer to drop the connection if it is idle
         if (*tcp_recv_timeout_ > 0) {
-            timeout_.reset(new asio::deadline_timer(io_));
-            timeout_->expires_from_now(
+            timeout_.reset(new asio::deadline_timer(io_)); // shouldn't throw
+            timeout_->expires_from_now( // consider any exception fatal.
                 boost::posix_time::milliseconds(*tcp_recv_timeout_));
             timeout_->async_wait(boost::bind(&do_timeout, boost::ref(*socket_),
                                  asio::placeholders::error));
@@ -144,29 +162,22 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
         CORO_YIELD async_read(*socket_, asio::buffer(data_.get(),
                               TCP_MESSAGE_LENGTHSIZE), *this);
         if (ec) {
-            socket_->close();
-            CORO_YIELD return;
+            LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_READLEN_FAIL).
+                arg(ec.message());
+            return;
         }
 
         /// Now read the message itself. (This is done in a different scope
         /// to allow inline variable declarations.)
         CORO_YIELD {
             InputBuffer dnsbuffer(data_.get(), length);
-            uint16_t msglen = dnsbuffer.readUint16();
+            const uint16_t msglen = dnsbuffer.readUint16();
             async_read(*socket_, asio::buffer(data_.get(), msglen), *this);
         }
-
         if (ec) {
-            socket_->close();
-            CORO_YIELD return;
-        }
-
-        // Due to possible timeouts and other bad behaviour, after the
-        // timely reads are done, there is a chance the socket has
-        // been closed already. So before we move on to the actual
-        // processing, check that, and stop if so.
-        if (!socket_->is_open()) {
-            CORO_YIELD return;
+            LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_READDATA_FAIL).
+                arg(ec.message());
+            return;
         }
 
         // Create an \c IOMessage object to store the query.
@@ -174,7 +185,12 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
         // (XXX: It would be good to write a factory function
         // that would quickly generate an IOMessage object without
         // all these calls to "new".)
-        peer_.reset(new TCPEndpoint(socket_->remote_endpoint()));
+        peer_.reset(new TCPEndpoint(socket_->remote_endpoint(ec)));
+        if (ec) {
+            LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_GETREMOTE_FAIL).
+                arg(ec.message());
+            return;
+        }
 
         // The TCP socket class has been extended with asynchronous functions
         // and takes as a template parameter a completion callback class.  As
@@ -183,7 +199,8 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
         // the underlying Boost TCP socket - DummyIOCallback is used.  This
         // provides the appropriate operator() but is otherwise functionless.
         iosock_.reset(new TCPSocket<DummyIOCallback>(*socket_));
-        io_message_.reset(new IOMessage(data_.get(), length, *iosock_, *peer_));
+        io_message_.reset(new IOMessage(data_.get(), length, *iosock_,
+                                        *peer_));
 
         // Perform any necessary operations prior to processing the incoming
         // packet (e.g., checking for queued configuration messages).
@@ -198,8 +215,7 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
         // If we don't have a DNS Lookup provider, there's no point in
         // continuing; we exit the coroutine permanently.
         if (lookup_callback_ == NULL) {
-            socket_->close();
-            CORO_YIELD return;
+            return;
         }
 
         // Reset or instantiate objects that will be needed by the
@@ -210,25 +226,24 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
 
         // Schedule a DNS lookup, and yield.  When the lookup is
         // finished, the coroutine will resume immediately after
-        // this point.
+        // this point.  On resume, this method should be called with its
+        // default parameter values (because of the signature of post()'s
+        // handler), so ec shouldn't indicate any error.
         CORO_YIELD io_.post(AsyncLookup<TCPServer>(*this));
+        assert(!ec);
 
         // The 'done_' flag indicates whether we have an answer
         // to send back.  If not, exit the coroutine permanently.
         if (!done_) {
             // TODO: should we keep the connection open for a short time
             // to see if new requests come in?
-            socket_->close();
-            CORO_YIELD return;
+            return;
         }
 
-        if (ec) {
-            CORO_YIELD return;
-        }
         // Call the DNS answer provider to render the answer into
         // wire format
-        (*answer_callback_)(*io_message_, query_message_,
-                            answer_message_, respbuf_);
+        (*answer_callback_)(*io_message_, query_message_, answer_message_,
+                            respbuf_);
 
         // Set up the response, beginning with two length bytes.
         lenbuf.writeUint16(respbuf_->getLength());
@@ -240,13 +255,22 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
         // (though we have nothing further to do, so the coroutine
         // will simply exit at that time).
         CORO_YIELD async_write(*socket_, bufs, *this);
+        if (ec) {
+            LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_WRITE_FAIL).
+                arg(ec.message());
+        }
 
-        // All done, cancel the timeout timer
+        // All done, cancel the timeout timer. if it throws, consider it fatal.
         timeout_->cancel();
 
         // TODO: should we keep the connection open for a short time
         // to see if new requests come in?
-        socket_->close();
+        socket_->close(ec);
+        if (ec) {
+            // close() should be unlikely to fail, but we've seen it fail once,
+            // so we log the event (at the lowest level of debug).
+            LOG_DEBUG(logger, 0, ASIODNS_TCP_CLOSE_FAIL).arg(ec.message());
+        }
     }
 }
 
@@ -259,14 +283,23 @@ TCPServer::asyncLookup() {
 }
 
 void TCPServer::stop() {
+    asio::error_code ec;
+
     /// we use close instead of cancel, with the same reason
     /// with udp server stop, refer to the udp server code
 
-    acceptor_->close();
+    acceptor_->close(ec);
+    if (ec) {
+        LOG_ERROR(logger, ASIODNS_TCP_CLOSE_ACCEPTOR_FAIL).arg(ec.message());
+    }
+
     // User may stop the server even when it hasn't started to
-    // run, in that that socket_ is empty
+    // run, in that case socket_ is empty
     if (socket_) {
-        socket_->close();
+        socket_->close(ec);
+        if (ec) {
+            LOG_ERROR(logger, ASIODNS_TCP_CLEANUP_CLOSE_FAIL).arg(ec.message());
+        }
     }
 }
 /// Post this coroutine on the ASIO service queue so that it will
@@ -275,6 +308,10 @@ void TCPServer::stop() {
 void
 TCPServer::resume(const bool done) {
     done_ = done;
+
+    // post() can throw due to memory allocation failure, but as like other
+    // cases of the entire BIND 10 implementation, we consider it fatal and
+    // let the exception be propagated.
     io_.post(*this);
 }
 

src/lib/asiodns/tests/dns_server_unittest.cc

@@ -117,7 +117,7 @@ public:
     DummyLookup() :
         allow_resume_(true)
     { }
-    void operator()(const IOMessage& io_message,
+    virtual void operator()(const IOMessage& io_message,
             isc::dns::MessagePtr message,
             isc::dns::MessagePtr answer_message,
             isc::util::OutputBufferPtr buffer,
@@ -147,6 +147,24 @@ class SimpleAnswer : public DNSAnswer, public ServerStopper {
 
 };
 
+/// \brief Mixture of DummyLookup and SimpleAnswer: build the answer in the
+/// lookup callback.  Used with SyncUDPServer.
+class SyncDummyLookup : public DummyLookup {
+public:
+    virtual void operator()(const IOMessage& io_message,
+                            isc::dns::MessagePtr message,
+                            isc::dns::MessagePtr answer_message,
+                            isc::util::OutputBufferPtr buffer,
+                            DNSServer* server) const
+    {
+        buffer->writeData(io_message.getData(), io_message.getDataSize());
+        stopServer();
+        if (allow_resume_) {
+            server->resume(true);
+        }
+    }
+};
+
 // \brief simple client, send one string to server and wait for response
 //  in case, server stopped and client can't get response, there is a timer wait
 //  for specified seconds (the value is just a estimate since server process logic is quite
@@ -353,6 +371,7 @@ class DNSServerTestBase : public::testing::Test {
             server_address_(ip::address::from_string(server_ip)),
             checker_(new DummyChecker()),
             lookup_(new DummyLookup()),
+            sync_lookup_(new SyncDummyLookup()),
             answer_(new SimpleAnswer()),
             udp_client_(new UDPClient(service,
                                       ip::udp::endpoint(server_address_,
@@ -375,6 +394,7 @@ class DNSServerTestBase : public::testing::Test {
             }
             delete checker_;
             delete lookup_;
+            delete sync_lookup_;
             delete answer_;
             delete udp_server_;
             delete udp_client_;
@@ -421,7 +441,8 @@ class DNSServerTestBase : public::testing::Test {
         asio::io_service service;
         const ip::address server_address_;
         DummyChecker* const checker_;
-        DummyLookup*  const lookup_;
+        DummyLookup* lookup_;     // we need to replace it in some cases
+        SyncDummyLookup*  const sync_lookup_;
         SimpleAnswer* const answer_;
         UDPClient*    const udp_client_;
         TCPClient*    const tcp_client_;
@@ -482,19 +503,34 @@ private:
 protected:
     // Using SetUp here so we can ASSERT_*
     void SetUp() {
-        const int fdUDP(getFd(SOCK_DGRAM));
-        ASSERT_NE(-1, fdUDP) << strerror(errno);
-        this->udp_server_ = new UDPServerClass(this->service, fdUDP, AF_INET6,
-                                               this->checker_, this->lookup_,
-                                               this->answer_);
-        const int fdTCP(getFd(SOCK_STREAM));
-        ASSERT_NE(-1, fdTCP) << strerror(errno);
-        this->tcp_server_ = new TCPServer(this->service, fdTCP, AF_INET6,
+        const int fd_udp(getFd(SOCK_DGRAM));
+        ASSERT_NE(-1, fd_udp) << strerror(errno);
+        this->udp_server_ = createServer(fd_udp, AF_INET6);
+        const int fd_tcp(getFd(SOCK_STREAM));
+        ASSERT_NE(-1, fd_tcp) << strerror(errno);
+        this->tcp_server_ = new TCPServer(this->service, fd_tcp, AF_INET6,
                                           this->checker_, this->lookup_,
                                           this->answer_);
     }
+
+    // A helper factory of the tested UDP server class: allow customization
+    // by template specialization.
+    UDPServerClass* createServer(int fd, int af) {
+        return (new UDPServerClass(this->service, fd, af,
+                                   this->checker_, this->lookup_,
+                                   this->answer_));
+    }
 };
 
+// Specialization for SyncUDPServer.  It needs to use SyncDummyLookup.
+template<>
+SyncUDPServer*
+FdInit<SyncUDPServer>::createServer(int fd, int af) {
+    delete this->lookup_;
+    this->lookup_ = new SyncDummyLookup;
+    return (new SyncUDPServer(this->service, fd, af, this->lookup_));
+}
+
 // This makes it the template as gtest wants it.
 template<class Parent>
 class DNSServerTest : public Parent { };
@@ -503,6 +539,11 @@ typedef ::testing::Types<FdInit<UDPServer>, FdInit<SyncUDPServer> >
     ServerTypes;
 TYPED_TEST_CASE(DNSServerTest, ServerTypes);
 
+// Some tests work only for SyncUDPServer, some others work only for
+// (non Sync)UDPServer.  We specialize these tests.
+typedef FdInit<UDPServer> AsyncServerTest;
+typedef FdInit<SyncUDPServer> SyncServerTest;
+
 typedef ::testing::Types<UDPServer, SyncUDPServer> UDPServerTypes;
 TYPED_TEST_CASE(DNSServerTestBase, UDPServerTypes);
 
@@ -513,7 +554,7 @@ asio::io_service* DNSServerTestBase<UDPServerClass>::current_service(NULL);
 
 // Test whether server stopped successfully after client get response
 // client will send query and start to wait for response, once client
-// get response, udp server will be stopped, the io service won't quit
+// get response, UDP server will be stopped, the io service won't quit
 // if udp server doesn't stop successfully.
 TYPED_TEST(DNSServerTest, stopUDPServerAfterOneQuery) {
     this->testStopServerByStopper(this->udp_server_, this->udp_client_,
@@ -532,8 +573,10 @@ TYPED_TEST(DNSServerTest, stopUDPServerBeforeItStartServing) {
 }
 
 
-// Test whether udp server stopped successfully during message check
-TYPED_TEST(DNSServerTest, stopUDPServerDuringMessageCheck) {
+// Test whether udp server stopped successfully during message check.
+// This only works for non-sync server; SyncUDPServer doesn't use checkin
+// callback.
+TEST_F(AsyncServerTest, stopUDPServerDuringMessageCheck) {
     this->testStopServerByStopper(this->udp_server_, this->udp_client_,
                                   this->checker_);
     EXPECT_EQ(std::string(""), this->udp_client_->getReceivedData());
@@ -548,12 +591,13 @@ TYPED_TEST(DNSServerTest, stopUDPServerDuringQueryLookup) {
     EXPECT_TRUE(this->serverStopSucceed());
 }
 
-// Test whether udp server stopped successfully during composing answer
-TYPED_TEST(DNSServerTest, stopUDPServerDuringPrepareAnswer) {
-    this->testStopServerByStopper(this->udp_server_, this->udp_client_,
-                                  this->answer_);
-    EXPECT_EQ(std::string(""), this->udp_client_->getReceivedData());
-    EXPECT_TRUE(this->serverStopSucceed());
+// Test whether UDP server stopped successfully during composing answer.
+// Only works for (non-sync) server because SyncUDPServer doesn't use answer
+// callback.
+TEST_F(AsyncServerTest, stopUDPServerDuringPrepareAnswer) {
+    testStopServerByStopper(udp_server_, udp_client_, answer_);
+    EXPECT_EQ(std::string(""), udp_client_->getReceivedData());
+    EXPECT_TRUE(serverStopSucceed());
 }
 
 void
@@ -565,7 +609,7 @@ stopServerManyTimes(DNSServer *server, unsigned int times) {
 
 // Test whether udp server stop interface can be invoked several times without
 // throw any exception
-TYPED_TEST(DNSServerTest, stopUDPServeMoreThanOnce) {
+TYPED_TEST(DNSServerTest, stopUDPServerMoreThanOnce) {
     ASSERT_NO_THROW({
         boost::function<void()> stop_server_3_times
             = boost::bind(stopServerManyTimes, this->udp_server_, 3);
@@ -668,14 +712,15 @@ TYPED_TEST(DNSServerTest, stopTCPServeMoreThanOnce) {
 TYPED_TEST(DNSServerTestBase, invalidFamily) {
     // We abuse DNSServerTestBase for this test, as we don't need the
     // initialization.
-    EXPECT_THROW(TypeParam(this->service, 0, AF_UNIX, this->checker_,
-                           this->lookup_, this->answer_),
-                 isc::InvalidParameter);
     EXPECT_THROW(TCPServer(this->service, 0, AF_UNIX, this->checker_,
                            this->lookup_, this->answer_),
                  isc::InvalidParameter);
 }
 
+TYPED_TEST(DNSServerTest, invalidFamilyUDP) {
+    EXPECT_THROW(this->createServer(0, AF_UNIX), isc::InvalidParameter);
+}
+
 // It raises an exception when invalid address family is passed
 TYPED_TEST(DNSServerTestBase, invalidTCPFD) {
     // We abuse DNSServerTestBase for this test, as we don't need the
@@ -694,7 +739,7 @@ TYPED_TEST(DNSServerTestBase, invalidTCPFD) {
                  isc::asiolink::IOError);
 }
 
-TYPED_TEST(DNSServerTestBase, DISABLED_invalidUDPFD) {
+TYPED_TEST(DNSServerTest, DISABLED_invalidUDPFD) {
     /*
      FIXME: The UDP server doesn't fail reliably with an invalid FD.
      We need to find a way to trigger it reliably (it seems epoll
@@ -702,14 +747,9 @@ TYPED_TEST(DNSServerTestBase, DISABLED_invalidUDPFD) {
      not the others, maybe we could make it run this at least on epoll-based
      systems).
     */
-    EXPECT_THROW(TypeParam(this->service, -1, AF_INET, this->checker_,
-                           this->lookup_, this->answer_),
-                 isc::asiolink::IOError);
+    EXPECT_THROW(this->createServer(-1, AF_INET), isc::asiolink::IOError);
 }
 
-// A specialized test type for SyncUDPServer.
-typedef FdInit<SyncUDPServer> SyncServerTest;
-
 // Check it rejects some of the unsupported operations
 TEST_F(SyncServerTest, unsupportedOps) {
     EXPECT_THROW(udp_server_->clone(), isc::Unexpected);
@@ -723,4 +763,10 @@ TEST_F(SyncServerTest, mustResume) {
                  isc::Unexpected);
 }
 
+// SyncUDPServer doesn't allow NULL lookup callback.
+TEST_F(SyncServerTest, nullLookupCallback) {
+    EXPECT_THROW(SyncUDPServer(service, 0, AF_INET, NULL),
+                 isc::InvalidParameter);
+}
+
 }

src/lib/asiodns/udp_server.cc

@@ -82,8 +82,8 @@ struct UDPServer::Data {
          answer_callback_(answer)
     {
         if (af != AF_INET && af != AF_INET6) {
-            isc_throw(InvalidParameter, "Address family must be either AF_INET "
-                      "or AF_INET6, not " << af);
+            isc_throw(InvalidParameter, "Address family must be either AF_INET"
+                      " or AF_INET6, not " << af);
         }
         LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_FD_ADD_UDP).arg(fd);
         try {
@@ -212,14 +212,19 @@ UDPServer::operator()(asio::error_code ec, size_t length) {
                     buffer(data_->data_.get(), MAX_LENGTH), *data_->sender_,
                     *this);
 
-                // Abort on fatal errors
-                // TODO: add log
+                // See TCPServer::operator() for details on error handling.
                 if (ec) {
                     using namespace asio::error;
-                    if (ec.value() != would_block && ec.value() != try_again &&
-                        ec.value() != interrupted) {
+                    const error_code::value_type err_val = ec.value();
+                    if (err_val == operation_aborted ||
+                        err_val == bad_descriptor) {
                         return;
                     }
+                    if (err_val != would_block && err_val != try_again &&
+                        err_val != interrupted) {
+                        LOG_ERROR(logger, ASIODNS_UDP_RECEIVE_FAIL).
+                            arg(ec.message());
+                    }
                 }
 
             } while (ec || length == 0);
@@ -270,7 +275,7 @@ UDPServer::operator()(asio::error_code ec, size_t length) {
         // If we don't have a DNS Lookup provider, there's no point in
         // continuing; we exit the coroutine permanently.
         if (data_->lookup_callback_ == NULL) {
-            CORO_YIELD return;
+            return;
         }
 
         // Instantiate objects that will be needed by the
@@ -287,7 +292,7 @@ UDPServer::operator()(asio::error_code ec, size_t length) {
         // The 'done_' flag indicates whether we have an answer
         // to send back.  If not, exit the coroutine permanently.
         if (!data_->done_) {
-            CORO_YIELD return;
+            return;
         }
 
         // Call the DNS answer provider to render the answer into
@@ -322,6 +327,8 @@ UDPServer::asyncLookup() {
 /// Stop the UDPServer
 void
 UDPServer::stop() {
+    asio::error_code ec;
+
     /// Using close instead of cancel, because cancel
     /// will only cancel the asynchronized event already submitted
     /// to io service, the events post to io service after
@@ -330,7 +337,10 @@ UDPServer::stop() {
     /// for it won't be scheduled by io service not matter it is
     /// submit to io service before or after close call. And we will
     //  get bad_descriptor error.
-    data_->socket_->close();
+    data_->socket_->close(ec);
+    if (ec) {
+        LOG_ERROR(logger, ASIODNS_UDP_CLOSE_FAIL).arg(ec.message());
+    }
 }
 
 /// Post this coroutine on the ASIO service queue so that it will
@@ -339,7 +349,7 @@ UDPServer::stop() {
 void
 UDPServer::resume(const bool done) {
     data_->done_ = done;
-    data_->io_.post(*this);
+    data_->io_.post(*this);  // this can throw, but can be considered fatal.
 }
 
 } // namespace asiodns

src/lib/asiolink/io_service.cc

@@ -24,6 +24,23 @@
 namespace isc {
 namespace asiolink {
 
+namespace {
+// A trivial wrapper for boost::function.  SunStudio doesn't seem to be capable
+// of handling a boost::function object if directly passed to
+// io_service::post().
+class CallbackWrapper {
+public:
+    CallbackWrapper(const boost::function<void()>& callback) :
+        callback_(callback)
+    {}
+    void operator()() {
+        callback_();
+    }
+private:
+    boost::function<void()> callback_;
+};
+}
+
 class IOServiceImpl {
 private:
     IOServiceImpl(const IOService& source);
@@ -64,7 +81,8 @@ public:
     /// generalized.
     asio::io_service& get_io_service() { return io_service_; };
     void post(const boost::function<void ()>& callback) {
-        io_service_.post(callback);
+        const CallbackWrapper wrapper(callback);
+        io_service_.post(wrapper);
     }
 private:
     asio::io_service io_service_;

src/lib/datasrc/Makefile.am

@@ -39,6 +39,9 @@ libb10_datasrc_la_SOURCES += master_loader_callbacks.cc
 libb10_datasrc_la_SOURCES += rrset_collection_base.h rrset_collection_base.cc
 libb10_datasrc_la_SOURCES += zone_loader.h zone_loader.cc
 libb10_datasrc_la_SOURCES += cache_config.h cache_config.cc
+libb10_datasrc_la_SOURCES += zone_table_accessor.h
+libb10_datasrc_la_SOURCES += zone_table_accessor_cache.h
+libb10_datasrc_la_SOURCES += zone_table_accessor_cache.cc
 nodist_libb10_datasrc_la_SOURCES = datasrc_messages.h datasrc_messages.cc
 libb10_datasrc_la_LDFLAGS = -no-undefined -version-info 1:0:1
 

src/lib/datasrc/memory/memory_messages.mes

@@ -126,13 +126,13 @@ RRset is split into multiple locations is not supported yet.
 Debug information. A zone object for this zone is being searched for in the
 in-memory data source.
 
-% DATASRC_MEMORY_MEM_LOAD_FROM_FILE loading zone '%1/%2' from file '%3'
-Debug information. The content of master file is being loaded into the memory.
-
 % DATASRC_MEMORY_MEM_LOAD_FROM_DATASRC loading zone '%1/%2' from other data source
 Debug information. The content of another  data source is being loaded
 into the memory.
 
+% DATASRC_MEMORY_MEM_LOAD_FROM_FILE loading zone '%1/%2' from file '%3'
+Debug information. The content of master file is being loaded into the memory.
+
 % DATASRC_MEMORY_MEM_NO_NSEC3PARAM NSEC3PARAM is missing for NSEC3-signed zone %1/%2
 The in-memory data source has loaded a zone signed with NSEC3 RRs,
 but it doesn't have a NSEC3PARAM RR at the zone origin.  It's likely that

src/lib/datasrc/tests/Makefile.am

@@ -60,6 +60,7 @@ run_unittests_SOURCES += client_list_unittest.cc
 run_unittests_SOURCES += master_loader_callbacks_test.cc
 run_unittests_SOURCES += zone_loader_unittest.cc
 run_unittests_SOURCES += cache_config_unittest.cc
+run_unittests_SOURCES += zone_table_accessor_unittest.cc
 
 # We need the actual module implementation in the tests (they are not part
 # of libdatasrc)

src/lib/datasrc/tests/zone_table_accessor_unittest.cc

@@ -0,0 +1,112 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <datasrc/zone_table_accessor_cache.h>
+#include <datasrc/cache_config.h>
+#include <datasrc/tests/mock_client.h>
+
+#include <exceptions/exceptions.h>
+
+#include <cc/data.h>
+
+#include <dns/name.h>
+
+#include <gtest/gtest.h>
+
+using namespace isc::dns;
+using namespace isc::datasrc;
+using namespace isc::datasrc::internal;
+using isc::data::Element;
+using isc::datasrc::unittest::MockDataSourceClient;
+
+namespace {
+
+// This test checks the abstract ZoneTableAccessor interface using
+// ZoneTableAccessorCache instances, thereby testing the top level interface
+// and the derived class behavior.  If ZoneTableAccessorCache becomes more
+// complicated we may have to separate some test cases into dedicated test
+// fixture.
+class ZoneTableAccessorTest : public ::testing::Test {
+protected:
+    ZoneTableAccessorTest() :
+        // The paths of the zone files are dummy and don't even exist,
+        // but it doesn't matter in this test.
+        config_spec_(Element::fromJSON(
+                         "{\"cache-enable\": true,"
+                         " \"params\": "
+                         "  {\"example.com\": \"/example-com.zone\","
+                         "   \"example.org\": \"/example-org.zone\"}"
+                         "}")),
+        cache_config_("MasterFiles", NULL, *config_spec_, true),
+        accessor_(cache_config_)
+    {}
+
+private:
+    const isc::data::ConstElementPtr config_spec_;
+    const CacheConfig cache_config_;
+protected:
+    ZoneTableAccessorCache accessor_;
+};
+
+TEST_F(ZoneTableAccessorTest, iteratorFromCache) {
+    // Confirm basic iterator behavior.
+    ZoneTableAccessor::IteratorPtr it = accessor_.getIterator();
+    ASSERT_TRUE(it);
+    EXPECT_FALSE(it->isLast());
+    EXPECT_EQ(0, it->getCurrent().index); // index is always 0 for this version
+    EXPECT_EQ(Name("example.com"), it->getCurrent().origin);
+
+    it->next();
+    EXPECT_FALSE(it->isLast());
+    EXPECT_EQ(0, it->getCurrent().index);
+    EXPECT_EQ(Name("example.org"), it->getCurrent().origin);
+
+    it->next();                 // shouldn't cause disruption
+    EXPECT_TRUE(it->isLast());
+
+    // getCurrent() and next() will be rejected once iterator reaches the end
+    EXPECT_THROW(it->getCurrent(), isc::InvalidOperation);
+    EXPECT_THROW(it->next(), isc::InvalidOperation);
+}
+
+TEST_F(ZoneTableAccessorTest, emptyTable) {
+    // Empty zone table is possible, while mostly useless.
+    const CacheConfig empty_config(
+        "MasterFiles", NULL, *Element::fromJSON(
+            "{\"cache-enable\": true, \"params\": {}}"), true);
+    ZoneTableAccessorCache accessor(empty_config);
+    ZoneTableAccessor::IteratorPtr it = accessor.getIterator();
+    ASSERT_TRUE(it);
+    EXPECT_TRUE(it->isLast());
+    EXPECT_THROW(it->getCurrent(), isc::InvalidOperation);
+    EXPECT_THROW(it->next(), isc::InvalidOperation);
+}
+
+TEST_F(ZoneTableAccessorTest, disabledTable) {
+    // Zone table based on disabled cache is effectively empty.
+    const char* zones[] = { "example.org.", "example.com.", NULL };
+    MockDataSourceClient mock_client(zones);
+    const CacheConfig mock_config(
+        "mock", &mock_client, *Element::fromJSON(
+            "{\"cache-enable\": false,"
+            " \"cache-zones\": [\"example.com\", \"example.org\"]}"), true);
+    ZoneTableAccessorCache accessor(mock_config);
+    ZoneTableAccessor::IteratorPtr it = accessor.getIterator();
+    ASSERT_TRUE(it);
+    EXPECT_TRUE(it->isLast());
+    EXPECT_THROW(it->getCurrent(), isc::InvalidOperation);
+    EXPECT_THROW(it->next(), isc::InvalidOperation);
+}
+
+}

src/lib/datasrc/zone_table_accessor.h

@@ -0,0 +1,212 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef DATASRC_ZONE_TABLE_ACCESSOR_H
+#define DATASRC_ZONE_TABLE_ACCESSOR_H
+
+#include <dns/name.h>
+
+#include <boost/noncopyable.hpp>
+#include <boost/shared_ptr.hpp>
+
+#include <stdint.h>
+
+namespace isc {
+namespace datasrc {
+
+/// \brief Information of a zone stored in a data source zone table.
+///
+/// This is a straightforward composite type that represents an entry of
+/// the conceptual zone table referenced by \c ZoneTableAccessor.
+/// An object of this structure is specifically intended to be returned by
+/// \c ZoneTableIterator.
+///
+/// This is essentially a read-only tuple; only created by
+/// \c ZoneTableAccessor, and once created it will be immutable.
+///
+/// \note Once Trac #2144 is completed, this struct must be defined as
+/// non-assignable because it has a const member variable.
+struct ZoneSpec {
+    /// \brief Constructor.
+    ZoneSpec(uint32_t index_param, const dns::Name& origin_param) :
+        index(index_param), origin(origin_param)
+    {}
+
+    /// \brief Numeric zone index.
+    ///
+    /// In the current initial version, this field is just a placeholder.
+    /// In the future, we'll probably define it as a unique index in the table
+    /// for that particular zone so that applications can distinguish
+    /// and specify different zones efficiently. Until it's fixed, this field
+    /// shouldn't be used by applications.
+    const uint32_t index;
+
+    /// \brief The origin name of the zone.
+    const dns::Name origin;
+};
+
+/// \brief A simple iterator of zone table.
+///
+/// This is an abstract base class providing simple iteration operation
+/// over zones stored in a data source.  A concrete object of this class
+/// is expected to be returned by \c ZoneTableAccessor::getIterator().
+///
+/// The interface is intentionally simplified and limited: it works
+/// "forward-only", i.e, only goes from begin to end one time; it's not
+/// copyable, assignable, nor comparable.  For the latter reasons it's not
+/// compatible with standard iterator traits.  It's simplified because it's
+/// not clear what kind of primitive can be used in specific data sources.
+/// In particular, iteration in a database-based data source would be very
+/// restrictive.  So it's better to begin with minimal guaranteed features
+/// at the base class.  If we find it possible to loosen the restriction
+/// as we implement more derived versions, we may extend the features later.
+///
+/// Likewise, this iterator does not guarantee the ordering of the zones
+/// returned by \c getCurrent().  It's probably possible to ensure some
+/// sorted order, but until we can be sure it's the case for many cases
+/// in practice, we'll not rely on it.
+///
+/// A concrete object of this class is created by specific derived
+/// implementation for the corresponding data source.  The implementation
+/// must ensure the iterator is located at the "beginning" of the zone table,
+/// and that subsequent calls to \c next() go through all the zones
+/// one by one, until \c isLast() returns \c true.  The implementation must
+/// support the concept of "empty table"; in that case \c isLast() will
+/// return \c true from the beginning.
+class ZoneTableIterator : boost::noncopyable {
+protected:
+    /// \brief The constructor.
+    ///
+    /// This class is not expected to be instantiated directly, so the
+    /// constructor is hidden from normal applications as protected.
+    ZoneTableIterator() {}
+
+public:
+    /// \brief The destructor.
+    virtual ~ZoneTableIterator() {}
+
+    /// \brief Return if the iterator reaches the end of the zone table.
+    virtual bool isLast() const = 0;
+
+    /// \brief Move the iterator to the next zone of the table.
+    ///
+    /// This method must not be called once the iterator reaches the end
+    /// of the table.
+    ///
+    /// \throw InvalidOperation called after reaching the end of table.
+    void next() {
+        // Perform common check, and delegate the actual work to the protected
+        // method.
+        if (isLast()) {
+            isc_throw(InvalidOperation,
+                      "next() called on iterator beyond end of zone table");
+        }
+        nextImpl();
+    }
+
+    /// \brief Return the information of the zone at which the iterator is
+    /// currently located in the form of \c ZoneSpec.
+    ///
+    /// This method must not be called once the iterator reaches the end
+    /// of the zone table.
+    ///
+    /// \throw InvalidOperation called after reaching the end of table.
+    ///
+    /// \return Information of the "current" zone.
+    ZoneSpec getCurrent() const {
+        // Perform common check, and delegate the actual work to the protected
+        // method.
+        if (isLast()) {
+            isc_throw(InvalidOperation,
+                      "getCurrent() called on iterator beyond "
+                      "end of zone table");
+        }
+        return (getCurrentImpl());
+    }
+
+protected:
+    /// \brief Actual implementation of \c next().
+    ///
+    /// Each derived class must provide the implementation of \c next()
+    /// in its data source specific form, except for the common
+    /// validation check.
+    virtual void nextImpl() = 0;
+
+    /// \brief Actual implementation of \c getCurrent().
+    ///
+    /// Each derived class must provide the implementation of
+    /// \c getCurrent() in its data source specific form, except for the
+    /// common validation check.
+    virtual ZoneSpec getCurrentImpl() const = 0;
+};
+
+/// \brief An abstract accessor to conceptual zone table for a data source.
+///
+/// This is an abstract base class providing common interfaces to get access
+/// to a conceptual "zone table" corresponding to a specific data source.
+/// A zone table would contain a set of information about DNS zones stored in
+/// the data source.  It's "conceptual" in that the actual form of the
+/// information is specific to the data source implementation.
+///
+/// The initial version of this class only provides one simple feature:
+/// iterating over the table so that an application can get a list of
+/// all zones of a specific data source (of a specific RR class).  In
+/// future, this class will be extended so that, e.g., applications can
+/// add or remove zones.
+///
+/// \note It may make sense to move \c DataSourceClient::createZone()
+/// and \c DataSourceClient::deleteZone() to this class.
+class ZoneTableAccessor : boost::noncopyable {
+protected:
+    /// \brief The constructor.
+    ///
+    /// This class is not expected to be instantiated directly, so the
+    /// constructor is hidden from normal applications as protected.
+    ZoneTableAccessor() {}
+
+public:
+    /// \brief Shortcut type for a smart pointer of \c ZoneTableIterator
+    typedef boost::shared_ptr<ZoneTableIterator> IteratorPtr;
+
+    /// \brief The destructor.
+    virtual ~ZoneTableAccessor() {}
+
+    /// \brief Return a zone table iterator.
+    ///
+    /// In general, the specific implementation of the iterator object would
+    /// contain some form of reference to the underlying data source
+    /// (e.g., a database connection or a pointer to memory region), which
+    /// would be valid only until the object that created the instance of
+    /// the accessor is destroyed.  The iterator must not be used beyond
+    /// the lifetime of such a creator object, and normally it's expected to
+    /// be even more ephemeral: it would be created by this method in a
+    /// single method or function and only used in that limited scope.
+    ///
+    /// \throw std::bad_alloc Memory allocation for the iterator object failed.
+    /// \throw Others There will be other cases as more implementations
+    /// are added (in this initial version, it's not really decided yet).
+    ///
+    /// \return A smart pointer to a newly created iterator object.  Once
+    /// returned, the \c ZoneTableAccessor effectively releases its ownership.
+    virtual IteratorPtr getIterator() const = 0;
+};
+
+}
+}
+
+#endif  // DATASRC_ZONE_TABLE_ACCESSOR_H
+
+// Local Variables:
+// mode: c++
+// End:

src/lib/datasrc/zone_table_accessor_cache.cc

@@ -0,0 +1,60 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <datasrc/zone_table_accessor_cache.h>
+#include <datasrc/cache_config.h>
+
+#include <exceptions/exceptions.h>
+
+namespace isc {
+namespace datasrc {
+namespace internal {
+
+namespace {
+// This is a straightforward wrapper of CacheConfig::ConstZoneIterator to
+// implement ZoneTableIterator interfaces.
+class ZoneTableIteratorCache : public ZoneTableIterator {
+public:
+    ZoneTableIteratorCache(const CacheConfig& config) :
+        it_(config.begin()),
+        it_end_(config.end())
+    {}
+
+    virtual void nextImpl() {
+        ++it_;
+    }
+
+    virtual ZoneSpec getCurrentImpl() const {
+        return (ZoneSpec(0, it_->first)); // index is always 0 in this version.
+    }
+
+    virtual bool isLast() const {
+        return (it_ == it_end_);
+    }
+
+private:
+    CacheConfig::ConstZoneIterator it_;
+    CacheConfig::ConstZoneIterator const it_end_;
+};
+}
+
+ZoneTableAccessor::IteratorPtr
+ZoneTableAccessorCache::getIterator() const {
+    return (ZoneTableAccessor::IteratorPtr(
+                new ZoneTableIteratorCache(config_)));
+}
+
+}
+}
+}

src/lib/datasrc/zone_table_accessor_cache.h

@@ -0,0 +1,76 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef DATASRC_ZONE_TABLE_ACCESSOR_CACHE_H
+#define DATASRC_ZONE_TABLE_ACCESSOR_CACHE_H
+
+#include <datasrc/zone_table_accessor.h>
+
+#include <boost/noncopyable.hpp>
+
+namespace isc {
+namespace datasrc {
+namespace internal {
+class CacheConfig;
+
+/// \brief A \c ZoneTableAccessor implementation for in-memory cache.
+///
+/// This class implements the \c ZoneTableAccessor interface for in-memory
+/// cache.  Its conceptual table consists of the zones that are specified
+/// to be loaded into memory in configuration.  Note that these zones
+/// may or may not actually be loaded in memory.  In fact, this class object
+/// is intended to be used by applications that load these zones into memory,
+/// so that the application can get a list of zones to be loaded.  Also, even
+/// after loading, some zone may still not be loaded, e.g., due to an error
+/// in the corresponding zone file.
+///
+/// An object of this class is expected to be returned by
+/// \c ConfigurableClientList.  Normal applications shouldn't instantiate
+/// this class directly.  It's still defined to be publicly visible for
+/// testing purposes but, to clarify the intent, it's hidden in the
+/// "internal" namespace.
+class ZoneTableAccessorCache : public ZoneTableAccessor {
+public:
+    /// \brief Constructor.
+    ///
+    /// This class takes a \c CacheConfig object and holds it throughout
+    /// its lifetime.  The caller must ensure that the configuration is
+    /// valid throughout the lifetime of this accessor object.
+    ///
+    /// \throw None
+    ///
+    /// \param config The cache configuration that the accessor refers to.
+    ZoneTableAccessorCache(const CacheConfig& config) : config_(config) {}
+
+    /// \brief In-memory cache version of \c getIterator().
+    ///
+    /// As returned from this version of iterator, \c ZoneSpec::index
+    /// will always be set to 0 at the moment.
+    ///
+    /// \throw None except std::bad_alloc in case of memory allocation failure
+    virtual IteratorPtr getIterator() const;
+
+private:
+    const CacheConfig& config_;
+};
+
+}
+}
+}
+
+#endif  // DATASRC_ZONE_TABLE_ACCESSOR_CACHE_H
+
+// Local Variables:
+// mode: c++
+// End: