[master] Merge branch 'trac2637'

Conflicts:
	src/bin/dhcp4/config_parser.cc
	src/bin/dhcp6/config_parser.cc
	src/lib/dhcpsrv/dhcpsrv_messages.mes
	src/lib/dhcpsrv/memfile_lease_mgr.cc

src/bin/dhcp4/config_parser.cc

@@ -743,32 +743,33 @@ private:
     void createOption() {
         // Option code is held in the uint32_t storage but is supposed to
         // be uint16_t value. We need to check that value in the configuration
-        // does not exceed range of uint16_t and is not zero.
+        // does not exceed range of uint8_t and is not zero.
         uint32_t option_code = getParam<uint32_t>("code", uint32_values_);
         if (option_code == 0) {
-            isc_throw(DhcpConfigError, "Parser error: value of 'code' must not"
-                      << " be equal to zero. Option code '0' is reserved in"
-                      << " DHCPv4.");
-        } else if (option_code > std::numeric_limits<uint16_t>::max()) {
-            isc_throw(DhcpConfigError, "Parser error: value of 'code' must not"
-                      << " exceed " << std::numeric_limits<uint16_t>::max());
+            isc_throw(DhcpConfigError, "option code must not be zero."
+                      << " Option code '0' is reserved in DHCPv4.");
+        } else if (option_code > std::numeric_limits<uint8_t>::max()) {
+            isc_throw(DhcpConfigError, "invalid option code '" << option_code
+                      << "', it must not exceed '"
+                      << std::numeric_limits<uint8_t>::max() << "'");
         }
         // Check that the option name has been specified, is non-empty and does not
         // contain spaces.
-        // @todo possibly some more restrictions apply here?
         std::string option_name = getParam<std::string>("name", string_values_);
         if (option_name.empty()) {
-            isc_throw(DhcpConfigError, "Parser error: option name must not be"
-                      << " empty");
+            isc_throw(DhcpConfigError, "name of the option with code '"
+                      << option_code << "' is empty");
         } else if (option_name.find(" ") != std::string::npos) {
-            isc_throw(DhcpConfigError, "Parser error: option name must not contain"
-                      << " spaces");
+            isc_throw(DhcpConfigError, "invalid option name '" << option_name
+                      << "', space character is not allowed");
         }
 
         std::string option_space = getParam<std::string>("space", string_values_);
         if (!OptionSpace::validateName(option_space)) {
-            isc_throw(DhcpConfigError, "invalid option space name'"
-                      << option_space << "'");
+            isc_throw(DhcpConfigError, "invalid option space name '"
+                      << option_space << "' specified for option '"
+                      << option_name << "' (code '" << option_code
+                      << "')");
         }
 
         OptionDefinitionPtr def;
@@ -823,7 +824,7 @@ private:
             try {
                 util::encode::decodeHex(option_data, binary);
             } catch (...) {
-                isc_throw(DhcpConfigError, "Parser error: option data is not a valid"
+                isc_throw(DhcpConfigError, "option data is not a valid"
                           << " string of hexadecimal digits: " << option_data);
             }
         }

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -47,18 +47,61 @@ namespace dhcp {
 ControlledDhcpv4Srv* ControlledDhcpv4Srv::server_ = NULL;
 
 ConstElementPtr
+ControlledDhcpv4Srv::dhcp4StubConfigHandler(ConstElementPtr) {
+    // This configuration handler is intended to be used only
+    // when the initial configuration comes in. To receive this
+    // configuration a pointer to this handler must be passed
+    // using ModuleCCSession constructor. This constructor will
+    // invoke the handler and will store the configuration for
+    // the configuration session when the handler returns success.
+    // Since this configuration is partial we just pretend to
+    // parse it and always return success. The function that
+    // initiates the session must get the configuration on its
+    // own using getFullConfig.
+    return (isc::config::createAnswer(0, "Configuration accepted."));
+}
+
+ConstElementPtr
 ControlledDhcpv4Srv::dhcp4ConfigHandler(ConstElementPtr new_config) {
-    LOG_DEBUG(dhcp4_logger, DBG_DHCP4_COMMAND, DHCP4_CONFIG_UPDATE)
-              .arg(new_config->str());
-    if (server_) {
-        return (configureDhcp4Server(*server_, new_config));
+    if (!server_ || !server_->config_session_) {
+        // That should never happen as we install config_handler
+        // after we instantiate the server.
+        ConstElementPtr answer =
+            isc::config::createAnswer(1, "Configuration rejected,"
+                                      " server is during startup/shutdown phase.");
+        return (answer);
     }
 
-    // That should never happen as we install config_handler after we instantiate
-    // the server.
-    ConstElementPtr answer = isc::config::createAnswer(1,
-           "Configuration rejected, server is during startup/shutdown phase.");
-    return (answer);
+    // The configuration passed to this handler function is partial.
+    // In other words, it just includes the values being modified.
+    // In the same time, there are dependencies between various
+    // DHCP configuration parsers. For example: the option value can
+    // be set if the definition of this option is set. If someone removes
+    // an existing option definition then the partial configuration that
+    // removes that definition is triggered while a relevant option value
+    // may remain configured. This eventually results in the DHCP server
+    // configuration being in the inconsistent state.
+    // In order to work around this problem we need to merge the new
+    // configuration with the existing (full) configuration.
+
+    // Let's create a new object that will hold the merged configuration.
+    boost::shared_ptr<MapElement> merged_config(new MapElement());
+    // Let's get the existing configuration.
+    ConstElementPtr full_config = server_->config_session_->getFullConfig();
+    // The full_config and merged_config should be always non-NULL
+    // but to provide some level of exception safety we check that they
+    // really are (in case we go out of memory).
+    if (full_config && merged_config) {
+        merged_config->setValue(full_config->mapValue());
+
+        // Merge an existing and new configuration.
+        isc::data::merge(merged_config, new_config);
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_COMMAND, DHCP4_CONFIG_UPDATE)
+            .arg(full_config->str());
+    }
+
+    // Configure the server.
+    return (configureDhcp4Server(*server_, merged_config));
 }
 
 ConstElementPtr
@@ -109,8 +152,15 @@ void ControlledDhcpv4Srv::establishSession() {
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_CCSESSION_STARTING)
               .arg(specfile);
     cc_session_ = new Session(io_service_.get_io_service());
+    // Create a session with the dummy configuration handler.
+    // Dumy configuration handler is internally invoked by the
+    // constructor and on success the constructor updates
+    // the current session with the configuration that had been
+    // commited in the previous session. If we did not install
+    // the dummy handler, the previous configuration would have
+    // been lost.
     config_session_ = new ModuleCCSession(specfile, *cc_session_,
-                                          NULL,
+                                          dhcp4StubConfigHandler,
                                           dhcp4CommandHandler, false);
     config_session_->start();
 

src/bin/dhcp4/ctrl_dhcp4_srv.h

@@ -94,6 +94,27 @@ protected:
     static isc::data::ConstElementPtr
     dhcp4ConfigHandler(isc::data::ConstElementPtr new_config);
 
+    /// @brief A dummy configuration handler that always returns success.
+    ///
+    /// This configuration handler does not perform configuration
+    /// parsing and always returns success. A dummy hanlder should
+    /// be installed using \ref isc::config::ModuleCCSession ctor
+    /// to get the initial configuration. This initial configuration
+    /// comprises values for only those elements that were modified
+    /// the previous session. The \ref dhcp4ConfigHandler can't be
+    /// used to parse the initial configuration because it needs the
+    /// full configuration to satisfy dependencies between the
+    /// various configuration values. Installing the dummy handler
+    /// that guarantees to return success causes initial configuration
+    /// to be stored for the session being created and that it can
+    /// be later accessed with \ref isc::ConfigData::getFullConfig.
+    ///
+    /// @param new_config new configuration.
+    ///
+    /// @return success configuration status.
+    static isc::data::ConstElementPtr
+    dhcp4StubConfigHandler(isc::data::ConstElementPtr new_config);
+
     /// @brief A callback for handling incoming commands.
     ///
     /// @param command textual representation of the command

src/bin/dhcp4/dhcp4.spec

@@ -70,7 +70,7 @@
             "item_default": False
           },
 
-          { "item_name": "record_types",
+          { "item_name": "record-types",
             "item_type": "string",
             "item_optional": false,
             "item_default": ""

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -347,7 +347,6 @@ TEST_F(Dhcp4ParserTest, subnetGlobalDefaults) {
         "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
         "    \"subnet\": \"192.0.2.0/24\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
 
     ElementPtr json = Element::fromJSON(config);
 
@@ -381,7 +380,6 @@ TEST_F(Dhcp4ParserTest, subnetLocal) {
         "    \"valid-lifetime\": 4,"
         "    \"subnet\": \"192.0.2.0/24\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
 
     ElementPtr json = Element::fromJSON(config);
 
@@ -410,7 +408,6 @@ TEST_F(Dhcp4ParserTest, poolOutOfSubnet) {
         "    \"pool\": [ \"192.0.4.0/28\" ],"
         "    \"subnet\": \"192.0.2.0/24\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
 
     ElementPtr json = Element::fromJSON(config);
 
@@ -435,7 +432,6 @@ TEST_F(Dhcp4ParserTest, poolPrefixLen) {
         "    \"pool\": [ \"192.0.2.128/28\" ],"
         "    \"subnet\": \"192.0.2.0/24\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
 
     ElementPtr json = Element::fromJSON(config);
 

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -890,7 +890,6 @@ TEST_F(Dhcpv4SrvTest, RenewBasic) {
 
     // let's create a lease and put it in the LeaseMgr
     uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
-    uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
     Lease4Ptr used(new Lease4(IOAddress("192.0.2.106"), hwaddr2, sizeof(hwaddr2),
                               &client_id_->getDuid()[0], client_id_->getDuid().size(),
                               temp_valid, temp_t1, temp_t2, temp_timestamp,

src/bin/dhcp6/config_parser.cc

@@ -776,29 +776,30 @@ private:
         // does not exceed range of uint16_t and is not zero.
         uint32_t option_code = getParam<uint32_t>("code", uint32_values_);
         if (option_code == 0) {
-            isc_throw(DhcpConfigError, "Parser error: value of 'code' must not"
-                      << " be equal to zero. Option code '0' is reserved in"
-                      << " DHCPv6.");
+            isc_throw(DhcpConfigError, "option code must not be zero."
+                      << " Option code '0' is reserved in DHCPv6.");
         } else if (option_code > std::numeric_limits<uint16_t>::max()) {
-            isc_throw(DhcpConfigError, "Parser error: value of 'code' must not"
-                      << " exceed " << std::numeric_limits<uint16_t>::max());
+            isc_throw(DhcpConfigError, "invalid option code '" << option_code
+                      << "', it must not exceed '"
+                      << std::numeric_limits<uint16_t>::max() << "'");
         }
         // Check that the option name has been specified, is non-empty and does not
         // contain spaces.
-        // @todo possibly some more restrictions apply here?
         std::string option_name = getParam<std::string>("name", string_values_);
         if (option_name.empty()) {
-            isc_throw(DhcpConfigError, "Parser error: option name must not be"
-                      << " empty");
+            isc_throw(DhcpConfigError, "name of the option with code '"
+                      << option_code << "' is empty");
         } else if (option_name.find(" ") != std::string::npos) {
-            isc_throw(DhcpConfigError, "Parser error: option name must not contain"
-                      << " spaces");
+            isc_throw(DhcpConfigError, "invalid option name '" << option_name
+                      << "', space character is not allowed");
         }
 
         std::string option_space = getParam<std::string>("space", string_values_);
         if (!OptionSpace::validateName(option_space)) {
-            isc_throw(DhcpConfigError, "invalid option space name'"
-                      << option_space << "'");
+            isc_throw(DhcpConfigError, "invalid option space name '"
+                      << option_space << "' specified for option '"
+                      << option_name << "' (code '" << option_code
+                      << "')");
         }
 
         OptionDefinitionPtr def;

src/bin/dhcp6/ctrl_dhcp6_srv.cc

@@ -45,19 +45,62 @@ namespace dhcp {
 ControlledDhcpv6Srv* ControlledDhcpv6Srv::server_ = NULL;
 
 ConstElementPtr
+ControlledDhcpv6Srv::dhcp6StubConfigHandler(ConstElementPtr) {
+    // This configuration handler is intended to be used only
+    // when the initial configuration comes in. To receive this
+    // configuration a pointer to this handler must be passed
+    // using ModuleCCSession constructor. This constructor will
+    // invoke the handler and will store the configuration for
+    // the configuration session when the handler returns success.
+    // Since this configuration is partial we just pretend to
+    // parse it and always return success. The function that
+    // initiates the session must get the configuration on its
+    // own using getFullConfig.
+    return (isc::config::createAnswer(0, "Configuration accepted."));
+}
+
+ConstElementPtr
 ControlledDhcpv6Srv::dhcp6ConfigHandler(ConstElementPtr new_config) {
-    LOG_DEBUG(dhcp6_logger, DBG_DHCP6_COMMAND, DHCP6_CONFIG_UPDATE)
-              .arg(new_config->str());
 
-    if (server_) {
-        return (configureDhcp6Server(*server_, new_config));
+    if (!server_ || !server_->config_session_) {
+        // That should never happen as we install config_handler
+        // after we instantiate the server.
+        ConstElementPtr answer =
+            isc::config::createAnswer(1, "Configuration rejected,"
+                                      " server is during startup/shutdown phase.");
+        return (answer);
     }
 
-    // That should never happen as we install config_handler after we instantiate
-    // the server.
-    ConstElementPtr answer = isc::config::createAnswer(1,
-           "Configuration rejected, server is during startup/shutdown phase.");
-    return (answer);
+    // The configuration passed to this handler function is partial.
+    // In other words, it just includes the values being modified.
+    // In the same time, there are dependencies between various
+    // DHCP configuration parsers. For example: the option value can
+    // be set if the definition of this option is set. If someone removes
+    // an existing option definition then the partial configuration that
+    // removes that definition is triggered while a relevant option value
+    // may remain configured. This eventually results in the DHCP server
+    // configuration being in the inconsistent state.
+    // In order to work around this problem we need to merge the new
+    // configuration with the existing (full) configuration.
+
+    // Let's create a new object that will hold the merged configuration.
+    boost::shared_ptr<MapElement> merged_config(new MapElement());
+    // Let's get the existing configuration.
+    ConstElementPtr full_config = server_->config_session_->getFullConfig();
+    // The full_config and merged_config should be always non-NULL
+    // but to provide some level of exception safety we check that they
+    // really are (in case we go out of memory).
+    if (full_config && merged_config) {
+        merged_config->setValue(full_config->mapValue());
+
+        // Merge an existing and new configuration.
+        isc::data::merge(merged_config, new_config);
+        LOG_DEBUG(dhcp6_logger, DBG_DHCP6_COMMAND, DHCP6_CONFIG_UPDATE)
+            .arg(merged_config->str());
+    }
+
+    // Configure the server.
+    return (configureDhcp6Server(*server_, merged_config));
 }
 
 ConstElementPtr
@@ -108,18 +151,26 @@ void ControlledDhcpv6Srv::establishSession() {
     LOG_DEBUG(dhcp6_logger, DBG_DHCP6_START, DHCP6_CCSESSION_STARTING)
               .arg(specfile);
     cc_session_ = new Session(io_service_.get_io_service());
+    // Create a session with the dummy configuration handler.
+    // Dumy configuration handler is internally invoked by the
+    // constructor and on success the constructor updates
+    // the current session with the configuration that had been
+    // commited in the previous session. If we did not install
+    // the dummy handler, the previous configuration would have
+    // been lost.
     config_session_ = new ModuleCCSession(specfile, *cc_session_,
-                                          NULL,
+                                          dhcp6StubConfigHandler,
                                           dhcp6CommandHandler, false);
     config_session_->start();
 
-    // We initially create ModuleCCSession() without configHandler, as
-    // the session module is too eager to send partial configuration.
-    // We want to get the full configuration, so we explicitly call
-    // getFullConfig() and then pass it to our configHandler.
+    // The constructor already pulled the configuration that had
+    // been created in the previous session thanks to the dummy
+    // handler. We can switch to the handler that will be
+    // parsing future changes to the configuration.
     config_session_->setConfigHandler(dhcp6ConfigHandler);
 
     try {
+        // Pull the full configuration out from the session.
         configureDhcp6Server(*this, config_session_->getFullConfig());
     } catch (const DhcpConfigError& ex) {
         LOG_ERROR(dhcp6_logger, DHCP6_CONFIG_LOAD_FAIL).arg(ex.what());

src/bin/dhcp6/ctrl_dhcp6_srv.h

@@ -92,6 +92,27 @@ protected:
     static isc::data::ConstElementPtr
     dhcp6ConfigHandler(isc::data::ConstElementPtr new_config);
 
+    /// @brief A dummy configuration handler that always returns success.
+    ///
+    /// This configuration handler does not perform configuration
+    /// parsing and always returns success. A dummy hanlder should
+    /// be installed using \ref isc::config::ModuleCCSession ctor
+    /// to get the initial configuration. This initial configuration
+    /// comprises values for only those elements that were modified
+    /// the previous session. The \ref dhcp6ConfigHandler can't be
+    /// used to parse the initial configuration because it needs the
+    /// full configuration to satisfy dependencies between the
+    /// various configuration values. Installing the dummy handler
+    /// that guarantees to return success causes initial configuration
+    /// to be stored for the session being created and that it can
+    /// be later accessed with \ref isc::ConfigData::getFullConfig.
+    ///
+    /// @param new_config new configuration.
+    ///
+    /// @return success configuration status.
+    static isc::data::ConstElementPtr
+    dhcp6StubConfigHandler(isc::data::ConstElementPtr new_config);
+
     /// @brief A callback for handling incoming commands.
     ///
     /// @param command textual representation of the command

src/bin/dhcp6/dhcp6.spec

@@ -76,7 +76,7 @@
             "item_default": False
           },
 
-          { "item_name": "record_types",
+          { "item_name": "record-types",
             "item_type": "string",
             "item_optional": false,
             "item_default": ""

src/bin/dhcp6/dhcp6_srv.cc

@@ -205,7 +205,7 @@ bool Dhcpv6Srv::run() {
 
                 LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA,
                           DHCP6_RESPONSE_DATA)
-                          .arg(rsp->getType()).arg(rsp->toText());
+                    .arg(static_cast<int>(rsp->getType())).arg(rsp->toText());
 
                 if (rsp->pack()) {
                     try {

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -351,7 +351,6 @@ TEST_F(Dhcp6ParserTest, subnetGlobalDefaults) {
         "    \"pool\": [ \"2001:db8:1::1 - 2001:db8:1::ffff\" ],"
         "    \"subnet\": \"2001:db8:1::/64\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
 
     ElementPtr json = Element::fromJSON(config);
 
@@ -390,7 +389,6 @@ TEST_F(Dhcp6ParserTest, subnetLocal) {
         "    \"valid-lifetime\": 4,"
         "    \"subnet\": \"2001:db8:1::/64\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
 
     ElementPtr json = Element::fromJSON(config);
 
@@ -516,7 +514,7 @@ TEST_F(Dhcp6ParserTest, poolOutOfSubnet) {
         "    \"pool\": [ \"4001:db8:1::/80\" ],"
         "    \"subnet\": \"2001:db8:1::/64\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
+
 
     ElementPtr json = Element::fromJSON(config);
 
@@ -544,7 +542,6 @@ TEST_F(Dhcp6ParserTest, poolPrefixLen) {
         "    \"pool\": [ \"2001:db8:1::/80\" ],"
         "    \"subnet\": \"2001:db8:1::/64\" } ],"
         "\"valid-lifetime\": 4000 }";
-    cout << config << endl;
 
     ElementPtr json = Element::fromJSON(config);
 

src/lib/dhcp/libdhcp++.cc

@@ -267,20 +267,12 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
     return (offset);
 }
 
-void LibDHCP::packOptions6(isc::util::OutputBuffer &buf,
-                           const isc::dhcp::Option::OptionCollection& options) {
-    for (Option::OptionCollection::const_iterator it = options.begin();
-         it != options.end(); ++it) {
-        it->second->pack(buf);
-    }
-}
-
 void
 LibDHCP::packOptions(isc::util::OutputBuffer& buf,
                      const Option::OptionCollection& options) {
     for (Option::OptionCollection::const_iterator it = options.begin();
          it != options.end(); ++it) {
-        it->second->pack4(buf);
+        it->second->pack(buf);
     }
 }
 

src/lib/dhcp/libdhcp++.h

@@ -88,16 +88,6 @@ public:
                                               uint16_t type,
                                               const OptionBuffer& buf);
 
-    /// Builds collection of options.
-    ///
-    /// Builds raw (on-wire) data for provided collection of options.
-    ///
-    /// @param buf output buffer (assembled options will be stored here)
-    /// @param options collection of options to store to
-    static void packOptions6(isc::util::OutputBuffer& buf,
-                             const isc::dhcp::Option::OptionCollection& options);
-
-
     /// @brief Stores options in a buffer.
     ///
     /// Stores all options defined in options containers in a on-wire

src/lib/dhcp/option.cc

@@ -84,51 +84,14 @@ Option::check() {
 }
 
 void Option::pack(isc::util::OutputBuffer& buf) {
-    switch (universe_) {
-    case V6:
-        return (pack6(buf));
-
-    case V4:
-        return (pack4(buf));
-
-    default:
-        isc_throw(BadValue, "Failed to pack " << type_ << " option as the "
-                  << "universe type is unknown.");
+    // Write a header.
+    packHeader(buf);
+    // Write data.
+    if (!data_.empty()) {
+        buf.writeData(&data_[0], data_.size());
     }
-}
-
-void
-Option::pack4(isc::util::OutputBuffer& buf) {
-    if (universe_ == V4) {
-        // Write a header.
-        packHeader(buf);
-        // Write data.
-        if (!data_.empty()) {
-            buf.writeData(&data_[0], data_.size());
-        }
-        // Write sub-options.
-        packOptions(buf);
-    } else {
-        isc_throw(BadValue, "Invalid universe type " << universe_);
-    }
-
-    return;
-}
-
-void Option::pack6(isc::util::OutputBuffer& buf) {
-    if (universe_ == V6) {
-        // Write a header.
-        packHeader(buf);
-        // Write data.
-        if (!data_.empty()) {
-            buf.writeData(&data_[0], data_.size());
-        }
-        // Write sub-options.
-        packOptions(buf);
-    } else {
-        isc_throw(BadValue, "Invalid universe type " << universe_);
-    }
-    return;
+    // Write sub-options.
+    packOptions(buf);
 }
 
 void
@@ -153,16 +116,7 @@ Option::packHeader(isc::util::OutputBuffer& buf) {
 
 void
 Option::packOptions(isc::util::OutputBuffer& buf) {
-    switch (universe_) {
-    case V4:
-        LibDHCP::packOptions(buf, options_);
-        return;
-    case V6:
-        LibDHCP::packOptions6(buf, options_);
-        return;
-    default:
-        isc_throw(isc::BadValue, "Invalid universe type " << universe_);
-    }
+    LibDHCP::packOptions(buf, options_);
 }
 
 void Option::unpack(OptionBufferConstIter begin,

src/lib/dhcp/option.h

@@ -158,28 +158,13 @@ public:
     ///
     /// Writes option in wire-format to buffer, returns pointer to first unused
     /// byte after stored option (that is useful for writing options one after
-    /// another). Used in DHCPv6 options.
-    ///
-    /// @todo Migrate DHCPv6 code to pack(OutputBuffer& buf) version
+    /// another).
     ///
     /// @param buf pointer to a buffer
     ///
     /// @throw BadValue Universe of the option is neither V4 nor V6.
     virtual void pack(isc::util::OutputBuffer& buf);
 
-    /// @brief Writes option in a wire-format to a buffer.
-    ///
-    /// Method will throw if option storing fails for some reason.
-    ///
-    /// @todo Once old (DHCPv6) implementation is rewritten,
-    /// unify pack4() and pack6() and rename them to just pack().
-    ///
-    /// @param buf output buffer (option will be stored there)
-    ///
-    /// @throw OutOfRange Option type is greater than 255.
-    /// @throw BadValue Universe is not V4.
-    virtual void pack4(isc::util::OutputBuffer& buf);
-
     /// @brief Parses received buffer.
     ///
     /// @param begin iterator to first byte of option data
@@ -317,13 +302,6 @@ public:
     virtual bool equal(const OptionPtr& other) const;
 
 protected:
-    /// Builds raw (over-wire) buffer of this option, including all
-    /// defined suboptions. Version for building DHCPv4 options.
-    ///
-    /// @param buf output buffer (built options will be stored here)
-    ///
-    /// @throw BadValue Universe is not V6.
-    virtual void pack6(isc::util::OutputBuffer& buf);
 
     /// @brief Store option's header in a buffer.
     ///

src/lib/dhcp/option4_addrlst.cc

@@ -64,7 +64,7 @@ Option4AddrLst::Option4AddrLst(uint8_t type, const IOAddress& addr)
 }
 
 void
-Option4AddrLst::pack4(isc::util::OutputBuffer& buf) {
+Option4AddrLst::pack(isc::util::OutputBuffer& buf) {
 
     if (addrs_.size() * V4ADDRESS_LEN > 255) {
         isc_throw(OutOfRange, "DHCPv4 Option4AddrLst " << type_ << " is too big."

src/lib/dhcp/option4_addrlst.h

@@ -87,11 +87,8 @@ public:
     ///
     /// Method will throw if option storing fails for some reason.
     ///
-    /// TODO Once old (DHCPv6) implementation is rewritten,
-    /// unify pack4() and pack6() and rename them to just pack().
-    ///
     /// @param buf output buffer (option will be stored there)
-    virtual void pack4(isc::util::OutputBuffer& buf);
+    virtual void pack(isc::util::OutputBuffer& buf);
 
     /// Returns string representation of the option.
     ///

src/lib/dhcp/option_custom.cc

@@ -387,14 +387,10 @@ OptionCustom::dataFieldToText(const OptionDataType data_type,
 }
 
 void
-OptionCustom::pack4(isc::util::OutputBuffer& buf) {
-    if (len() > 255) {
-        isc_throw(OutOfRange, "DHCPv4 Option " << type_
-                  << " value is too high. At most 255 is supported.");
-    }
+OptionCustom::pack(isc::util::OutputBuffer& buf) {
 
-    buf.writeUint8(type_);
-    buf.writeUint8(len() - getHeaderLen());
+    // Pack DHCP header (V4 or V6).
+    packHeader(buf);
 
     // Write data from buffers.
     for (std::vector<OptionBuffer>::const_iterator it = buffers_.begin();
@@ -411,21 +407,6 @@ OptionCustom::pack4(isc::util::OutputBuffer& buf) {
     packOptions(buf);
 }
 
-void
-OptionCustom::pack6(isc::util::OutputBuffer& buf) {
-    buf.writeUint16(type_);
-    buf.writeUint16(len() - getHeaderLen());
-
-    // Write data from buffers.
-    for (std::vector<OptionBuffer>::const_iterator it = buffers_.begin();
-         it != buffers_.end(); ++it) {
-        if (!it->empty()) {
-            buf.writeData(&(*it)[0], it->size());
-        }
-    }
-
-    packOptions(buf);
-}
 
 asiolink::IOAddress
 OptionCustom::readAddress(const uint32_t index) const {

src/lib/dhcp/option_custom.h

@@ -249,6 +249,11 @@ public:
     void writeString(const std::string& text,
                      const uint32_t index = 0);
 
+    /// @brief Writes DHCP option in a wire format to a buffer.
+    ///
+    /// @param buf output buffer (option will be stored there).
+    virtual void pack(isc::util::OutputBuffer& buf);
+
     /// @brief Parses received buffer.
     ///
     /// @param begin iterator to first byte of option data
@@ -278,18 +283,6 @@ public:
     void setData(const OptionBufferConstIter first,
                  const OptionBufferConstIter last);
 
-protected:
-
-    /// @brief Writes DHCPv4 option in a wire format to a buffer.
-    ///
-    /// @param buf output buffer (option will be stored there).
-    virtual void pack4(isc::util::OutputBuffer& buf);
-
-    /// @brief Writes DHCPv6 option in a wire format to a buffer.
-    ///
-    /// @param buf output buffer (built options will be stored here)
-    virtual void pack6(isc::util::OutputBuffer& buf);
-
 private:
 
     /// @brief Verify that the option comprises an array of values.

src/lib/dhcp/option_definition.cc

@@ -216,8 +216,8 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
         const RecordFieldsCollection& records = getRecordFields();
         if (records.size() > values.size()) {
             isc_throw(InvalidOptionValue, "number of data fields for the option"
-                      << " type " << type_ << " is greater than number of values"
-                      << " provided.");
+                      << " type '" <<  getCode() << "' is greater than number"
+                      << " of values provided.");
         }
         for (size_t i = 0; i < records.size(); ++i) {
             writeToBuffer(util::str::trim(values[i]),
@@ -444,14 +444,8 @@ OptionDefinition::writeToBuffer(const std::string& value,
         OptionDataTypeUtil::writeString(value, buf);
         return;
     case OPT_FQDN_TYPE:
-        {
-            // FQDN implementation is not terribly complicated but will require
-            // creation of some additional logic (maybe object) that will parse
-            // the fqdn into labels.
-            isc_throw(isc::NotImplemented, "write of FQDN record into option buffer"
-                      " is not supported yet");
-            return;
-        }
+        OptionDataTypeUtil::writeFqdn(value, buf);
+        return;
     default:
         // We hit this point because invalid option data type has been specified
         // This may be the case because 'empty' or 'record' data type has been

src/lib/dhcp/pkt4.cc

@@ -219,7 +219,7 @@ void Pkt4::check() {
     uint8_t msg_type = getType();
     if (msg_type > DHCPLEASEACTIVE) {
         isc_throw(BadValue, "Invalid DHCP message type received: "
-                  << msg_type);
+                  << static_cast<int>(msg_type));
     }
 }
 
@@ -230,10 +230,10 @@ uint8_t Pkt4::getType() const {
     }
 
     // Check if Message Type is specified as OptionInt<uint8_t>
-    boost::shared_ptr<OptionInt<uint8_t> > typeOpt =
+    boost::shared_ptr<OptionInt<uint8_t> > type_opt =
         boost::dynamic_pointer_cast<OptionInt<uint8_t> >(generic);
-    if (typeOpt) {
-        return (typeOpt->getValue());
+    if (type_opt) {
+        return (type_opt->getValue());
     }
 
     // Try to use it as generic option
@@ -253,7 +253,6 @@ void Pkt4::setType(uint8_t dhcp_type) {
     }
 }
 
-
 void Pkt4::repack() {
     bufferOut_.writeData(&data_[0], data_.size());
 }
@@ -263,7 +262,7 @@ Pkt4::toText() {
     stringstream tmp;
     tmp << "localAddr=" << local_addr_.toText() << ":" << local_port_
         << " remoteAddr=" << remote_addr_.toText()
-        << ":" << remote_port_ << ", msgtype=" << getType()
+        << ":" << remote_port_ << ", msgtype=" << static_cast<int>(getType())
         << ", transid=0x" << hex << transid_ << dec << endl;
 
     for (isc::dhcp::Option::OptionCollection::iterator opt=options_.begin();

src/lib/dhcp/pkt6.cc

@@ -90,7 +90,7 @@ Pkt6::packUDP() {
         bufferOut_.writeUint8( (transid_) & 0xff );
 
         // the rest are options
-        LibDHCP::packOptions6(bufferOut_, options_);
+        LibDHCP::packOptions(bufferOut_, options_);
     }
     catch (const Exception& e) {
         /// @todo: throw exception here once we turn this function to void.
@@ -155,8 +155,8 @@ Pkt6::toText() {
     tmp << "localAddr=[" << local_addr_.toText() << "]:" << local_port_
         << " remoteAddr=[" << remote_addr_.toText()
         << "]:" << remote_port_ << endl;
-    tmp << "msgtype=" << msg_type_ << ", transid=0x" << hex << transid_
-        << dec << endl;
+    tmp << "msgtype=" << static_cast<int>(msg_type_) << ", transid=0x" <<
+        hex << transid_ << dec << endl;
     for (isc::dhcp::Option::OptionCollection::iterator opt=options_.begin();
          opt != options_.end();
          ++opt) {

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -273,7 +273,7 @@ TEST_F(LibDhcpTest, packOptions6) {
 
     OutputBuffer assembled(512);
 
-    EXPECT_NO_THROW(LibDHCP::packOptions6(assembled, opts));
+    EXPECT_NO_THROW(LibDHCP::packOptions(assembled, opts));
     EXPECT_EQ(sizeof(v6packed), assembled.getLength());
     EXPECT_EQ(0, memcmp(assembled.getData(), v6packed, sizeof(v6packed)));
 }

src/lib/dhcp/tests/option4_addrlst_unittest.cc

@@ -155,7 +155,7 @@ TEST_F(Option4AddrLstTest, assembly1) {
 
     OutputBuffer buf(100);
     EXPECT_NO_THROW(
-        opt->pack4(buf);
+        opt->pack(buf);
     );
 
     ASSERT_EQ(6, opt->len());
@@ -198,7 +198,7 @@ TEST_F(Option4AddrLstTest, assembly4) {
 
     OutputBuffer buf(100);
     EXPECT_NO_THROW(
-        opt->pack4(buf);
+        opt->pack(buf);
     );
 
     ASSERT_EQ(18, opt->len()); // 2(header) + 4xsizeof(IPv4addr)

src/lib/dhcp/tests/option_unittest.cc

@@ -116,7 +116,7 @@ TEST_F(OptionTest, v4_data1) {
     // now store that option into a buffer
     OutputBuffer buf(100);
     EXPECT_NO_THROW(
-        opt->pack4(buf);
+        opt->pack(buf);
     );
 
     // check content of that buffer
@@ -173,7 +173,7 @@ TEST_F(OptionTest, v4_data2) {
     // now store that option into a buffer
     OutputBuffer buf(100);
     EXPECT_NO_THROW(
-        opt->pack4(buf);
+        opt->pack(buf);
     );
 
     // check content of that buffer
@@ -471,7 +471,7 @@ TEST_F(OptionTest, setUintX) {
     // verify setUint8
     opt1->setUint8(255);
     EXPECT_EQ(255, opt1->getUint8());
-    opt1->pack4(outBuf_);
+    opt1->pack(outBuf_);
     EXPECT_EQ(3, opt1->len());
     EXPECT_EQ(3, outBuf_.getLength());
     uint8_t exp1[] = {125, 1, 255};
@@ -480,7 +480,7 @@ TEST_F(OptionTest, setUintX) {
     // verify getUint16
     outBuf_.clear();
     opt2->setUint16(12345);
-    opt2->pack4(outBuf_);
+    opt2->pack(outBuf_);
     EXPECT_EQ(12345, opt2->getUint16());
     EXPECT_EQ(4, opt2->len());
     EXPECT_EQ(4, outBuf_.getLength());
@@ -490,7 +490,7 @@ TEST_F(OptionTest, setUintX) {
     // verify getUint32
     outBuf_.clear();
     opt4->setUint32(0x12345678);
-    opt4->pack4(outBuf_);
+    opt4->pack(outBuf_);
     EXPECT_EQ(0x12345678, opt4->getUint32());
     EXPECT_EQ(6, opt4->len());
     EXPECT_EQ(6, outBuf_.getLength());
@@ -505,7 +505,7 @@ TEST_F(OptionTest, setData) {
                               buf_.begin(), buf_.begin() + 10));
     buf_.resize(20, 1);
     opt1->setData(buf_.begin(), buf_.end());
-    opt1->pack4(outBuf_);
+    opt1->pack(outBuf_);
     ASSERT_EQ(outBuf_.getLength() - opt1->getHeaderLen(), buf_.size());
     const uint8_t* test_data = static_cast<const uint8_t*>(outBuf_.getData());
     EXPECT_TRUE(0 == memcmp(&buf_[0], test_data + opt1->getHeaderLen(),
@@ -518,7 +518,7 @@ TEST_F(OptionTest, setData) {
     outBuf_.clear();
     buf_.resize(5, 1);
     opt2->setData(buf_.begin(), buf_.end());
-    opt2->pack4(outBuf_);
+    opt2->pack(outBuf_);
     ASSERT_EQ(outBuf_.getLength() - opt1->getHeaderLen(), buf_.size());
     test_data = static_cast<const uint8_t*>(outBuf_.getData());
     EXPECT_TRUE(0 == memcmp(&buf_[0], test_data + opt1->getHeaderLen(),

src/lib/dhcpsrv/alloc_engine.h

@@ -214,8 +214,8 @@ protected:
     /// @param clientid client identifier
     /// @param hwaddr client's hardware address
     /// @param lease lease to be renewed
-    /// @param renewed lease (typically the same passed as lease parameter)
-    ///        or NULL if the lease cannot be renewed
+    /// @param fake_allocation is this real i.e. REQUEST (false) or just picking
+    ///        an address for DISCOVER that is not really allocated (true)
     Lease4Ptr
     renewLease4(const SubnetPtr& subnet,
                 const ClientIdPtr& clientid,

src/lib/dhcpsrv/lease_mgr.h

@@ -114,9 +114,18 @@ public:
 /// leases.
 struct Lease {
 
+    /// @brief Constructor
+    ///
+    /// @param addr IP address
+    /// @param t1 renewal time
+    /// @param t2 rebinding time
+    /// @param valid_lft Lifetime of the lease
+    /// @param subnet_id Subnet identification
+    /// @param cltt Client last transmission time
     Lease(const isc::asiolink::IOAddress& addr, uint32_t t1, uint32_t t2,
           uint32_t valid_lft, SubnetID subnet_id, time_t cltt);
 
+    /// @brief Destructor
     virtual ~Lease() {}
 
     /// @brief IPv4 ot IPv6 address
@@ -226,13 +235,14 @@ struct Lease4 : public Lease {
 
     /// @brief Constructor
     ///
-    /// @param addr IPv4 address as unsigned 32-bit integer in network byte
-    ///        order.
+    /// @param addr IPv4 address.
     /// @param hwaddr Hardware address buffer
     /// @param hwaddr_len Length of hardware address buffer
     /// @param clientid Client identification buffer
     /// @param clientid_len Length of client identification buffer
     /// @param valid_lft Lifetime of the lease
+    /// @param t1 renewal time
+    /// @param t2 rebinding time
     /// @param cltt Client last transmission time
     /// @param subnet_id Subnet identification
     Lease4(const isc::asiolink::IOAddress& addr, const uint8_t* hwaddr, size_t hwaddr_len,

src/lib/dhcpsrv/option_space_container.h

@@ -41,7 +41,7 @@ public:
     /// @brief Adds a new item to the option_space.
     ///
     /// @param item reference to the item being added.
-    /// @param name of the option space.
+    /// @param option_space name of the option space.
     void addItem(const ItemType& item, const std::string& option_space) {
         ItemsContainerPtr items = getItems(option_space);
         items->push_back(item);