From: Jelte Jansen Date: Tue, 30 Oct 2012 11:27:10 +0000 (+0100) Subject: [2213] address review comments X-Git-Tag: trac2487_base~23^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d3bdb3c1c1dca514322fefdaac18a253364dfd2f;p=thirdparty%2Fkea.git [2213] address review comments - add comments about hardcoded class default - removed AuthSrv::loadZone() again - more data integrity checks in datasrc clients mgr loadZone handler --- diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc index 73c2ae4823..157ae03bae 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 5e0c17f11b..2d53666266 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 89aee3ff93..9fbfb42c5c 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 dde6dcaaf8..e4932879a5 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::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 19eb44b3c2..2ac9010008 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()); }