From: Stephen Morris Date: Wed, 24 Oct 2012 13:12:06 +0000 (+0100) Subject: [2342] Address review changes (in comment:5 of the ticket) X-Git-Tag: trac2487_base~1^2~54 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b0ed08d21421ca321502cda1e9811a4bfe560275;p=thirdparty%2Fkea.git [2342] Address review changes (in comment:5 of the ticket) --- diff --git a/configure.ac b/configure.ac index ec41ca1887..0c7653675b 100644 --- a/configure.ac +++ b/configure.ac @@ -735,9 +735,9 @@ LIBS=$LIBS_SAVED # the --with-mysql-config (default to /usr/bin/mysql-config). By default, # the software is not built with MySQL support enabled. mysql_config="no" -AC_ARG_WITH([mysql-config], - AC_HELP_STRING([--with-mysql-config=PATH], - [specify the path to the mysql_config script]), +AC_ARG_WITH([dhcp-mysql], + AC_HELP_STRING([--with-dhcp-mysql=PATH], + [path to the MySQL 'mysql_config' script (MySQL is used for the DHCP database)]), [mysql_config="$withval"]) if test "${mysql_config}" = "yes" ; then @@ -747,11 +747,8 @@ elif test "${mysql_config}" != "no" ; then fi if test "$MYSQL_CONFIG" != "" ; then - if test -d "$MYSQL_CONFIG" ; then - AC_MSG_ERROR([Specified mysql_config program ${MYSQL_CONFIG} is a directory]) - fi - if ! test -x "$MYSQL_CONFIG" ; then - AC_MSG_ERROR([--with-mysql-config should point to a mysql_config program]) + if test -d "$MYSQL_CONFIG" -o ! -x "$MYSQL_CONFIG" ; then + AC_MSG_ERROR([--with-dhcp-mysql should point to a mysql_config program]) fi MYSQL_CPPFLAGS=`$MYSQL_CONFIG --cflags` @@ -1466,8 +1463,6 @@ dnl includes too ${LOG4CPLUS_LIBS} SQLite: $SQLITE_CFLAGS $SQLITE_LIBS - MySQL: $MYSQL_CPPFLAGS - $MYSQL_LIBS Features: $enable_features diff --git a/src/lib/dhcp/Makefile.am b/src/lib/dhcp/Makefile.am index 74c4491e53..945eaee0d9 100644 --- a/src/lib/dhcp/Makefile.am +++ b/src/lib/dhcp/Makefile.am @@ -20,8 +20,8 @@ lib_LTLIBRARIES = libb10-dhcp++.la libb10-dhcpsrv.la libb10_dhcp___la_SOURCES = libb10_dhcp___la_SOURCES += dhcp6.h dhcp4.h libb10_dhcp___la_SOURCES += duid.cc duid.h -libb10_dhcp___la_SOURCES += iface_mgr_bsd.cc 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 diff --git a/src/lib/dhcp/dhcpdb_create.mysql b/src/lib/dhcp/dhcpdb_create.mysql index e4b7771c06..41bea247fb 100644 --- a/src/lib/dhcp/dhcpdb_create.mysql +++ b/src/lib/dhcp/dhcpdb_create.mysql @@ -28,53 +28,63 @@ # # source dhcpdb_create.mysql + # Holds the IPv4 leases. CREATE TABLE lease4 ( - address INT UNSIGNED UNIQUE NOT NULL, # IPv4 address - hwaddr VARBINARY(20), # Hardware address - client_id VARBINARY(128), # Client ID - lease_time INT UNSIGNED, # Length of the lease (seconds) - expire TIMESTAMP, # Expiration time of the lease - subnet_id INT UNSIGNED # Subnet identification + address INT UNSIGNED PRIMARY KEY NOT NULL, # IPv4 address + hwaddr VARBINARY(20), # Hardware address + client_id VARBINARY(128), # Client ID + lease_time INT UNSIGNED, # Length of the lease (seconds) + expire TIMESTAMP, # Expiration time of the lease + subnet_id INT UNSIGNED # Subnet identification ) ENGINE = INNODB; -# Holds the IPv6 leases +# Holds the IPv6 leases. +# N.B. The use of a VARCHAR for the address is temporary for development: +# it will eventually be replaced by BINARY(16). CREATE TABLE lease6 ( - address VARCHAR(40) UNIQUE NOT NULL, # IPv6 address (actually 39 is max) - hwaddr VARBINARY(20), # Hardware address - client_id VARBINARY(128), # Client ID - lease_time INT UNSIGNED, # Length of the lease (seconds) - expire TIMESTAMP, # Expiration time of the lease - subnet_id INT UNSIGNED, # Subnet identification - pref_lifetime INT UNSIGNED, # Preferred lifetime - lease_type TINYINT, # Lease type - iaid INT UNSIGNED, # IA ID - prefix_len TINYINT UNSIGNED # For IA_PD only + address VARCHAR(40) PRIMARY KEY NOT NULL, # IPv6 address + hwaddr VARBINARY(20), # Hardware address + client_id VARBINARY(128), # Client ID + lease_time INT UNSIGNED, # Length of the lease (seconds) + expire TIMESTAMP, # Expiration time of the lease + subnet_id INT UNSIGNED, # Subnet identification + pref_lifetime INT UNSIGNED, # Preferred lifetime + lease_type TINYINT, # Lease type (see lease6_types + # table for possible values) + iaid INT UNSIGNED, # See Section 10 of RFC 3315 + prefix_len TINYINT UNSIGNED # For IA_PD only ) ENGINE = INNODB; # ... and a definition of lease6 types. This table is a convenience for # users of the database - if they want to view the lease table and use the # type names, they can join this table with the lease6 table CREATE TABLE lease6_types ( - lease_type TINYINT UNIQUE NOT NULL, # Lease type code - name VARCHAR(5) # Name of the lease type + lease_type TINYINT PRIMARY KEY NOT NULL, # Lease type code. + name VARCHAR(5) # Name of the lease type ); +START TRANSACTION; INSERT INTO lease6_types VALUES (0, "IA_NA"); # Non-temporary v6 addresses INSERT INTO lease6_types VALUES (1, "IA_TA"); # Temporary v6 addresses INSERT INTO lease6_types VALUES (2, "IA_PD"); # Prefix delegations +COMMIT; # Finally, the version of the schema. We start at 0.1 during development. # This table is only modified during schema upgrades. For historical reasons # (related to the names of the columns in the BIND 10 DNS database file), the # first column is called "version" and not "major". CREATE TABLE schema_version ( - version INT NOT NULL, # Major version number - minor INT NOT NULL # Minor version number + version INT PRIMARY KEY NOT NULL, # Major version number + minor INT # Minor version number ); +START TRANSACTION; INSERT INTO schema_version VALUES (0, 1); - COMMIT; +# Notes: +# +# Indexes +# ======= # It is likely that additional indexes will be needed. However, the # increase in lookup performance from these will come at the expense # of a decrease in performance during insert operations due to the need @@ -90,3 +100,17 @@ COMMIT; # For lease stability: if a client requests a new lease, try to find an # existing or recently expired lease for it so that it can keep using the # same IP address. +# +# Field Sizes +# =========== +# If any of the VARxxx field sizes are altered, the lengths in the MySQL +# backend source file (mysql_lease_mgr.cc) must be correspondingly changed. +# +# Portability +# =========== +# The "ENGINE = INNODB" on some tables is not portablea to another database +# and will need to be removed. +# +# Some columns contain binary data so are stored as VARBINARY instead of +# VARCHAR. This may be non-portable between databases: in this case, the +# definition should be changed to VARCHAR. diff --git a/src/lib/dhcp/lease_mgr.h b/src/lib/dhcp/lease_mgr.h index 73155516c2..6d74e5e703 100644 --- a/src/lib/dhcp/lease_mgr.h +++ b/src/lib/dhcp/lease_mgr.h @@ -164,6 +164,7 @@ struct Lease4 { /// @brief Constructor /// /// Initialize fields that don't have a default constructor. + /// @TODO Remove this Lease4() : addr_(0) {} }; @@ -280,7 +281,7 @@ struct Lease6 { /// @brief Constructor /// /// Initialize fields that don't have a default constructor. - Lease6() : addr_(0) {} + Lease6() : addr_("::") {} }; /// @brief Pointer to a Lease6 structure. @@ -336,7 +337,7 @@ public: /// @exception DbOperationError Database function failed virtual bool addLease(const Lease6Ptr& lease) = 0; - /// @brief Returns existing IPv4 lease for specified IPv4 address and subnet_id + /// @brief Returns IPv4 lease for specified IPv4 address and subnet_id /// /// This method is used to get a lease for specific subnet_id. There can be /// at most one lease for any given subnet, so this method returns a single @@ -522,7 +523,6 @@ public: /// As host reservation is outside of scope for 2012, support for hosts /// is currently postponed. - protected: /// @brief returns value of the parameter std::string getParameter(const std::string& name) const; @@ -535,9 +535,6 @@ protected: ParameterMap parameters_; }; -/// @brief Pointer to a Lease Manager -typedef boost::shared_ptr LeaseMgrPtr; - }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/lib/dhcp/lease_mgr_factory.cc b/src/lib/dhcp/lease_mgr_factory.cc index d69f8af044..adc8ce0a36 100644 --- a/src/lib/dhcp/lease_mgr_factory.cc +++ b/src/lib/dhcp/lease_mgr_factory.cc @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -36,6 +37,12 @@ using namespace std; namespace isc { namespace dhcp { +boost::scoped_ptr& +LeaseMgrFactory::getLeaseMgrPtr() { + static boost::scoped_ptr leaseMgrPtr; + return (leaseMgrPtr); +} + LeaseMgr::ParameterMap LeaseMgrFactory::parse(const std::string& dbconfig) { LeaseMgr::ParameterMap mapped_tokens; @@ -64,7 +71,7 @@ LeaseMgrFactory::parse(const std::string& dbconfig) { return (mapped_tokens); } -LeaseMgrPtr +void LeaseMgrFactory::create(const std::string& dbconfig) { const std::string type = "type"; @@ -78,14 +85,30 @@ LeaseMgrFactory::create(const std::string& dbconfig) { // Yes, check what it is. #ifdef HAVE_MYSQL if (parameters[type] == string("mysql")) { - return LeaseMgrPtr(new MySqlLeaseMgr(parameters)); + getLeaseMgrPtr().reset(new MySqlLeaseMgr(parameters)); + return; } #endif // Get here on no match - isc_throw(InvalidParameter, "Database configuration parameter 'type' does " + isc_throw(InvalidType, "Database configuration parameter 'type' does " "not specify a supported database backend"); } +void +LeaseMgrFactory::destroy() { + getLeaseMgrPtr().reset(); +} + +LeaseMgr& +LeaseMgrFactory::instance() { + LeaseMgr* lmptr = getLeaseMgrPtr().get(); + if (lmptr == NULL) { + isc_throw(NoLeaseManager, "no current lease manager is available"); + } + return (*lmptr); +} + + }; // namespace dhcp }; // namespace isc diff --git a/src/lib/dhcp/lease_mgr_factory.h b/src/lib/dhcp/lease_mgr_factory.h index 11057d0b36..164d2f93b8 100644 --- a/src/lib/dhcp/lease_mgr_factory.h +++ b/src/lib/dhcp/lease_mgr_factory.h @@ -22,14 +22,25 @@ namespace isc { namespace dhcp { -/// @brief Invalid Type Exception +/// @brief Invalid type exception /// /// Thrown when the factory doesn't recognise the type of the backend. class InvalidType : public Exception { +public: InvalidType(const char* file, size_t line, const char* what) : isc::Exception(file, line, what) {} }; +/// @brief No lease manager exception +/// +/// Thrown if an attempt is made to get a reference to the current lease +/// manager and none is currently available. +class NoLeaseManager : public Exception { +public: + NoLeaseManager(const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) {} +}; + /// @brief Lease Manager Factory /// /// This class comprises nothing but static methods used to create a lease @@ -44,8 +55,12 @@ public: /// @brief Create an instance of a lease manager. /// /// Each database backend has its own lease manager type. This static - /// method returns a lease manager of the appropriate type, based on the - /// the data in the input argument. + /// method sets the "current" lease manager to be a manager of the + /// appropriate type. The actual lease manager is returned by the + /// "instance" method. + /// + /// Note: when called, the current lease manager is *always* destroyed + /// and a new one created - even if the parameters are the same. /// /// dbconfig is a generic way of passing parameters. Parameters are passed /// in the "name=value" format, separated by spaces. The data MUST include @@ -57,8 +72,27 @@ public: /// are back-end specific, although must include the "type" keyword /// which gives the backend in use. /// - /// @return Implementation of lease manager for the specified database. - static LeaseMgrPtr create(const std::string& dbconfig); + /// @exception InvalidParameter dbconfig string does not contain the + /// "type" keyword. + /// @exception InvalidType The "type" keyword in dbconfig does not identify + /// a supported backend. + static void create(const std::string& dbconfig); + + /// @brief Destroy lease manager + /// + /// Destroys the current lease manager object. This should have the effect + /// of closing the database connection. The method is a no-op if no + /// lease manager is available. + static void destroy(); + + /// @brief Return Current Lease Manager + /// + /// Returns an instance of the "current" lease manager. An exception + /// will be thrown if none is available. + /// + /// @exception NoLeaseManager No lease manager is available: use create() + /// to create one before calling this method. + static LeaseMgr& instance(); /// @brief Parse Database Parameters /// @@ -69,6 +103,15 @@ public: /// /// @return std::map<>std::string, std::string> Map of keyword/value pairs. static LeaseMgr::ParameterMap parse(const std::string& dbconfig); + +private: + /// @brief Hold pointer to lease manager + /// + /// Holds a pointer to the singleton lease manager. The singleton + /// is encapsulated in this method to avoid a "static initialization + /// fiasco" if defined in an external static variable. + static boost::scoped_ptr& getLeaseMgrPtr(); + }; }; // end of isc::dhcp namespace diff --git a/src/lib/dhcp/mysql_lease_mgr.cc b/src/lib/dhcp/mysql_lease_mgr.cc index aac06822cf..1f56ec942d 100644 --- a/src/lib/dhcp/mysql_lease_mgr.cc +++ b/src/lib/dhcp/mysql_lease_mgr.cc @@ -24,6 +24,25 @@ using namespace std; +namespace { +///@{ +/// @brief Maximum Size of Database Fields +/// +/// The following constants define buffer sizes for variable length database +/// fields. The values should be greater than or equal to the length set in +/// the schema definition. +/// +/// The exception is the length of any VARCHAR fields: these should be set +/// greater than or equal to the length of the field plus 2: this allows for +/// the insertion of a trailing null regardless of whether the data returned +/// contains a trailing null (the documentation is not clear on this point). + +const size_t ADDRESS6_TEXT_MAX_LEN = 42; // Max size of a IPv6 text buffer +const size_t DUID_MAX_LEN = 128; // Max size of a DUID +const size_t HWADDR_MAX_LEN = 20; // Max size of a hardware address +///@} +}; + namespace isc { namespace dhcp { @@ -290,16 +309,17 @@ private: // Note: All array legths are equal to the corresponding variable in the // schema. std::string addr6_; ///< String form of address - char addr6_buffer_[42]; ///< Character array form of V6 address + char addr6_buffer_[ADDRESS6_TEXT_MAX_LEN]; ///< Character + ///< array form of V6 address unsigned long addr6_length_; ///< Length of the address MYSQL_BIND bind_[10]; ///< Static array for speed of access std::vector clientid_; ///< Client identification - uint8_t clientid_buffer_[128]; ///< Buffer form of client ID + uint8_t clientid_buffer_[DUID_MAX_LEN]; ///< Buffer form of DUID unsigned long clientid_length_; ///< Length of client ID my_bool error_[10]; ///< For error reporting MYSQL_TIME expire_; ///< Lease expiry time const my_bool false_; ///< "false" for MySql - uint8_t hwaddr_buffer_[20]; ///< Hardware address buffer + uint8_t hwaddr_buffer_[HWADDR_MAX_LEN]; ///< Hardware address buffer unsigned long hwaddr_length_; ///< Length of hardware address uint32_t iaid_; ///< Identity association ID Lease6Ptr lease_; ///< Pointer to lease object diff --git a/src/lib/dhcp/mysql_lease_mgr.h b/src/lib/dhcp/mysql_lease_mgr.h index 50ccb72ec5..4a7ba74942 100644 --- a/src/lib/dhcp/mysql_lease_mgr.h +++ b/src/lib/dhcp/mysql_lease_mgr.h @@ -244,7 +244,6 @@ public: /// @exception DbOperationError if the commit failed. virtual void commit(); - /// @brief Rollback Transactions /// /// Rolls back all pending database operations. On databases that don't diff --git a/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc index 03e6a29940..2739a90bb7 100644 --- a/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc +++ b/src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc @@ -68,7 +68,8 @@ public: /// Open the database. MySqlLeaseMgrTest() { - lmptr_ = LeaseMgrFactory::create(validConnectionString()); + LeaseMgrFactory::create(validConnectionString()); + lmptr_ = &(LeaseMgrFactory::instance()); } /// @brief Destructor @@ -78,9 +79,10 @@ public: virtual ~MySqlLeaseMgrTest() { lmptr_->rollback(); + LeaseMgrFactory::destroy(); } - LeaseMgrPtr lmptr_; // Pointer to the lease manager + LeaseMgr* lmptr_; // Pointer to the lease manager }; @@ -92,32 +94,37 @@ public: /// opened: the fixtures assume that and check basic operations. TEST(MySqlOpenTest, OpenDatabase) { - LeaseMgrPtr lmptr; + // Check that attempting to get an instance of the lease manager when + // none is set throws an exception. + EXPECT_THROW(LeaseMgrFactory::instance(), NoLeaseManager); // Check that wrong specification of backend throws an exception. // (This is really a check on LeaseMgrFactory, but is convenient to // perform here.) - EXPECT_THROW(lmptr = LeaseMgrFactory::create(connectionString( + EXPECT_THROW(LeaseMgrFactory::create(connectionString( INVALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD)), - InvalidParameter); + InvalidType); // Check that invalid login data causes an exception. - EXPECT_THROW(lmptr = LeaseMgrFactory::create(connectionString( + EXPECT_THROW(LeaseMgrFactory::create(connectionString( VALID_TYPE, INVALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD)), DbOpenError); - EXPECT_THROW(lmptr = LeaseMgrFactory::create(connectionString( + EXPECT_THROW(LeaseMgrFactory::create(connectionString( VALID_TYPE, VALID_NAME, INVALID_HOST, VALID_USER, VALID_PASSWORD)), DbOpenError); - EXPECT_THROW(lmptr = LeaseMgrFactory::create(connectionString( + EXPECT_THROW(LeaseMgrFactory::create(connectionString( VALID_TYPE, VALID_NAME, VALID_HOST, INVALID_USER, VALID_PASSWORD)), DbOpenError); - EXPECT_THROW(lmptr = LeaseMgrFactory::create(connectionString( + EXPECT_THROW(LeaseMgrFactory::create(connectionString( VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, INVALID_PASSWORD)), DbOpenError); - // Check that database opens correctly and that version is as expected - ASSERT_NO_THROW(lmptr = LeaseMgrFactory::create(validConnectionString())); - ASSERT_TRUE(lmptr); + // Check that database opens correctly. + ASSERT_NO_THROW(LeaseMgrFactory::create(validConnectionString())); + EXPECT_NO_THROW((void) LeaseMgrFactory::instance()); + + // ... and tidy up. + LeaseMgrFactory::destroy(); } /// @brief Check conversion functions