From: Stephen Morris Date: Wed, 24 Oct 2012 18:18:33 +0000 (+0100) Subject: [2342] Address remaining issues in review comment 6. X-Git-Tag: trac2487_base~1^2~51 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=39c82ec533e3642015ce742a132b31350e91c928;p=thirdparty%2Fkea.git [2342] Address remaining issues in review comment 6. --- diff --git a/src/lib/dhcp/Makefile.am b/src/lib/dhcp/Makefile.am index 945eaee0d9..ff3b9375e0 100644 --- a/src/lib/dhcp/Makefile.am +++ b/src/lib/dhcp/Makefile.am @@ -24,12 +24,7 @@ libb10_dhcp___la_SOURCES += iface_mgr.cc iface_mgr.h libb10_dhcp___la_SOURCES += iface_mgr_bsd.cc libb10_dhcp___la_SOURCES += iface_mgr_linux.cc libb10_dhcp___la_SOURCES += iface_mgr_sun.cc -libb10_dhcp___la_SOURCES += lease_mgr.cc lease_mgr.h -libb10_dhcp___la_SOURCES += lease_mgr_factory.cc lease_mgr_factory.h libb10_dhcp___la_SOURCES += libdhcp++.cc libdhcp++.h -if HAVE_MYSQL -libb10_dhcp___la_SOURCES += mysql_lease_mgr.cc mysql_lease_mgr.h -endif libb10_dhcp___la_SOURCES += option4_addrlst.cc option4_addrlst.h libb10_dhcp___la_SOURCES += option6_addrlst.cc option6_addrlst.h libb10_dhcp___la_SOURCES += option6_iaaddr.cc option6_iaaddr.h @@ -43,21 +38,26 @@ libb10_dhcp___la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) libb10_dhcp___la_LIBADD = $(top_builddir)/src/lib/asiolink/libb10-asiolink.la libb10_dhcp___la_LIBADD += $(top_builddir)/src/lib/util/libb10-util.la libb10_dhcp___la_LDFLAGS = -no-undefined -version-info 2:0:0 -if HAVE_MYSQL -libb10_dhcp___la_LDFLAGS += $(MYSQL_LIBS) -endif libb10_dhcpsrv_la_SOURCES = cfgmgr.cc cfgmgr.h +libb10_dhcpsrv_la_SOURCES += addr_utilities.cc addr_utilities.h +libb10_dhcpsrv_la_SOURCES += lease_mgr.cc lease_mgr.h +libb10_dhcpsrv_la_SOURCES += lease_mgr_factory.cc lease_mgr_factory.h +if HAVE_MYSQL +libb10_dhcpsrv_la_SOURCES += mysql_lease_mgr.cc mysql_lease_mgr.h +endif libb10_dhcpsrv_la_SOURCES += pool.cc pool.h libb10_dhcpsrv_la_SOURCES += subnet.cc subnet.h libb10_dhcpsrv_la_SOURCES += triplet.h -libb10_dhcpsrv_la_SOURCES += addr_utilities.cc addr_utilities.h libb10_dhcpsrv_la_CXXFLAGS = $(AM_CXXFLAGS) libb10_dhcpsrv_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) libb10_dhcpsrv_la_LIBADD = $(top_builddir)/src/lib/asiolink/libb10-asiolink.la libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/util/libb10-util.la libb10_dhcpsrv_la_LDFLAGS = -no-undefined -version-info 2:0:0 +if HAVE_MYSQL +libb10_dhcpsrv_la_LDFLAGS += $(MYSQL_LIBS) +endif EXTRA_DIST = README diff --git a/src/lib/dhcp/lease_mgr_factory.h b/src/lib/dhcp/lease_mgr_factory.h index 164d2f93b8..abd1bba60d 100644 --- a/src/lib/dhcp/lease_mgr_factory.h +++ b/src/lib/dhcp/lease_mgr_factory.h @@ -50,6 +50,9 @@ public: /// /// Strictly speaking these functions could be stand-alone functions. However, /// it is convenient to encapsulate them in a class for naming purposes. +/// +/// @TODO: Will need to develop some form of registration mechanism for +/// user-supplied backends (so that there is no need to modify the code). class LeaseMgrFactory { public: /// @brief Create an instance of a lease manager. diff --git a/src/lib/dhcp/mysql_lease_mgr.cc b/src/lib/dhcp/mysql_lease_mgr.cc index 0d7a4edd76..8fb73bda0d 100644 --- a/src/lib/dhcp/mysql_lease_mgr.cc +++ b/src/lib/dhcp/mysql_lease_mgr.cc @@ -22,6 +22,7 @@ #include #include +using namespace isc; using namespace std; namespace { @@ -48,9 +49,15 @@ namespace dhcp { /// @brief Exchange MySQL and Lease6 Data /// -/// On the INSERT, SELECT and UPDATE statements, an array of MYSQL_BIND -/// structures must be built to reflect the data being inserted or retrieved -/// from the database. +/// On any MySQL operation, arrays of MYSQL_BIND structures must be built to +/// describe the parameters in the prepared statements. Where information is +/// inserted or retrieved - INSERT, UPDATE, SELECT - one array describes the +/// data being exchanged with the database. The other array describes the +/// WHERE clause of the statement. +/// +/// The array describing the information exchanged is common between the +/// INSERT, UPDATE and SELECT statements, and this class handles the creation +/// of that array and the insertion/extraction of data into/from it. /// /// Owing to the MySQL API, the process requires some intermediate variables /// to hold things like length etc. This object holds the intermediate @@ -67,9 +74,7 @@ public: /// @brief Create MYSQL_BIND objects for Lease6 Pointer /// - /// Fills in the bind_ objects for the Lease6 passed to it. - /// - /// The MySQL documentation + /// Fills in the MYSQL_BIND objects for the Lease6 passed to it. /// /// @param lease Lease object to be added to the database /// @@ -78,7 +83,7 @@ public: /// valid only for as long as (1) this object is in existence and /// (2) the lease object passed to it is in existence. The /// caller should NOT delete it. - MYSQL_BIND* CreateBindForSend(const Lease6Ptr& lease) { + MYSQL_BIND* createBindForSend(const Lease6Ptr& lease) { // Store lease object to ensure it remains valid. lease_ = lease; @@ -145,7 +150,7 @@ public: lease_type_ = lease_->type_; bind_[7].buffer_type = MYSQL_TYPE_TINY; bind_[7].buffer = reinterpret_cast(&lease_type_); - bind_[7].is_unsigned = static_cast(1); + bind_[7].is_unsigned = true_; // iaid: unsigned int // Can use lease_->iaid_ directly as it is of type uint32_t. @@ -239,7 +244,7 @@ public: bind_[8].buffer = reinterpret_cast(&iaid_); bind_[8].is_unsigned = true_; bind_[8].error = &error_[8]; - + // prefix_len: unsigned tinyint bind_[9].buffer_type = MYSQL_TYPE_TINY; bind_[9].buffer = reinterpret_cast(&prefixlen_); @@ -297,7 +302,10 @@ public: break; default: - isc_throw(BadValue, "invalid lease type returned"); + isc_throw(BadValue, "invalid lease type returned (" << + lease_type_ << ") for lease with address " << + result->addr_.toText() << ". Only 0, 1, or 2 " + "are allowed."); } result->iaid_ = iaid_; result->prefixlen_ = prefixlen_; @@ -332,6 +340,44 @@ private: }; +MySqlLeaseMgr::MySqlLeaseMgr(const LeaseMgr::ParameterMap& parameters) + : LeaseMgr(parameters), mysql_(NULL) { + + // Allocate context for MySQL - it is destroyed in the destructor. + mysql_ = mysql_init(NULL); + if (mysql_ == NULL) { + isc_throw(DbOpenError, "unable to initialize MySQL"); + } + + // Open the database + openDatabase(); + + // Disable autocommit + my_bool result = mysql_autocommit(mysql_, 0); + if (result != 0) { + isc_throw(DbOperationError, mysql_error(mysql_)); + } + + // Prepare all statements likely to be used. + prepareStatements(); +} + +MySqlLeaseMgr::~MySqlLeaseMgr() { + // Free up the prepared statements, ignoring errors. (What would we do + // about them - we're destroying this object and are not really concerned + // with errors on a database connection that it about to go away.) + for (int i = 0; i < statements_.size(); ++i) { + if (statements_[i] != NULL) { + (void) mysql_stmt_close(statements_[i]); + statements_[i] = NULL; + } + } + + // Close the database + mysql_close(mysql_); + mysql_ = NULL; +} + // Time conversion methods. // @@ -390,14 +436,13 @@ void MySqlLeaseMgr::openDatabase() { // Set up the values of the parameters - const char* host = NULL; + const char* host = "localhost"; string shost; try { shost = getParameter("host"); host = shost.c_str(); } catch (...) { - // No host. Fine, we'll use NULL - ; + // No host. Fine, we'll use "localhost" } const char* user = NULL; @@ -443,12 +488,13 @@ MySqlLeaseMgr::prepareStatement(StatementIndex index, const char* text) { // Validate that there is space for the statement in the statements array // and that nothing has been placed there before. if ((index >= statements_.size()) || (statements_[index] != NULL)) { - isc_throw(InvalidParameter, "invalid prepared statement index or " - "statement index not null"); + isc_throw(InvalidParameter, "invalid prepared statement index (" << + static_cast(index) << ") or indexed prepared " << + "statement is not null"); } // All OK, so prepare the statement - raw_statements_[index] = std::string(text); + text_statements_[index] = std::string(text); statements_[index] = mysql_stmt_init(mysql_); if (statements_[index] == NULL) { @@ -469,8 +515,8 @@ MySqlLeaseMgr::prepareStatements() { statements_.clear(); statements_.resize(NUM_STATEMENTS, NULL); - raw_statements_.clear(); - raw_statements_.resize(NUM_STATEMENTS, std::string("")); + text_statements_.clear(); + text_statements_.resize(NUM_STATEMENTS, std::string("")); // Now allocate the statements prepareStatement(DELETE_LEASE6, @@ -489,45 +535,10 @@ MySqlLeaseMgr::prepareStatements() { "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"); } - -MySqlLeaseMgr::MySqlLeaseMgr(const LeaseMgr::ParameterMap& parameters) - : LeaseMgr(parameters), mysql_(NULL) { - - // Allocate context for MySQL - it is destroyed in the destructor. - mysql_ = mysql_init(NULL); - - // Open the database - openDatabase(); - - // Disable autocommit - my_bool result = mysql_autocommit(mysql_, 0); - if (result != 0) { - isc_throw(DbOperationError, mysql_error(mysql_)); - } - - // Prepare all statements likely to be used. - prepareStatements(); -} - -MySqlLeaseMgr::~MySqlLeaseMgr() { - // Free up the prepared statements, ignoring errors. (What would we do - // about them - we're destroying this object and are not really concerned - // with errors on a database connection that it about to go away.) - for (int i = 0; i < statements_.size(); ++i) { - if (statements_[i] != NULL) { - (void) mysql_stmt_close(statements_[i]); - statements_[i] = NULL; - } - } - - // Close the database - mysql_close(mysql_); - mysql_ = NULL; -} - bool MySqlLeaseMgr::addLease(const Lease4Ptr& /* lease */) { - + isc_throw(NotImplemented, "MySqlLeaseMgr::addLease(const Lease4Ptr&) " + "not implemented yet"); return (false); } @@ -536,7 +547,7 @@ MySqlLeaseMgr::addLease(const Lease6Ptr& lease) { // Create the MYSQL_BIND array for the lease MySqlLease6Exchange exchange; - MYSQL_BIND* bind = exchange.CreateBindForSend(lease); + MYSQL_BIND* bind = exchange.createBindForSend(lease); // Bind the parameters to the statement int status = mysql_stmt_bind_param(statements_[INSERT_LEASE6], bind); @@ -562,33 +573,45 @@ MySqlLeaseMgr::addLease(const Lease6Ptr& lease) { Lease4Ptr MySqlLeaseMgr::getLease4(const isc::asiolink::IOAddress& /* addr */, SubnetID /* subnet_id */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease4(const IOAddress&, SubnetID) " + "not implemented yet"); return (Lease4Ptr()); } Lease4Ptr MySqlLeaseMgr::getLease4(const isc::asiolink::IOAddress& /* addr */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease4(const IOAddress&) " + "not implemented yet"); return (Lease4Ptr()); } Lease4Collection MySqlLeaseMgr::getLease4(const HWAddr& /* hwaddr */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease4(const HWAddr&) " + "not implemented yet"); return (Lease4Collection()); } Lease4Ptr MySqlLeaseMgr::getLease4(const HWAddr& /* hwaddr */, SubnetID /* subnet_id */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease4(const HWAddr&, SubnetID) " + "not implemented yet"); return (Lease4Ptr()); } Lease4Collection MySqlLeaseMgr::getLease4(const ClientId& /* clientid */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease4(const ClientID&) " + "not implemented yet"); return (Lease4Collection()); } Lease4Ptr MySqlLeaseMgr::getLease4(const ClientId& /* clientid */, SubnetID /* subnet_id */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease4(const ClientID&, SubnetID) " + "not implemented yet"); return (Lease4Ptr()); } @@ -629,14 +652,14 @@ MySqlLeaseMgr::getLease6(const isc::asiolink::IOAddress& addr) const { if (status == 0) { try { result = exchange.getLeaseData(); - } catch (isc::BadValue) { + } catch (const isc::BadValue& ex) { // Free up result set. (void) mysql_stmt_free_result(statements_[GET_LEASE6]); // Lease type is returned, to rethrow the exception with a bit // more data. - isc_throw(BadValue, "invalid lease type returned for <" << - raw_statements_[GET_LEASE6] << ">"); + isc_throw(BadValue, ex.what() << ". Statement is <" << + text_statements_[GET_LEASE6] << ">"); } // As the address is the primary key in the table, we can't return @@ -658,25 +681,35 @@ MySqlLeaseMgr::getLease6(const isc::asiolink::IOAddress& addr) const { Lease6Collection MySqlLeaseMgr::getLease6(const DUID& /* duid */, uint32_t /* iaid */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease6(const DUID&) " + "not implemented yet"); return (Lease6Collection()); } Lease6Ptr MySqlLeaseMgr::getLease6(const DUID& /* duid */, uint32_t /* iaid */, SubnetID /* subnet_id */) const { + isc_throw(NotImplemented, "MySqlLeaseMgr::getLease4(const DUID&, SubnetID) " + "not implemented yet"); return (Lease6Ptr()); } void MySqlLeaseMgr::updateLease4(const Lease4Ptr& /* lease4 */) { + isc_throw(NotImplemented, "MySqlLeaseMgr::updateLease4(const Lease4Ptr&) " + "not implemented yet"); } void MySqlLeaseMgr::updateLease6(const Lease6Ptr& /* lease6 */) { + isc_throw(NotImplemented, "MySqlLeaseMgr::updateLease6(const Lease6Ptr&) " + "not implemented yet"); } bool MySqlLeaseMgr::deleteLease4(const isc::asiolink::IOAddress& /* addr */) { + isc_throw(NotImplemented, "MySqlLeaseMgr::deleteLease4(const IOAddress&) " + "not implemented yet"); return (false); } @@ -712,7 +745,13 @@ MySqlLeaseMgr::deleteLease6(const isc::asiolink::IOAddress& addr) { std::string MySqlLeaseMgr::getName() const { - return (std::string("")); + std::string name = ""; + try { + name = getParameter("name"); + } catch (...) { + ; + } + return (name); } std::string @@ -729,7 +768,7 @@ MySqlLeaseMgr::getVersion() const { int status = mysql_stmt_execute(statements_[GET_VERSION]); if (status != 0) { isc_throw(DbOperationError, "unable to execute <" - << raw_statements_[GET_VERSION] << "> - reason: " << + << text_statements_[GET_VERSION] << "> - reason: " << mysql_error(mysql_)); } diff --git a/src/lib/dhcp/mysql_lease_mgr.h b/src/lib/dhcp/mysql_lease_mgr.h index 375a0eec78..349b3bb839 100644 --- a/src/lib/dhcp/mysql_lease_mgr.h +++ b/src/lib/dhcp/mysql_lease_mgr.h @@ -22,7 +22,7 @@ namespace isc { namespace dhcp { -/// @brief Abstract Lease Manager +/// @brief MySQL Lease Manager /// /// This is a concrete API for the backend for the MySQL database. class MySqlLeaseMgr : public LeaseMgr { @@ -31,10 +31,10 @@ public: /// /// Uses the following keywords in the parameters passed to it to /// connect to the database: - /// - name - Name of the database to which to connect - /// - host - Host name to which to connect - /// - user - Username under which to connect. - /// - password - Password for "user" on the database. + /// - name - Name of the database to which to connect (mandatory) + /// - host - Host to which to connect (optional, defaults to "localhost") + /// - user - Username under which to connect (optional) + /// - password - Password for "user" on the database (optional) /// /// If the database is successfully opened, the version number in the /// schema_version table will be checked against hard-coded value in @@ -44,6 +44,10 @@ public: /// /// @param parameters A data structure relating keywords and values /// concerned with the database. + /// + /// @exception DbOpenError Error opening the database + /// @exception DbOperationError An operation on the open database has + /// failed. MySqlLeaseMgr(const ParameterMap& parameters); /// @brief Destructor (closes database) @@ -156,6 +160,11 @@ public: /// @param addr address of the searched lease /// /// @return smart pointer to the lease (or NULL if a lease is not found) + /// + /// @exception BadValue record retrieved from database had an invalid + /// lease type field. + /// @exception DbOperationError MySQL operation failed, exception will give + /// text indicating the reason. virtual Lease6Ptr getLease6(const isc::asiolink::IOAddress& addr) const; /// @brief Returns existing IPv6 leases for a given DUID+IA combination @@ -208,6 +217,9 @@ public: /// @param addr IPv4 address of the lease to be deleted. /// /// @return true if deletion was successful, false if no such lease exists + /// + /// @exception DbOperationError MySQL operation failed, exception will give + /// text indicating the reason. virtual bool deleteLease6(const isc::asiolink::IOAddress& addr); /// @brief Returns backend name. @@ -234,6 +246,9 @@ public: /// B>=A and B=C (it is ok to have newer backend, as it should be backward /// compatible) /// Also if B>C, some database upgrade procedure may be triggered + /// + /// @exception DbOperationError MySQL operation failed, exception will give + /// text indicating the reason. virtual std::pair getVersion() const; /// @brief Commit Transactions @@ -335,6 +350,11 @@ private: /// /// Creates the prepared statements for all of the SQL statements used /// by the MySQL backend. + /// + /// @exception DbOperationError MySQL operation failed, exception will give + /// text indicating the reason. + /// @exception InvalidParameter 'index' is not valid for the vector. This + /// represents an internal error within the code. void prepareStatements(); /// @brief Open Database @@ -360,7 +380,7 @@ private: const char* what) const { if (status != 0) { isc_throw(DbOperationError, what << " for <" << - raw_statements_[index] << ">, reason: " << + text_statements_[index] << ">, reason: " << mysql_error(mysql_) << " (error code " << mysql_errno(mysql_) << ")"); } @@ -368,7 +388,7 @@ private: // Members MYSQL* mysql_; ///< MySQL context object - std::vector raw_statements_; ///< Raw text of statements + std::vector text_statements_; ///< Raw text of statements std::vector statements_; ///< Prepared statements }; diff --git a/src/lib/dhcp/tests/Makefile.am b/src/lib/dhcp/tests/Makefile.am index 931940fafd..05f02b8402 100644 --- a/src/lib/dhcp/tests/Makefile.am +++ b/src/lib/dhcp/tests/Makefile.am @@ -31,11 +31,6 @@ TESTS += libdhcp++_unittests libdhcpsrv_unittests libdhcp___unittests_SOURCES = run_unittests.cc libdhcp___unittests_SOURCES += duid_unittest.cc libdhcp___unittests_SOURCES += iface_mgr_unittest.cc -libdhcp___unittests_SOURCES += lease_mgr_factory_unittest.cc -libdhcp___unittests_SOURCES += lease_mgr_unittest.cc -if HAVE_MYSQL -libdhcp___unittests_SOURCES += mysql_lease_mgr_unittest.cc -endif libdhcp___unittests_SOURCES += libdhcp++_unittest.cc libdhcp___unittests_SOURCES += option4_addrlst_unittest.cc libdhcp___unittests_SOURCES += option6_addrlst_unittest.cc @@ -48,14 +43,15 @@ libdhcp___unittests_SOURCES += pkt6_unittest.cc libdhcp___unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES) $(LOG4CPLUS_INCLUDES) libdhcp___unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS) libdhcp___unittests_CXXFLAGS = $(AM_CXXFLAGS) -if HAVE_MYSQL -libdhcp___unittests_CPPFLAGS += $(MYSQL_CPPFLAGS) -libdhcp___unittests_LDFLAGS += $(MYSQL_LIBS) -endif libdhcpsrv_unittests_SOURCES = run_unittests.cc libdhcpsrv_unittests_SOURCES += addr_utilities_unittest.cc libdhcpsrv_unittests_SOURCES += cfgmgr_unittest.cc +libdhcpsrv_unittests_SOURCES += lease_mgr_factory_unittest.cc +libdhcpsrv_unittests_SOURCES += lease_mgr_unittest.cc +if HAVE_MYSQL +libdhcpsrv_unittests_SOURCES += mysql_lease_mgr_unittest.cc +endif libdhcpsrv_unittests_SOURCES += pool_unittest.cc libdhcpsrv_unittests_SOURCES += subnet_unittest.cc libdhcpsrv_unittests_SOURCES += triplet_unittest.cc @@ -67,7 +63,12 @@ libdhcpsrv_unittests_LDADD = $(GTEST_LDADD) libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libb10-dhcpsrv.la +libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la +if HAVE_MYSQL +libdhcpsrv_unittests_CPPFLAGS += $(MYSQL_CPPFLAGS) +libdhcpsrv_unittests_LDFLAGS += $(MYSQL_LIBS) +endif if USE_CLANGPP diff --git a/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc index 74d769aa5f..e8305e4d98 100644 --- a/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc +++ b/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc @@ -310,4 +310,11 @@ TEST_F(MySqlLeaseMgrTest, BasicLease6) { detailCompareLease6(l2, l_returned); } +/// @brief Check getName() returns correct database name +TEST_F(MySqlLeaseMgrTest, getName) { + EXPECT_EQ(std::string("keatest"), lmptr_->getName()); + + // @TODO: check for the negative +} + }; // end of anonymous namespace