Skip to content

GitLab

Commit 39c82ec5 authored by Stephen Morris's avatar Stephen Morris
Browse files

[2342] Address remaining issues in review comment 6.

parent 3e52b2fe
Hide whitespace changes
Inline Side-by-side
src/lib/dhcp/Makefile.am
View file @ 39c82ec5
......@@ -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
......
src/lib/dhcp/lease_mgr_factory.h
View file @ 39c82ec5
......@@ -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.
......
src/lib/dhcp/mysql_lease_mgr.cc
View file @ 39c82ec5
......@@ -22,6 +22,7 @@
#include <dhcp/mysql_lease_mgr.h>
#include <asiolink/io_address.h>
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<char*>(&lease_type_);
bind_[7].is_unsigned = static_cast<my_bool>(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<char*>(&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<char*>(&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<int>(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_));
}
......
src/lib/dhcp/mysql_lease_mgr.h
View file @ 39c82ec5
......@@ -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<uint32_t, uint32_t> 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<std::string> raw_statements_; ///< Raw text of statements
std::vector<std::string> text_statements_; ///< Raw text of statements
std::vector<MYSQL_STMT*> statements_; ///< Prepared statements
};
......
src/lib/dhcp/tests/Makefile.am
View file @ 39c82ec5
......@@ -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
......
src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc
View file @ 39c82ec5
......@@ -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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment