[#93,!51] Addressed review comments.

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
index da74d7fd9a96bac899c1a883ef95c501d49b099e..887001b0cb25fdf5d7e0594f99ece3e2dceb9b0e 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
@@ -301,8 +301,7 @@ public:
             if (!out_bindings[20]->amNull() &&
                 (out_bindings[21]->getInteger<uint32_t>() != 0) &&
                 (out_bindings[22]->getInteger<uint32_t>() != 0) &&
-                ((last_pool_id == 0) ||
-                 (out_bindings[20]->getInteger<uint64_t>() > last_pool_id))) {
+                (out_bindings[20]->getInteger<uint64_t>() > last_pool_id)) {
                 last_pool_id = out_bindings[20]->getInteger<uint64_t>();
                 last_pool.reset(new Pool4(IOAddress(out_bindings[21]->getInteger<uint32_t>()),
                                           IOAddress(out_bindings[22]->getInteger<uint32_t>())));
@@ -311,8 +310,7 @@ public:
 
             // Parse pool specific option.
             if (last_pool && !out_bindings[25]->amNull() &&
-                ((last_pool_option_id == 0) ||
-                (last_pool_option_id < out_bindings[25]->getInteger<uint64_t>()))) {
+                (last_pool_option_id < out_bindings[25]->getInteger<uint64_t>())) {
                 last_pool_option_id = out_bindings[25]->getInteger<uint64_t>();
 
                 OptionDescriptorPtr desc = processOptionRow(Option::V4, out_bindings.begin() + 25);
@@ -323,8 +321,7 @@ public:
 
             // Parse subnet specific option.
             if (!out_bindings[37]->amNull() &&
-                ((last_option_id == 0) ||
-                (last_option_id < out_bindings[37]->getInteger<uint64_t>()))) {
+                (last_option_id < out_bindings[37]->getInteger<uint64_t>())) {
                 last_option_id = out_bindings[37]->getInteger<uint64_t>();
 
                 OptionDescriptorPtr desc = processOptionRow(Option::V4, out_bindings.begin() + 37);
@@ -419,8 +416,7 @@ public:
                           [this, &last_pool_id, &last_pool_option_id, &last_pool,
                            &pools, &pool_ids]
                           (MySqlBindingCollection& out_bindings) {
-            if ((last_pool_id == 0) ||
-                (out_bindings[0]->getInteger<uint64_t>() > last_pool_id)) {
+            if (out_bindings[0]->getInteger<uint64_t>() > last_pool_id) {
 
                 last_pool_id = out_bindings[0]->getInteger<uint64_t>();
 
@@ -432,8 +428,7 @@ public:
 
             // Parse pool specific option.
             if (last_pool && !out_bindings[5]->amNull() &&
-                ((last_pool_option_id == 0) ||
-                (last_pool_option_id < out_bindings[5]->getInteger<uint64_t>()))) {
+                (last_pool_option_id < out_bindings[5]->getInteger<uint64_t>())) {
                 last_pool_option_id = out_bindings[5]->getInteger<uint64_t>();
 
                 OptionDescriptorPtr desc = processOptionRow(Option::V4, out_bindings.begin() + 5);
@@ -693,8 +688,7 @@ public:
             // If this is the first shared network or the shared network id in this
             // row points to the next shared network we use the data in the
             // row to create the new shared network instance.
-            if ((last_network_id == 0) ||
-                (last_network_id != out_bindings[0]->getInteger<uint64_t>())) {
+            if (last_network_id != out_bindings[0]->getInteger<uint64_t>()) {
 
                 last_network_id = out_bindings[0]->getInteger<uint64_t>();
                 last_network.reset(new SharedNetwork4(out_bindings[1]->getString()));
@@ -776,8 +770,7 @@ public:
 
             // Parse option.
             if (!out_bindings[13]->amNull() &&
-                ((last_option_id == 0) ||
-                (last_option_id < out_bindings[13]->getInteger<uint64_t>()))) {
+                (last_option_id < out_bindings[13]->getInteger<uint64_t>())) {
                 last_option_id = out_bindings[13]->getInteger<uint64_t>();
 
                 OptionDescriptorPtr desc = processOptionRow(Option::V4, out_bindings.begin() + 13);

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc
index 0d953956e66296974350e28634df675923b9475b..e32544282d09c23bed81031905045f509e47ef94 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc
@@ -212,10 +212,18 @@ MySqlConfigBackendImpl::getOptions(const int index,
 OptionDescriptorPtr
 MySqlConfigBackendImpl::processOptionRow(const Option::Universe& universe,
                                          MySqlBindingCollection::iterator first_binding) {
-    std::string space = (*(first_binding + 4))->getStringOrDefault(DHCP4_OPTION_SPACE);
+    // Some of the options have standard or custom definitions.
+    // Depending whether the option has a definition or not a different
+    // C++ class may be used to represent the option. Therefore, the
+    // first thing to do is to see if there is a definition for our
+    // parsed option. The option code and space is needed for it.
+    std::string space = (*(first_binding + 4))->getString();
     uint16_t code = (*(first_binding + 1))->getInteger<uint8_t>();
 
+    // See if the option has standard definition.
     OptionDefinitionPtr def = LibDHCP::getOptionDef(space, code);
+    // If there is no definition but the option is vendor specific,
+    // we should search the definition within the vendor option space.
     if (!def && (space != DHCP4_OPTION_SPACE) && (space != DHCP6_OPTION_SPACE)) {
         uint32_t vendor_id = LibDHCP::optionSpaceToVendorId(space);
         if (vendor_id > 0) {
@@ -223,23 +231,30 @@ MySqlConfigBackendImpl::processOptionRow(const Option::Universe& universe,
         }
     }
 
+    // Still haven't found the definition. Check if user has specified
+    // option definition in the server configuration.
     if (!def) {
         def = LibDHCP::getRuntimeOptionDef(space, code);
     }
 
+    // Option can be stored as a blob or formatted value in the configuration.
     std::vector<uint8_t> blob;
     if (!(*(first_binding + 2))->amNull()) {
         blob = (*(first_binding + 2))->getBlob();
     }
     OptionBuffer buf(blob.begin(), blob.end());
 
+    // Get formatted value if available.
     std::string formatted_value = (*(first_binding + 3))->getStringOrDefault("");
 
     OptionPtr option;
     if (!def) {
+        // No option definition. Create generic option instance.
         option.reset(new Option(universe, code, buf.begin(), buf.end()));
 
     } else {
+        // Option definition found. Use formatted value if available or
+        // a blob.
         if (formatted_value.empty()) {
             option = def->optionFactory(universe, code, buf.begin(),
                                         buf.end());
@@ -252,11 +267,14 @@ MySqlConfigBackendImpl::processOptionRow(const Option::Universe& universe,
         }
     }
 
+    // Check if the option is persistent.
     bool persistent = static_cast<bool>((*(first_binding + 5))->getIntegerOrDefault<uint8_t>(0));
 
+    // Create option descriptor which encapsulates our option and adds
+    // additional information, i.e. whether the option is persistent,
+    // its option space and timestamp.
     OptionDescriptorPtr desc(new OptionDescriptor(option, persistent, formatted_value));
     desc->space_name_ = space;
-
     desc->setModificationTime((*(first_binding + 11))->getTimestamp());
 
     return (desc);