[trac743] Replace direct output by liblog calls

While creating messages and extended descriptions, I noticed a few potential problems, which I also fixed;

- If the config manager does not accept the modules specification, or if the
  'get current config' command returns with an error, the system used to
  ignore that fact apart from printing an error, and continue running. It
  seems much better to explicitely fail, since the module would be running
  independent of what its config is supposed to be, which would be really
  unstable
- If the module cannot parse a command it gets over the command channel,
  it used to send back an error. If we have multiple modules listening on
  the same channel, this would result in multiple errors being returned
  It will now log an error, but it will not send back an answer

src/lib/Makefile.am

@@ -1,3 +1,3 @@
-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

@@ -9,10 +9,17 @@ AM_CPPFLAGS += $(BOOST_INCLUDES)
 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
 libcfgclient_la_SOURCES += module_spec.h module_spec.cc
 libcfgclient_la_SOURCES += ccsession.cc ccsession.h
-libcfgclient_la_SOURCES += configdef.h configdef.cc
+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

@@ -135,6 +135,15 @@ public:
 };
 
 ///
+/// \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
 /// the command channel
@@ -154,6 +163,10 @@ 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 given spec_file_name cannot
+     *            be read successfully
+     *
      * @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

@@ -0,0 +1,26 @@
+// 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

@@ -0,0 +1,53 @@
+// 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 the levels used to output debug messages in the CONFIG.  Note that
+/// higher numbers equate to more verbose (and detailed) output.
+
+// The first level traces normal operations - asking the CONFIG for an address,
+// and cancelling a lookup.  It also records when the CONFIG calls back to the
+// resolver to resolve something.
+//const int CONFIG_DBG_TRACE = 10;
+
+// The next level extends the normal operations and records the results of the
+// lookups.
+//const int CONFIG_DBG_RESULTS = 20;
+
+// Additional information on the usage of the names - the RTT values obtained
+// when queries were done.
+//const int CONFIG_DBG_RTT = 30;
+
+
+/// \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::logger is the CONFIG logger
+
+} // namespace config
+} // namespace isc
+
+#endif // __CONFIG_LOG__H

src/lib/config/configdef.mes

@@ -0,0 +1,50 @@
+# 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/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());
 }