Skip to content
GitLab
Commit 0e9d93b9 authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[4281] Addressed review comments.

parent 960a6361
Hide whitespace changes
Inline Side-by-side
src/lib/dhcp/libdhcp++.cc
View file @ 0e9d93b9
......@@ -848,27 +848,23 @@ LibDHCP::initVendorOptsIsc6() {
uint32_t
LibDHCP::optionSpaceToVendorId(const std::string& option_space) {
if (option_space.size() < 8) {
// 8 is a minimal length of "vendor-X" format
return (0);
}
if (option_space.substr(0,7) != "vendor-") {
// 8 is a minimal length of "vendor-X" format
if ((option_space.size() < 8) || (option_space.substr(0,7) != "vendor-")) {
return (0);
}
// text after "vendor-", supposedly numbers only
std::string x = option_space.substr(7);
int64_t check;
try {
// text after "vendor-", supposedly numbers only
std::string x = option_space.substr(7);
check = boost::lexical_cast<int64_t>(x);
} catch (const boost::bad_lexical_cast &) {
return (0);
}
if (check > std::numeric_limits<uint32_t>::max()) {
return (0);
}
if (check < 0) {
if ((check < 0) || (check > std::numeric_limits<uint32_t>::max())) {
return (0);
}
......
src/lib/dhcpsrv/dhcpsrv_messages.mes
View file @ 0e9d93b9
......@@ -539,6 +539,15 @@ information from the MySQL hosts database.
The code has issued a rollback call. All outstanding transaction will
be rolled back and not committed to the database.
% DHCPSRV_MYSQL_START_TRANSACTION starting new MySQL transaction
A debug message issued whena new MySQL transaction is being started.
This message is typically not issued when inserting data into a
single table because the server doesn't explicitly start
transactions in this case. This message is issued when data is
inserted into multiple tables with multiple INSERT statements
and there may be a need to rollback the whole transaction if
any of these INSERT statements fail.
% DHCPSRV_MYSQL_UPDATE_ADDR4 updating IPv4 lease for address %1
A debug message issued when the server is attempting to update IPv4
lease from the MySQL database for the specified address.
......
src/lib/dhcpsrv/mysql_connection.cc
View file @ 0e9d93b9
......@@ -30,13 +30,7 @@ const int MLM_MYSQL_FETCH_FAILURE = 1;
MySqlTransaction::MySqlTransaction(MySqlConnection& conn)
: conn_(conn), committed_(false) {
// We create prepared statements for all other queries, but MySQL
// don't support prepared statements for START TRANSACTION.
int status = mysql_query(conn_.mysql_, "START TRANSACTION");
if (status != 0) {
isc_throw(DbOperationError, "unable to start transaction, "
"reason: " << mysql_error(conn_.mysql_));
}
conn_.startTransaction();
}
MySqlTransaction::~MySqlTransaction() {
......@@ -284,20 +278,35 @@ MySqlConnection::convertFromDatabaseTime(const MYSQL_TIME& expire,
cltt = mktime(&expire_tm) - valid_lifetime;
}
void MySqlConnection::commit() {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_MYSQL_COMMIT);
if (mysql_commit(mysql_) != 0) {
isc_throw(DbOperationError, "commit failed: "
<< mysql_error(mysql_));
}
void
MySqlConnection::startTransaction() {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
DHCPSRV_MYSQL_START_TRANSACTION);
// We create prepared statements for all other queries, but MySQL
// don't support prepared statements for START TRANSACTION.
int status = mysql_query(mysql_, "START TRANSACTION");
if (status != 0) {
isc_throw(DbOperationError, "unable to start transaction, "
"reason: " << mysql_error(mysql_));
}
}
void MySqlConnection::rollback() {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_MYSQL_ROLLBACK);
if (mysql_rollback(mysql_) != 0) {
isc_throw(DbOperationError, "rollback failed: "
<< mysql_error(mysql_));
}
void
MySqlConnection::commit() {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_MYSQL_COMMIT);
if (mysql_commit(mysql_) != 0) {
isc_throw(DbOperationError, "commit failed: "
<< mysql_error(mysql_));
}
}
void
MySqlConnection::rollback() {
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_MYSQL_ROLLBACK);
if (mysql_rollback(mysql_) != 0) {
isc_throw(DbOperationError, "rollback failed: "
<< mysql_error(mysql_));
}
}
......
src/lib/dhcpsrv/mysql_connection.h
View file @ 0e9d93b9
......@@ -316,6 +316,9 @@ public:
uint32_t valid_lifetime, time_t& cltt);
///@}
/// @brief Starts Transaction
void startTransaction();
/// @brief Commit Transactions
///
/// Commits all pending database operations. On databases that don't
......
src/lib/dhcpsrv/mysql_host_data_source.cc
View file @ 0e9d93b9
......@@ -1791,15 +1791,15 @@ public:
/// @brief Destructor.
~MySqlHostDataSourceImpl();
/// @brief Executes query which inserts a row into one of the tables.
/// @brief Executes statements which inserts a row into one of the tables.
///
/// @param stindex Index of a statement being executed.
/// @param bind Vector of MYSQL_BIND objects to be used when making the
/// query.
///
/// @throw isc::dhcp::DuplicateEntry Database throws duplicate entry error
void addQuery(MySqlHostDataSource::StatementIndex stindex,
std::vector<MYSQL_BIND>& bind);
void addStatement(MySqlHostDataSource::StatementIndex stindex,
std::vector<MYSQL_BIND>& bind);
/// @brief Inserts IPv6 Reservation into ipv6_reservation table.
///
......@@ -1935,12 +1935,12 @@ MySqlHostDataSourceImpl(const MySqlConnection::ParameterMap& parameters)
// Open the database.
conn_.openDatabase();
// Enable autocommit. To avoid a flush to disk on every commit, the global
// Disable autocommit. To avoid a flush to disk on every commit, the global
// parameter innodb_flush_log_at_trx_commit should be set to 2. This will
// cause the changes to be written to the log, but flushed to disk in the
// background every second. Setting the parameter to that value will speed
// up the system, but at the risk of losing data if the system crashes.
my_bool result = mysql_autocommit(conn_.mysql_, 1);
my_bool result = mysql_autocommit(conn_.mysql_, 0);
if (result != 0) {
isc_throw(DbOperationError, mysql_error(conn_.mysql_));
}
......@@ -1966,8 +1966,8 @@ MySqlHostDataSourceImpl::~MySqlHostDataSourceImpl() {
}
void
MySqlHostDataSourceImpl::addQuery(MySqlHostDataSource::StatementIndex stindex,
std::vector<MYSQL_BIND>& bind) {
MySqlHostDataSourceImpl::addStatement(MySqlHostDataSource::StatementIndex stindex,
std::vector<MYSQL_BIND>& bind) {
// Bind the parameters to the statement
int status = mysql_stmt_bind_param(conn_.statements_[stindex], &bind[0]);
......@@ -1991,7 +1991,7 @@ MySqlHostDataSourceImpl::addResv(const IPv6Resrv& resv,
std::vector<MYSQL_BIND> bind =
host_ipv6_reservation_exchange_->createBindForSend(resv, id);
addQuery(MySqlHostDataSource::INSERT_V6_RESRV, bind);
addStatement(MySqlHostDataSource::INSERT_V6_RESRV, bind);
}
void
......@@ -2004,7 +2004,7 @@ MySqlHostDataSourceImpl::addOption(const MySqlHostDataSource::StatementIndex& st
host_option_exchange_->createBindForSend(opt_desc, opt_space,
subnet_id, id);
addQuery(stindex, bind);
addStatement(stindex, bind);
}
void
......@@ -2178,22 +2178,12 @@ MySqlHostDataSource::add(const HostPtr& host) {
// Create the MYSQL_BIND array for the host
std::vector<MYSQL_BIND> bind = impl_->host_exchange_->createBindForSend(host);
// ... and call addHost() code.
impl_->addQuery(INSERT_HOST, bind);
// ... and insert the host.
impl_->addStatement(INSERT_HOST, bind);
// Gets the last inserted hosts id
uint64_t host_id = 0;
// Insert IPv6 reservations.
IPv6ResrvRange v6resv = host->getIPv6Reservations();
if (std::distance(v6resv.first, v6resv.second) > 0) {
host_id = mysql_insert_id(impl_->conn_.mysql_);
for (IPv6ResrvIterator resv = v6resv.first; resv != v6resv.second;
++resv) {
impl_->addResv(resv->second, host_id);
}
}
// Insert DHCPv4 options.
ConstCfgOptionPtr cfg_option4 = host->getCfgOption4();
if (cfg_option4) {
......@@ -2206,6 +2196,19 @@ MySqlHostDataSource::add(const HostPtr& host) {
impl_->addOptions(INSERT_V6_OPTION, cfg_option6, host_id);
}
// Insert IPv6 reservations.
IPv6ResrvRange v6resv = host->getIPv6Reservations();
if (std::distance(v6resv.first, v6resv.second) > 0) {
// Set host_id if it wasn't set when we inserted options.
if (host_id == 0) {
host_id = mysql_insert_id(impl_->conn_.mysql_);
}
for (IPv6ResrvIterator resv = v6resv.first; resv != v6resv.second;
++resv) {
impl_->addResv(resv->second, host_id);
}
}
// Everything went fine, so explicitly commit the transaction.
transaction.commit();
}
......
src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
View file @ 0e9d93b9
......@@ -99,8 +99,8 @@ GenericHostDataSourceTest::initializeHost4(const std::string& address,
// subnet4 with subnet6.
static SubnetID subnet4 = 0;
static SubnetID subnet6 = 100;
subnet4++;
subnet6++;
++subnet4;
++subnet6;
IOAddress addr(address);
HostPtr host(new Host(&ident[0], ident.size(), id, subnet4, subnet6, addr));
......@@ -346,6 +346,11 @@ GenericHostDataSourceTest::compareOptions(const ConstCfgOptionPtr& cfg1,
option_spaces.insert(option_spaces.end(), vendor_spaces.begin(),
vendor_spaces.end());
// Make sure that the number of option spaces is equal in both
// configurations.
EXPECT_EQ(option_spaces.size(), cfg1->getOptionSpaceNames().size());
EXPECT_EQ(vendor_spaces.size(), cfg1->getVendorIdsSpaceNames().size());
// Iterate over all option spaces existing in cfg2.
BOOST_FOREACH(std::string space, option_spaces) {
// Retrieve options belonging to the current option space.
......@@ -428,66 +433,72 @@ GenericHostDataSourceTest::createVendorOption(const Option::Universe& universe,
void
GenericHostDataSourceTest::addTestOptions(const HostPtr& host,
const bool formatted) const {
// Add DHCPv4 options.
CfgOptionPtr opts = host->getCfgOption4();
opts->add(createOption<OptionString>(Option::V4, DHO_BOOT_FILE_NAME,
true, formatted, "my-boot-file"),
DHCP4_OPTION_SPACE);
opts->add(createOption<OptionUint8>(Option::V4, DHO_DEFAULT_IP_TTL,
false, formatted, 64),
DHCP4_OPTION_SPACE);
opts->add(createOption<OptionUint32>(Option::V4, 1, false, formatted, 312131),
"vendor-encapsulated-options");
opts->add(createAddressOption<Option4AddrLst>(254, false, formatted, "192.0.2.3"),
DHCP4_OPTION_SPACE);
opts->add(createEmptyOption(Option::V4, 1, true), "isc");
opts->add(createAddressOption<Option4AddrLst>(2, false, formatted, "10.0.0.5",
"10.0.0.3", "10.0.3.4"),
"isc");
const bool formatted,
const AddedOptions& added_options) const {
OptionDefSpaceContainer defs;
// Add definitions for DHCPv4 non-standard options.
defs.addItem(OptionDefinitionPtr(new OptionDefinition("vendor-encapsulated-1",
1, "uint32")),
"vendor-encapsulated-options");
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-254", 254,
"ipv4-address", true)),
DHCP4_OPTION_SPACE);
defs.addItem(OptionDefinitionPtr(new OptionDefinition("isc-1", 1, "empty")),
"isc");
defs.addItem(OptionDefinitionPtr(new OptionDefinition("isc-2", 2,
"ipv4-address", true)),
"isc");
// Add DHCPv6 options.
opts = host->getCfgOption6();
opts->add(createOption<OptionString>(Option::V6, D6O_BOOTFILE_URL,
true, formatted, "my-boot-file"),
DHCP6_OPTION_SPACE);
opts->add(createOption<OptionUint32>(Option::V6, D6O_INFORMATION_REFRESH_TIME,
false, formatted, 3600),
DHCP6_OPTION_SPACE);
opts->add(createVendorOption(Option::V6, false, formatted, 2495),
DHCP6_OPTION_SPACE);
opts->add(createAddressOption<Option6AddrLst>(1024, false, formatted,
"2001:db8:1::1"),
DHCP6_OPTION_SPACE);
opts->add(createEmptyOption(Option::V6, 1, true), "isc2");
opts->add(createAddressOption<Option6AddrLst>(2, false, formatted, "3000::1",
"3000::2", "3000::3"),
"isc2");
// Add definitions for DHCPv6 non-standard options.
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-1024", 1024,
"ipv6-address", true)),
DHCP6_OPTION_SPACE);
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-1", 1, "empty")),
"isc2");
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-2", 2,
"ipv6-address", true)),
"isc2");
if ((added_options == DHCP4_ONLY) || (added_options == DHCP4_AND_DHCP6)) {
// Add DHCPv4 options.
CfgOptionPtr opts = host->getCfgOption4();
opts->add(createOption<OptionString>(Option::V4, DHO_BOOT_FILE_NAME,
true, formatted, "my-boot-file"),
DHCP4_OPTION_SPACE);
opts->add(createOption<OptionUint8>(Option::V4, DHO_DEFAULT_IP_TTL,
false, formatted, 64),
DHCP4_OPTION_SPACE);
opts->add(createOption<OptionUint32>(Option::V4, 1, false, formatted, 312131),
"vendor-encapsulated-options");
opts->add(createAddressOption<Option4AddrLst>(254, false, formatted, "192.0.2.3"),
DHCP4_OPTION_SPACE);
opts->add(createEmptyOption(Option::V4, 1, true), "isc");
opts->add(createAddressOption<Option4AddrLst>(2, false, formatted, "10.0.0.5",
"10.0.0.3", "10.0.3.4"),
"isc");
// Add definitions for DHCPv4 non-standard options.
defs.addItem(OptionDefinitionPtr(new OptionDefinition("vendor-encapsulated-1",
1, "uint32")),
"vendor-encapsulated-options");
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-254", 254,
"ipv4-address", true)),
DHCP4_OPTION_SPACE);
defs.addItem(OptionDefinitionPtr(new OptionDefinition("isc-1", 1, "empty")),
"isc");
defs.addItem(OptionDefinitionPtr(new OptionDefinition("isc-2", 2,
"ipv4-address", true)),
"isc");
}
if ((added_options == DHCP6_ONLY) || (added_options == DHCP4_AND_DHCP6)) {
// Add DHCPv6 options.
CfgOptionPtr opts = host->getCfgOption6();
opts->add(createOption<OptionString>(Option::V6, D6O_BOOTFILE_URL,
true, formatted, "my-boot-file"),
DHCP6_OPTION_SPACE);
opts->add(createOption<OptionUint32>(Option::V6, D6O_INFORMATION_REFRESH_TIME,
false, formatted, 3600),
DHCP6_OPTION_SPACE);
opts->add(createVendorOption(Option::V6, false, formatted, 2495),
DHCP6_OPTION_SPACE);
opts->add(createAddressOption<Option6AddrLst>(1024, false, formatted,
"2001:db8:1::1"),
DHCP6_OPTION_SPACE);
opts->add(createEmptyOption(Option::V6, 1, true), "isc2");
opts->add(createAddressOption<Option6AddrLst>(2, false, formatted, "3000::1",
"3000::2", "3000::3"),
"isc2");
// Add definitions for DHCPv6 non-standard options.
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-1024", 1024,
"ipv6-address", true)),
DHCP6_OPTION_SPACE);
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-1", 1, "empty")),
"isc2");
defs.addItem(OptionDefinitionPtr(new OptionDefinition("option-2", 2,
"ipv6-address", true)),
"isc2");
}
// Register created "runtime" option definitions. They will be used by a
// host data source to convert option data into the appropriate option
......@@ -1083,7 +1094,7 @@ void GenericHostDataSourceTest::testMultipleReservationsDifferentOrder(){
void GenericHostDataSourceTest::testOptionsReservations4(const bool formatted) {
HostPtr host = initializeHost4("192.0.2.5", Host::IDENT_HWADDR);
// Add a bunch of DHCPv4 and DHCPv6 options for the host.
ASSERT_NO_THROW(addTestOptions(host, formatted));
ASSERT_NO_THROW(addTestOptions(host, formatted, DHCP4_ONLY));
// Insert host and the options into respective tables.
ASSERT_NO_THROW(hdsptr_->add(host));
// Subnet id will be used in quries to the database.
......@@ -1104,14 +1115,12 @@ void GenericHostDataSourceTest::testOptionsReservations4(const bool formatted) {
// get4(subnet_id, address)
ConstHostPtr host_by_addr = hdsptr_->get4(subnet_id, IOAddress("192.0.2.5"));
ASSERT_NO_FATAL_FAILURE(compareHosts(host, host_by_addr));
hdsptr_->get4(subnet_id, IOAddress("192.0.2.5"));
}
void GenericHostDataSourceTest::testOptionsReservations6(const bool formatted) {
HostPtr host = initializeHost6("2001:db8::1", Host::IDENT_DUID, false);
// Add a bunch of DHCPv4 and DHCPv6 options for the host.
ASSERT_NO_THROW(addTestOptions(host, formatted));
ASSERT_NO_THROW(addTestOptions(host, formatted, DHCP6_ONLY));
// Insert host, options and IPv6 reservations into respective tables.
ASSERT_NO_THROW(hdsptr_->add(host));
// Subnet id will be used in queries to the database.
......@@ -1132,7 +1141,7 @@ void GenericHostDataSourceTest::testOptionsReservations46(const bool formatted)
HostPtr host = initializeHost6("2001:db8::1", Host::IDENT_HWADDR, false);
// Add a bunch of DHCPv4 and DHCPv6 options for the host.
ASSERT_NO_THROW(addTestOptions(host, formatted));
ASSERT_NO_THROW(addTestOptions(host, formatted, DHCP4_AND_DHCP6));
// Insert host, options and IPv6 reservations into respective tables.
ASSERT_NO_THROW(hdsptr_->add(host));
// Subnet id will be used in queries to the database.
......
src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h
View file @ 0e9d93b9
......@@ -35,6 +35,16 @@ public:
V6
};
/// @brief Options to be inserted into a host.
///
/// Parameter of this type is passed to the @ref addTestOptions to
/// control which option types should be inserted into a host.
enum AddedOptions {
DHCP4_ONLY,
DHCP6_ONLY,
DHCP4_AND_DHCP6
};
/// @brief Default constructor.
GenericHostDataSourceTest();
......@@ -319,7 +329,10 @@ public:
/// @param host Host object into which options should be added.
/// @param formatted A boolean value selecting if the formatted option
/// value should be used (if true), or binary value (if false).
void addTestOptions(const HostPtr& host, const bool formatted) const;
/// @param added_options Controls which options should be inserted into
/// a host: DHCPv4, DHCPv6 options or both.
void addTestOptions(const HostPtr& host, const bool formatted,
const AddedOptions& added_options) const;
/// @brief Pointer to the host data source
HostDataSourcePtr hdsptr_;
......
Supports Markdown
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