Merge branch 'master' into trac466

ChangeLog

@@ -1,3 +1,33 @@
+  147.	[bug]		jinmei
+	python/isc/config: Fixed a bug that importing custom configuration
+	(in b10-config.db) of a remote module didn't work.
+	(Trac #478, git ea4a481)
+
+  146.	[func]		jelte
+	Command arguments were not validated internally against their
+	specifications. This change fixes that (on the C++ side, Python
+	side depends on an as yet planned addition). Note: this is only
+	an added internal check, the cli already checks format.
+	(Trac #473, git 5474eba181cb2fdd80e2b2200e072cd0a13a4e52)
+
+  145.	[func]*		jinmei
+	b10-auth: added a new command 'loadzone' for (re)loading a
+	specific zone.  The command syntax is generic but it is currently
+	only feasible for class IN in memory data source.  To reload a
+	zone "example.com" via bindctl, execute the command as follows:
+	> Auth loadzone origin = example.com
+	(Trac #467)
+
+  144.	[build]		jinmei
+	Introduced a workaround for clang++ build on FreeBSD (and probably
+	some other OSes).  If building BIND 10 fails with clang++ due to
+	a link error about "__dso_handle", try again from the configure
+	script with CXX_LIBTOOL_LDFLAGS=-L/usr/lib (the path actually
+	doesn't matter; the important part is the -L flag).  This
+	workaround is not automatically enabled as it's difficult to
+	detect the need for it dynamically, and must be enabled via the
+	variable by hand. (Trac #474, git cfde436)
+
   143.	[build]		jinmei
 	Fixed build problems with clang++ in unit tests due to recent
 	changes.  No behavior change. (Trac #448, svn r4133)
@@ -781,8 +811,12 @@ bind10-devel-20100421 released on April 21, 2010
 bind10-devel-20100319 released on March 19, 2010
 
 For complete code revision history, see http://bind10.isc.org/browser
-Specific subversion changesets can be accessed at:
-	http://bind10.isc.org/changeset/rrrr
+Specific git changesets can be accessed at:
+	http://bind10.isc.org/changeset/?reponame=&old=rrrr^&new=rrrr
+or after cloning the original git repository by executing:
+	% git diff rrrr^ rrrr
+Subversion changesets are not accessible any more.  The subversion
+revision numbers will be replaced with corresponding git revisions.
 Trac tickets can be accessed at: https://bind10.isc.org/ticket/nnn
 
 LEGEND

configure.ac

@@ -9,7 +9,23 @@ AC_CONFIG_HEADERS([config.h])
 
 # Checks for programs.
 AC_PROG_CXX
+
+# Libtool configuration
+#
+# On FreeBSD (and probably some others), clang++ does not meet an autoconf
+# assumption in identifying libtool configuration regarding shared library:
+# the configure script will execute "$CC -shared $CFLAGS -v -o" and expect
+# the output contains -Lxxx or -Ryyy.  This is the case for g++, but not for
+# clang++, and, as a result, it will cause various errors in linking programs
+# or running them with a shared object (such as some of our python scripts).
+# To work around this problem we define a temporary variable
+# "CXX_LIBTOOL_LDFLAGS".  It's expected to be defined as, e.g, "-L/usr/lib"
+# to temporarily fake the output so that it will be compatible with that of
+# g++.
+CFLAGS_SAVED=$CFLAGS
+CFLAGS="$CFLAGS $CXX_LIBTOOL_LDFLAGS"
 AC_PROG_LIBTOOL
+CFLAGS=$CFLAGS_SAVED
 
 # Use C++ language
 AC_LANG([C++])

src/bin/auth/Makefile.am

@@ -40,6 +40,7 @@ b10_auth_SOURCES = query.cc query.h
 b10_auth_SOURCES += auth_srv.cc auth_srv.h
 b10_auth_SOURCES += change_user.cc change_user.h
 b10_auth_SOURCES += config.cc config.h
+b10_auth_SOURCES += command.cc command.h
 b10_auth_SOURCES += common.h
 b10_auth_SOURCES += statistics.cc statistics.h
 b10_auth_SOURCES += main.cc

src/bin/auth/auth.spec.pre.in

@@ -64,8 +64,25 @@
         "command_name": "sendstats",
         "command_description": "Send data to a statistics module at once",
         "command_args": []
+      },
+      {
+        "command_name": "loadzone",
+        "command_description": "(Re)load a specified zone",
+        "command_args": [
+          {
+            "item_name": "class", "item_type": "string",
+            "item_optional": true, "item_default": "IN"
+          },
+	  {
+            "item_name": "origin", "item_type": "string",
+            "item_optional": false, "item_default": ""
+          },
+	  {
+            "item_name": "datasrc", "item_type": "string",
+            "item_optional": true, "item_default": "memory"
+          }
+        ]
       }
     ]
   }
 }
-

src/bin/auth/auth_srv.cc

@@ -195,11 +195,20 @@ private:
 
 AuthSrv::AuthSrv(const bool use_cache, AbstractXfroutClient& xfrout_client) :
     impl_(new AuthSrvImpl(use_cache, xfrout_client)),
+    io_service_(NULL),
     checkin_(new ConfigChecker(this)),
     dns_lookup_(new MessageLookup(this)),
     dns_answer_(new MessageAnswer(this))
 {}
 
+void
+AuthSrv::stop() {
+    if (io_service_ == NULL) {
+        throw FatalError("Assumption failure; server is stopped before start");
+    }
+    io_service_->stop();
+}
+
 AuthSrv::~AuthSrv() {
     delete impl_;
     delete checkin_;
@@ -299,8 +308,8 @@ AuthSrv::getConfigSession() const {
     return (impl_->config_session_);
 }
 
-AuthSrv::ConstMemoryDataSrcPtr
-AuthSrv::getMemoryDataSrc(const RRClass& rrclass) const {
+AuthSrv::MemoryDataSrcPtr
+AuthSrv::getMemoryDataSrc(const RRClass& rrclass) {
     // XXX: for simplicity, we only support the IN class right now.
     if (rrclass != impl_->memory_datasrc_class_) {
         isc_throw(InvalidParameter,

src/bin/auth/auth_srv.h

@@ -87,6 +87,15 @@ public:
     ~AuthSrv();
     //@}
 
+    /// Stop the server.
+    ///
+    /// It stops the internal event loop of the server and subsequently
+    /// returns the control to the top level context.
+    /// The server must have been associated with an \c IOService object;
+    /// otherwise an exception of \c FatalError will be thrown.
+    /// This method never throws an exception otherwise.
+    void stop();
+
     /// \brief Process an incoming DNS message, then signal 'server' to resume 
     ///
     /// A DNS query (or other message) has been received by a \c DNSServer
@@ -265,8 +274,7 @@ public:
     /// \param rrclass The RR class of the requested in-memory data source.
     /// \return A pointer to the in-memory data source, if configured;
     /// otherwise NULL.
-    ConstMemoryDataSrcPtr
-    getMemoryDataSrc(const isc::dns::RRClass& rrclass) const;
+    MemoryDataSrcPtr getMemoryDataSrc(const isc::dns::RRClass& rrclass);
 
     /// Sets or replaces the in-memory data source of the specified RR class.
     ///

src/bin/auth/command.cc

@@ -0,0 +1,252 @@
+// Copyright (C) 2010  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 <string>
+
+#include <boost/scoped_ptr.hpp>
+#include <boost/shared_ptr.hpp>
+
+#include <exceptions/exceptions.h>
+
+#include <dns/rrclass.h>
+
+#include <cc/data.h>
+
+#include <datasrc/memory_datasrc.h>
+
+#include <config/ccsession.h>
+
+#include <auth/auth_srv.h>
+#include <auth/command.h>
+
+using namespace std;
+using boost::shared_ptr;
+using boost::scoped_ptr;
+using namespace isc::dns;
+using namespace isc::data;
+using namespace isc::datasrc;
+using namespace isc::config;
+
+namespace {
+/// An exception that is thrown if an error occurs while handling a command
+/// on an \c AuthSrv object.
+///
+/// Currently it's only used internally, since \c execAuthServerCommand()
+/// (which is the only interface to this module) catches all \c isc::
+/// exceptions and converts them.
+class AuthCommandError : public isc::Exception {
+public:
+    AuthCommandError(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) {}
+};
+
+/// An abstract base class that represents a single command identifier
+/// for an \c AuthSrv object.
+///
+/// Each of derived classes of \c AuthCommand, which are hidden inside the
+/// implementation, corresponds to a command executed on \c AuthSrv, such as
+/// "shutdown".  The derived class is responsible to execute the corresponding
+/// command with the given command arguments (if any) in its \c exec()
+/// method.
+///
+/// In the initial implementation the existence of the command classes is
+/// hidden inside the implementation since the only public interface is
+/// \c execAuthServerCommand(), which does not expose this class.
+/// In future, we may want to make this framework more dynamic, i.e.,
+/// registering specific derived classes run time outside of this
+/// implementation.  If and when that happens the definition of the abstract
+/// class will be published.
+class AuthCommand {
+    ///
+    /// \name Constructors and Destructor
+    ///
+    /// Note: The copy constructor and the assignment operator are
+    /// intentionally defined as private to make it explicit that this is a
+    /// pure base class.
+    //@{
+private:
+    AuthCommand(const AuthCommand& source);
+    AuthCommand& operator=(const AuthCommand& source);
+protected:
+    /// \brief The default constructor.
+    ///
+    /// This is intentionally defined as \c protected as this base class should
+    /// never be instantiated (except as part of a derived class).
+    AuthCommand() {}
+public:
+    /// The destructor.
+    virtual ~AuthCommand() {}
+    //@}
+
+    /// Execute a single control command.
+    ///
+    /// Specific derived methods can throw exceptions.  When called via
+    /// \c execAuthServerCommand(), all BIND 10 exceptions are caught
+    /// and converted into an error code.
+    /// The derived method may also throw an exception of class
+    /// \c AuthCommandError when it encounters an internal error, such as
+    /// semantics error on the command arguments.
+    ///
+    /// \param server The \c AuthSrv object on which the command is executed.
+    /// \param args Command specific argument.
+    virtual void exec(AuthSrv& server, isc::data::ConstElementPtr args) = 0;
+};
+
+// Handle the "shutdown" command.  No argument is assumed.
+class ShutdownCommand : public AuthCommand {
+public:
+    virtual void exec(AuthSrv& server, isc::data::ConstElementPtr) {
+        server.stop();
+    }
+};
+
+// Handle the "sendstats" command.  No argument is assumed.
+class SendStatsCommand : public AuthCommand {
+public:
+    virtual void exec(AuthSrv& server, isc::data::ConstElementPtr) {
+        if (server.getVerbose()) {
+            cerr << "[b10-auth] command 'sendstats' received" << endl;
+        }
+        server.submitStatistics();
+    }
+};
+
+// Handle the "loadzone" command.
+class LoadZoneCommand : public AuthCommand {
+public:
+    virtual void exec(AuthSrv& server, isc::data::ConstElementPtr args) {
+        // parse and validate the args.
+        if (!validate(server, args)) {
+            return;
+        }
+
+        // Load a new zone and replace the current zone with the new one.
+        // TODO: eventually this should be incremental or done in some way
+        // that doesn't block other server operations.
+        // TODO: we may (should?) want to check the "last load time" and
+        // the timestamp of the file and skip loading if the file isn't newer.
+        shared_ptr<MemoryZone> newzone(new MemoryZone(oldzone->getClass(),
+                                                      oldzone->getOrigin()));
+        newzone->load(oldzone->getFileName());
+        oldzone->swap(*newzone);
+
+        if (server.getVerbose()) {
+            cerr << "[b10-auth] Loaded zone '" << newzone->getOrigin()
+                 << "'/" << newzone->getClass() << endl;
+        }
+    }
+
+private:
+    shared_ptr<MemoryZone> oldzone; // zone to be updated with the new file.
+
+    // A helper private method to parse and validate command parameters.
+    // On success, it sets 'oldzone' to the zone to be updated.
+    // It returns true if everything is okay; and false if the command is
+    // valid but there's no need for further process.
+    bool validate(AuthSrv& server, isc::data::ConstElementPtr args) {
+        if (args == NULL) {
+            isc_throw(AuthCommandError, "Null argument");
+        }
+
+        // In this initial implementation, we assume memory data source
+        // for class IN by default.
+        ConstElementPtr datasrc_elem = args->get("datasrc");
+        if (datasrc_elem) {
+            if (datasrc_elem->stringValue() == "sqlite3") {
+                if (server.getVerbose()) {
+                    cerr << "[b10-auth] Nothing to do for loading sqlite3"
+                         << endl;
+                }
+                return (false);
+            } else if (datasrc_elem->stringValue() != "memory") {
+                // (note: at this point it's guaranteed that datasrc_elem
+                // is of string type)
+                isc_throw(AuthCommandError,
+                          "Data source type " << datasrc_elem->stringValue()
+                          << " is not supported");
+            }
+        }
+
+        ConstElementPtr class_elem = args->get("class");
+        const RRClass zone_class = class_elem ?
+            RRClass(class_elem->stringValue()) : RRClass::IN();
+
+        AuthSrv::MemoryDataSrcPtr datasrc(server.getMemoryDataSrc(zone_class));
+        if (datasrc == NULL) {
+            isc_throw(AuthCommandError, "Memory data source is disabled");
+        }
+
+        ConstElementPtr origin_elem = args->get("origin");
+        if (!origin_elem) {
+            isc_throw(AuthCommandError, "Zone origin is missing");
+        }
+        const Name origin(origin_elem->stringValue());
+
+        // Get the current zone
+        const MemoryDataSrc::FindResult result = datasrc->findZone(origin);
+        if (result.code != result::SUCCESS) {
+            isc_throw(AuthCommandError, "Zone " << origin <<
+                      " is not found in data source");
+        }
+
+        oldzone = boost::dynamic_pointer_cast<MemoryZone>(result.zone);
+
+        return (true);
+    }
+};
+
+// The factory of command objects.
+AuthCommand*
+createAuthCommand(const string& command_id) {
+    // For the initial implementation we use a naive if-else blocks
+    // (see also createAuthConfigParser())
+    if (command_id == "shutdown") {
+        return (new ShutdownCommand());
+    } else if (command_id == "sendstats") {
+        return (new SendStatsCommand());
+    } else if (command_id == "loadzone") {
+        return (new LoadZoneCommand());
+    } else if (false && command_id == "_throw_exception") {
+        // This is for testing purpose only and should not appear in the
+        // actual configuration syntax.
+        // XXX: ModuleCCSession doesn't seem to validate commands (unlike
+        // config), so we should disable this case for now.
+        throw runtime_error("throwing for test");
+    }
+
+    isc_throw(AuthCommandError, "Unknown command identifier: " << command_id);
+}
+} // end of unnamed namespace
+
+ConstElementPtr
+execAuthServerCommand(AuthSrv& server, const string& command_id,
+                      ConstElementPtr args)
+{
+    if (server.getVerbose()) {
+        cerr << "[b10-auth] Received '" << command_id << "' command" << endl;
+    }
+
+    try {
+        scoped_ptr<AuthCommand>(createAuthCommand(command_id))->exec(server,
+                                                                     args);
+    } catch (const isc::Exception& ex) {
+        if (server.getVerbose()) {
+            cerr << "[b10-auth] Command '" << command_id
+                 << "' execution failed: " << ex.what() << endl;
+        }
+        return (createAnswer(1, ex.what()));
+    }
+
+    return (createAnswer());
+}

src/bin/auth/command.h

@@ -0,0 +1,61 @@
+// Copyright (C) 2010  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 <string>
+
+#include <cc/data.h>
+
+#ifndef __COMMAND_H
+#define __COMMAND_H 1
+
+class AuthSrv;
+
+/// Execute a control command on \c AuthSrv
+///
+/// It executes the operation identified by \c command_id with arguments
+/// \c args on \c server, and returns the result in the form of the standard
+/// command/config answer message (see \c isc::config::createAnswer()).
+///
+/// This method internally performs minimal validation on \c command_id and
+/// \c args to not cause a surprising result such as a crash, but it is
+/// generally expected that the caller has performed syntax level validation
+/// based on the command specification for the authoritative server.
+/// For example, the caller is responsible \c command_id is a valid command
+/// name for the authoritative server.
+///
+/// The execution of the command may internally trigger an exception; for
+/// instance, if a string that is expected to be a valid domain name is bogus,
+/// the underlying DNS library will throw an exception.  These internal
+/// exceptions will be caught inside this function, and will be converted
+/// to a return value with a non 0 error code.
+/// However, exceptions thrown outside of BIND 10 modules (including standard
+/// exceptions) are expected to be handled at a higher layer, and will be
+/// propagated to the caller.  In particular if memory allocation fails and
+/// \c std::bad_alloc is thrown it will be propagated.
+///
+/// \param server The \c AuthSrv object on which the command is executed.
+/// \param command_id The identifier of the command (such as "shutdown")
+/// \param args Command specific argument in a map type of
+/// \c isc::data::Element, or a \c NULL \c ElementPtr if the argument isn't
+/// specified.
+/// \return The result of the command operation.
+isc::data::ConstElementPtr
+execAuthServerCommand(AuthSrv& server, const std::string& command_id,
+                      isc::data::ConstElementPtr args);
+
+#endif // __COMMAND_H
+
+// Local Variables:
+// mode: c++
+// End:

src/bin/auth/main.cc

@@ -42,6 +42,7 @@
 #include <auth/spec_config.h>
 #include <auth/common.h>
 #include <auth/config.h>
+#include <auth/command.h>
 #include <auth/change_user.h>
 #include <auth/auth_srv.h>
 #include <asiolink/asiolink.h>
@@ -80,23 +81,8 @@ my_config_handler(ConstElementPtr new_config) {
 
 ConstElementPtr
 my_command_handler(const string& command, ConstElementPtr args) {
-    ConstElementPtr answer = createAnswer();
-
-    if (command == "print_message") {
-        cout << args << endl;
-        /* let's add that message to our answer as well */
-        answer = createAnswer(0, args);
-    } else if (command == "shutdown") {
-        io_service.stop();
-    } else if (command == "sendstats") {
-        if (verbose_mode) {
-            cerr << "[b10-auth] command 'sendstats' received" << endl;
-        }
-        assert(auth_server != NULL);
-        auth_server->submitStatistics();
-    }
-
-    return (answer);
+    assert(auth_server != NULL);
+    return (execAuthServerCommand(*auth_server, command, args));
 }
 
 void

src/bin/auth/statistics.h

@@ -18,6 +18,7 @@
 #define __STATISTICS_H 1
 
 #include <cc/session.h>
+#include <stdint.h>
 
 class AuthCountersImpl;
 

src/bin/auth/tests/Makefile.am

@@ -2,8 +2,9 @@ AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
 AM_CPPFLAGS += -I$(top_builddir)/src/lib/dns -I$(top_srcdir)/src/bin
 AM_CPPFLAGS += -I$(top_builddir)/src/lib/cc
 AM_CPPFLAGS += $(BOOST_INCLUDES)
-AM_CPPFLAGS += -DTEST_DATA_DIR=\"$(top_srcdir)/src/lib/testutils/testdata\"
+AM_CPPFLAGS += -DTEST_DATA_DIR=\"$(abs_top_srcdir)/src/lib/testutils/testdata\"
 AM_CPPFLAGS += -DTEST_DATA_BUILDDIR=\"$(abs_top_builddir)/src/lib/testutils/testdata\"
+AM_CPPFLAGS += -DINSTALL_PROG=\"$(abs_top_srcdir)/install-sh\"
 
 AM_CXXFLAGS = $(B10_CXXFLAGS)
 
@@ -22,9 +23,11 @@ run_unittests_SOURCES += ../auth_srv.h ../auth_srv.cc
 run_unittests_SOURCES += ../query.h ../query.cc
 run_unittests_SOURCES += ../change_user.h ../change_user.cc
 run_unittests_SOURCES += ../config.h ../config.cc
+run_unittests_SOURCES += ../command.h ../command.cc
 run_unittests_SOURCES += ../statistics.h ../statistics.cc
 run_unittests_SOURCES += auth_srv_unittest.cc
 run_unittests_SOURCES += config_unittest.cc
+run_unittests_SOURCES += command_unittest.cc
 run_unittests_SOURCES += query_unittest.cc
 run_unittests_SOURCES += change_user_unittest.cc
 run_unittests_SOURCES += statistics_unittest.cc

src/bin/auth/tests/auth_srv_unittest.cc

@@ -30,6 +30,7 @@
 
 #include <datasrc/memory_datasrc.h>
 #include <auth/auth_srv.h>
+#include <auth/common.h>
 #include <auth/statistics.h>
 
 #include <dns/tests/unittest_util.h>
@@ -640,4 +641,13 @@ TEST_F(AuthSrvTest, queryCounterUnexpected) {
                                        response_obuffer, &dnsserv),
                  isc::Unexpected);
 }
+
+TEST_F(AuthSrvTest, stop) {
+    // normal case is covered in command_unittest.cc.  we should primarily
+    // test it here, but the current design of the stop test takes time,
+    // so we consolidate the cases in the command tests.
+
+    // stop before start is prohibited.
+    EXPECT_THROW(server.stop(), FatalError);
+}
 }

src/bin/auth/tests/command_unittest.cc

@@ -0,0 +1,292 @@
+// Copyright (C) 2010  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 <cassert>
+#include <cstdlib>
+#include <string>
+#include <stdexcept>
+
+#include <boost/bind.hpp>
+
+#include <gtest/gtest.h>
+
+#include <dns/name.h>
+#include <dns/rrclass.h>
+#include <dns/rrtype.h>
+
+#include <cc/data.h>
+
+#include <config/ccsession.h>
+
+#include <datasrc/memory_datasrc.h>
+
+#include <auth/auth_srv.h>
+#include <auth/config.h>
+#include <auth/command.h>
+
+#include <asiolink/asiolink.h>
+
+#include <testutils/mockups.h>
+
+using namespace std;
+using namespace isc::dns;
+using namespace isc::data;
+using namespace isc::datasrc;
+using namespace isc::config;
+
+namespace {
+class AuthConmmandTest : public ::testing::Test {
+protected:
+    AuthConmmandTest() : server(false, xfrout), rcode(-1) {
+        server.setStatisticsSession(&statistics_session);
+    }
+    void checkAnswer(const int expected_code) {
+        parseAnswer(rcode, result);
+        EXPECT_EQ(expected_code, rcode);
+    }
+    MockSession statistics_session;
+    MockXfroutClient xfrout;
+    AuthSrv server;
+    AuthSrv::ConstMemoryDataSrcPtr memory_datasrc;
+    ConstElementPtr result;
+    int rcode;
+public:
+    void stopServer();          // need to be public for boost::bind
+};
+
+TEST_F(AuthConmmandTest, unknownCommand) {
+    result = execAuthServerCommand(server, "no_such_command",
+                                   ConstElementPtr());
+    parseAnswer(rcode, result);
+    EXPECT_EQ(1, rcode);
+}
+
+TEST_F(AuthConmmandTest, DISABLED_unexpectedException) {
+    // execAuthServerCommand() won't catch standard exceptions.
+    // Skip this test for now: ModuleCCSession doesn't seem to validate
+    // commands.
+    EXPECT_THROW(execAuthServerCommand(server, "_throw_exception",
+                                       ConstElementPtr()),
+                 runtime_error);
+}
+
+TEST_F(AuthConmmandTest, sendStatistics) {
+    result = execAuthServerCommand(server, "sendstats", ConstElementPtr());
+    // Just check some message has been sent.  Detailed tests specific to
+    // statistics are done in its own tests.
+    EXPECT_EQ("Stats", statistics_session.getMessageDest());
+    checkAnswer(0);
+}
+
+void
+AuthConmmandTest::stopServer() {
+    result = execAuthServerCommand(server, "shutdown", ConstElementPtr());
+    parseAnswer(rcode, result);
+    assert(rcode == 0); // make sure the test stops when something is wrong
+}
+
+TEST_F(AuthConmmandTest, shutdown) {
+    asiolink::IOService io_service;
+    asiolink::IntervalTimer itimer(io_service);
+    server.setIOService(io_service);
+    itimer.setupTimer(boost::bind(&AuthConmmandTest::stopServer, this), 1);
+    io_service.run();
+    EXPECT_EQ(0, rcode);
+}
+
+// A helper function commonly used for the "loadzone" command tests.
+// It configures the server with a memory data source containing two
+// zones, and checks the zones are correctly loaded.
+void
+zoneChecks(AuthSrv& server) {
+    EXPECT_TRUE(server.getMemoryDataSrc(RRClass::IN()));
+    EXPECT_EQ(Zone::SUCCESS, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test1.example")).zone->
+              find(Name("ns.test1.example"), RRType::A()).code);
+    EXPECT_EQ(Zone::NXRRSET, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test1.example")).zone->
+              find(Name("ns.test1.example"), RRType::AAAA()).code);
+    EXPECT_EQ(Zone::SUCCESS, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test2.example")).zone->
+              find(Name("ns.test2.example"), RRType::A()).code);
+    EXPECT_EQ(Zone::NXRRSET, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test2.example")).zone->
+              find(Name("ns.test2.example"), RRType::AAAA()).code);
+}
+
+void
+configureZones(AuthSrv& server) {
+    ASSERT_EQ(0, system(INSTALL_PROG " " TEST_DATA_DIR "/test1.zone.in "
+                        TEST_DATA_BUILDDIR "/test1.zone.copied"));
+    ASSERT_EQ(0, system(INSTALL_PROG " " TEST_DATA_DIR "/test2.zone.in "
+                        TEST_DATA_BUILDDIR "/test2.zone.copied"));
+    configureAuthServer(server, Element::fromJSON(
+                            "{\"datasources\": "
+                            " [{\"type\": \"memory\","
+                            "   \"zones\": "
+                            "[{\"origin\": \"test1.example\","
+                            "  \"file\": \""
+                               TEST_DATA_BUILDDIR "/test1.zone.copied\"},"
+                            " {\"origin\": \"test2.example\","
+                            "  \"file\": \""
+                               TEST_DATA_BUILDDIR "/test2.zone.copied\"}"
+                            "]}]}"));
+    zoneChecks(server);
+}
+
+void
+newZoneChecks(AuthSrv& server) {
+    EXPECT_TRUE(server.getMemoryDataSrc(RRClass::IN()));
+    EXPECT_EQ(Zone::SUCCESS, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test1.example")).zone->
+              find(Name("ns.test1.example"), RRType::A()).code);
+    // now test1.example should have ns/AAAA
+    EXPECT_EQ(Zone::SUCCESS, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test1.example")).zone->
+              find(Name("ns.test1.example"), RRType::AAAA()).code);
+
+    // test2.example shouldn't change
+    EXPECT_EQ(Zone::SUCCESS, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test2.example")).zone->
+              find(Name("ns.test2.example"), RRType::A()).code);
+    EXPECT_EQ(Zone::NXRRSET, server.getMemoryDataSrc(RRClass::IN())->
+              findZone(Name("ns.test2.example")).zone->
+              find(Name("ns.test2.example"), RRType::AAAA()).code);
+}
+
+TEST_F(AuthConmmandTest, loadZone) {
+    configureZones(server);
+
+    ASSERT_EQ(0, system(INSTALL_PROG " " TEST_DATA_DIR
+                        "/test1-new.zone.in "
+                        TEST_DATA_BUILDDIR "/test1.zone.copied"));
+    ASSERT_EQ(0, system(INSTALL_PROG " " TEST_DATA_DIR
+                        "/test2-new.zone.in "
+                        TEST_DATA_BUILDDIR "/test2.zone.copied"));
+
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\"}"));
+    checkAnswer(0);
+    newZoneChecks(server);
+}
+
+TEST_F(AuthConmmandTest, loadBrokenZone) {
+    configureZones(server);
+
+    ASSERT_EQ(0, system(INSTALL_PROG " " TEST_DATA_DIR
+                        "/test1-broken.zone.in "
+                        TEST_DATA_BUILDDIR "/test1.zone.copied"));
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\"}"));
+    checkAnswer(1);
+    zoneChecks(server);     // zone shouldn't be replaced
+}
+
+TEST_F(AuthConmmandTest, loadUnreadableZone) {
+    configureZones(server);
+
+    // install the zone file as unreadable
+    ASSERT_EQ(0, system(INSTALL_PROG " -m 000 " TEST_DATA_DIR
+                        "/test1.zone.in "
+                        TEST_DATA_BUILDDIR "/test1.zone.copied"));
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\"}"));
+    checkAnswer(1);
+    zoneChecks(server);     // zone shouldn't be replaced
+}
+
+TEST_F(AuthConmmandTest, loadZoneWithoutDataSrc) {
+    // try to execute load command without configuring the zone beforehand.
+    // it should fail.
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\"}"));
+    checkAnswer(1);
+}
+
+TEST_F(AuthConmmandTest, loadSqlite3DataSrc) {
+    // For sqlite3 data source we don't have to do anything (the data source
+    // (re)loads itself automatically)
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\","
+                                       " \"datasrc\": \"sqlite3\"}"));
+    checkAnswer(0);
+}
+
+TEST_F(AuthConmmandTest, loadZoneInvalidParams) {
+    configureZones(server);
+
+    // null arg
+    result = execAuthServerCommand(server, "loadzone", ElementPtr());
+    checkAnswer(1);
+
+    // zone class is bogus
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\","
+                                       " \"class\": \"no_such_class\"}"));
+    checkAnswer(1);
+
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\","
+                                       " \"class\": 1}"));
+    checkAnswer(1);
+
+    // unsupported zone class
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\","
+                                       " \"class\": \"CH\"}"));
+    checkAnswer(1);
+
+    // unsupported data source class
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\","
+                                       " \"datasrc\": \"not supported\"}"));
+    checkAnswer(1);
+
+    // data source is bogus
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"test1.example\","
+                                       " \"datasrc\": 0}"));
+    checkAnswer(1);
+
+    // origin is missing
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON("{}"));
+    checkAnswer(1);
+
+    // zone doesn't exist in the data source
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON("{\"origin\": \"xx\"}"));
+    checkAnswer(1);
+
+    // origin is bogus
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON(
+                                       "{\"origin\": \"...\"}"));
+    checkAnswer(1);
+
+    result = execAuthServerCommand(server, "loadzone",
+                                   Element::fromJSON("{\"origin\": 10}"));
+    checkAnswer(1);
+}
+}

src/lib/asiolink/asiolink.cc

@@ -260,7 +260,7 @@ convertAddr(const string& address) {
         isc_throw(IOError, "Invalid IP address '" << &address << "': "
             << err.message());
     }
-    return addr;
+    return (addr);
 }
 
 }

src/lib/config/ccsession.cc

@@ -135,8 +135,6 @@ createCommand(const std::string& command, ConstElementPtr arg) {
     return (cmd);
 }
 
-/// Returns "" and empty ElementPtr() if this does not
-/// look like a command
 std::string
 parseCommand(ConstElementPtr& arg, ConstElementPtr command) {
     if (command &&
@@ -149,7 +147,7 @@ parseCommand(ConstElementPtr& arg, ConstElementPtr command) {
             if (cmd->size() > 1) {
                 arg = cmd->get(1);
             } else {
-                arg = ElementPtr();
+                arg = Element::createMap();
             }
             return (cmd->get(0)->stringValue());
         } else {
@@ -257,7 +255,7 @@ ModuleCCSession::handleConfigUpdate(ConstElementPtr new_config) {
     ElementPtr errors = Element::createList();
     if (!config_handler_) {
         answer = createAnswer(1, module_name_ + " does not have a config handler");
-    } else if (!module_specification_.validate_config(new_config, false,
+    } else if (!module_specification_.validateConfig(new_config, false,
                                                       errors)) {
         std::stringstream ss;
         ss << "Error in config validation: ";
@@ -286,6 +284,51 @@ ModuleCCSession::hasQueuedMsgs() const {
     return (session_.hasQueuedMsgs());
 }
 
+ConstElementPtr
+ModuleCCSession::checkConfigUpdateCommand(const std::string& target_module,
+                                          ConstElementPtr arg)
+{
+    if (target_module == module_name_) {
+        return (handleConfigUpdate(arg));
+    } else {
+        // ok this update is not for us, if we have this module
+        // in our remote config list, update that
+        updateRemoteConfig(target_module, arg);
+        // we're not supposed to answer to this, so return
+        return (ElementPtr());
+    }
+}
+
+ConstElementPtr
+ModuleCCSession::checkModuleCommand(const std::string& cmd_str,
+                                    const std::string& target_module,
+                                    ConstElementPtr arg) const
+{
+    if (target_module == module_name_) {
+        if (command_handler_) {
+            ElementPtr errors = Element::createList();
+            if (module_specification_.validateCommand(cmd_str,
+                                                       arg,
+                                                       errors)) {
+                return (command_handler_(cmd_str, arg));
+            } else {
+                std::stringstream ss;
+                ss << "Error in command validation: ";
+                BOOST_FOREACH(ConstElementPtr error,
+                              errors->listValue()) {
+                    ss << error->stringValue();
+                }
+                return (createAnswer(3, ss.str()));
+            }
+        } else {
+            return (createAnswer(1,
+                                 "Command given but no "
+                                 "command handler for module"));
+        }
+    }
+    return (ElementPtr());
+}
+
 int
 ModuleCCSession::checkCommand() {
     ConstElementPtr cmd, routing, data;
@@ -302,23 +345,9 @@ ModuleCCSession::checkCommand() {
             std::string cmd_str = parseCommand(arg, data);
             std::string target_module = routing->get("group")->stringValue();
             if (cmd_str == "config_update") {
-                if (target_module == module_name_) {
-                    answer = handleConfigUpdate(arg);
-                } else {
-                    // ok this update is not for us, if we have this module
-                    // in our remote config list, update that
-                    updateRemoteConfig(target_module, arg);
-                    // we're not supposed to answer to this, so return
-                    return (0);
-                }
+                answer = checkConfigUpdateCommand(target_module, arg);
             } else {
-                if (target_module == module_name_) {
-                    if (command_handler_) {
-                        answer = command_handler_(cmd_str, arg);
-                    } else {
-                        answer = createAnswer(1, "Command given but no command handler for module");
-                    }
-                }
+                answer = checkModuleCommand(cmd_str, target_module, arg);
             }
         } catch (const CCSessionError& re) {
             // TODO: Once we have logging and timeouts, we should not

src/lib/config/ccsession.h

@@ -91,11 +91,36 @@ isc::data::ConstElementPtr createCommand(const std::string& command,
 /// \brief Parses the given command into a string containing the actual
 ///        command and an ElementPtr containing the optional argument.
 ///
+/// Raises a CCSessionError if this is not a well-formed command
+///
+/// Example code: (command_message is a ConstElementPtr that is
+/// passed here)
+/// \code
+/// ElementPtr command_message = Element::fromJSON("{ \"command\": [\"foo\", { \"bar\": 123 } ] }");
+/// try {
+///     ConstElementPtr args;
+///     std::string command_str = parseCommand(args, command_message);
+///     if (command_str == "foo") {
+///         std::cout << "The command 'foo' was given" << std::endl;
+///         if (args->contains("bar")) {
+///             std::cout << "It had argument name 'bar' set, which has"
+///                       << "value " 
+///                       << args->get("bar")->intValue();
+///         }
+///     } else {
+///         std::cout << "Unknown command '" << command_str << std::endl;
+///     }
+/// } catch (CCSessionError cse) {
+///     std::cerr << "Bad command in CC Session: "
+///     << cse.what() << std::endl;
+/// }
+/// \endcode
+/// 
 /// \param arg This value will be set to the ElementPtr pointing to
-///        the argument, or to an empty ElementPtr if there was none.
+///        the argument, or to an empty Map (ElementPtr) if there was none.
 /// \param command The command message containing the command (as made
 ///        by createCommand()
-/// \return The command string
+/// \return The command name
 std::string parseCommand(isc::data::ConstElementPtr& arg,
                          isc::data::ConstElementPtr command);
 
@@ -244,6 +269,15 @@ private:
     isc::data::ConstElementPtr handleConfigUpdate(
         isc::data::ConstElementPtr new_config);
 
+    isc::data::ConstElementPtr checkConfigUpdateCommand(
+        const std::string& target_module,
+        isc::data::ConstElementPtr arg);
+
+    isc::data::ConstElementPtr checkModuleCommand(
+        const std::string& cmd_str,
+        const std::string& target_module,
+        isc::data::ConstElementPtr arg) const;
+
     isc::data::ConstElementPtr(*config_handler_)(
         isc::data::ConstElementPtr new_config);
     isc::data::ConstElementPtr(*command_handler_)(

src/lib/config/module_spec.cc

@@ -177,17 +177,47 @@ ModuleSpec::getModuleDescription() const {
 }
 
 bool
-ModuleSpec::validate_config(ConstElementPtr data, const bool full) const {
+ModuleSpec::validateConfig(ConstElementPtr data, const bool full) const {
     ConstElementPtr spec = module_specification->find("config_data");
-    return (validate_spec_list(spec, data, full, ElementPtr()));
+    return (validateSpecList(spec, data, full, ElementPtr()));
 }
 
 bool
-ModuleSpec::validate_config(ConstElementPtr data, const bool full,
+ModuleSpec::validateCommand(const std::string& command,
+                             ConstElementPtr args,
+                             ElementPtr errors) const
+{
+    if (args->getType() != Element::map) {
+        errors->add(Element::create("args for command " +
+                                    command + " is not a map"));
+        return (false);
+    }
+
+    ConstElementPtr commands_spec = module_specification->find("commands");
+    if (!commands_spec) {
+        // there are no commands according to the spec.
+        errors->add(Element::create("The given module has no commands"));
+        return (false);
+    }
+
+    BOOST_FOREACH(ConstElementPtr cur_command, commands_spec->listValue()) {
+        if (cur_command->get("command_name")->stringValue() == command) {
+            return (validateSpecList(cur_command->get("command_args"),
+                                       args, true, errors));
+        }
+    }
+
+    // this command is unknown
+    errors->add(Element::create("Unknown command " + command));
+    return (false);
+}
+
+bool
+ModuleSpec::validateConfig(ConstElementPtr data, const bool full,
                             ElementPtr errors) const
 {
     ConstElementPtr spec = module_specification->find("config_data");
-    return (validate_spec_list(spec, data, full, errors));
+    return (validateSpecList(spec, data, full, errors));
 }
 
 ModuleSpec
@@ -264,7 +294,7 @@ check_type(ConstElementPtr spec, ConstElementPtr element) {
 }
 
 bool
-ModuleSpec::validate_item(ConstElementPtr spec, ConstElementPtr data,
+ModuleSpec::validateItem(ConstElementPtr spec, ConstElementPtr data,
                           const bool full, ElementPtr errors) const
 {
     if (!check_type(spec, data)) {
@@ -286,14 +316,14 @@ ModuleSpec::validate_item(ConstElementPtr spec, ConstElementPtr data,
                 return (false);
             }
             if (list_spec->get("item_type")->stringValue() == "map") {
-                if (!validate_item(list_spec, list_el, full, errors)) {
+                if (!validateItem(list_spec, list_el, full, errors)) {
                     return (false);
                 }
             }
         }
     }
     if (data->getType() == Element::map) {
-        if (!validate_spec_list(spec->get("map_item_spec"), data, full, errors)) {
+        if (!validateSpecList(spec->get("map_item_spec"), data, full, errors)) {
             return (false);
         }
     }
@@ -302,7 +332,7 @@ ModuleSpec::validate_item(ConstElementPtr spec, ConstElementPtr data,
 
 // spec is a map with item_name etc, data is a map
 bool
-ModuleSpec::validate_spec(ConstElementPtr spec, ConstElementPtr data,
+ModuleSpec::validateSpec(ConstElementPtr spec, ConstElementPtr data,
                           const bool full, ElementPtr errors) const
 {
     std::string item_name = spec->get("item_name")->stringValue();
@@ -311,7 +341,7 @@ ModuleSpec::validate_spec(ConstElementPtr spec, ConstElementPtr data,
     data_el = data->get(item_name);
     
     if (data_el) {
-        if (!validate_item(spec, data_el, full, errors)) {
+        if (!validateItem(spec, data_el, full, errors)) {
             return (false);
         }
     } else {
@@ -327,13 +357,13 @@ ModuleSpec::validate_spec(ConstElementPtr spec, ConstElementPtr data,
 
 // spec is a list of maps, data is a map
 bool
-ModuleSpec::validate_spec_list(ConstElementPtr spec, ConstElementPtr data,
+ModuleSpec::validateSpecList(ConstElementPtr spec, ConstElementPtr data,
                                const bool full, ElementPtr errors) const
 {
     bool validated = true;
     std::string cur_item_name;
     BOOST_FOREACH(ConstElementPtr cur_spec_el, spec->listValue()) {
-        if (!validate_spec(cur_spec_el, data, full, errors)) {
+        if (!validateSpec(cur_spec_el, data, full, errors)) {
             validated = false;
         }
     }

src/lib/config/module_spec.h

@@ -88,23 +88,66 @@ namespace isc { namespace config {
         /// \param data The base \c Element of the data to check
         /// \return true if the data conforms to the specification,
         /// false otherwise.
-        bool validate_config(isc::data::ConstElementPtr data,
+        bool validateConfig(isc::data::ConstElementPtr data,
                              const bool full = false) const;
 
+        /// Validates the arguments for the given command
+        ///
+        /// This checks the command and argument against the
+        /// specification in the module's .spec file.
+        ///
+        /// A command is considered valid if:
+        /// - it is known (the 'command' string must have an entry in
+        ///                the specification)
+        /// - the args is a MapElement
+        /// - args contains all mandatory arguments
+        /// - args does not contain unknown arguments
+        /// - all arguments in args match their specification
+        /// If all of these are true, this function returns \c true
+        /// If not, this method returns \c false
+        ///
+        /// Example usage:
+        /// \code
+        ///     ElementPtr errors = Element::createList();
+        ///     if (module_specification_.validateCommand(cmd_str,
+        ///                                               arg,
+        ///                                               errors)) {
+        ///         std::cout << "Command is valid" << std::endl;
+        ///     } else {
+        ///         std::cout << "Command is invalid: " << std::endl;
+        ///         BOOST_FOREACH(ConstElementPtr error,
+        ///                       errors->listValue()) {
+        ///             std::cout << error->stringValue() << std::endl;
+        ///         }
+        ///     }
+        /// \endcode
+        ///
+        /// \param command The command to validate the arguments for
+        /// \param args A dict containing the command parameters
+        /// \param errors An ElementPtr pointing to a ListElement. Any
+        ///               errors that are found are added as
+        ///               StringElements to this list
+        /// \return true if the command is known and the parameters are correct
+        ///         false otherwise
+        bool validateCommand(const std::string& command,
+                              isc::data::ConstElementPtr args,
+                              isc::data::ElementPtr errors) const;
+
+
         /// errors must be of type ListElement
-        bool validate_config(isc::data::ConstElementPtr data, const bool full,
+        bool validateConfig(isc::data::ConstElementPtr data, const bool full,
                              isc::data::ElementPtr errors) const;
 
     private:
-        bool validate_item(isc::data::ConstElementPtr spec,
-                           isc::data::ConstElementPtr data,
-                           const bool full,
-                           isc::data::ElementPtr errors) const;
-        bool validate_spec(isc::data::ConstElementPtr spec,
+        bool validateItem(isc::data::ConstElementPtr spec,
+                          isc::data::ConstElementPtr data,
+                          const bool full,
+                          isc::data::ElementPtr errors) const;
+        bool validateSpec(isc::data::ConstElementPtr spec,
                            isc::data::ConstElementPtr data,
                            const bool full,
                            isc::data::ElementPtr errors) const;
-        bool validate_spec_list(isc::data::ConstElementPtr spec,
+        bool validateSpecList(isc::data::ConstElementPtr spec,
                                 isc::data::ConstElementPtr data,
                                 const bool full,
                                 isc::data::ElementPtr errors) const;

src/lib/config/tests/ccsession_unittests.cc

@@ -137,7 +137,7 @@ TEST_F(CCSessionTest, parseCommand) {
 
     cmd = parseCommand(arg, el("{ \"command\": [ \"my_command\" ] }"));
     EXPECT_EQ("my_command", cmd);
-    EXPECT_TRUE(isNull(arg));
+    EXPECT_EQ(*arg, *Element::createMap());
 
     cmd = parseCommand(arg, el("{ \"command\": [ \"my_command\", 1 ] }"));
     EXPECT_EQ("my_command", cmd);
@@ -193,8 +193,8 @@ ConstElementPtr my_command_handler(const std::string& command,
     if (command == "good_command") {
         return (createAnswer());
     } else if (command == "command_with_arg") {
-        if (arg) {
-            if (arg->getType() == Element::integer) {
+        if (arg->contains("number")) {
+            if (arg->get("number")->getType() == Element::integer) {
                 return (createAnswer(0, el("2")));
             } else {
                 return (createAnswer(1, "arg bad type"));
@@ -235,10 +235,10 @@ TEST_F(CCSessionTest, checkCommand) {
     // client will ask for config
     session.getMessages()->add(createAnswer(0, el("{}")));
 
-    EXPECT_FALSE(session.haveSubscription("Spec2", "*"));
-    ModuleCCSession mccs(ccspecfile("spec2.spec"), session, my_config_handler,
+    EXPECT_FALSE(session.haveSubscription("Spec29", "*"));
+    ModuleCCSession mccs(ccspecfile("spec29.spec"), session, my_config_handler,
                          my_command_handler);
-    EXPECT_TRUE(session.haveSubscription("Spec2", "*"));
+    EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
 
     EXPECT_EQ(2, session.getMsgQueue()->size());
     ConstElementPtr msg;
@@ -252,19 +252,19 @@ TEST_F(CCSessionTest, checkCommand) {
     EXPECT_EQ(0, result);
 
     // not a command, should be ignored
-    session.addMessage(el("1"), "Spec2", "*");
+    session.addMessage(el("1"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(0, result);
-
-    session.addMessage(el("{ \"command\": [ \"good_command\" ] }"), "Spec2",
+    session.addMessage(el("{ \"command\": [ \"good_command\" ] }"), "Spec29",
                        "*");
+
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
     EXPECT_EQ("{ \"result\": [ 0 ] }", msg->str());
     EXPECT_EQ(0, result);
 
-    session.addMessage(el("{ \"command\": \"bad_command\" }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": \"bad_command\" }"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
@@ -272,37 +272,37 @@ TEST_F(CCSessionTest, checkCommand) {
     EXPECT_EQ(0, result);
 
     session.addMessage(el("{ \"command\": [ \"bad_command\" ] }"),
-                       "Spec2", "*");
+                       "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
     EXPECT_EQ("{ \"result\": [ 1, \"bad command\" ] }", msg->str());
     EXPECT_EQ(0, result);
 
-    session.addMessage(el("{ \"command\": [ \"command_with_arg\", 1 ] }"),
-                       "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"command_with_arg\", {\"number\": 1} ] }"),
+                       "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
     EXPECT_EQ("{ \"result\": [ 0, 2 ] }", msg->str());
     EXPECT_EQ(0, result);
 
-    session.addMessage(el("{ \"command\": [ \"command_with_arg\" ] }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"command_with_arg\" ] }"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
     EXPECT_EQ("{ \"result\": [ 1, \"arg missing\" ] }", msg->str());
     EXPECT_EQ(0, result);
 
-    session.addMessage(el("{ \"command\": [ \"command_with_arg\", \"asdf\" ] }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"command_with_arg\", \"asdf\" ] }"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
-    EXPECT_EQ("{ \"result\": [ 1, \"arg bad type\" ] }", msg->str());
+    EXPECT_EQ("{ \"result\": [ 3, \"Error in command validation: args for command command_with_arg is not a map\" ] }", msg->str());
     EXPECT_EQ(0, result);
 
     mccs.setCommandHandler(NULL);
-    session.addMessage(el("{ \"command\": [ \"whatever\" ] }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"whatever\" ] }"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
@@ -310,7 +310,7 @@ TEST_F(CCSessionTest, checkCommand) {
     EXPECT_EQ(0, result);
 
     EXPECT_EQ(1, mccs.getValue("item1")->intValue());
-    session.addMessage(el("{ \"command\": [ \"config_update\", { \"item1\": 2 } ] }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"config_update\", { \"item1\": 2 } ] }"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
@@ -318,7 +318,7 @@ TEST_F(CCSessionTest, checkCommand) {
     EXPECT_EQ(0, result);
     EXPECT_EQ(2, mccs.getValue("item1")->intValue());
 
-    session.addMessage(el("{ \"command\": [ \"config_update\", { \"item1\": \"asdf\" } ] }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"config_update\", { \"item1\": \"asdf\" } ] }"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
@@ -326,7 +326,7 @@ TEST_F(CCSessionTest, checkCommand) {
     EXPECT_EQ(0, result);
     EXPECT_EQ(2, mccs.getValue("item1")->intValue());
 
-    session.addMessage(el("{ \"command\": [ \"config_update\", { \"item1\": 5 } ] }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"config_update\", { \"item1\": 5 } ] }"), "Spec29", "*");
     result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
@@ -387,9 +387,9 @@ TEST_F(CCSessionTest, ignoreRemoteConfigCommands) {
     // client will ask for config
     session.getMessages()->add(createAnswer(0, el("{  }")));
 
-    EXPECT_FALSE(session.haveSubscription("Spec2", "*"));
-    ModuleCCSession mccs(ccspecfile("spec2.spec"), session, my_config_handler, my_command_handler);
-    EXPECT_TRUE(session.haveSubscription("Spec2", "*"));
+    EXPECT_FALSE(session.haveSubscription("Spec29", "*"));
+    ModuleCCSession mccs(ccspecfile("spec29.spec"), session, my_config_handler, my_command_handler);
+    EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
 
     EXPECT_EQ(2, session.getMsgQueue()->size());
     ConstElementPtr msg;
@@ -404,7 +404,7 @@ TEST_F(CCSessionTest, ignoreRemoteConfigCommands) {
     msg = session.getFirstMessage(group, to);
 
     // Check if commands for the module are handled
-    session.addMessage(el("{ \"command\": [ \"good_command\" ] }"), "Spec2", "*");
+    session.addMessage(el("{ \"command\": [ \"good_command\" ] }"), "Spec29", "*");
     int result = mccs.checkCommand();
     EXPECT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);

src/lib/config/tests/module_spec_unittests.cc

@@ -30,7 +30,7 @@ std::string specfile(const std::string name) {
 }
 
 void
-module_spec_error(const std::string& file,
+moduleSpecError(const std::string& file,
                const std::string& error1,
                const std::string& error2 = "",
                const std::string& error3 = "")
@@ -52,7 +52,7 @@ TEST(ModuleSpec, ReadingSpecfiles) {
                               ->stringValue(), "Spec1");
     dd = moduleSpecFromFile(specfile("spec2.spec"));
     EXPECT_EQ(dd.getFullSpec()->get("config_data")->size(), 6);
-    module_spec_error("doesnotexist",
+    moduleSpecError("doesnotexist",
                    "Error opening ",
                    specfile("doesnotexist"),
                    ": No such file or directory");
@@ -81,69 +81,65 @@ TEST(ModuleSpec, ReadingSpecfiles) {
 }
 
 TEST(ModuleSpec, SpecfileItems) {
-    module_spec_error("spec3.spec",
+    moduleSpecError("spec3.spec",
                    "item_name missing in { \"item_default\": 1, \"item_optional\": false, \"item_type\": \"integer\" }");
-    module_spec_error("spec4.spec",
+    moduleSpecError("spec4.spec",
                    "item_type missing in { \"item_default\": 1, \"item_name\": \"item1\", \"item_optional\": false }");
-    module_spec_error("spec5.spec",
+    moduleSpecError("spec5.spec",
                    "item_optional missing in { \"item_default\": 1, \"item_name\": \"item1\", \"item_type\": \"integer\" }");
-    module_spec_error("spec6.spec",
+    moduleSpecError("spec6.spec",
                    "item_default missing in { \"item_name\": \"item1\", \"item_optional\": false, \"item_type\": \"integer\" }");
-    module_spec_error("spec9.spec",
+    moduleSpecError("spec9.spec",
                    "item_default not of type integer");
-    module_spec_error("spec10.spec",
+    moduleSpecError("spec10.spec",
                    "item_default not of type real");
-    module_spec_error("spec11.spec",
+    moduleSpecError("spec11.spec",
                    "item_default not of type boolean");
-    module_spec_error("spec12.spec",
+    moduleSpecError("spec12.spec",
                    "item_default not of type string");
-    module_spec_error("spec13.spec",
+    moduleSpecError("spec13.spec",
                    "item_default not of type list");
-    module_spec_error("spec14.spec",
+    moduleSpecError("spec14.spec",
                    "item_default not of type map");
-    module_spec_error("spec15.spec",
+    moduleSpecError("spec15.spec",
                    "badname is not a valid type name");
 }
 
 TEST(ModuleSpec, SpecfileConfigData) {
-    module_spec_error("spec7.spec",
+    moduleSpecError("spec7.spec",
                    "module_name missing in {  }");
-    module_spec_error("spec8.spec",
+    moduleSpecError("spec8.spec",
                    "No module_spec in specification");
-    module_spec_error("spec16.spec",
+    moduleSpecError("spec16.spec",
                    "config_data is not a list of elements");
-    module_spec_error("spec21.spec",
+    moduleSpecError("spec21.spec",
                    "commands is not a list of elements");
 }
 
 TEST(ModuleSpec, SpecfileCommands) {
-    module_spec_error("spec17.spec",
+    moduleSpecError("spec17.spec",
                    "command_name missing in { \"command_args\": [ { \"item_default\": \"\", \"item_name\": \"message\", \"item_optional\": false, \"item_type\": \"string\" } ], \"command_description\": \"Print the given message to stdout\" }");
-    module_spec_error("spec18.spec",
+    moduleSpecError("spec18.spec",
                    "command_args missing in { \"command_description\": \"Print the given message to stdout\", \"command_name\": \"print_message\" }");
-    module_spec_error("spec19.spec",
+    moduleSpecError("spec19.spec",
                    "command_args not of type list");
-    module_spec_error("spec20.spec",
+    moduleSpecError("spec20.spec",
                    "somethingbad is not a valid type name");
-/*
-    module_spec_error("spec22.spec",
-                   "");
-*/
 }
 
 bool
-data_test(const ModuleSpec& dd, const std::string& data_file_name) {
+dataTest(const ModuleSpec& dd, const std::string& data_file_name) {
     std::ifstream data_file;
 
     data_file.open(specfile(data_file_name).c_str());
     ConstElementPtr data = Element::fromJSON(data_file, data_file_name);
     data_file.close();
 
-    return (dd.validate_config(data));
+    return (dd.validateConfig(data));
 }
 
 bool
-data_test_with_errors(const ModuleSpec& dd, const std::string& data_file_name,
+dataTestWithErrors(const ModuleSpec& dd, const std::string& data_file_name,
                       ElementPtr errors)
 {
     std::ifstream data_file;
@@ -152,27 +148,64 @@ data_test_with_errors(const ModuleSpec& dd, const std::string& data_file_name,
     ConstElementPtr data = Element::fromJSON(data_file, data_file_name);
     data_file.close();
 
-    return (dd.validate_config(data, true, errors));
+    return (dd.validateConfig(data, true, errors));
 }
 
 TEST(ModuleSpec, DataValidation) {
     ModuleSpec dd = moduleSpecFromFile(specfile("spec22.spec"));
 
-    EXPECT_TRUE(data_test(dd, "data22_1.data"));
-    EXPECT_FALSE(data_test(dd, "data22_2.data"));
-    EXPECT_FALSE(data_test(dd, "data22_3.data"));
-    EXPECT_FALSE(data_test(dd, "data22_4.data"));
-    EXPECT_FALSE(data_test(dd, "data22_5.data"));
-    EXPECT_TRUE(data_test(dd, "data22_6.data"));
-    EXPECT_TRUE(data_test(dd, "data22_7.data"));
-    EXPECT_FALSE(data_test(dd, "data22_8.data"));
-    EXPECT_FALSE(data_test(dd, "data22_9.data"));
+    EXPECT_TRUE(dataTest(dd, "data22_1.data"));
+    EXPECT_FALSE(dataTest(dd, "data22_2.data"));
+    EXPECT_FALSE(dataTest(dd, "data22_3.data"));
+    EXPECT_FALSE(dataTest(dd, "data22_4.data"));
+    EXPECT_FALSE(dataTest(dd, "data22_5.data"));
+    EXPECT_TRUE(dataTest(dd, "data22_6.data"));
+    EXPECT_TRUE(dataTest(dd, "data22_7.data"));
+    EXPECT_FALSE(dataTest(dd, "data22_8.data"));
+    EXPECT_FALSE(dataTest(dd, "data22_9.data"));
 
     ElementPtr errors = Element::createList();
-    EXPECT_FALSE(data_test_with_errors(dd, "data22_8.data", errors));
+    EXPECT_FALSE(dataTestWithErrors(dd, "data22_8.data", errors));
     EXPECT_EQ("[ \"Type mismatch\" ]", errors->str());
 
     errors = Element::createList();
-    EXPECT_FALSE(data_test_with_errors(dd, "data22_9.data", errors));
+    EXPECT_FALSE(dataTestWithErrors(dd, "data22_9.data", errors));
     EXPECT_EQ("[ \"Unknown item value_does_not_exist\" ]", errors->str());
 }
+
+TEST(ModuleSpec, CommandValidation) {
+    ModuleSpec dd = moduleSpecFromFile(specfile("spec2.spec"));
+    ConstElementPtr arg = Element::fromJSON("{}");
+    ElementPtr errors = Element::createList();
+
+    EXPECT_TRUE(dd.validateCommand("shutdown", arg, errors));
+    EXPECT_EQ(errors->size(), 0);
+
+    errors = Element::createList();
+    EXPECT_FALSE(dd.validateCommand("unknowncommand", arg, errors));
+    EXPECT_EQ(errors->size(), 1);
+    EXPECT_EQ(errors->get(0)->stringValue(), "Unknown command unknowncommand");
+
+    errors = Element::createList();
+    EXPECT_FALSE(dd.validateCommand("print_message", arg, errors));
+    EXPECT_EQ(errors->size(), 1);
+    EXPECT_EQ(errors->get(0)->stringValue(), "Non-optional value missing");
+
+    errors = Element::createList();
+    arg = Element::fromJSON("{ \"message\": \"Hello\" }");
+    EXPECT_TRUE(dd.validateCommand("print_message", arg, errors));
+    EXPECT_EQ(errors->size(), 0);
+
+    errors = Element::createList();
+    arg = Element::fromJSON("{ \"message\": \"Hello\", \"unknown_second_arg\": 1 }");
+    EXPECT_FALSE(dd.validateCommand("print_message", arg, errors));
+    EXPECT_EQ(errors->size(), 1);
+    EXPECT_EQ(errors->get(0)->stringValue(), "Unknown item unknown_second_arg");
+
+    errors = Element::createList();
+    arg = Element::fromJSON("{ \"message\": 1 }");
+    EXPECT_FALSE(dd.validateCommand("print_message", arg, errors));
+    EXPECT_EQ(errors->size(), 1);
+    EXPECT_EQ(errors->get(0)->stringValue(), "Type mismatch");
+
+}

src/lib/config/tests/testdata/Makefile.am

@@ -49,3 +49,4 @@ EXTRA_DIST += spec25.spec
 EXTRA_DIST += spec26.spec
 EXTRA_DIST += spec27.spec
 EXTRA_DIST += spec28.spec
+EXTRA_DIST += spec29.spec

src/lib/config/tests/testdata/spec29.spec

@@ -0,0 +1,35 @@
+{
+  "module_spec": {
+    "module_name": "Spec29",
+    "config_data": [
+      { "item_name": "item1",
+        "item_type": "integer",
+        "item_optional": false,
+        "item_default": 1
+      }
+    ],
+    "commands": [
+      {
+        "command_name": "good_command",
+        "command_description": "A good command",
+        "command_args": []
+      },
+      {
+        "command_name": "bad_command",
+        "command_description": "A bad command",
+        "command_args": []
+      },
+      {
+        "command_name": "command_with_arg",
+        "command_description": "A command with an (integer) argument",
+        "command_args": [ {
+          "item_name": "number",
+          "item_type": "integer",
+          "item_optional": true,
+          "item_default": 1
+        } ]
+      }
+    ]
+  }
+}
+

src/lib/datasrc/memory_datasrc.cc

@@ -40,6 +40,7 @@ struct MemoryZone::MemoryZoneImpl {
     // Information about the zone
     RRClass zone_class_;
     Name origin_;
+    string file_name_;
 
     // Some type aliases
     /*
@@ -84,8 +85,8 @@ struct MemoryZone::MemoryZoneImpl {
         DomainNode* node;
         switch (domains->insert(name, &node)) {
             // Just check it returns reasonable results
-            case DomainTree::SUCCEED:
-            case DomainTree::ALREADYEXIST:
+            case DomainTree::SUCCESS:
+            case DomainTree::ALREADYEXISTS:
                 break;
             // Something odd got out
             default:
@@ -275,10 +276,21 @@ MemoryZone::load(const string& filename) {
     masterLoad(filename.c_str(), getOrigin(), getClass(),
         boost::bind(&MemoryZoneImpl::addFromLoad, impl_, _1, &tmp));
     // If it went well, put it inside
+    impl_->file_name_ = filename;
     tmp.swap(impl_->domains_);
     // And let the old data die with tmp
 }
 
+void
+MemoryZone::swap(MemoryZone& zone) {
+    std::swap(impl_, zone.impl_);
+}
+
+const string
+MemoryZone::getFileName() const {
+    return (impl_->file_name_);
+}
+
 /// Implementation details for \c MemoryDataSrc hidden from the public
 /// interface.
 ///

src/lib/datasrc/memory_datasrc.h

@@ -15,6 +15,8 @@
 #ifndef __MEMORY_DATA_SOURCE_H
 #define __MEMORY_DATA_SOURCE_H 1
 
+#include <string>
+
 #include <datasrc/zonetable.h>
 
 namespace isc {
@@ -98,6 +100,20 @@ public:
         { }
     };
 
+    /// Return the master file name of the zone
+    ///
+    /// This method returns the name of the zone's master file to be loaded.
+    /// The returned string will be an empty unless the zone has successfully
+    /// loaded a zone.
+    ///
+    /// This method should normally not throw an exception.  But the creation
+    /// of the return string may involve a resource allocation, and if it
+    /// fails, the corresponding standard exception will be thrown.
+    ///
+    /// \return The name of the zone file loaded in the zone, or an empty
+    /// string if the zone hasn't loaded any file.
+    const std::string getFileName() const;
+
     /// \brief Load zone from masterfile.
     ///
     /// This loads data from masterfile specified by filename. It replaces
@@ -122,6 +138,15 @@ public:
     ///     This will probably be needed when a better implementation of
     ///     configuration reloading is written.
     void load(const std::string& filename);
+
+    /// Exchanges the content of \c this zone with that of the given \c zone.
+    ///
+    /// This method never throws an exception.
+    ///
+    /// \param zone Another \c MemoryZone object which is to be swapped with
+    /// \c this zone.
+    void swap(MemoryZone& zone);
+
 private:
     /// \name Hidden private data
     //@{
@@ -158,10 +183,6 @@ private:
 /// The findZone() method takes a domain name and returns the best matching \c
 /// MemoryZone in the form of (Boost) shared pointer, so that it can provide
 /// the general interface for all data sources.
-///
-/// Currently, \c FindResult::zone is immutable for safety.
-/// In future versions we may want to make it changeable.  For example,
-/// we may want to allow configuration update on an existing zone.
 class MemoryDataSrc {
 public:
     /// \brief A helper structure to represent the search result of
@@ -180,11 +201,11 @@ public:
     /// See the description of \c find() for the semantics of the member
     /// variables.
     struct FindResult {
-        FindResult(result::Result param_code, const ConstZonePtr param_zone) :
+        FindResult(result::Result param_code, const ZonePtr param_zone) :
             code(param_code), zone(param_zone)
         {}
         const result::Result code;
-        const ConstZonePtr zone;
+        const ZonePtr zone;
     };
 
     ///

src/lib/datasrc/rbtree.h

@@ -17,11 +17,11 @@
 
 //! \file datasrc/rbtree.h
 ///
-/// \note The purpose of the RBTree is to provide a generic map with 
-/// domain names as the key that can be used by various BIND 10 modules or
-/// even by other applications.  However, because of some unresolved design
-/// issue, the design and interface are not fixed, and RBTree isn't ready to
-/// be used as a base data structure by other modules.
+/// \note The purpose of the RBTree is to provide a generic map with
+///     domain names as the key that can be used by various BIND 10 modules or
+///     even by other applications.  However, because of some unresolved design
+///     issue, the design and interface are not fixed, and RBTree isn't ready
+///     to be used as a base data structure by other modules.
 
 #include <dns/name.h>
 #include <boost/utility.hpp>
@@ -35,16 +35,18 @@ namespace datasrc {
 
 namespace helper {
 
-/// Helper function to remove the base domain from super domain
+/// \brief Helper function to remove the base domain from super domain.
 ///
-/// the precondition of this function is the super_name contains the
+/// The precondition of this function is the super_name contains the
 /// sub_name so
 /// \code Name a("a.b.c");
 /// Name b("b.c");
 /// Name c = a - b;
 /// \endcode
+/// c will contain "a".
 ///
-/// \note function in this namespace is not intended to be used outside.
+/// \note Functions in this namespace is not intended to be used outside of
+///     RBTree implementation.
 inline isc::dns::Name
 operator-(const isc::dns::Name& super_name, const isc::dns::Name& sub_name) {
     return (super_name.split(0, super_name.getLabelCount() -
@@ -55,63 +57,95 @@ operator-(const isc::dns::Name& super_name, const isc::dns::Name& sub_name) {
 template <typename T>
 class RBTree;
 
-/// \brief \c RBNode use by RBTree to store any data related to one domain name
+/// \brief \c RBNode is used by RBTree to store any data related to one domain
+///     name.
 ///
-/// It has two roles, the first one is as one node in the \c RBTree,
-/// the second one is to store the data related to one domain name and maintain
-/// the domain name hierarchy struct in one domain name space.
-/// As for the first role, it has left, right, parent and color members
-/// which is used to keep the balance of the \c RBTree.
-/// As for the second role, \c RBNode use down pointer to refer to all its sub
-/// domains, so the name of current node always relative to the up node. since
-/// we only has down pointer without up pointer, so we can only walk down from
-/// top domain to sub domain.
-/// One special kind of node is non-terminal node
-/// which has subdomains with RRset but itself doesn't have any RRsets.
+/// This is meant to be used only from RBTree. It is meaningless to inherit it
+/// or create instances of it from elsewhere. For that reason, the constructor
+/// is private.
 ///
-/// \note \c RBNode basically used internally by RBTree, it is meaningless to
-/// inherited from it or create it without \c RBTree.
+/// It serves three roles. One is to keep structure of the \c RBTree as a
+/// red-black tree. For that purpose, it has left, right and parent pointers
+/// and color member. These are private and accessed only from within the tree.
+///
+/// The second one is to store data for one domain name. The data related
+/// functions can be used to access and set the data.
+///
+/// The third role is to keep the hierarchy of domains. The down pointer points
+/// to a subtree of subdomains. Note that we can traverse the hierarchy down,
+/// but not up.
+///
+/// One special kind of node is non-terminal node. It has subdomains with
+/// RRsets, but doesn't have any RRsets itself.
 template <typename T>
 class RBNode : public boost::noncopyable {
-public:
-    /// only \c RBTree can create and destroy \c RBNode
+private:
+    /// The RBNode is meant for use from within RBTree, so it has access to
+    /// it.
     friend class RBTree<T>;
-    typedef boost::shared_ptr<T> NodeDataPtr;
 
-    /// \name Destructor
-    /// \note it's seems a little strange that constructor is private
-    /// but deconstructor left public, the reason is for some smart pointer
-    /// like std::auto_ptr, they needs to delete RBNode in sometimes, but
-    /// \code delete *pointer_to_node \endcode shouldn't be called directly
+    /// \name Constructors
+    ///
+    /// \note The existence of a RBNode without a RBTree is meaningless.
+    ///     Therefore the constructors are private.
     //@{
-    ~RBNode();
+
+    /// \brief Default constructor.
+    ///
+    /// This constructor is provided specifically for generating a special
+    /// "null" node.
+    RBNode();
+
+    /// \brief Constructor from the node name.
+    ///
+    /// \param name The *relative* domain name (if this will live inside
+    ///     a.b.c and is called d.e.a.b.c, then you pass d.e).
+    RBNode(const isc::dns::Name& name);
     //@}
 
-    /// \name Test functions
+public:
+    /// \brief Alias for shared pointer to the data.
+    typedef boost::shared_ptr<T> NodeDataPtr;
+
+    /// \brief Destructor
+    ///
+    /// It might seem strange that constructors are private and destructor
+    /// public, but this is needed because of shared pointers need access
+    /// to the destructor.
+    ///
+    /// You should never call anything like:
+    /// \code delete pointer_to_node; \endcode
+    /// The RBTree handles both creation and destructoion of nodes.
+    ~RBNode();
+
+    /// \name Getter functions.
     //@{
-    /// \brief return the name of current node, it's relative to its top node
+    /// \brief Return the name of current node.
+    ///
+    /// It's relative to its containing node.
     ///
     /// To get the absolute name of one node, the node path from the top node
-    /// to current node has to be recorded
+    /// to current node has to be recorded.
     const isc::dns::Name& getName() const { return (name_); }
 
-    /// \brief return the data store in this node
-    /// \note, since the data is managed by RBNode, developer should not
-    /// free the pointer
+    /// \brief Return the data stored in this node.
+    ///
+    /// You should not delete the data, it is handled by shared pointers.
     NodeDataPtr& getData() { return (data_); }
-    /// \brief return the data stored in this node, read-only version
+    /// \brief Return the data stored in this node.
     const NodeDataPtr& getData() const { return (data_); }
 
-    /// \brief return whether the node has related data
-    /// \note it's meaningless has empty \c RBNode in one RBTree, the only
-    /// exception is for non-terminal node which has sub domain nodes who
-    /// has data(rrset)
+    /// \brief return whether the node has related data.
+    ///
+    /// There can be empty nodes inside the RBTree. They are usually the
+    /// non-terminal domains, but it is possible (yet probably meaningless)
+    /// empty nodes anywhere.
     bool isEmpty() const { return (data_.get() == NULL); }
     //@}
 
-    /// \name Modify functions
+    /// \name Setter functions.
     //@{
-    /// \breif set the data stored in the node
+    /// \brief Set the data stored in the node.
     void setData(const NodeDataPtr& data) { data_ = data; }
     //@}
 
@@ -122,8 +156,6 @@ public:
     /// These methods never throw an exception.
     //@{
     /// Return if callback is enabled at the node.
-    ///
-    /// This method never throws an exception.
     bool isCallbackEnabled() const { return (callback_required_); }
 
     /// Enable callback at the node.
@@ -137,55 +169,45 @@ public:
 private:
     /// \brief Define rbnode color
     enum RBNodeColor {BLACK, RED};
-
-    /// \name Constructors
-    /// \note \c Single RBNode is meaningless without living inside one \c RBTree
-    /// the creation and destroy of one \c RBNode is handle by host \c RBTree, so
-    /// the constructors and destructor of \c RBNode is left private
-    //@{
-    /// \brief Default constructor.
-    ///
-    /// This constructor is provided specifically for generating a special
-    /// "null" node, and is intended be used only internally.
-    RBNode();
-
-    /// \brief Constructor from the node name.
-    ///
-    /// \param name The domain name corresponding to the node.
-    RBNode(const isc::dns::Name& name);
-    //@}
-
     /// This is a factory class method of a special singleton null node.
     static RBNode<T>* NULL_NODE() {
         static RBNode<T> null_node;
         return (&null_node);
     }
 
-    /// data to maintain the rbtree balance
+    /// \name Data to maintain the rbtree structure.
+    //@{
     RBNode<T>*  parent_;
     RBNode<T>*  left_;
     RBNode<T>*  right_;
     RBNodeColor color_;
+    //@}
 
+    /// \brief Relative name of the node.
     isc::dns::Name     name_;
+    /// \brief Data stored here.
     NodeDataPtr       data_;
 
-    /// the down pointer points to the root node of sub domains of current
-    /// domain
-    /// \par Adding down pointer to \c RBNode is for two purpose:
-    /// \li Accelerate the search process, with sub domain tree, it split the
-    /// big flat tree into several hierarchy trees
-    /// \li It save memory useage, so same label won't be saved several times
+    /// \brief The subdomain tree.
+    ///
+    /// This points to the root node of trees of subdomains of this domain.
+    ///
+    /// \par Adding down pointer to \c RBNode has two purposes:
+    /// \li Accelerate the search process, with sub domain tree, it splits the
+    ///     big flat tree into several hierarchy trees.
+    /// \li It saves memory useage as it allows storing only relative names,
+    ///     avoiding storage of the same domain labels multiple times.
     RBNode<T>*  down_;
 
-    // If true, callback should be called at this node in search.
-    // (This may have to become part of more general "attribute flags")
+    /// \brief If callback should be called when traversing this node in
+    /// RBTree::find().
+    ///
+    /// \todo It might be needed to put it into more general attributes field.
     bool callback_required_;
 };
 
 
-// typically each node should has a name associate with it
-// this construction is only used to create \c NULLNODE
+// This is only to support NULL nodes.
 template <typename T>
 RBNode<T>::RBNode() :
     parent_(this),
@@ -216,30 +238,39 @@ template <typename T>
 RBNode<T>::~RBNode() {
 }
 
-// note: the following class description is documented using C-style comments
+// note: the following class description is documented using multiline comments
 // because the verbatim diagram contain a backslash, which could be interpreted
-// as part of a multi-line comment with C++ style comments.
+// as escape of newline in singleline comment.
 /**
- *  \brief \c RBTree class represents all the domains with the same suffix,
- *  so it can be used to store the domains in one zone.
- * 
- *  \c RBTree is a generic red black tree, and contains all the nodes with
- *  the same suffix, since each name may have sub domain names
- *  so \c RBTree is a recursive data structure namely tree in tree.
- *  So for one zone, several RBTrees may be involved. But from outside, the sub
- *  tree is opaque for end users.
- * 
- *  \c RBTree split the domain space into hierarchy red black trees, nodes in one
- *  tree has the same base name. The benefit of this struct is that:
- *  - enhance the query performace compared with one big flat red black tree
- *  - decrase the memory footprint to save common labels only once.
- * 
+ *  \brief \c RBTree class represents all the domains with the same suffix.
+ *      It can be used to store the domains in one zone, for example.
+ *
+ *  RBTree is a generic map from domain names to any kind of data. Internally,
+ *  it uses a red-black tree. However, it isn't one tree containing everything.
+ *  Subdomains are trees, so this structure is recursive - trees inside trees.
+ *  But, from the interface point of view, it is opaque data structure.
+ *
+ *  \c RBTree splits the domain space into hierarchy red black trees; nodes
+ * in one tree has the same base name. The benefit of this struct is that:
+ *  - Enhances the query performace compared with one big flat red black tree.
+ *  - Decreases the memory footprint, as it doesn't store the suffix labels
+ *      multiple times.
+ *
+ * \anchor diagram
+ *
+ * with the following names:
+ *  - a
+ *  - b
+ *  - c
+ *  - x.d.e.f
+ *  - z.d.e.f
+ *  - g.h
+ *  - o.w.y.d.e.f
+ *  - p.w.y.d.e.f
+ *  - q.w.y.d.e.f
+ *
+ * the tree will looks like:
  *  \verbatim
-  with the following names:
-      a       x.d.e.f     o.w.y.d.e.f
-      b       z.d.e.f     p.w.y.d.e.f
-      c       g.h         q.w.y.d.e.f
-      the tree will looks like:
                                 b
                               /   \
                              a    d.e.f
@@ -253,33 +284,38 @@ RBNode<T>::~RBNode() {
                                      p
                                     / \
                                    o   q
- *  \endverbatim
+   \endverbatim
  *  \note open problems:
- *  - current find funciton only return non-empty nodes, so there is no difference
- *    between find one not exist name with empty non-terminal nodes, but in DNS query
- *    logic, they are different
+ *  - current \c find() function only returns non-empty nodes, so there is no
+ *    difference between find a non existent name and a name corresponding to
+ *    an empty non-terminal nodes, but in DNS query logic, they are different
  *  \todo
  *  - add remove interface
- *  - add iterator to iterate the whole rbtree while may needed by axfr
- *  - since \c RBNode only has down pointer without up pointer, the node path during finding
- *    should be recorded for later use
+ *  - add iterator to iterate over the whole \c RBTree.  This may be necessary,
+ *    for example, to support AXFR.
+ *  - since \c RBNode only has down pointer without up pointer, the node path
+ *    during finding should be recorded for later use
  */
 template <typename T>
 class RBTree : public boost::noncopyable {
     friend class RBNode<T>;
 public:
-    /// \brief The return value for the \c find() insert() and erase() method
+    /// \brief The return value for the \c find() and insert() methods
     enum Result {
-        SUCCEED, //insert or erase succeed
-        EXACTMATCH, //find the target name
-        PARTIALMATCH, //find part of target name
-        NOTFOUND,  // for find function means no related name found
-                   // for erase function means erase not exist name
-        ALREADYEXIST, //for insert operation, the name to insert already exist
+        SUCCESS,    ///< Insert was successful
+        /// \brief The node returned from find mathes exactly the name given
+        EXACTMATCH,
+        PARTIALMATCH, ///< A superdomain node was found
+        NOTFOUND,   ///< Not even any superdomain was found
+        /// \brief Returned by insert() if a node of the name already exists
+        ALREADYEXISTS,
     };
 
     /// \name Constructor and Destructor
     //@{
+    /// The constructor.
+    ///
+    /// It never throws an exception.
     explicit RBTree();
 
     /// \b Note: RBTree is not intended to be inherited so the destructor
@@ -287,130 +323,157 @@ public:
     ~RBTree();
     //@}
 
-    /// \name Inquery methods
-    //@{
-    /// \brief Find the node that gives a longest match against the given name
-    ///
-    /// This method searches the \c RBTree for a node whose name is a longest
-    /// match against \c name.  The found node, if any, is returned via the
-    /// \c node pointer.
-    /// By default, nodes that don't have data will be ignored, and the result
-    /// can be \c NOTFOUND even if there is a node whose name matches the
-    /// given \c name.
-    /// We'll soon introduce a "no data OK" mode in this method.  It would
-    /// match any node of the tree regardless of whether the node has data
-    /// or not.
-    /// Since the tree is "compressed", i.e., a node can contain multiple
-    /// name labels, there are counter intuitive cases in the "no data OK"
-    /// mode.  For example, see the diagram of the class description.
-    /// Name "y.d.e.f" is logically contained in the tree as part of the
-    /// "compressed" node of "w.y".  But the search logic of this method
-    /// cannot find the logical match, and would return a \c PARTIALMATCH
-    /// result pointing to node "d.e.f".  To correctly identify the real
-    /// longest match, "y.d.e.f" with empty data, the caller needs to
-    /// perform additional steps.
-    ///
-    /// This version of \c find() method is templated to allow the caller
-    /// to specify a "hook" at nodes that give a partial match.
-    /// When the search encounters a node with data that partially matches
-    /// \c name (i.e. node's name is a superdomain of \c name) and has
-    /// enabled callback (via the \c RBNode::enableCallback() method), if
-    /// \c callback is non \c NULL then the callback function is called
-    /// with the argument of a reference to the node and the given
-    /// callback argument (\c callback_arg).  The template parameter specifies
-    /// the type of the callback argument.
-    /// The callback function returns either \c true or \c false, meaning
-    /// the search should stop or continue, respectively.
-    /// If the return value is \c true the search stops immediately at the
-    /// node even if there could be a longer matching name below it.
-    /// In reality, this convoluted callback rule is specifically intended
-    /// to be used to handle a zone cut (delegation) at a name search inside
-    /// a zone, and won't be used in any other cases.
-    /// Other applications of the tree won't need callbacks, and they should
-    /// use the non templated version of the \c find() method.
-    ///
-    /// Since the expected usage of callback is very limited, we do not
-    /// generalize the interface so that it can be an arbitrary functions or
-    /// functor objects in favor of simplicity and efficiency.
-    ///
-    /// This method involves operations on names that can throw an exception.
+    /// \name Find methods
+    ///
+    /// \brief Find the node that gives a longest match against the given name.
+    ///
+    /// \anchor find
+    ///
+    /// These methods search the RBTree for a node whose name is a longest
+    /// against name. The found node, if any, is returned via the node pointer.
+    ///
+    /// By default, nodes that don't have data (see RBNode::isEmpty) are
+    /// ignored and the result can be NOTFOUND even if there's a node whose
+    /// name mathes. The plan is to introduce a "no data OK" mode for this
+    /// method, that would match any node of the tree regardless of wheather
+    /// the node has any data or not.
+    ///
+    /// The case with "no data OK" mode is not as easy as it seems. For example
+    /// in the diagram shown in the class description, the name y.d.e.f is
+    /// logically contained in the tree as part of the node w.y.  It cannot be
+    /// identified simply by checking whether existing nodes (such as
+    /// d.e.f or w.y) has data.
+    ///
+    /// These methods involves operations on names that can throw an exception.
     /// If that happens the exception will be propagated to the caller.
     /// The callback function should generally not throw an exception, but
     /// if it throws, the exception will be propagated to the caller.
     ///
+    /// The \c name parameter says what should be found. The node parameter
+    /// is output only and in case of EXACTMATCH and PARTIALMATCH, it is set
+    /// to a pointer to the found node.
+    ///
+    /// They return:
+    ///  - EXACTMATCH when a node with the same name as requested exists.
+    ///  - PARTIALMATCH when a node with the same name does not exist (or is
+    ///    empty), but there's a (nonempty) superdomain of the requested one.
+    ///    The superdomain with longest name is returned through the node
+    ///    parameter. Beware that if you store a zone in the tree, you may get
+    ///    PARTIALMATCH with zone apex when the given domain name is not there.
+    ///    You should not try to delegate into another zone in that case.
+    ///  - NOTFOUND if there's no node with the same name nor any superdomain
+    ///    of it. In that case, node parameter is left intact.
+    //@{
+
+    /// \brief Find with callback.
+    ///
+    /// \anchor callback
+    ///
+    /// This version of find calls the callback whenever traversing (on the
+    /// way from root down the tree) a marked node on the way down through the
+    /// domain namespace (see RBNode::enableCallback and related functions).
+    ///
+    /// If you return true from the callback, the search is stopped and a
+    /// PARTIALMATCH is returned with the given node. Note that this node
+    /// doesn't really need to be the one with longest possible match.
+    ///
+    /// This callback mechanism was designed with zone cut (delegation)
+    /// processing in mind. The marked nodes would be the ones at delegation
+    /// points. It is not expected that any other applications would need
+    /// callbacks; they should use the versions of find without callbacks.
+    /// The callbacks are not general functors for the same reason - we don't
+    /// expect it to be needed.
+    ///
     /// \param name Target to be found
     /// \param node On success (either \c EXACTMATCH or \c PARTIALMATCH)
-    /// it will store a pointer to the matching node
+    ///     it will store a pointer to the matching node
     /// \param callback If non \c NULL, a call back function to be called
-    /// at "delegation" nodes (see above).
+    ///     at marked nodes (see above).
     /// \param callback_arg A caller supplied argument to be passed to
-    /// \c callback.
-    ///
-    /// \return \c EXACTMATCH A node that whose name is equal to \c name is
-    /// found.  \c *node will be set to point to that node.
-    /// \return \c PARTIALMATCH There is a no exact match, but a superdomain
-    /// of \c name exists.  \c node will be set to point to the node whose
-    /// name is the longest among such superdomains.
-    /// \return \c NOTFOUND There is no exact or partial match against \c name
-    /// \c *node will be intact in this case.
+    ///     \c callback.
+    ///
+    /// \return As described above, but in case of callback returning true,
+    ///     it returns immediately with the current node.
     template <typename CBARG>
     Result find(const isc::dns::Name& name, RBNode<T>** node,
                 bool (*callback)(const RBNode<T>&, CBARG),
                 CBARG callback_arg) const;
 
-    /// Same as the other version, but the returned \c node will be immutable.
+    /// \brief Find with callback returning immutable node.
+    ///
+    /// It has the same behaviour as the find with \ref callback version.
     template <typename CBARG>
     Result find(const isc::dns::Name& name, const RBNode<T>** node,
                 bool (*callback)(const RBNode<T>&, CBARG),
                 CBARG callback_arg) const;
 
-    /// Same as the templated version, but does not use callback.
+    /// \brief Simple find.
     ///
-    /// Applications except the zone implementation should generally use the
-    /// non templated version.
+    /// Acts as described in the \ref find section.
     Result find(const isc::dns::Name& name, RBNode<T>** node) const {
         return (find<void*>(name, node, NULL, NULL));
     }
 
-    /// Same as the templated version, but does not use callback, and the
-    /// returned \c node will be immutable.
+    /// \brieg Simple find returning immutable node.
     ///
-    /// In general, this version should be preferred over the other non
-    /// templated version, unless the caller knows it should modify the
-    /// returned node.
+    /// Acts as described in the \ref find section, but returns immutable node
+    /// pointer.
     Result find(const isc::dns::Name& name, const RBNode<T>** node) const {
         return (find<void*>(name, node, NULL, NULL));
     }
-
-    /// \brief Get the total node count in the tree
-    /// the node count including the node created common suffix node,
-    /// this function will only be used for debuging
-    int getNodeCount() const { return (node_count_);}
     //@}
 
+    /// \brief Get the total number of nodes in the tree
+    ///
+    /// It includes nodes internally created as a result of adding a domain
+    /// name that is a subdomain of an existing node of the tree.
+    /// This function is mainly intended to be used for debugging.
+    int getNodeCount() const { return (node_count_); }
+
     /// \name Debug function
     //@{
-    /// \brief print the nodes in the trees
+    /// \brief Print the nodes in the trees.
+    ///
+    /// \param os A \c std::ostream object to which the tree is printed.
+    /// \param depth A factor of the initial indentation.  Each line
+    /// will begin with space character repeating <code>5 * depth</code>
+    /// times.
     void dumpTree(std::ostream& os, unsigned int depth = 0) const;
     //@}
 
-    /// \name Modify function
+    /// \name Modify functions
     //@{
-    /// \brief Insert the domain name into the tree
-    /// \param name The name to be inserted into the tree
-    /// \param inserted_node If no node with the name in the tree,
-    /// new \c RBNode will be created, otherwise nothing will be done.
-    /// Anyway the pointer point to the node with the name will be assigned to
-    /// inserted_node
+    /// \brief Insert the domain name into the tree.
+    ///
+    /// It either finds an already existing node of the given name or inserts
+    /// a new one, if none exists yet. In any case, the inserted_node parameter
+    /// is set to point to that node. You can fill data into it or modify it.
+    /// So, if you don't know if a node exists or not and you need to modify
+    /// it, just call insert and act by the result.
+    ///
+    /// Please note that the tree can add some empty nodes by itself, so don't
+    /// assume that if you didn't insert a node of that name it doesn't exist.
+    ///
+    /// This method normally involves resource allocation.  If it fails
+    /// the corresponding standard exception will be thrown.
+    ///
+    /// This method does not provide the strong exception guarantee in its
+    /// strict sense; if an exception is thrown in the middle of this
+    /// method, the internal structure may change.  However, it should
+    /// still retain the same property as a mapping container before this
+    /// method is called.  For example, the result of \c find() should be
+    /// the same.  This method provides the weak exception guarantee in its
+    /// normal sense.
+    ///
+    /// \param name The name to be inserted into the tree.
+    /// \param inserted_node This is an output parameter and is set to the
+    ///     node.
+    ///
     /// \return
-    //  - SUCCEED means no node exists in the tree with the name before insert
-    /// - ALREADYEXIST means already has the node with the given name
-    //
-    /// \node To modify the data related with one name but not sure the name has
-    /// inserted or not, it is better to call \c insert,instead of
-    /// \c find(), in case the name isn't exist and needs to insert again
+    ///  - SUCCESS The node was added.
+    ///  - ALREADYEXISTS There was already a node of that name, so it was not
+    ///     added.
     Result insert(const isc::dns::Name& name, RBNode<T>** inserted_node);
-    //@}
 
     /// \brief Swaps two tree's contents.
     ///
@@ -421,6 +484,7 @@ public:
         std::swap(NULLNODE, other.NULLNODE);
         std::swap(node_count_, other.node_count_);
     }
+    //@}
 
 private:
     /// \name RBTree balance functions
@@ -435,14 +499,15 @@ private:
     /// \brief delete tree whose root is equal to node
     void deleteHelper(RBNode<T> *node);
     /// \brief find the node with name
-    /// \param name is the target, up will points to the base domain of
-    /// the tree which name resides, node will point to the target node
-    /// if we has exact same name or partical name in current tree.
-    /// so for example, in zone a, we has
-    /// b.a, c.b.a and d.b.a search c.b.a, up will points to b.a.
-    /// and node will points to c.b.a
-    /// \note parameter up now is not used by any funciton, but we are gonna
-    /// need it soon to implement function like remove
+    ///
+    /// Internal searching function.
+    ///
+    /// \param name What should be found.
+    /// \param up It will point to the node whose down pointer points
+    ///     to the tree containing node. If we looked for o.w.y.d.e.f in the
+    ///     \ref diagram, the up would point to the w.y node.
+    ///     This parameter is not used currently, but it will be soon.
+    /// \param node The found node.
     template <typename CBARG>
     Result findHelper(const isc::dns::Name& name, const RBNode<T>** up,
                       RBNode<T>** node,
@@ -450,9 +515,7 @@ private:
                       CBARG callback_arg) const;
     void dumpTreeHelper(std::ostream& os, const RBNode<T>* node,
                         unsigned int depth) const;
-    /// for indent purpose, add certian mount empty charachter to output stream
-    /// according to the depth. This is a helper function which is only used when
-    /// dump tree
+    /// \brief Indentation helper function for dumpTree
     static void indent(std::ostream& os, unsigned int depth);
 
     /// Split one node into two nodes, keep the old node and create one new
@@ -611,7 +674,7 @@ RBTree<T>::insert(const isc::dns::Name& target_name, RBNode<T>** new_node) {
             if (new_node != NULL) {
                 *new_node = current;
             }
-            return (ALREADYEXIST);
+            return (ALREADYEXISTS);
         } else {
             const int common_label_count = compare_result.getCommonLabels();
             if (common_label_count == 1) {
@@ -664,7 +727,7 @@ RBTree<T>::insert(const isc::dns::Name& target_name, RBNode<T>** new_node) {
 
     ++node_count_;
     node.release();
-    return (SUCCEED);
+    return (SUCCESS);
 }
 
 template <typename T>
@@ -675,10 +738,13 @@ RBTree<T>::nodeFission(RBNode<T>& node, const isc::dns::Name& base_name) {
     // using auto_ptr here is to avoid memory leak in case of exception raised
     // after the RBNode creation
     std::auto_ptr<RBNode<T> > down_node(new RBNode<T>(sub_name));
+    node.name_ = base_name;
+    // the rest of this function should be exception free so that it keeps
+    // consistent behavior (i.e., a weak form of strong exception guarantee)
+    // even if code after the call to this function throws an exception.
     std::swap(node.data_, down_node->data_);
     std::swap(node.callback_required_, down_node->callback_required_);
     down_node->down_ = node.down_;
-    node.name_ = base_name;
     node.down_ = down_node.get();
     // root node of sub tree, the initial color is BLACK
     down_node->color_ = RBNode<T>::BLACK;

src/lib/datasrc/tests/memory_datasrc_unittest.cc

@@ -405,4 +405,58 @@ TEST_F(MemoryZoneTest, load) {
         MasterLoadError);
 }
 
+TEST_F(MemoryZoneTest, swap) {
+    // build one zone with some data
+    MemoryZone zone1(class_, origin_);
+    EXPECT_EQ(result::SUCCESS, zone1.add(rr_ns_));
+    EXPECT_EQ(result::SUCCESS, zone1.add(rr_ns_aaaa_));
+
+    // build another zone of a different RR class with some other data
+    const Name other_origin("version.bind");
+    ASSERT_NE(origin_, other_origin); // make sure these two are different
+    MemoryZone zone2(RRClass::CH(), other_origin);
+    EXPECT_EQ(result::SUCCESS,
+              zone2.add(RRsetPtr(new RRset(Name("version.bind"),
+                                           RRClass::CH(), RRType::TXT(),
+                                           RRTTL(0)))));
+
+    zone1.swap(zone2);
+    EXPECT_EQ(other_origin, zone1.getOrigin());
+    EXPECT_EQ(origin_, zone2.getOrigin());
+    EXPECT_EQ(RRClass::CH(), zone1.getClass());
+    EXPECT_EQ(RRClass::IN(), zone2.getClass());
+    // make sure the zone data is swapped, too
+    findTest(origin_, RRType::NS(), Zone::NXDOMAIN, false, ConstRRsetPtr(),
+             &zone1);
+    findTest(other_origin, RRType::TXT(), Zone::SUCCESS, false,
+             ConstRRsetPtr(), &zone1);
+    findTest(origin_, RRType::NS(), Zone::SUCCESS, false, ConstRRsetPtr(),
+             &zone2);
+    findTest(other_origin, RRType::TXT(), Zone::NXDOMAIN, false,
+             ConstRRsetPtr(), &zone2);
+}
+
+TEST_F(MemoryZoneTest, getFileName) {
+    // for an empty zone the file name should also be empty.
+    EXPECT_TRUE(zone_.getFileName().empty());
+
+    // if loading a zone fails the file name shouldn't be set.
+    EXPECT_THROW(zone_.load(TEST_DATA_DIR "/root.zone"), MasterLoadError);
+    EXPECT_TRUE(zone_.getFileName().empty());
+
+    // after a successful load, the specified file name should be set
+    MemoryZone rootzone(class_, Name("."));
+    EXPECT_NO_THROW(rootzone.load(TEST_DATA_DIR "/root.zone"));
+    EXPECT_EQ(TEST_DATA_DIR "/root.zone", rootzone.getFileName());
+    // overriding load, which will fail
+    EXPECT_THROW(rootzone.load(TEST_DATA_DIR "/duplicate_rrset.zone"),
+                 MasterLoadError);
+    // the file name should be intact.
+    EXPECT_EQ(TEST_DATA_DIR "/root.zone", rootzone.getFileName());
+
+    // After swap, file names should also be swapped.
+    zone_.swap(rootzone);
+    EXPECT_EQ(TEST_DATA_DIR "/root.zone", zone_.getFileName());
+    EXPECT_TRUE(rootzone.getFileName().empty());
+}
 }

src/lib/datasrc/tests/rbtree_unittest.cc

@@ -90,62 +90,62 @@ TEST_F(RBTreeTest, setGetData) {
 }
 
 TEST_F(RBTreeTest, insertNames) {
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("d.e.f"), &rbtnode));
     EXPECT_EQ(Name("d.e.f"), rbtnode->getName());
     EXPECT_EQ(13, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("."), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("."), &rbtnode));
     EXPECT_EQ(Name("."), rbtnode->getName());
     EXPECT_EQ(14, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("example.com"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("example.com"), &rbtnode));
     EXPECT_EQ(15, rbtree.getNodeCount());
     rbtnode->setData(RBNode<int>::NodeDataPtr(new int(12)));
 
-    // return ALREADYEXIST, since node "example.com" already has been explicitly inserted
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("example.com"), &rbtnode));
+    // return ALREADYEXISTS, since node "example.com" already has been explicitly inserted
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("example.com"), &rbtnode));
     EXPECT_EQ(15, rbtree.getNodeCount());
 
     // split the node "d.e.f"
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("k.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("k.e.f"), &rbtnode));
     EXPECT_EQ(Name("k"), rbtnode->getName());
     EXPECT_EQ(17, rbtree.getNodeCount());
 
     // split the node "g.h"
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("h"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("h"), &rbtnode));
     EXPECT_EQ(Name("h"), rbtnode->getName());
     EXPECT_EQ(18, rbtree.getNodeCount());
 
     // add child domain
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("m.p.w.y.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("m.p.w.y.d.e.f"), &rbtnode));
     EXPECT_EQ(Name("m"), rbtnode->getName());
     EXPECT_EQ(19, rbtree.getNodeCount());
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("n.p.w.y.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("n.p.w.y.d.e.f"), &rbtnode));
     EXPECT_EQ(Name("n"), rbtnode->getName());
     EXPECT_EQ(20, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("l.a"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("l.a"), &rbtnode));
     EXPECT_EQ(Name("l"), rbtnode->getName());
     EXPECT_EQ(21, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("r.d.e.f"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("s.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("r.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("s.d.e.f"), &rbtnode));
     EXPECT_EQ(23, rbtree.getNodeCount());
 
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("h.w.y.d.e.f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("h.w.y.d.e.f"), &rbtnode));
 
     // add more nodes one by one to cover leftRotate and rightRotate
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("f"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("m"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("nm"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("om"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("k"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("l"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("fe"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("ge"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("i"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("ae"), &rbtnode));
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("n"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("f"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("m"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("nm"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("om"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("k"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("l"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("fe"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("ge"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("i"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("ae"), &rbtnode));
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("n"), &rbtnode));
 }
 
 TEST_F(RBTreeTest, findName) {
@@ -177,7 +177,7 @@ testCallback(const RBNode<int>&, bool* callack_checker) {
 
 TEST_F(RBTreeTest, callback) {
     // by default callback isn't enabled
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("callback.example"),
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("callback.example"),
                                                   &rbtnode));
     rbtnode->setData(RBNode<int>::NodeDataPtr(new int(1)));
     EXPECT_FALSE(rbtnode->isCallbackEnabled());
@@ -192,11 +192,11 @@ TEST_F(RBTreeTest, callback) {
     rbtnode->enableCallback();
     // add more levels below and above the callback node for partial match.
     RBNode<int>* subrbtnode;
-    EXPECT_EQ(RBTree<int>::SUCCEED, rbtree.insert(Name("sub.callback.example"),
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("sub.callback.example"),
                                                   &subrbtnode));
     subrbtnode->setData(RBNode<int>::NodeDataPtr(new int(2)));
     RBNode<int>* parentrbtnode;
-    EXPECT_EQ(RBTree<int>::ALREADYEXIST, rbtree.insert(Name("example"),
+    EXPECT_EQ(RBTree<int>::ALREADYEXISTS, rbtree.insert(Name("example"),
                                                        &parentrbtnode));
     //  the chilld/parent nodes shouldn't "inherit" the callback flag.
     // "rbtnode" may be invalid due to the insertion, so we need to re-find

src/lib/datasrc/zonetable.cc

@@ -51,8 +51,8 @@ struct ZoneTable::ZoneTableImpl {
         ZoneNode* node(NULL);
         switch (zones_.insert(zone->getOrigin(), &node)) {
             // This is OK
-            case ZoneTree::SUCCEED:
-            case ZoneTree::ALREADYEXIST:
+            case ZoneTree::SUCCESS:
+            case ZoneTree::ALREADYEXISTS:
                 break;
             // Can Not Happen
             default:
@@ -85,12 +85,12 @@ struct ZoneTable::ZoneTableImpl {
                 break;
             // We have no data there, so translate the pointer to NULL as well
             case ZoneTree::NOTFOUND:
-                return (FindResult(result::NOTFOUND, ConstZonePtr()));
+                return (FindResult(result::NOTFOUND, ZonePtr()));
             // Can Not Happen
             default:
                 assert(0);
                 // Because of warning
-                return (FindResult(result::NOTFOUND, ConstZonePtr()));
+                return (FindResult(result::NOTFOUND, ZonePtr()));
         }
 
         // Can Not Happen (remember, NOTFOUND is handled)

src/lib/datasrc/zonetable.h

@@ -41,11 +41,11 @@ namespace datasrc {
 class ZoneTable {
 public:
     struct FindResult {
-        FindResult(result::Result param_code, const ConstZonePtr param_zone) :
+        FindResult(result::Result param_code, const ZonePtr param_zone) :
             code(param_code), zone(param_zone)
         {}
         const result::Result code;
-        const ConstZonePtr zone;
+        const ZonePtr zone;
     };
     ///
     /// \name Constructors and Destructor.

src/lib/python/isc/config/ccsession.py

@@ -285,7 +285,7 @@ class ModuleCCSession(ConfigData):
         if answer:
             rcode, value = parse_answer(answer)
             if rcode == 0:
-                if value != None and self.get_module_spec().validate_config(False, value):
+                if value != None and module_spec.validate_config(False, value):
                     module_cfg.set_local_config(value);
 
         # all done, add it

src/lib/python/isc/config/tests/ccsession_test.py

@@ -530,7 +530,19 @@ class TestModuleCCSession(unittest.TestCase):
         self.assertTrue("Spec2" in fake_session.subscriptions)
         mccs = None
         self.assertFalse("Spec2" in fake_session.subscriptions)
-        
+
+    def test_remote_module_with_custom_config(self):
+        fake_session = FakeModuleCCSession()
+        mccs = self.create_session("spec1.spec", None, None, fake_session)
+        # override the default config value for "item1".  add_remote_config()
+        # should incorporate the overridden value, and we should be abel to
+        # get it via get_remote_config_value().
+        fake_session.group_sendmsg({'result': [0, {"item1": 10}]}, 'Spec2')
+        rmodname = mccs.add_remote_config(self.spec_file("spec2.spec"))
+        value, default = mccs.get_remote_config_value(rmodname, "item1")
+        self.assertEqual(10, value)
+        self.assertEqual(False, default)
+
     def test_ignore_command_remote_module(self):
         # Create a Spec1 module and subscribe to remote config for Spec2
         fake_session = FakeModuleCCSession()

src/lib/testutils/testdata/Makefile.am

@@ -1,4 +1,4 @@
-CLEANFILES = *.wire
+CLEANFILES = *.wire *.copied
 
 BUILT_SOURCES = badExampleQuery_fromWire.wire examplequery_fromWire.wire
 BUILT_SOURCES += iqueryresponse_fromWire.wire multiquestion_fromWire.wire
@@ -25,5 +25,11 @@ EXTRA_DIST += example.com.zone example.net.zone example.org.zone example.zone
 EXTRA_DIST += example.com
 EXTRA_DIST += example.sqlite3
 
+EXTRA_DIST += test1.zone.in
+EXTRA_DIST += test1-new.zone.in
+EXTRA_DIST += test1-broken.zone.in
+EXTRA_DIST += test2.zone.in
+EXTRA_DIST += test2-new.zone.in
+
 .spec.wire:
 	$(abs_top_builddir)/src/lib/dns/tests/testdata/gen-wiredata.py -o $@ $<

src/lib/testutils/testdata/test1-broken.zone.in

@@ -0,0 +1,5 @@
+test1.example.    3600    IN  SOA ns.test1.example. admin.test1.example. 1241 3600 1800 2419200 7200
+test1.example.    3600    IN  NS ns.test1.example.
+ns.test1.example.	3600    IN  A 192.0.2.1
+;; out-of-zone RR
+www.example.com.	3600    IN  AAAA 2001:db8::80

src/lib/testutils/testdata/test1-new.zone.in

@@ -0,0 +1,4 @@
+test1.example.    3600    IN  SOA ns.test1.example. admin.test1.example. 1238 3600 1800 2419200 7200
+test1.example.    3600    IN  NS ns.test1.example.
+ns.test1.example.	3600    IN  A 192.0.2.1
+ns.test1.example.	3600    IN  AAAA 2001:db8::1

src/lib/testutils/testdata/test1.zone.in

@@ -0,0 +1,3 @@
+test1.example.    3600    IN  SOA ns.test1.example. admin.test1.example. 1235 3600 1800 2419200 7200
+test1.example.    3600    IN  NS ns.test1.example.
+ns.test1.example.	3600    IN  A 192.0.2.1

src/lib/testutils/testdata/test2-new.zone.in

@@ -0,0 +1,4 @@
+test2.example.    3600    IN  SOA ns.test2.example. admin.test2.example. 1242 3600 1800 2419200 7200
+test2.example.    3600    IN  NS ns.test2.example.
+ns.test2.example.	3600    IN  A 192.0.2.2
+ns.test2.example.	3600    IN  AAAA 2001:db8::2

src/lib/testutils/testdata/test2.zone.in

@@ -0,0 +1,3 @@
+test2.example.    3600    IN  SOA ns.test2.example. admin.test2.example. 1238 3600 1800 2419200 7200
+test2.example.    3600    IN  NS ns.test2.example.
+ns.test2.example.	3600    IN  A 192.0.2.2