]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[4281] Addressed review comments.
authorMarcin Siodelski <marcin@isc.org>
Wed, 25 May 2016 13:28:41 +0000 (15:28 +0200)
committerMarcin Siodelski <marcin@isc.org>
Wed, 25 May 2016 13:28:41 +0000 (15:28 +0200)
src/lib/dhcp/libdhcp++.cc
src/lib/dhcpsrv/dhcpsrv_messages.mes
src/lib/dhcpsrv/mysql_connection.cc
src/lib/dhcpsrv/mysql_connection.h
src/lib/dhcpsrv/mysql_host_data_source.cc
src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h

index 3b746ef9b8db76a279752bd7036d1bf4bf611de3..eeebcf5da7f373fb82bd1d68832a744c87faf5b7 100644 (file)
@@ -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);
     }
 
index 712dab52fae8a7eb9601aeb39a807b3501229616..4821a4342b34d3a6f5faec90361102269833413f 100644 (file)
@@ -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.
index 6da2dae458dce4f5c39a31fecc51c65603801aca..432805c34e2ccc346435d76d0acd0f3be0ef0f97 100755 (executable)
@@ -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_));
+    }
 }
 
 
index 09dfe7230385b8bc88a57887cca79ab4c4b91a46..636ae700f414d996d6b8c320733e244669e2f52e 100755 (executable)
@@ -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
index 3556865a1e0ff021bce5e1af18e3e48bb087efdf..9e0c425b92b6c24510c95f43f922e8462c457f70 100644 (file)
@@ -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();
 }
index 415cb7a4a226ab9a6e8ad2c3611a5363655b9812..ba5971c426b47445b57f8d0ea1c1eb2d3dadfff0 100644 (file)
@@ -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.
index 1e8bb758451550e4496df154835b860ede6a35f2..cf5421b19e288527437d1b2b9e23ed8d9130f726 100644 (file)
@@ -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_;