BIND 10 trac2213, updated. d3bdb3c1c1dca514322fefdaac18a253364dfd2f [2213] address review comments
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Oct 30 13:36:47 UTC 2012
The branch, trac2213 has been updated
via d3bdb3c1c1dca514322fefdaac18a253364dfd2f (commit)
from 105208c1e8cd0a77cb8960013a391cc7b48f7a22 (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 d3bdb3c1c1dca514322fefdaac18a253364dfd2f
Author: Jelte Jansen <jelte at isc.org>
Date: Tue Oct 30 12:27:10 2012 +0100
[2213] address review comments
- add comments about hardcoded class default
- removed AuthSrv::loadZone() again
- more data integrity checks in datasrc clients mgr loadZone handler
-----------------------------------------------------------------------
Summary of changes:
src/bin/auth/auth_srv.cc | 5 --
src/bin/auth/auth_srv.h | 14 -----
src/bin/auth/command.cc | 2 +-
src/bin/auth/datasrc_clients_mgr.h | 61 +++++++++++++++-----
src/bin/auth/tests/datasrc_clients_mgr_unittest.cc | 20 +++++--
5 files changed, 63 insertions(+), 39 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc
index 73c2ae4..157ae03 100644
--- a/src/bin/auth/auth_srv.cc
+++ b/src/bin/auth/auth_srv.cc
@@ -922,8 +922,3 @@ void
AuthSrv::setTCPRecvTimeout(size_t timeout) {
dnss_->setTCPRecvTimeout(timeout);
}
-
-void
-AuthSrv::loadZone(ConstElementPtr args) {
- impl_->datasrc_clients_mgr_.loadZone(args);
-}
diff --git a/src/bin/auth/auth_srv.h b/src/bin/auth/auth_srv.h
index 5e0c17f..2d53666 100644
--- a/src/bin/auth/auth_srv.h
+++ b/src/bin/auth/auth_srv.h
@@ -320,20 +320,6 @@ public:
/// open forever.
void setTCPRecvTimeout(size_t timeout);
- /// \brief Reloads a zone
- ///
- /// This method should only be called from the LoadZoneCommand class,
- /// internally it will tell the clients builder thread to reload
- /// the zone specified in the arguments in the background.
- ///
- /// \param args Element argument that should be a map of the form
- /// { "class": "IN", "origin": "example.com" }
- /// (but class is optional and will default to IN)
- ///
- /// \exception LoadZoneCommandError if the args value is null, or not in
- /// the expected format
- void loadZone(isc::data::ConstElementPtr args);
-
private:
AuthSrvImpl* impl_;
isc::asiolink::SimpleCallback* checkin_;
diff --git a/src/bin/auth/command.cc b/src/bin/auth/command.cc
index 89aee3f..9fbfb42 100644
--- a/src/bin/auth/command.cc
+++ b/src/bin/auth/command.cc
@@ -176,7 +176,7 @@ public:
virtual ConstElementPtr exec(AuthSrv& server,
isc::data::ConstElementPtr args)
{
- server.loadZone(args);
+ server.getDataSrcClientsMgr().loadZone(args);
return (createAnswer());
}
};
diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h
index dde6dca..e493287 100644
--- a/src/bin/auth/datasrc_clients_mgr.h
+++ b/src/bin/auth/datasrc_clients_mgr.h
@@ -45,11 +45,16 @@
namespace isc {
namespace auth {
-/// \brief An exception that is thrown if the arguments to the loadZone
-/// call are not in the right format
-class LoadZoneCommandError : public isc::Exception {
+/// \brief An exception that is thrown if initial checks for a command fail
+///
+/// This is raised *before* the command to the thread is constructed and
+/// sent, so the application can still handle them (and therefore it is
+/// public, as opposed to InternalCommandError).
+///
+/// And example of its use is currently in loadZone().
+class CommandError : public isc::Exception {
public:
- LoadZoneCommandError(const char* file, size_t line, const char* what) :
+ CommandError(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
@@ -253,30 +258,50 @@ public:
/// { "class": "IN", "origin": "example.com" }
/// (but class is optional and will default to IN)
///
- /// \exception LoadZoneCommandError if the args value is null, or not in
- /// the expected format
+ /// \exception CommandError if the args value is null, or not in
+ /// the expected format, or contains
+ /// a bad origin or class string
void
loadZone(data::ConstElementPtr args) {
if (!args) {
- isc_throw(LoadZoneCommandError, "loadZone argument empty");
+ isc_throw(CommandError, "loadZone argument empty");
}
if (args->getType() != isc::data::Element::map) {
- isc_throw(LoadZoneCommandError, "loadZone argument not a map");
+ isc_throw(CommandError, "loadZone argument not a map");
}
if (!args->contains("origin")) {
- isc_throw(LoadZoneCommandError,
+ isc_throw(CommandError,
"loadZone argument has no 'origin' value");
+ } else {
+ // Also check if it really is a valid name
+ try {
+ dns::Name(args->get("origin")->stringValue());
+ } catch (const isc::Exception& exc) {
+ isc_throw(CommandError,
+ "bad origin: " << exc.what());
+ }
}
+
if (args->get("origin")->getType() != data::Element::string) {
- isc_throw(LoadZoneCommandError,
+ isc_throw(CommandError,
"loadZone argument 'origin' value not a string");
}
- if (args->contains("class") &&
- args->get("class")->getType() != data::Element::string) {
- isc_throw(LoadZoneCommandError,
- "loadZone argument 'class' value not a string");
+ if (args->contains("class")) {
+ if (args->get("class")->getType() != data::Element::string) {
+ isc_throw(CommandError,
+ "loadZone argument 'class' value not a string");
+ } else {
+ try {
+ dns::RRClass(args->get("class")->stringValue());
+ } catch (const isc::Exception& exc) {
+ isc_throw(CommandError,
+ "bad class: " << exc.what());
+ }
+ }
}
+ // Slightly more advanced checks
+
sendCommand(datasrc_clientmgr_internal::LOADZONE, args);
}
@@ -532,6 +557,14 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::doLoadZone(
assert(arg);
assert(arg->get("origin"));
+ // TODO: currently, we hardcode IN as the default for the optional
+ // 'class' argument. We should really derive this from the specification,
+ // but atm the config/command API does not allow that to be done
+ // easily. Once that is in place (tickets have yet to be created,
+ // as we need to do a tiny bit of design work for that), this
+ // code can be replaced with the original part:
+ // assert(arg->get("class"));
+ // const dns::RRClass(arg->get("class")->stringValue());
isc::data::ConstElementPtr class_elem = arg->get("class");
const dns::RRClass rrclass(class_elem ?
dns::RRClass(class_elem->stringValue()) :
diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc
index 19eb44b..2ac9010 100644
--- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc
+++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc
@@ -211,7 +211,12 @@ TEST(DataSrcClientsMgrTest, reload) {
// Should fail with non-string 'class' value
args->set("class", Element::create(1));
- EXPECT_THROW(mgr.loadZone(args), LoadZoneCommandError);
+ EXPECT_THROW(mgr.loadZone(args), CommandError);
+ EXPECT_EQ(2, FakeDataSrcClientsBuilder::command_queue->size());
+
+ // And with badclass
+ args->set("class", Element::create("BADCLASS"));
+ EXPECT_THROW(mgr.loadZone(args), CommandError);
EXPECT_EQ(2, FakeDataSrcClientsBuilder::command_queue->size());
// Should succeed without 'class'
@@ -221,19 +226,24 @@ TEST(DataSrcClientsMgrTest, reload) {
// but fail without origin, without sending new commands
args->remove("origin");
- EXPECT_THROW(mgr.loadZone(args), LoadZoneCommandError);
+ EXPECT_THROW(mgr.loadZone(args), CommandError);
EXPECT_EQ(3, FakeDataSrcClientsBuilder::command_queue->size());
// And for 'origin' that is not a string
args->set("origin", Element::create(1));
- EXPECT_THROW(mgr.loadZone(args), LoadZoneCommandError);
+ EXPECT_THROW(mgr.loadZone(args), CommandError);
+ EXPECT_EQ(3, FakeDataSrcClientsBuilder::command_queue->size());
+
+ // And origin that is not a correct name
+ args->set("origin", Element::create(".."));
+ EXPECT_THROW(mgr.loadZone(args), CommandError);
EXPECT_EQ(3, FakeDataSrcClientsBuilder::command_queue->size());
// same for empty data and data that is not a map
EXPECT_THROW(mgr.loadZone(isc::data::ConstElementPtr()),
- LoadZoneCommandError);
+ CommandError);
EXPECT_THROW(mgr.loadZone(isc::data::Element::createList()),
- LoadZoneCommandError);
+ CommandError);
EXPECT_EQ(3, FakeDataSrcClientsBuilder::command_queue->size());
}
More information about the bind10-changes
mailing list