BIND 10 trac1004, updated. 251a32a1fd1e7be23d59790e57a4b40fbcdceae3 [trac1004] add tests and additional fixes
BIND 10 source code commits
bind10-changes at lists.isc.org
Sun Jun 26 21:53:30 UTC 2011
The branch, trac1004 has been updated
via 251a32a1fd1e7be23d59790e57a4b40fbcdceae3 (commit)
from 935bd760ed4f39213f8db8eab730bf41dc217da9 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 251a32a1fd1e7be23d59790e57a4b40fbcdceae3
Author: Jelte Jansen <jelte at isc.org>
Date: Sun Jun 26 23:49:23 2011 +0200
[trac1004] add tests and additional fixes
also do a few fixes and address a couple of comments;
- made getRelatedLoggers public (so we can test it)
- use set instead of vector
- don't allow '*foo' etc.
- added tests
- added tests for the cfgmgr b10logging plugin
-----------------------------------------------------------------------
Summary of changes:
src/bin/cfgmgr/plugins/b10logging.py | 19 +++-
src/bin/cfgmgr/plugins/tests/Makefile.am | 2 +-
src/bin/cfgmgr/plugins/tests/logging_test.py | 135 ++++++++++++++++++++++++++
src/lib/config/ccsession.cc | 51 +++-------
src/lib/config/ccsession.h | 37 +++++++-
src/lib/config/tests/ccsession_unittests.cc | 54 ++++++++++
6 files changed, 258 insertions(+), 40 deletions(-)
create mode 100644 src/bin/cfgmgr/plugins/tests/logging_test.py
-----------------------------------------------------------------------
diff --git a/src/bin/cfgmgr/plugins/b10logging.py b/src/bin/cfgmgr/plugins/b10logging.py
index 6af3f66..e288c6d 100644
--- a/src/bin/cfgmgr/plugins/b10logging.py
+++ b/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 "
diff --git a/src/bin/cfgmgr/plugins/tests/Makefile.am b/src/bin/cfgmgr/plugins/tests/Makefile.am
index 725d391..07b7a85 100644
--- a/src/bin/cfgmgr/plugins/tests/Makefile.am
+++ b/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)
diff --git a/src/bin/cfgmgr/plugins/tests/logging_test.py b/src/bin/cfgmgr/plugins/tests/logging_test.py
new file mode 100644
index 0000000..818a596
--- /dev/null
+++ b/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()
diff --git a/src/lib/config/ccsession.cc b/src/lib/config/ccsession.cc
index 43f5c58..7eafd43 100644
--- a/src/lib/config/ccsession.cc
+++ b/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>
@@ -246,35 +247,13 @@ readLoggersConf(std::vector<isc::log::LoggerSpecification>& specs,
specs.push_back(logger_spec);
}
+} // end anonymous namespace
+
-// 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
-//
-// \param The original 'loggers' config list
-// \returns ListElement containing only loggers relevant for this
-// module, where * is replaced by the root logger name
ConstElementPtr
getRelatedLoggers(ConstElementPtr loggers) {
// Keep a list of names for easier lookup later
- std::vector<std::string> our_names;
+ std::set<std::string> our_names;
const std::string& root_name = isc::log::getRootLoggerName();
ElementPtr result = isc::data::Element::createList();
@@ -282,7 +261,7 @@ getRelatedLoggers(ConstElementPtr loggers) {
BOOST_FOREACH(ConstElementPtr cur_logger, loggers->listValue()) {
const std::string cur_name = cur_logger->get("name")->stringValue();
if (cur_name.find(root_name) == 0) {
- our_names.push_back(cur_name);
+ our_names.insert(cur_name);
result->add(cur_logger);
}
}
@@ -290,23 +269,27 @@ getRelatedLoggers(ConstElementPtr loggers) {
// now find the * names
BOOST_FOREACH(ConstElementPtr cur_logger, loggers->listValue()) {
std::string cur_name = cur_logger->get("name")->stringValue();
- // if name starts with *, replace * with root logger name
- if (cur_name.length() > 0 && cur_name[0] == '*') {
+ // if name is '*', or starts with '*.', replace * with root
+ // logger name
+ if (cur_name.length() > 0 && cur_name[0] == '*' &&
+ !(cur_name.length() > 1 && 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 (std::find(our_names.begin(), our_names.end(),
- cur_name) != our_names.end()) {
- result->add(cur_logger);
+ 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
+ // there's no direct copy (yet), but we can use JSON
+ ElementPtr new_logger = isc::data::Element::fromJSON(cur_logger->str());
+ ConstElementPtr new_name = Element::create(cur_name);
+ new_logger->set("name", new_name);
+ result->add(new_logger);
}
}
}
return result;
}
-} // end anonymous namespace
-
void
default_logconfig_handler(const std::string& module_name,
ConstElementPtr new_config,
diff --git a/src/lib/config/ccsession.h b/src/lib/config/ccsession.h
index 0d4b7f3..5c6975a 100644
--- a/src/lib/config/ccsession.h
+++ b/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
+/// \returns 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:
diff --git a/src/lib/config/tests/ccsession_unittests.cc b/src/lib/config/tests/ccsession_unittests.cc
index e5fe049..1dfd9fb 100644
--- a/src/lib/config/tests/ccsession_unittests.cc
+++ b/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,56 @@ 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\" },"
+ " { \"name\": \"bind10\" }]",
+ "[ { \"name\": \"bind10\" } ]");
+ doRelatedLoggersTest("[ { \"name\": \"bind10\" }]",
+ "[ { \"name\": \"bind10\" } ]");
+ 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\" } ]");
+}
+
}
More information about the bind10-changes
mailing list