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

Conflicts:
	ChangeLog

ChangeLog

-236.	[bug]		naokikambe
+237.	[bug]		naokikambe
 	Resolved that the stats module wasn't configurable in bindctl in
 	spite of its having configuration items. The configuration part
 	was removed from the original spec file "stats.spec" and was
@@ -12,6 +12,12 @@
 	and commands of stats module, if it requires.
 	(Trac#719, git xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx)
 
+236.	[func]		jelte
+	C++ client side of configuration now uses BIND10 logging system.
+	It also has improved error handling when communicating with the
+	rest of the system.
+	(Trac #743, git 86632c12308c3ed099d75eb828f740c526dd7ec0)
+
 235.	[func]		jinmei
 	libdns++: added support for TSIG signing and verification.  It can
 	be done using a newly introduced TSIGContext class.

src/lib/Makefile.am

-SUBDIRS = exceptions util cryptolink dns cc config python xfr bench \
-          log asiolink asiodns nsas cache resolve testutils datasrc \
+SUBDIRS = exceptions util log cryptolink dns cc config python xfr \
+          bench asiolink asiodns nsas cache resolve testutils datasrc \
           server_common

src/lib/config/Makefile.am

@@ -2,9 +2,24 @@ SUBDIRS = . tests
 
 AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
 AM_CPPFLAGS += -I$(top_builddir)/src/lib/cc
+AM_CPPFLAGS += -I$(top_srcdir)/src/lib/log -I$(top_builddir)/src/lib/log
 AM_CPPFLAGS += $(BOOST_INCLUDES)
 
+# Define rule to build logging source files from message file
+configdef.h configdef.cc: configdef.mes
+	$(top_builddir)/src/lib/log/compiler/message $(top_srcdir)/src/lib/config/configdef.mes
+
+BUILT_SOURCES = configdef.h configdef.cc
+
 lib_LTLIBRARIES = libcfgclient.la
-libcfgclient_la_SOURCES = config_data.h config_data.cc module_spec.h module_spec.cc ccsession.cc ccsession.h
+libcfgclient_la_SOURCES = config_data.h config_data.cc
+libcfgclient_la_SOURCES += module_spec.h module_spec.cc
+libcfgclient_la_SOURCES += ccsession.cc ccsession.h
+libcfgclient_la_SOURCES += config_log.h config_log.cc
+
+nodist_libcfgclient_la_SOURCES  = configdef.h configdef.cc
+
+# The message file should be in the distribution.
+EXTRA_DIST = configdef.mes
 
-CLEANFILES = *.gcno *.gcda
+CLEANFILES = *.gcno *.gcda configdef.h configdef.cc

src/lib/config/ccsession.cc

@@ -12,12 +12,6 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-// 
-// todo: generalize this and make it into a specific API for all modules
-//       to use (i.e. connect to cc, send config and commands, get config,
-//               react on config change announcements)
-//
-
 #include <config.h>
 
 #include <stdexcept>
@@ -38,6 +32,7 @@
 #include <cc/session.h>
 #include <exceptions/exceptions.h>
 
+#include <config/config_log.h>
 #include <config/ccsession.h>
 
 using namespace std;
@@ -164,18 +159,18 @@ ModuleCCSession::readModuleSpecification(const std::string& filename) {
     // this file should be declared in a @something@ directive
     file.open(filename.c_str());
     if (!file) {
-        cout << "error opening " << filename << ": " << strerror(errno) << endl;
-        exit(1);
+        LOG_ERROR(config_logger, CONFIG_FOPEN_ERR).arg(filename).arg(strerror(errno));
+        isc_throw(CCSessionInitError, strerror(errno));
     }
 
     try {
         module_spec = moduleSpecFromFile(file, true);
     } catch (const JSONError& pe) {
-        cout << "Error parsing module specification file: " << pe.what() << endl;
-        exit(1);
+        LOG_ERROR(config_logger, CONFIG_JSON_PARSE).arg(filename).arg(pe.what());
+        isc_throw(CCSessionInitError, pe.what());
     } catch (const ModuleSpecError& dde) {
-        cout << "Error reading module specification file: " << dde.what() << endl;
-        exit(1);
+        LOG_ERROR(config_logger, CONFIG_MODULE_SPEC).arg(filename).arg(dde.what());
+        isc_throw(CCSessionInitError, dde.what());
     }
     file.close();
     return (module_spec);
@@ -223,7 +218,8 @@ ModuleCCSession::ModuleCCSession(
     int rcode;
     ConstElementPtr err = parseAnswer(rcode, answer);
     if (rcode != 0) {
-        std::cerr << "[" << module_name_ << "] Error in specification: " << answer << std::endl;
+        LOG_ERROR(config_logger, CONFIG_MANAGER_MOD_SPEC).arg(answer->str());
+        isc_throw(CCSessionInitError, answer->str());
     }
     
     setLocalConfig(Element::fromJSON("{}"));
@@ -236,7 +232,8 @@ ModuleCCSession::ModuleCCSession(
         if (rcode == 0) {
             handleConfigUpdate(new_config);
         } else {
-            std::cerr << "[" << module_name_ << "] Error getting config: " << new_config << std::endl;
+            LOG_ERROR(config_logger, CONFIG_MANAGER_CONFIG).arg(new_config->str());
+            isc_throw(CCSessionInitError, answer->str());
         }
     }
 
@@ -348,15 +345,13 @@ ModuleCCSession::checkCommand() {
                 answer = checkModuleCommand(cmd_str, target_module, arg);
             }
         } catch (const CCSessionError& re) {
-            // TODO: Once we have logging and timeouts, we should not
-            // answer here (potential interference)
-            answer = createAnswer(1, re.what());
+            LOG_ERROR(config_logger, CONFIG_CCSESSION_MSG).arg(re.what());
         }
         if (!isNull(answer)) {
             session_.reply(routing, answer);
         }
     }
-    
+
     return (0);
 }
 

src/lib/config/ccsession.h

@@ -134,6 +134,15 @@ public:
         isc::Exception(file, line, what) {}
 };
 
+///
+/// \brief This exception is thrown if the constructor fails
+///
+class CCSessionInitError : public isc::Exception {
+public:
+    CCSessionInitError(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) {}
+};
+
 ///
 /// \brief This module keeps a connection to the command channel,
 /// holds configuration information, and handles messages from
@@ -154,6 +163,11 @@ public:
      * AbstractSession without establishing the session.
      * Note: the design decision on who is responsible for establishing the
      * session is in flux, and may change in near future.
+     *
+     * \exception CCSessionInitError when the initialization fails,
+     *            either because the file cannot be read or there is
+     *            a communication problem with the config manager.
+     *
      * @param command_handler A callback function pointer to be called when
      * a control command from a remote agent needs to be performed on the
      * local module.

src/lib/config/config_log.cc

+// Copyright (C) 2011  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.
+
+/// Defines the logger used by the config lib
+
+#include "config/config_log.h"
+
+namespace isc {
+namespace config {
+
+isc::log::Logger config_logger("config");
+
+} // namespace nsas
+} // namespace isc
+

src/lib/config/config_log.h

+// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef __CONFIG_LOG__H
+#define __CONFIG_LOG__H
+
+#include <log/macros.h>
+#include "configdef.h"
+
+namespace isc {
+namespace config {
+
+/// \brief Config Logging
+///
+/// Defines logger object for config log messages
+
+/// \brief Config Logger
+///
+/// Define the logger used to log messages.  We could define it in multiple
+/// modules, but defining in a single module and linking to it saves time and
+/// space.
+extern isc::log::Logger config_logger;    // isc::config::config_logger is the CONFIG logger
+
+} // namespace config
+} // namespace isc
+
+#endif // __CONFIG_LOG__H

src/lib/config/configdef.mes

+# Copyright (C) 2011  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.
+
+$PREFIX CONFIG_
+$NAMESPACE isc::config
+
+% FOPEN_ERR     error opening %1: %2
+There was an error opening the given file.
+
+% JSON_PARSE    JSON parse error in %1: %2
+There was a parse error in the JSON file. The given file does not appear
+to be in valid JSON format. Please verify that the filename is correct
+and that the contents are valid JSON.
+
+% MODULE_SPEC   module specification error in %1: %2
+The given file does not appear to be a valid specification file. Please
+verify that the filename is correct and that its contents are a valid
+BIND10 module specification.
+
+% MANAGER_MOD_SPEC    module specification not accepted by cfgmgr: %1
+The module specification file for this module was rejected by the
+configuration manager. The full error message answer from the
+configuration manager is appended to the log error. The most likely
+cause is that the module is of a different (specification file) version
+than the running configuration manager.
+
+% MANAGER_CONFIG    error getting configuration from cfgmgr: %1
+The configuration manager returned an error when this module requested
+the configuration. The full error message answer from the configuration
+manager is appended to the log error. The most likely cause is that
+the module is of a different (command specification) version than the
+running configuration manager.
+
+% CCSESSION_MSG error in CC session message: %1
+There was a problem with an incoming message on the command and control
+channel. The message does not appear to be a valid command, and is
+missing a required element or contains an unknown data format. This
+most likely means that another BIND10 module is sending a bad message.
+The message itself is ignored by this module.

src/lib/config/tests/Makefile.am

@@ -24,6 +24,7 @@ run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
 run_unittests_LDADD =  $(GTEST_LDADD)
 run_unittests_LDADD += $(top_builddir)/src/lib/cc/libcc.la
 run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la
+run_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la
 run_unittests_LDADD += libfake_session.la
 run_unittests_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la
 

src/lib/config/tests/ccsession_unittests.cc

@@ -264,10 +264,7 @@ TEST_F(CCSessionTest, checkCommand) {
 
     session.addMessage(el("{ \"command\": \"bad_command\" }"), "Spec29", "*");
     result = mccs.checkCommand();
-    EXPECT_EQ(1, session.getMsgQueue()->size());
-    msg = session.getFirstMessage(group, to);
-    EXPECT_EQ("{ \"result\": [ 1, \"Command part in command message missing, empty, or not a list\" ] }", msg->str());
-    EXPECT_EQ(0, result);
+    EXPECT_EQ(0, session.getMsgQueue()->size());
 
     session.addMessage(el("{ \"command\": [ \"bad_command\" ] }"),
                        "Spec29", "*");
@@ -432,4 +429,24 @@ TEST_F(CCSessionTest, ignoreRemoteConfigCommands) {
     EXPECT_EQ(0, session.getMsgQueue()->size());
 }
 
+TEST_F(CCSessionTest, initializationFail) {
+    // bad specification
+    EXPECT_THROW(ModuleCCSession(ccspecfile("spec8.spec"), session,
+                                 NULL, NULL), CCSessionInitError);
+
+    // file that does not exist
+    EXPECT_THROW(ModuleCCSession(ccspecfile("does_not_exist_spec"),
+                                 session, NULL, NULL),
+                                 CCSessionInitError);
+
+
+    session.getMessages()->add(createAnswer(1, el("\"just an error\"")));
+
+    EXPECT_FALSE(session.haveSubscription("Spec29", "*"));
+    EXPECT_THROW(ModuleCCSession(ccspecfile("spec29.spec"), session,
+                                 my_config_handler, my_command_handler),
+                                 CCSessionInitError);
+    EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
+}
+
 }

src/lib/config/tests/run_unittests.cc

@@ -13,9 +13,16 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <gtest/gtest.h>
+#include <log/logger_support.h>
 
 int
 main(int argc, char* argv[]) {
     ::testing::InitGoogleTest(&argc, argv);
+
+    // TODO: UNCOMMENT ON MERGE
+    // (this is the call we want in master, but branch point does not
+    // have this yet)
+    //isc::log::initLogger();
+
     return (RUN_ALL_TESTS());
 }

src/lib/dns/tests/tsig_unittest.cc

@@ -871,7 +871,7 @@ TEST_F(TSIGTest, verifyAfterSendResponse) {
                  TSIGContextError);
 }
 
-TEST_F(TSIGTest, signAterVerified) {
+TEST_F(TSIGTest, signAfterVerified) {
     // Likewise, once the context verifies a response, it shouldn't for
     // signing any more.