Browse Source

[2342] Address review changes (in comment:5 of the ticket)

Stephen Morris 12 years ago
parent
commit
b0ed08d214

+ 5 - 10
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

+ 1 - 1
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

+ 46 - 22
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.

+ 3 - 6
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<LeaseMgr> LeaseMgrPtr;
-
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
 

+ 26 - 3
src/lib/dhcp/lease_mgr_factory.cc

@@ -23,6 +23,7 @@
 #include <utility>
 
 #include <boost/foreach.hpp>
+#include <boost/scoped_ptr.hpp>
 #include <boost/algorithm/string.hpp>
 #include <exceptions/exceptions.h>
 #include <dhcp/lease_mgr_factory.h>
@@ -36,6 +37,12 @@ using namespace std;
 namespace isc {
 namespace dhcp {
 
+boost::scoped_ptr<LeaseMgr>&
+LeaseMgrFactory::getLeaseMgrPtr() {
+    static boost::scoped_ptr<LeaseMgr> 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

+ 48 - 5
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<LeaseMgr>& getLeaseMgrPtr();
+
 };
 
 }; // end of isc::dhcp namespace

+ 23 - 3
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<uint8_t> 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

+ 0 - 1
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

+ 19 - 12
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