From: Marcin Siodelski Date: Wed, 25 May 2016 13:28:41 +0000 (+0200) Subject: [4281] Addressed review comments. X-Git-Tag: trac4106_update_base~7^2~3^2~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0e9d93b91f064693ca01e1484b287e1dbeb11c09;p=thirdparty%2Fkea.git [4281] Addressed review comments. --- diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index 3b746ef9b8..eeebcf5da7 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -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(x); + } catch (const boost::bad_lexical_cast &) { return (0); } - if (check > std::numeric_limits::max()) { - return (0); - } - if (check < 0) { + + if ((check < 0) || (check > std::numeric_limits::max())) { return (0); } diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 712dab52fa..4821a4342b 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -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. diff --git a/src/lib/dhcpsrv/mysql_connection.cc b/src/lib/dhcpsrv/mysql_connection.cc index 6da2dae458..432805c34e 100755 --- a/src/lib/dhcpsrv/mysql_connection.cc +++ b/src/lib/dhcpsrv/mysql_connection.cc @@ -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_)); + } } diff --git a/src/lib/dhcpsrv/mysql_connection.h b/src/lib/dhcpsrv/mysql_connection.h index 09dfe72303..636ae700f4 100755 --- a/src/lib/dhcpsrv/mysql_connection.h +++ b/src/lib/dhcpsrv/mysql_connection.h @@ -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 diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index 3556865a1e..9e0c425b92 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc @@ -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& bind); + void addStatement(MySqlHostDataSource::StatementIndex stindex, + std::vector& 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& bind) { +MySqlHostDataSourceImpl::addStatement(MySqlHostDataSource::StatementIndex stindex, + std::vector& 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 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 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(); } diff --git a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc index 415cb7a4a2..ba5971c426 100644 --- a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc @@ -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(Option::V4, DHO_BOOT_FILE_NAME, - true, formatted, "my-boot-file"), - DHCP4_OPTION_SPACE); - opts->add(createOption(Option::V4, DHO_DEFAULT_IP_TTL, - false, formatted, 64), - DHCP4_OPTION_SPACE); - opts->add(createOption(Option::V4, 1, false, formatted, 312131), - "vendor-encapsulated-options"); - opts->add(createAddressOption(254, false, formatted, "192.0.2.3"), - DHCP4_OPTION_SPACE); - opts->add(createEmptyOption(Option::V4, 1, true), "isc"); - opts->add(createAddressOption(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(Option::V6, D6O_BOOTFILE_URL, - true, formatted, "my-boot-file"), - DHCP6_OPTION_SPACE); - opts->add(createOption(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(1024, false, formatted, - "2001:db8:1::1"), - DHCP6_OPTION_SPACE); - opts->add(createEmptyOption(Option::V6, 1, true), "isc2"); - opts->add(createAddressOption(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(Option::V4, DHO_BOOT_FILE_NAME, + true, formatted, "my-boot-file"), + DHCP4_OPTION_SPACE); + opts->add(createOption(Option::V4, DHO_DEFAULT_IP_TTL, + false, formatted, 64), + DHCP4_OPTION_SPACE); + opts->add(createOption(Option::V4, 1, false, formatted, 312131), + "vendor-encapsulated-options"); + opts->add(createAddressOption(254, false, formatted, "192.0.2.3"), + DHCP4_OPTION_SPACE); + opts->add(createEmptyOption(Option::V4, 1, true), "isc"); + opts->add(createAddressOption(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(Option::V6, D6O_BOOTFILE_URL, + true, formatted, "my-boot-file"), + DHCP6_OPTION_SPACE); + opts->add(createOption(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(1024, false, formatted, + "2001:db8:1::1"), + DHCP6_OPTION_SPACE); + opts->add(createEmptyOption(Option::V6, 1, true), "isc2"); + opts->add(createAddressOption(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. diff --git a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h index 1e8bb75845..cf5421b19e 100644 --- a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h +++ b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h @@ -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_;