[trac736] add some tests and refactor a bit

src/bin/resolver/main.cc

@@ -209,7 +209,7 @@ main(int argc, char* argv[]) {
         config_session = new ModuleCCSession(specfile, *cc_session,
                                              my_config_handler,
                                              my_command_handler,
-                                             true);
+                                             true, true);
         LOG_DEBUG(resolver_logger, RESOLVER_DBG_INIT, RESOLVER_CONFIGCHAN);
 
         // FIXME: This does not belong here, but inside Boss

src/lib/config/ccsession.cc

@@ -156,24 +156,96 @@ parseCommand(ConstElementPtr& arg, ConstElementPtr command) {
 }
 
 namespace {
-    // Temporary workaround function for missing functionality in
-    // getValue() (main problem described in ticket #993)
-    // This returns either the value set for the given relative id,
-    // or its default value
-    // (intentially defined here so this interface does not get
-    // included in ConfigData as it is)
-    ConstElementPtr getValueOrDefault(ConstElementPtr config_part,
-                                      const std::string& relative_id,
-                                      const ConfigData& config_data,
-                                      const std::string& full_id) {
-        if (config_part->contains(relative_id)) {
-            return config_part->get(relative_id);
-        } else {
-            return config_data.getDefaultValue(full_id);
+// Temporary workaround functions for missing functionality in
+// getValue() (main problem described in ticket #993)
+// This returns either the value set for the given relative id,
+// or its default value
+// (intentially defined here so this interface does not get
+// included in ConfigData as it is)
+ConstElementPtr getValueOrDefault(ConstElementPtr config_part,
+                                  const std::string& relative_id,
+                                  const ConfigData& config_data,
+                                  const std::string& full_id) {
+    if (config_part->contains(relative_id)) {
+        return config_part->get(relative_id);
+    } else {
+        return config_data.getDefaultValue(full_id);
+    }
+}
+
+// Reads a output_option subelement of a logger configuration,
+// and sets the values thereing to the given OutputOption struct,
+// or defaults values if they are not provided (from config_data).
+void
+readOutputOptionConf(isc::log::OutputOption& output_option,
+                     ConstElementPtr output_option_el,
+                     const ConfigData& config_data)
+{
+    ConstElementPtr destination_el = getValueOrDefault(output_option_el,
+                                    "destination", config_data,
+                                    "loggers/output_options/destination");
+    output_option.destination = isc::log::getDestination(destination_el->stringValue());
+    ConstElementPtr stream_el = getValueOrDefault(output_option_el,
+                                    "stream", config_data,
+                                    "loggers/output_options/stream");
+    output_option.stream = isc::log::getStream(stream_el->stringValue());
+    output_option.flush = getValueOrDefault(output_option_el,
+                              "flush", config_data,
+                              "loggers/output_options/flush")->boolValue();
+    output_option.facility = getValueOrDefault(output_option_el,
+                                 "facility", config_data,
+                                 "loggers/output_options/facility")->stringValue();
+    output_option.filename = getValueOrDefault(output_option_el,
+                                 "filename", config_data,
+                                 "loggers/output_options/filename")->stringValue();
+    output_option.maxsize = getValueOrDefault(output_option_el,
+                                "maxsize", config_data,
+                                "loggers/output_options/maxsize")->intValue();
+    output_option.maxver = getValueOrDefault(output_option_el,
+                               "maxver", config_data,
+                               "loggers/output_options/maxver")->intValue();
+}
+
+// Reads a full 'loggers' configuration, and adds the loggers therein
+// to the given vector, fills in blanks with defaults from config_data
+void
+readLoggersConf(std::vector<isc::log::LoggerSpecification>& specs,
+                ConstElementPtr logger,
+                const ConfigData& config_data)
+{
+    const std::string lname = logger->get("name")->stringValue();
+    ConstElementPtr severity_el = getValueOrDefault(logger,
+                                      "severity", config_data,
+                                      "loggers/severity");
+    isc::log::Severity severity = isc::log::getSeverity(
+                                      severity_el->stringValue());
+    int dbg_level = getValueOrDefault(logger, "debuglevel",
+                                      config_data,
+                                      "loggers/debuglevel")->intValue();
+    bool additive = getValueOrDefault(logger, "additive", config_data,
+                                      "loggers/additive")->boolValue();
+
+    isc::log::LoggerSpecification logger_spec(
+        lname, severity, dbg_level, additive
+    );
+
+    if (logger->contains("output_options")) {
+        BOOST_FOREACH(ConstElementPtr output_option_el,
+                      logger->get("output_options")->listValue()) {
+            // create outputoptions
+            isc::log::OutputOption output_option;
+            readOutputOptionConf(output_option,
+                                 output_option_el,
+                                 config_data);
+            logger_spec.addOutputOption(output_option);
         }
     }
+
+    specs.push_back(logger_spec);
 }
 
+} // end anonymous namespace
+
 void
 my_logconfig_handler(const std::string&n, ConstElementPtr new_config, const ConfigData& config_data) {
     config_data.getModuleSpec().validateConfig(new_config, true);
@@ -183,35 +255,7 @@ my_logconfig_handler(const std::string&n, ConstElementPtr new_config, const Conf
     if (new_config->contains("loggers")) {
         BOOST_FOREACH(ConstElementPtr logger,
                       new_config->get("loggers")->listValue()) {
-            // create LoggerOptions
-            const std::string lname = logger->get("name")->stringValue();
-            ConstElementPtr severity_el = getValueOrDefault(logger, "severity", config_data, "loggers/severity");
-            isc::log::Severity severity = isc::log::getSeverity(severity_el->stringValue());
-            int dbg_level = getValueOrDefault(logger, "debuglevel", config_data, "loggers/debuglevel")->intValue();
-            bool additive = getValueOrDefault(logger, "additive", config_data, "loggers/additive")->boolValue();
-            isc::log::LoggerSpecification logger_spec(
-                lname, severity, dbg_level, additive
-            );
-            if (logger->contains("output_options")) {
-                BOOST_FOREACH(ConstElementPtr output_option_el,
-                              logger->get("output_options")->listValue()) {
-                    // create outputoptions
-                    isc::log::OutputOption output_option;
-                    ConstElementPtr destination_el = getValueOrDefault(output_option_el, "destination", config_data, "loggers/output_options/destination");
-                    output_option.destination = isc::log::getDestination(destination_el->stringValue());
-                    ConstElementPtr stream_el = getValueOrDefault(output_option_el, "stream", config_data, "loggers/output_options/stream");
-                    output_option.stream = isc::log::getStream(stream_el->stringValue());
-                    output_option.flush = getValueOrDefault(output_option_el, "flush", config_data, "loggers/output_options/flush")->boolValue();
-                    output_option.facility = getValueOrDefault(output_option_el, "facility", config_data, "loggers/output_options/facility")->stringValue();
-                    output_option.filename = getValueOrDefault(output_option_el, "filename", config_data, "loggers/output_options/filename")->stringValue();
-                    output_option.maxsize = getValueOrDefault(output_option_el, "maxsize", config_data, "loggers/output_options/maxsize")->intValue();
-                    output_option.maxver = getValueOrDefault(output_option_el, "maxver", config_data, "loggers/output_options/maxver")->intValue();
-
-                    logger_spec.addOutputOption(output_option);
-                }
-            }
-
-            specs.push_back(logger_spec);
+            readLoggersConf(specs, logger, config_data);
         }
     }
 
@@ -307,14 +351,15 @@ ModuleCCSession::ModuleCCSession(
         }
     }
 
-    if (start_immediately) {
-        start();
-    }
-
     // Keep track of logging settings automatically
     if (handle_logging) {
         addRemoteConfig("Logging", my_logconfig_handler, false);
     }
+
+    if (start_immediately) {
+        start();
+    }
+
 }
 
 void

src/lib/config/config_data.cc

@@ -99,11 +99,6 @@ find_spec_part(ConstElementPtr spec, const std::string& identifier) {
     while(sep != std::string::npos) {
         std::string part = id.substr(0, sep);
 
-        // As long as we are not in the 'final' element as specified
-        // by the identifier, we want to automatically traverse list
-        // and map specifications
-        spec_part = findListOrMapSubSpec(spec_part);
-
         if (spec_part->getType() == Element::list) {
             spec_part = findItemInSpecList(spec_part, part, identifier);
         } else {
@@ -112,6 +107,13 @@ find_spec_part(ConstElementPtr spec, const std::string& identifier) {
         }
         id = id.substr(sep + 1);
         sep = id.find("/");
+
+        // As long as we are not in the 'final' element as specified
+        // by the identifier, we want to automatically traverse list
+        // and map specifications
+        if (id != "" && id != "/") {
+            spec_part = findListOrMapSubSpec(spec_part);
+        }
     }
     if (id != "" && id != "/") {
         if (spec_part->getType() == Element::list) {
@@ -124,7 +126,7 @@ find_spec_part(ConstElementPtr spec, const std::string& identifier) {
             } else {
                 isc_throw(DataNotFoundError, "Element above " + id +
                                              " in " + identifier +
-                                             " is not a map");
+                                             " is not a map: " + spec_part->str());
             }
         }
     }

src/lib/config/tests/ccsession_unittests.cc

@@ -160,6 +160,10 @@ TEST_F(CCSessionTest, session1) {
     EXPECT_EQ("ConfigManager", group);
     EXPECT_EQ("*", to);
     EXPECT_EQ(0, session.getMsgQueue()->size());
+
+    // without explicit argument, the session should not automatically
+    // subscribe to logging config
+    EXPECT_FALSE(session.haveSubscription("Logging", "*"));
 }
 
 TEST_F(CCSessionTest, session2) {
@@ -597,6 +601,27 @@ TEST_F(CCSessionTest, delayedStart) {
                  FakeSession::DoubleRead);
 }
 
+TEST_F(CCSessionTest, loggingStart) {
+    // provide the logging module spec
+    ConstElementPtr log_spec = moduleSpecFromFile(LOG_SPEC_FILE).getFullSpec();
+    session.getMessages()->add(createAnswer(0, log_spec));
+    // just give an empty config
+    session.getMessages()->add(createAnswer(0, el("{}")));
+    ModuleCCSession mccs(ccspecfile("spec2.spec"), session, NULL, NULL,
+                         true, true);
+    EXPECT_TRUE(session.haveSubscription("Logging", "*"));
+}
+
+TEST_F(CCSessionTest, loggingStartBadSpec) {
+    // provide the logging module spec
+    session.getMessages()->add(createAnswer(0, el("{}")));
+    // just give an empty config
+    session.getMessages()->add(createAnswer(0, el("{}")));
+    EXPECT_THROW(new ModuleCCSession(ccspecfile("spec2.spec"), session,
+                 NULL, NULL, true, true), ModuleSpecError);
+    EXPECT_FALSE(session.haveSubscription("Logging", "*"));
+}
+
 // Similar to the above, but more implicitly by calling addRemoteConfig().
 // We should construct ModuleCCSession with start_immediately being false
 // if we need to call addRemoteConfig().

src/lib/config/tests/config_data_unittests.cc

@@ -64,15 +64,14 @@ TEST(ConfigData, getValue) {
     EXPECT_EQ("{  }", cd.getValue(is_default, "value6/")->str());
     EXPECT_TRUE(is_default);
     EXPECT_EQ("[  ]", cd.getValue("value8")->str());
+    EXPECT_EQ("empty", cd.getValue("value8/a")->stringValue());
 
     EXPECT_THROW(cd.getValue("")->str(), DataNotFoundError);
     EXPECT_THROW(cd.getValue("/")->str(), DataNotFoundError);
     EXPECT_THROW(cd.getValue("no_such_item")->str(), DataNotFoundError);
     EXPECT_THROW(cd.getValue("value6/a")->str(), DataNotFoundError);
     EXPECT_THROW(cd.getValue("value6/no_such_item")->str(), DataNotFoundError);
-    EXPECT_THROW(cd.getValue("value8/a")->str(), DataNotFoundError);
-    EXPECT_THROW(cd.getValue("value8/a")->str(), DataNotFoundError);
-    EXPECT_THROW(cd.getValue("value8/a")->str(), DataNotFoundError);
+    EXPECT_THROW(cd.getValue("value8/b")->str(), DataNotFoundError);
 
     ModuleSpec spec1 = moduleSpecFromFile(std::string(TEST_DATA_PATH) + "/spec1.spec");
     ConfigData cd1 = ConfigData(spec1);

src/lib/config/tests/data_def_unittests_config.h.in

@@ -13,3 +13,4 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #define TEST_DATA_PATH "@abs_srcdir@/testdata"
+#define LOG_SPEC_FILE "@abs_top_srcdir@/src/bin/cfgmgr/plugins/logging.spec"