Merge branch 'trac1004'

src/bin/cfgmgr/plugins/b10logging.py

@@ -48,6 +48,19 @@ def check(config):
         for logger in config['loggers']:
             # name should always be present
             name = logger['name']
+            # report an error if name starts with * but not *.,
+            # or if * is not the first character.
+            # TODO: we might want to also warn or error if the
+            # logger name is not an existing module, but we can't
+            # really tell that from here at this point
+            star_pos = name.find('*')
+            if star_pos > 0 or\
+               name == '*.' or\
+               (star_pos == 0 and len(name) > 1 and name[1] != '.'):
+                errors.append("Bad logger name: '" + name + "': * can "
+                              "only be used instead of the full "
+                              "first-level name, e.g. '*' or "
+                              "'*.subsystem'")
 
             if 'severity' in logger and\
                logger['severity'].lower() not in ALLOWED_SEVERITIES:
@@ -71,11 +84,11 @@ def check(config):
                                'output' in output_option and\
                                output_option['output'] not in ALLOWED_STREAMS:
                                 errors.append("bad output for logger " + name +
-                                              ": " + output_option['stream'] +
+                                              ": " + output_option['output'] +
                                               ", must be stdout or stderr")
                             elif destination == "file" and\
-                                 'output' not in output_option or\
-                                 output_option['output'] == "":
+                                 ('output' not in output_option or\
+                                  output_option['output'] == ""):
                                     errors.append("destination set to file but "
                                                   "output not set to any "
                                                   "filename for logger "

src/bin/cfgmgr/plugins/tests/Makefile.am

@@ -1,5 +1,5 @@
 PYCOVERAGE_RUN = @PYCOVERAGE_RUN@
-PYTESTS = tsig_keys_test.py
+PYTESTS = tsig_keys_test.py logging_test.py
 
 EXTRA_DIST = $(PYTESTS)
 

src/bin/cfgmgr/plugins/tests/logging_test.py

@@ -0,0 +1,135 @@
+# Copyright (C) 2011  Internet Systems Consortium.
+#
+# Permission to use, copy, modify, and 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 INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM 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.
+
+# Make sure we can load the module, put it into path
+import sys
+import os
+sys.path.extend(os.environ["B10_TEST_PLUGIN_DIR"].split(':'))
+
+import b10logging
+import unittest
+
+class LoggingConfCheckTest(unittest.TestCase):
+    def test_load(self):
+        """
+        Checks the entry point returns the correct values.
+        """
+        (spec, check) = b10logging.load()
+        # It returns the checking function
+        self.assertEqual(check, b10logging.check)
+        # The plugin stores it's spec
+        self.assertEqual(spec, b10logging.spec)
+
+    def test_logger_conf(self):
+        self.assertEqual(None,
+                         b10logging.check({'loggers':
+                                          [{'name': '*',
+                                            'severity': 'DEBUG',
+                                            'debuglevel': 50,
+                                            'output_options':
+                                            [{'destination': 'file',
+                                              'output': '/some/file'
+                                            }]
+                                           },
+                                           {'name': 'b10-resolver',
+                                            'severity': 'WARN',
+                                            'additive': True,
+                                            'output_options':
+                                            [{'destination': 'console',
+                                              'output': 'stderr',
+                                              'flush': True
+                                            }]
+                                           },
+                                           {'name': 'b10-resolver.resolver',
+                                            'severity': 'ERROR',
+                                            'output_options': []
+                                           },
+                                           {'name': '*.cache',
+                                            'severity': 'INFO'
+                                           }
+                                          ]}))
+    def do_bad_name_test(self, name):
+        err_str = "Bad logger name: '" + name + "': * can only be "\
+                  "used instead of the full first-level name, e.g. "\
+                  "'*' or '*.subsystem'"
+        self.assertEqual(err_str,
+                         b10logging.check({'loggers':
+                                          [{'name': name,
+                                            'severity': 'DEBUG'},
+                                          ]}))
+        
+    def test_logger_bad_name(self):
+        self.do_bad_name_test("*.")
+        self.do_bad_name_test("*foo")
+        self.do_bad_name_test("*foo.lib")
+        self.do_bad_name_test("foo*")
+        self.do_bad_name_test("foo*.lib")
+
+    def test_logger_bad_severity(self):
+        self.assertEqual('bad severity value for logger *: BADVAL',
+                         b10logging.check({'loggers':
+                                          [{'name': '*',
+                                            'severity': 'BADVAL'}]}))
+
+    def test_logger_bad_destination(self):
+        self.assertEqual('bad destination for logger *: baddest',
+                         b10logging.check({'loggers':
+                                          [{'name': '*',
+                                            'severity': 'INFO',
+                                            'output_options': [
+                                            { 'destination': 'baddest' }
+                                            ]}]}))
+
+    def test_logger_bad_console_output(self):
+        self.assertEqual('bad output for logger *: bad_output, must be stdout or stderr',
+                         b10logging.check({'loggers':
+                                          [{'name': '*',
+                                            'severity': 'INFO',
+                                            'output_options': [
+                                            { 'destination': 'console',
+                                              'output': 'bad_output'
+                                            }
+                                            ]}]}))
+
+    def test_logger_bad_file_output(self):
+        self.assertEqual('destination set to file but output not set to any filename for logger *',
+                         b10logging.check({'loggers':
+                                          [{'name': '*',
+                                            'severity': 'INFO',
+                                            'output_options': [
+                                            { 'destination': 'file' }
+                                            ]}]}))
+
+    def test_logger_bad_syslog_output(self):
+        self.assertEqual('destination set to syslog but output not set to any facility for logger *',
+                         b10logging.check({'loggers':
+                                          [{'name': '*',
+                                            'severity': 'INFO',
+                                            'output_options': [
+                                            { 'destination': 'syslog' }
+                                            ]}]}))
+
+    def test_logger_bad_type(self):
+        self.assertEqual('123 should be a string',
+                         b10logging.check({'loggers':
+                                          [{'name': 123,
+                                            'severity': 'INFO'}]}))
+        self.assertEqual('123 should be a string',
+                         b10logging.check({'loggers':
+                                          [{'name': 'bind10',
+                                            'severity': 123}]}))
+
+if __name__ == '__main__':
+        unittest.main()

src/lib/cc/data.h

@@ -479,7 +479,7 @@ public:
         return (true);
     }
     using Element::setValue;
-    bool setValue(std::map<std::string, ConstElementPtr>& v) {
+    bool setValue(const std::map<std::string, ConstElementPtr>& v) {
         m = v;
         return (true);
     }

src/lib/config/ccsession.cc

@@ -23,6 +23,7 @@
 #include <fstream>
 #include <sstream>
 #include <cerrno>
+#include <set>
 
 #include <boost/bind.hpp>
 #include <boost/foreach.hpp>
@@ -38,6 +39,7 @@
 #include <log/logger_support.h>
 #include <log/logger_specification.h>
 #include <log/logger_manager.h>
+#include <log/logger_name.h>
 
 using namespace std;
 
@@ -213,7 +215,8 @@ readLoggersConf(std::vector<isc::log::LoggerSpecification>& specs,
                 ConstElementPtr logger,
                 const ConfigData& config_data)
 {
-    const std::string lname = logger->get("name")->stringValue();
+    std::string lname = logger->get("name")->stringValue();
+
     ConstElementPtr severity_el = getValueOrDefault(logger,
                                       "severity", config_data,
                                       "loggers/severity");
@@ -246,6 +249,50 @@ readLoggersConf(std::vector<isc::log::LoggerSpecification>& specs,
 
 } // end anonymous namespace
 
+
+ConstElementPtr
+getRelatedLoggers(ConstElementPtr loggers) {
+    // Keep a list of names for easier lookup later
+    std::set<std::string> our_names;
+    const std::string& root_name = isc::log::getRootLoggerName();
+
+    ElementPtr result = isc::data::Element::createList();
+
+    BOOST_FOREACH(ConstElementPtr cur_logger, loggers->listValue()) {
+        const std::string cur_name = cur_logger->get("name")->stringValue();
+        if (cur_name == root_name || cur_name.find(root_name + ".") == 0) {
+            our_names.insert(cur_name);
+            result->add(cur_logger);
+        }
+    }
+
+    // now find the * names
+    BOOST_FOREACH(ConstElementPtr cur_logger, loggers->listValue()) {
+        std::string cur_name = cur_logger->get("name")->stringValue();
+        // if name is '*', or starts with '*.', replace * with root
+        // logger name
+        if (cur_name == "*" || cur_name.length() > 1 &&
+            cur_name[0] == '*' && cur_name[1] == '.') {
+
+            cur_name = root_name + cur_name.substr(1);
+            // now add it to the result list, but only if a logger with
+            // that name was not configured explicitely
+            if (our_names.find(cur_name) == our_names.end()) {
+                // we substitute the name here already, but as
+                // we are dealing with consts, we copy the data
+                ElementPtr new_logger(Element::createMap());
+                // since we'll only be updating one first-level element,
+                // and we return as const again, a shallow map copy is
+                // enough
+                new_logger->setValue(cur_logger->mapValue());
+                new_logger->set("name", Element::create(cur_name));
+                result->add(new_logger);
+            }
+        }
+    }
+    return result;
+}
+
 void
 default_logconfig_handler(const std::string& module_name,
                           ConstElementPtr new_config,
@@ -255,8 +302,9 @@ default_logconfig_handler(const std::string& module_name,
     std::vector<isc::log::LoggerSpecification> specs;
 
     if (new_config->contains("loggers")) {
+        ConstElementPtr loggers = getRelatedLoggers(new_config->get("loggers"));
         BOOST_FOREACH(ConstElementPtr logger,
-                      new_config->get("loggers")->listValue()) {
+                      loggers->listValue()) {
             readLoggersConf(specs, logger, config_data);
         }
     }

src/lib/config/ccsession.h

@@ -373,8 +373,41 @@ default_logconfig_handler(const std::string& module_name,
                           isc::data::ConstElementPtr new_config,
                           const ConfigData& config_data);
 
-}
-}
+
+/// \brief Returns the loggers related to this module
+///
+/// This function does two things;
+/// - it drops the configuration parts for loggers for other modules
+/// - it replaces the '*' in the name of the loggers by the name of
+///   this module, but *only* if the expanded name is not configured
+///   explicitely
+///
+/// Examples: if this is the module b10-resolver,
+/// For the config names ['*', 'b10-auth']
+/// The '*' is replaced with 'b10-resolver', and this logger is used.
+/// 'b10-auth' is ignored (of course, it will not be in the b10-auth
+/// module).
+///
+/// For ['*', 'b10-resolver']
+/// The '*' is ignored, and only 'b10-resolver' is used.
+///
+/// For ['*.reslib', 'b10-resolver']
+/// Or ['b10-resolver.reslib', '*']
+/// Both are used, where the * will be expanded to b10-resolver
+///
+/// \note This is a public function at this time, but mostly for
+/// the purposes of testing. Once we can directly test what loggers
+/// are running, this function may be moved to the unnamed namespace
+///
+/// \param loggers the original 'loggers' config list
+/// \return ListElement containing only loggers relevant for this
+///         module, where * is replaced by the root logger name
+isc::data::ConstElementPtr
+getRelatedLoggers(isc::data::ConstElementPtr loggers);
+
+} // namespace config
+
+} // namespace isc
 #endif // __CCSESSION_H
 
 // Local Variables:

src/lib/config/tests/ccsession_unittests.cc

@@ -24,6 +24,8 @@
 
 #include <config/tests/data_def_unittests_config.h>
 
+#include <log/logger_name.h>
+
 using namespace isc::data;
 using namespace isc::config;
 using namespace isc::cc;
@@ -632,4 +634,64 @@ TEST_F(CCSessionTest, doubleStartWithAddRemoteConfig) {
     EXPECT_THROW(mccs.addRemoteConfig(ccspecfile("spec2.spec")),
                  FakeSession::DoubleRead);
 }
+
+namespace {
+void doRelatedLoggersTest(const char* input, const char* expected) {
+    ConstElementPtr all_conf = isc::data::Element::fromJSON(input);
+    ConstElementPtr expected_conf = isc::data::Element::fromJSON(expected);
+    EXPECT_EQ(*expected_conf, *isc::config::getRelatedLoggers(all_conf));
+}
+} // end anonymous namespace
+
+TEST(LogConfigTest, relatedLoggersTest) {
+    // make sure logger configs for 'other' programs are ignored,
+    // and that * is substituted correctly
+    // The default root logger name is "bind10"
+    doRelatedLoggersTest("[{ \"name\": \"other_module\" }]",
+                         "[]");
+    doRelatedLoggersTest("[{ \"name\": \"other_module.somelib\" }]",
+                         "[]");
+    doRelatedLoggersTest("[{ \"name\": \"bind10_other\" }]",
+                         "[]");
+    doRelatedLoggersTest("[{ \"name\": \"bind10_other.somelib\" }]",
+                         "[]");
+    doRelatedLoggersTest("[ { \"name\": \"other_module\" },"
+                         "  { \"name\": \"bind10\" }]",
+                         "[ { \"name\": \"bind10\" } ]");
+    doRelatedLoggersTest("[ { \"name\": \"bind10\" }]",
+                         "[ { \"name\": \"bind10\" } ]");
+    doRelatedLoggersTest("[ { \"name\": \"bind10.somelib\" }]",
+                         "[ { \"name\": \"bind10.somelib\" } ]");
+    doRelatedLoggersTest("[ { \"name\": \"other_module.somelib\" },"
+                         "  { \"name\": \"bind10.somelib\" }]",
+                         "[ { \"name\": \"bind10.somelib\" } ]");
+    doRelatedLoggersTest("[ { \"name\": \"other_module.somelib\" },"
+                         "  { \"name\": \"bind10\" },"
+                         "  { \"name\": \"bind10.somelib\" }]",
+                         "[ { \"name\": \"bind10\" },"
+                         "  { \"name\": \"bind10.somelib\" } ]");
+    doRelatedLoggersTest("[ { \"name\": \"*\" }]",
+                         "[ { \"name\": \"bind10\" } ]");
+    doRelatedLoggersTest("[ { \"name\": \"*.somelib\" }]",
+                         "[ { \"name\": \"bind10.somelib\" } ]");
+    doRelatedLoggersTest("[ { \"name\": \"*\", \"severity\": \"DEBUG\" },"
+                         "  { \"name\": \"bind10\", \"severity\": \"WARN\"}]",
+                         "[ { \"name\": \"bind10\", \"severity\": \"WARN\"} ]");
+    doRelatedLoggersTest("[ { \"name\": \"*\", \"severity\": \"DEBUG\" },"
+                         "  { \"name\": \"some_module\", \"severity\": \"WARN\"}]",
+                         "[ { \"name\": \"bind10\", \"severity\": \"DEBUG\"} ]");
+
+    // make sure 'bad' things like '*foo.x' or '*lib' are ignored
+    // (cfgmgr should have already caught it in the logconfig plugin
+    // check, and is responsible for reporting the error)
+    doRelatedLoggersTest("[ { \"name\": \"*foo\" }]",
+                         "[ ]");
+    doRelatedLoggersTest("[ { \"name\": \"*foo.bar\" }]",
+                         "[ ]");
+    doRelatedLoggersTest("[ { \"name\": \"*foo\" },"
+                         "  { \"name\": \"*foo.lib\" },"
+                         "  { \"name\": \"bind10\" } ]",
+                         "[ { \"name\": \"bind10\" } ]");
+}
+
 }