Merge branch 'master' into trac2593

ChangeLog

@@ -1,3 +1,20 @@
+545.	[func]		jinmei
+	libdns++: the SOA Rdata class now uses the generic lexer in
+	constructors from text.  This means that the MNAME and RNAME of an
+	SOA RR in a zone file can now be non absolute (the origin name
+	in that context will be used), e.g., when loaded by b10-loadzone.
+	(Trac #2500, git 019ca218027a218921519f205139b96025df2bb5)
+
+544.	[func]		tomek
+	b10-dhcp4: Allocation engine support for IPv4 added. Currently
+	supported operations are server selection (Discover/Offer),
+	address assignment (Request/Ack), address renewal (Request/Ack),
+	and address release (Release). Expired leases can be reused.
+	Some options (e.g. Router Option) are still hardcoded, so the
+	DHCPv4 server is not yet usable, although its address allocation
+	is operational.
+	(Trac #2320, git 60606cabb1c9584700b1f642bf2af21a35c64573)
+
 543.	[func]*		jelte
 	When calling getFullConfig() as a module, , the configuration is now
 	returned as properly-structured JSON.  Previously, the structure had

doc/guide/bind10-guide.xml

@@ -3343,23 +3343,21 @@ then change those defaults with config set Resolver/forward_addresses[0]/address
 
     <note>
       <para>
-        As of November 2012, the DHCPv4 component is a
-        skeleton server. That means that while it is capable of
-        performing DHCP configuration, it is not fully functional.
-        In particular, it does not have a functional lease
-        database. This means that they will assign the same, fixed,
-        hardcoded addresses to any client that will ask. See <xref
-        linkend="dhcp4-limit"/> for a
-        detailed description.
+        As of January 2013, the DHCPv4 component is a work in progress.
+        That means that while it is capable of performing DHCP configuration,
+        it is not fully functional.  The server is able to offer,
+        assign, renew, release and reuse expired leases, but some of the
+        options are not configurable yet. In particular Router option is hardcoded.
+        This means that the server is not really usable in actual deployments
+        yet. See <xref linkend="dhcp4-limit"/> for a detailed description.
       </para>
     </note>
 
     <section id="dhcp4-usage">
       <title>DHCPv4 Server Usage</title>
       <para>BIND 10 has provided the DHCPv4 server component since December
-      2011. It is a skeleton server and can be described as an early
-      prototype that is not fully functional yet. It is mature enough
-      to conduct first tests in lab environment, but it has
+      2011. It is current experimental implementation and is not fully functional
+      yet. It is mature enough to conduct tests in lab environment, but it has
       significant limitations. See <xref linkend="dhcp4-limit"/> for
       details.
       </para>
@@ -3480,9 +3478,10 @@ Dhcp4/subnet4	         []     list    (default)</screen>
       </para>
 
       <para>
-        Note: Although configuration is now accepted, it is not internally used
-        by they server yet.  At this stage of development, the only way to alter
-        server configuration is to modify its source code. To do so, please edit
+        Note: Although configuration is now accepted, some parts of it is not internally used
+        by they server yet. Address pools are used, but option definitons are not.
+        The only way to alter some options (e.g. Router Option or DNS servers and Domain name)
+        is to modify source code. To do so, please edit
         src/bin/dhcp6/dhcp4_srv.cc file, modify the following parameters and
         recompile:
         <screen>
@@ -3505,7 +3504,7 @@ const std::string HARDCODED_SERVER_ID = "192.0.2.1";</screen>
       <itemizedlist>
           <listitem>
             <simpara>RFC2131: Supported messages are DISCOVER, OFFER,
-            REQUEST, and ACK.</simpara>
+            REQUEST, ACK, NAK, RELEASE.</simpara>
           </listitem>
           <listitem>
             <simpara>RFC2132: Supported options are: PAD (0),
@@ -3513,6 +3512,10 @@ const std::string HARDCODED_SERVER_ID = "192.0.2.1";</screen>
             Domain Name (15), DNS Servers (6), IP Address Lease Time
             (51), Subnet mask (1), and Routers (3).</simpara>
           </listitem>
+          <listitem>
+            <simpara>RFC6842: Server responses include client-id option
+            if client sent it in its message.</simpara>
+          </listitem>
       </itemizedlist>
     </section>
 
@@ -3533,20 +3536,6 @@ const std::string HARDCODED_SERVER_ID = "192.0.2.1";</screen>
             communication).</simpara>
           </listitem>
           <listitem>
-            <simpara><command>b10-dhcp4</command> provides a single,
-            fixed, hardcoded lease to any client that asks.  There is
-            no lease manager implemented. If two clients request
-            addresses, they will both get the same fixed
-            address.</simpara>
-          </listitem>
-          <listitem>
-            <simpara><command>b10-dhcp4</command> does not support any
-            configuration mechanisms yet. The whole configuration is
-            currently hardcoded. The only way to tweak configuration
-            is to directly modify source code. See see <xref
-            linkend="dhcp4-config"/> for details.</simpara>
-          </listitem>
-          <listitem>
             <simpara>Upon start, the server will open sockets on all
             interfaces that are not loopback, are up and running and
             have IPv4 address.</simpara>
@@ -3574,9 +3563,9 @@ const std::string HARDCODED_SERVER_ID = "192.0.2.1";</screen>
             sending ICMP echo request.</simpara>
           </listitem>
           <listitem>
-            <simpara>Address renewal (RENEW), rebinding (REBIND),
-            confirmation (CONFIRM), duplication report (DECLINE) and
-            release (RELEASE) are not supported yet.</simpara>
+            <simpara>Address rebinding (REQUEST/Rebinding), confirmation
+            (CONFIRM) and duplication report (DECLINE) are not supported
+            yet.</simpara>
           </listitem>
           <listitem>
             <simpara>DNS Update is not supported yet.</simpara>

src/bin/dhcp4/config_parser.cc

@@ -1114,7 +1114,7 @@ private:
         subnet_.reset(new Subnet4(addr, len, t1, t2, valid));
 
         for (PoolStorage::iterator it = pools_.begin(); it != pools_.end(); ++it) {
-            subnet_->addPool4(*it);
+            subnet_->addPool(*it);
         }
 
         Subnet::OptionContainerPtr options = subnet_->getOptionDescriptors("dhcp4");

src/bin/dhcp4/dhcp4_messages.mes

@@ -26,12 +26,6 @@ to establish a session with the BIND 10 control channel.
 A debug message listing the command (and possible arguments) received
 from the BIND 10 control system by the IPv4 DHCP server.
 
-% DHCP4_CONFIG_COMPLETE DHCPv4 server has completed configuration: %1
-This is an informational message announcing the successful processing of a
-new configuration. it is output during server startup, and when an updated
-configuration is committed by the administrator.  Additional information
-may be provided.
-
 % DHCP4_CONFIG_LOAD_FAIL failed to load configuration: %1
 This critical error message indicates that the initial DHCPv4
 configuration has failed. The server will start, but nothing will be
@@ -41,19 +35,52 @@ served until the configuration has been corrected.
 This is an informational message reporting that the configuration has
 been extended to include the specified IPv4 subnet.
 
+% DHCP4_CONFIG_OPTION_DUPLICATE multiple options with the code %1 added to the subnet %2
+This warning message is issued on an attempt to configure multiple options
+with the same option code for a particular subnet. Adding multiple options
+is uncommon for DHCPv4, but is not prohibited.
+
+% DHCP4_CONFIG_UPDATE updated configuration received: %1
+A debug message indicating that the IPv4 DHCP server has received an
+updated configuration from the BIND 10 configuration system.
+
 % DHCP4_CONFIG_START DHCPv4 server is processing the following configuration: %1
 This is a debug message that is issued every time the server receives a
 configuration. That happens at start up and also when a server configuration
 change is committed by the administrator.
 
-% DHCP4_CONFIG_UPDATE updated configuration received: %1
-A debug message indicating that the IPv4 DHCP server has received an
-updated configuration from the BIND 10 configuration system.
+% DHCP4_CONFIG_COMPLETE DHCPv4 server has completed configuration: %1
+This is an informational message announcing the successful processing of a
+new configuration. It is output during server startup, and when an updated
+configuration is committed by the administrator.  Additional information
+may be provided.
 
-% DHCP4_CONFIG_OPTION_DUPLICATE multiple options with the code: %1 added to the subnet: %2
-This warning message is issued on an attempt to configure multiple options with the
-same option code for the particular subnet. Adding multiple options is uncommon
-for DHCPv4, but it is not prohibited.
+% DHCP4_DB_BACKEND_STARTED lease database started (type: %1, name: %2)
+This informational message is printed every time DHCPv4 server is started
+and gives both the type and name of the database being used to store
+lease and other information.
+
+% DHCP4_LEASE_ADVERT lease %1 advertised (client client-id %2, hwaddr %3)
+This debug message indicates that the server successfully advertised
+a lease. It is up to the client to choose one server out of othe advertised
+and continue allocation with that server. This is a normal behavior and
+indicates successful operation.
+
+% DHCP4_LEASE_ADVERT_FAIL failed to advertise a lease for client client-id %1, hwaddr %2
+This message indicates that the server has failed to offer a lease to
+the specified client after receiving a DISCOVER message from it. There are
+many possible reasons for such a failure.
+
+% DHCP4_LEASE_ALLOC lease %1 has been allocated for client-id %2, hwaddr %3
+This debug message indicates that the server successfully granted a lease
+in response to client's REQUEST message. This is a normal behavior and
+incicates successful operation.
+
+% DHCP4_LEASE_ALLOC_FAIL failed to grant a lease for client-id %1, hwaddr %2
+This message indicates that the server failed to grant a lease to the
+specified client after receiving a REQUEST message from it.  There are many
+possible reasons for such a failure. Additional messages will indicate the
+reason.
 
 % DHCP4_NOT_RUNNING IPv4 DHCP server is not running
 A warning message is issued when an attempt is made to shut down the
@@ -91,6 +118,43 @@ to be a programming error: please raise a bug report.
 % DHCP4_QUERY_DATA received packet type %1, data is <%2>
 A debug message listing the data received from the client.
 
+% DHCP4_RELEASE address %1 belonging to client-id %2, hwaddr %3 was released properly.
+This debug message indicates that an address was released properly. It
+is a normal operation during client shutdown.
+
+% DHCP4_RELEASE_EXCEPTION exception %1 while trying to release address %2
+This message is output when an error was encountered during an attempt
+to process a RELEASE message. The error will not affect the client,
+which does not expect any response from the server for RELEASE
+messages. Depending on the nature of problem, it may affect future
+server operation.
+
+% DHCP4_RELEASE_FAIL failed to remove lease for address %1 for duid %2, hwaddr %3
+This error message indicates that the software failed to remove a
+lease from the lease database. It is probably due to an error during a
+database operation: resolution will most likely require administrator
+intervention (e.g. check if DHCP process has sufficient privileges to
+update the database). It may also be triggered if a lease was manually
+removed from the database during RELEASE message processing.
+
+% DHCP4_RELEASE_FAIL_NO_LEASE client (client-id %2) tried to release address %1, but there is no lease for such address.
+This warning message is printed when client attempts to release a lease,
+but no such lease is known to the server.
+
+% DHCP4_RELEASE_FAIL_WRONG_CLIENT_ID client (client-id %2) tried to release address %1, but it belongs to client (client-id %3)
+This warning message indicates that client tried to release an address
+that belongs to a different client. This should not happen in normal
+circumstances and may indicate a misconfiguration of the client.  However,
+since the client releasing the address will stop using it anyway, there
+is a good chance that the situation will correct itself.
+
+% DHCP4_RELEASE_FAIL_WRONG_HWADDR client (client-id %2) tried to release address %1, but sent from a wrong hardware address (%3)
+This warning message indicates that client tried to release an address
+that does belong to it, but the lease information was associated with
+a different hardware address. One possible reason for using different
+hardware address is that a cloned virtual machine was not updated and
+both clones use the same client-id.
+
 % DHCP4_RESPONSE_DATA responding with packet type %1, data is <%2>
 A debug message listing the data returned to the client.
 
@@ -131,3 +195,13 @@ processed any command-line switches and is starting.
 This is a debug message issued during the IPv4 DHCP server startup.
 It lists some information about the parameters with which the server
 is running.
+
+% DHCP4_SUBNET_SELECTED the %1 subnet was selected for client assignment
+This is a debug message noting the selection of a subnet to be used for
+address and option assignment.  Subnet selection is one of the early
+steps in the processing of incoming client message.
+
+% DHCP4_SUBNET_SELECTION_FAILED failed to select a subnet for incoming packet, src: %1, type: %2
+This warning message is output when a packet was received from a subnet
+for which the DHCPv4 server has not been configured. The most probable
+cause is a misconfiguration of the server.

src/bin/dhcp4/dhcp4_srv.cc

@@ -16,9 +16,19 @@
 #include <dhcp/dhcp4.h>
 #include <dhcp/iface_mgr.h>
 #include <dhcp/option4_addrlst.h>
+#include <dhcp/option_int.h>
 #include <dhcp/pkt4.h>
+#include <dhcp/duid.h>
+#include <dhcp/hwaddr.h>
 #include <dhcp4/dhcp4_log.h>
 #include <dhcp4/dhcp4_srv.h>
+#include <dhcpsrv/utils.h>
+#include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/lease_mgr.h>
+#include <dhcpsrv/lease_mgr_factory.h>
+#include <dhcpsrv/subnet.h>
+#include <dhcpsrv/utils.h>
+#include <dhcpsrv/addr_utilities.h>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -28,26 +38,35 @@ using namespace std;
 
 // These are hardcoded parameters. Currently this is a skeleton server that only
 // grants those options and a single, fixed, hardcoded lease.
-const std::string HARDCODED_LEASE = "192.0.2.222"; // assigned lease
-const std::string HARDCODED_NETMASK = "255.255.255.0";
-const uint32_t    HARDCODED_LEASE_TIME = 60; // in seconds
 const std::string HARDCODED_GATEWAY = "192.0.2.1";
 const std::string HARDCODED_DNS_SERVER = "192.0.2.2";
 const std::string HARDCODED_DOMAIN_NAME = "isc.example.com";
 const std::string HARDCODED_SERVER_ID = "192.0.2.1";
 
-Dhcpv4Srv::Dhcpv4Srv(uint16_t port) {
+Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig) {
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_OPEN_SOCKET).arg(port);
     try {
         // First call to instance() will create IfaceMgr (it's a singleton)
         // it may throw something if things go wrong
         IfaceMgr::instance();
 
-        /// @todo: instantiate LeaseMgr here once it is imlpemented.
-        IfaceMgr::instance().openSockets4(port);
+        if (port) {
+            // open sockets only if port is non-zero. Port 0 is used
+            // for non-socket related testing.
+            IfaceMgr::instance().openSockets4(port);
+        }
 
         setServerID();
 
+        // Instantiate LeaseMgr
+        LeaseMgrFactory::create(dbconfig);
+        LOG_INFO(dhcp4_logger, DHCP4_DB_BACKEND_STARTED)
+            .arg(LeaseMgrFactory::instance().getType())
+            .arg(LeaseMgrFactory::instance().getName());
+
+        // Instantiate allocation engine
+        alloc_engine_.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100));
+
     } catch (const std::exception &e) {
         LOG_ERROR(dhcp4_logger, DHCP4_SRV_CONSTRUCT_ERROR).arg(e.what());
         shutdown_ = true;
@@ -167,18 +186,11 @@ Dhcpv4Srv::run() {
 
 void
 Dhcpv4Srv::setServerID() {
-    /// TODO implement this for real once interface detection (ticket 1237)
-    /// is done. Use hardcoded server-id for now.
-
-#if 0
-    // uncomment this once ticket 1350 is merged.
-    IOAddress srvId("127.0.0.1");
-    serverid_ = OptionPtr(
-      new Option4AddrLst(Option::V4, DHO_DHCP_SERVER_IDENTIFIER, srvId));
-#endif
+    /// @todo: implement this for real (see ticket #2588)
+    serverid_ = OptionPtr(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER,
+                                             IOAddress(HARDCODED_SERVER_ID)));
 }
 
-
 void Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     answer->setIface(question->getIface());
     answer->setIndex(question->getIndex());
@@ -188,9 +200,7 @@ void Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     answer->setHops(question->getHops());
 
     // copy MAC address
-    vector<uint8_t> mac(question->getChaddr(),
-                        question->getChaddr() + Pkt4::MAX_CHADDR_LEN);
-    answer->setHWAddr(question->getHtype(), question->getHlen(), mac);
+    answer->setHWAddr(question->getHWAddr());
 
     // relay address
     answer->setGiaddr(question->getGiaddr());
@@ -203,21 +213,21 @@ void Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
         answer->setRemoteAddr(question->getRemoteAddr());
     }
 
+    // Let's copy client-id to response. See RFC6842.
+    OptionPtr client_id = question->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
+    if (client_id) {
+        answer->addOption(client_id);
+    }
 }
 
 void Dhcpv4Srv::appendDefaultOptions(Pkt4Ptr& msg, uint8_t msg_type) {
     OptionPtr opt;
 
     // add Message Type Option (type 53)
-    std::vector<uint8_t> tmp;
-    tmp.push_back(static_cast<uint8_t>(msg_type));
-    opt = OptionPtr(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE, tmp));
-    msg->addOption(opt);
+    msg->setType(msg_type);
 
     // DHCP Server Identifier (type 54)
-    opt = OptionPtr
-        (new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER, IOAddress(HARDCODED_SERVER_ID)));
-    msg->addOption(opt);
+    msg->addOption(getServerID());
 
     // more options will be added here later
 }
@@ -237,25 +247,109 @@ void Dhcpv4Srv::appendRequestedOptions(Pkt4Ptr& msg) {
     msg->addOption(opt);
 }
 
-void Dhcpv4Srv::tryAssignLease(Pkt4Ptr& msg) {
-    OptionPtr opt;
+void Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
+
+    // We need to select a subnet the client is connected in.
+    Subnet4Ptr subnet = selectSubnet(question);
+    if (!subnet) {
+        // This particular client is out of luck today. We do not have
+        // information about the subnet he is connected to. This likely means
+        // misconfiguration of the server (or some relays). We will continue to
+        // process this message, but our response will be almost useless: no
+        // addresses or prefixes, no subnet specific configuration etc. The only
+        // thing this client can get is some global information (like DNS
+        // servers).
+
+        // perhaps this should be logged on some higher level? This is most likely
+        // configuration bug.
+        LOG_ERROR(dhcp4_logger, DHCP4_SUBNET_SELECTION_FAILED)
+            .arg(question->getRemoteAddr().toText())
+            .arg(serverReceivedPacketName(question->getType()));
+        answer->setType(DHCPNAK);
+        answer->setYiaddr(IOAddress("0.0.0.0"));
+        return;
+    }
 
-    // TODO: Implement actual lease assignment here
-    msg->setYiaddr(IOAddress(HARDCODED_LEASE));
+    LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_SUBNET_SELECTED)
+        .arg(subnet->toText());
 
-    // IP Address Lease time (type 51)
-    opt = OptionPtr(new Option(Option::V4, DHO_DHCP_LEASE_TIME));
-    opt->setUint32(HARDCODED_LEASE_TIME);
-    msg->addOption(opt);
-    // TODO: create Option_IntArray that holds list of integers, similar to Option4_AddrLst
+    // Get client-id option
+    ClientIdPtr client_id;
+    OptionPtr opt = question->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
+    if (opt) {
+        client_id = ClientIdPtr(new ClientId(opt->getData()));
+    }
+    // client-id is not mandatory in DHCPv4
+
+    IOAddress hint = question->getYiaddr();
+
+    HWAddrPtr hwaddr = question->getHWAddr();
+
+    // "Fake" allocation is processing of DISCOVER message. We pretend to do an
+    // allocation, but we do not put the lease in the database. That is ok,
+    // because we do not guarantee that the user will get that exact lease. If
+    // the user selects this server to do actual allocation (i.e. sends REQUEST)
+    // it should include this hint. That will help us during the actual lease
+    // allocation.
+    bool fake_allocation = (question->getType() == DHCPDISCOVER);
+
+    // Use allocation engine to pick a lease for this client. Allocation engine
+    // will try to honour the hint, but it is just a hint - some other address
+    // may be used instead. If fake_allocation is set to false, the lease will
+    // be inserted into the LeaseMgr as well.
+    Lease4Ptr lease = alloc_engine_->allocateAddress4(subnet, client_id, hwaddr,
+                                                      hint, fake_allocation);
+
+    if (lease) {
+        // We have a lease! Let's set it in the packet and send it back to
+        // the client.
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, fake_allocation?
+                  DHCP4_LEASE_ADVERT:DHCP4_LEASE_ALLOC)
+            .arg(lease->addr_.toText())
+            .arg(client_id?client_id->toText():"(no client-id)")
+            .arg(hwaddr?hwaddr->toText():"(no hwaddr info)");
+
+        answer->setYiaddr(lease->addr_);
+
+        // IP Address Lease time (type 51)
+        opt = OptionPtr(new Option(Option::V4, DHO_DHCP_LEASE_TIME));
+        opt->setUint32(lease->valid_lft_);
+        answer->addOption(opt);
+
+        // @todo: include real router information here
+        // Router (type 3)
+        opt = OptionPtr(new Option4AddrLst(DHO_ROUTERS, IOAddress(HARDCODED_GATEWAY)));
+        answer->addOption(opt);
+
+        // Subnet mask (type 1)
+        answer->addOption(getNetmaskOption(subnet));
+
+        // @todo: send renew timer option (T1, option 58)
+        // @todo: send rebind timer option (T2, option 59)
 
-    // Subnet mask (type 1)
-    opt = OptionPtr(new Option4AddrLst(DHO_SUBNET_MASK, IOAddress(HARDCODED_NETMASK)));
-    msg->addOption(opt);
+    } else {
+        // Allocation engine did not allocate a lease. The engine logged
+        // cause of that failure. The only thing left is to insert
+        // status code to pass the sad news to the client.
+
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, fake_allocation?
+                  DHCP4_LEASE_ADVERT_FAIL:DHCP4_LEASE_ALLOC_FAIL)
+            .arg(client_id?client_id->toText():"(no client-id)")
+            .arg(hwaddr?hwaddr->toText():"(no hwaddr info)")
+            .arg(hint.toText());
+
+        answer->setType(DHCPNAK);
+        answer->setYiaddr(IOAddress("0.0.0.0"));
+    }
+}
 
-    // Router (type 3)
-    opt = OptionPtr(new Option4AddrLst(DHO_ROUTERS, IOAddress(HARDCODED_GATEWAY)));
-    msg->addOption(opt);
+OptionPtr Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) {
+    uint32_t netmask = getNetmask4(subnet->get().second);
+
+    OptionPtr opt(new OptionInt<uint32_t>(Option::V4,
+                  DHO_SUBNET_MASK, netmask));
+
+    return (opt);
 }
 
 Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
@@ -266,7 +360,7 @@ Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
     appendDefaultOptions(offer, DHCPOFFER);
     appendRequestedOptions(offer);
 
-    tryAssignLease(offer);
+    assignLease(discover, offer);
 
     return (offer);
 }
@@ -279,13 +373,77 @@ Pkt4Ptr Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
     appendDefaultOptions(ack, DHCPACK);
     appendRequestedOptions(ack);
 
-    tryAssignLease(ack);
+    assignLease(request, ack);
 
     return (ack);
 }
 
 void Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
-    /// TODO: Implement this.
+
+    // Try to find client-id
+    ClientIdPtr client_id;
+    OptionPtr opt = release->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
+    if (opt) {
+        client_id = ClientIdPtr(new ClientId(opt->getData()));
+    }
+
+    try {
+        // Do we have a lease for that particular address?
+        Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(release->getYiaddr());
+
+        if (!lease) {
+            // No such lease - bogus release
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_NO_LEASE)
+                .arg(release->getYiaddr().toText())
+                .arg(release->getHWAddr()->toText())
+                .arg(client_id ? client_id->toText() : "(no client-id)");
+            return;
+        }
+
+        // Does the hardware address match? We don't want one client releasing
+        // second client's leases.
+        if (lease->hwaddr_ != release->getHWAddr()->hwaddr_) {
+            // @todo: Print hwaddr from lease as part of ticket #2589
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_WRONG_HWADDR)
+                .arg(release->getYiaddr().toText())
+                .arg(client_id ? client_id->toText() : "(no client-id)")
+                .arg(release->getHWAddr()->toText());
+            return;
+        }
+
+        // Does the lease have client-id info? If it has, then check it with what
+        // the client sent us.
+        if (lease->client_id_ && client_id && *lease->client_id_ != *client_id) {
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_WRONG_CLIENT_ID)
+                .arg(release->getYiaddr().toText())
+                .arg(client_id->toText())
+                .arg(lease->client_id_->toText());
+            return;
+        }
+
+        // Ok, hw and client-id match - let's release the lease.
+        if (LeaseMgrFactory::instance().deleteLease(lease->addr_)) {
+
+            // Release successful - we're done here
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE)
+                .arg(lease->addr_.toText())
+                .arg(client_id ? client_id->toText() : "(no client-id)")
+                .arg(release->getHWAddr()->toText());
+        } else {
+
+            // Release failed -
+            LOG_ERROR(dhcp4_logger, DHCP4_RELEASE_FAIL)
+                .arg(lease->addr_.toText())
+                .arg(client_id ? client_id->toText() : "(no client-id)")
+                .arg(release->getHWAddr()->toText());
+        }
+    } catch (const isc::Exception& ex) {
+        // Rethrow the exception with a bit more data.
+        LOG_ERROR(dhcp4_logger, DHCP4_RELEASE_EXCEPTION)
+            .arg(ex.what())
+            .arg(release->getYiaddr());
+    }
+
 }
 
 void Dhcpv4Srv::processDecline(Pkt4Ptr& decline) {
@@ -327,3 +485,42 @@ Dhcpv4Srv::serverReceivedPacketName(uint8_t type) {
     }
     return (UNKNOWN);
 }
+
+Subnet4Ptr Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
+
+    // Is this relayed message?
+    IOAddress relay = question->getGiaddr();
+    if (relay.toText() == "0.0.0.0") {
+
+        // Yes: Use relay address to select subnet
+        return (CfgMgr::instance().getSubnet4(relay));
+    } else {
+
+        // No: Use client's address to select subnet
+        return (CfgMgr::instance().getSubnet4(question->getRemoteAddr()));
+    }
+}
+
+void Dhcpv4Srv::sanityCheck(const Pkt4Ptr& pkt, RequirementLevel serverid) {
+    OptionPtr server_id = pkt->getOption(DHO_DHCP_SERVER_IDENTIFIER);
+    switch (serverid) {
+    case FORBIDDEN:
+        if (server_id) {
+            isc_throw(RFCViolation, "Server-id option was not expected, but "
+                      << "received in " << serverReceivedPacketName(pkt->getType()));
+        }
+        break;
+
+    case MANDATORY:
+        if (!server_id) {
+            isc_throw(RFCViolation, "Server-id option was expected, but not "
+                      " received in message "
+                      << serverReceivedPacketName(pkt->getType()));
+        }
+        break;
+
+    case OPTIONAL:
+        // do nothing here
+        ;
+    }
+}

src/bin/dhcp4/dhcp4_srv.h

@@ -18,14 +18,16 @@
 #include <dhcp/dhcp4.h>
 #include <dhcp/pkt4.h>
 #include <dhcp/option.h>
+#include <dhcpsrv/subnet.h>
+#include <dhcpsrv/alloc_engine.h>
 
 #include <boost/noncopyable.hpp>
 
 #include <iostream>
 
 namespace isc {
-
 namespace dhcp {
+
 /// @brief DHCPv4 server service.
 ///
 /// This singleton class represents DHCPv4 server. It contains all
@@ -44,6 +46,14 @@ namespace dhcp {
 class Dhcpv4Srv : public boost::noncopyable {
 
     public:
+
+    /// @brief defines if certain option may, must or must not appear
+    typedef enum {
+        FORBIDDEN,
+        MANDATORY,
+        OPTIONAL
+    } RequirementLevel;
+
     /// @brief Default constructor.
     ///
     /// Instantiates necessary services, required to run DHCPv4 server.
@@ -54,7 +64,10 @@ class Dhcpv4Srv : public boost::noncopyable {
     /// for testing purposes.
     ///
     /// @param port specifies port number to listen on
-    Dhcpv4Srv(uint16_t port = DHCP4_SERVER_PORT);
+    /// @param dbconfig Lease manager configuration string.  The default
+    ///        of the "memfile" manager is used for testing.
+    Dhcpv4Srv(uint16_t port = DHCP4_SERVER_PORT,
+              const char* dbconfig = "type=memfile");
 
     /// @brief Destructor. Used during DHCPv4 service shutdown.
     ~Dhcpv4Srv();
@@ -82,6 +95,8 @@ class Dhcpv4Srv : public boost::noncopyable {
     /// As the operation of the method does not depend on any server state, it
     /// is declared static.
     ///
+    /// @todo: This should be named static Pkt4::getName()
+    ///
     /// @param type DHCPv4 packet type
     ///
     /// @return Pointer to "const" string containing the packet name.
@@ -90,6 +105,17 @@ class Dhcpv4Srv : public boost::noncopyable {
     static const char* serverReceivedPacketName(uint8_t type);
 
 protected:
+
+    /// @brief verifies if specified packet meets RFC requirements
+    ///
+    /// Checks if mandatory option is really there, that forbidden option
+    /// is not there, and that client-id or server-id appears only once.
+    ///
+    /// @param pkt packet to be checked
+    /// @param serverid expectation regarding server-id option
+    /// @throw RFCViolation if any issues are detected
+    void sanityCheck(const Pkt4Ptr& pkt, RequirementLevel serverid);
+
     /// @brief Processes incoming DISCOVER and returns response.
     ///
     /// Processes received DISCOVER message and verifies that its sender
@@ -156,11 +182,19 @@ protected:
     /// client and assigning it. Options corresponding to the lease
     /// are added to specific message.
     ///
-    /// Note: Lease manager is not implemented yet, so this method
-    /// used fixed, hardcoded lease.
+    /// @param question DISCOVER or REQUEST message from client
+    /// @param answer OFFER or ACK/NAK message (lease options will be added here)
+    void assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer);
+
+    /// @brief Attempts to renew received addresses
+    ///
+    /// Attempts to renew existing lease. This typically includes finding a lease that
+    /// corresponds to the received address. If no such lease is found, a status code
+    /// response is generated.
     ///
-    /// @param msg OFFER or ACK message (lease options will be added here)
-    void tryAssignLease(Pkt4Ptr& msg);
+    /// @param renew client's message asking for renew
+    /// @param reply server's response (ACK or NAK)
+    void renewLease(const Pkt4Ptr& renew, Pkt4Ptr& reply);
 
     /// @brief Appends default options to a message
     ///
@@ -184,12 +218,33 @@ protected:
     //          previously stored configuration and no network interfaces available)
     void setServerID();
 
+    /// @brief Selects a subnet for a given client's packet.
+    ///
+    /// @param question client's message
+    /// @return selected subnet (or NULL if no suitable subnet was found)
+    isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& question);
+
     /// server DUID (to be sent in server-identifier option)
     OptionPtr serverid_;
 
     /// indicates if shutdown is in progress. Setting it to true will
     /// initiate server shutdown procedure.
     volatile bool shutdown_;
+
+    private:
+
+    /// @brief Constructs netmask option based on subnet4
+    /// @param subnet subnet for which the netmask will be calculated
+    ///
+    /// @return Option that contains netmask information
+    static OptionPtr getNetmaskOption(const Subnet4Ptr& subnet);
+
+    /// @brief Allocation Engine.
+    /// Pointer to the allocation engine that we are currently using
+    /// It must be a pointer, because we will support changing engines
+    /// during normal operation (e.g. to use different allocators)
+    boost::shared_ptr<AllocEngine> alloc_engine_;
+
 };
 
 }; // namespace isc::dhcp

src/bin/dhcp4/tests/dhcp4_unittests.cc

@@ -13,13 +13,16 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <log/logger_support.h>
-
+#include <dhcp4/dhcp4_log.h>
 #include <gtest/gtest.h>
 
 int
 main(int argc, char* argv[]) {
 
     ::testing::InitGoogleTest(&argc, argv);
+
+    // See the documentation of the B10_* environment variables in
+    // src/lib/log/README for info on how to tweak logging
     isc::log::initLogger();
 
     int result = RUN_ALL_TESTS();

src/bin/dhcp6/config_parser.cc

@@ -1148,7 +1148,7 @@ private:
 
         // Add pools to it.
         for (PoolStorage::iterator it = pools_.begin(); it != pools_.end(); ++it) {
-            subnet_->addPool6(*it);
+            subnet_->addPool(*it);
         }
 
         Subnet::OptionContainerPtr options = subnet_->getOptionDescriptors("dhcp6");

src/bin/dhcp6/dhcp6_messages.mes

@@ -61,9 +61,9 @@ A debug message indicating that the IPv6 DHCP server has received an
 updated configuration from the BIND 10 configuration system.
 
 % DHCP6_DB_BACKEND_STARTED lease database started (type: %1, name: %2)
-This informational message is printed every time DHCPv6 is started.
-It indicates what database backend type is being to store lease and
-other information.
+This informational message is printed every time the IPv6 DHCP server
+is started.  It indicates what database backend type is being to store
+lease and other information.
 
 % DHCP6_LEASE_WITHOUT_DUID lease for address %1 does not have a DUID
 This error message indicates a database consistency failure. The lease

src/bin/dhcp6/dhcp6_srv.cc

@@ -32,6 +32,7 @@
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/subnet.h>
+#include <dhcpsrv/utils.h>
 #include <exceptions/exceptions.h>
 #include <util/io_utilities.h>
 #include <util/range_utilities.h>
@@ -405,7 +406,7 @@ void Dhcpv6Srv::sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid,
     switch (serverid) {
     case FORBIDDEN:
         if (server_ids.size() > 0) {
-            isc_throw(RFCViolation, "Exactly 1 server-id option expected, but "
+            isc_throw(RFCViolation, "Server-id option was not expected, but "
                       << server_ids.size() << " received in " << pkt->getName());
         }
         break;
@@ -452,7 +453,10 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) {
 
         // perhaps this should be logged on some higher level? This is most likely
         // configuration bug.
-        LOG_ERROR(dhcp6_logger, DHCP6_SUBNET_SELECTION_FAILED);
+        LOG_ERROR(dhcp6_logger, DHCP6_SUBNET_SELECTION_FAILED)
+            .arg(question->getRemoteAddr().toText())
+            .arg(question->getName());
+
     } else {
         LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED)
             .arg(subnet->toText());

src/bin/dhcp6/dhcp6_srv.h

@@ -31,22 +31,6 @@
 namespace isc {
 namespace dhcp {
 
-/// An exception that is thrown if a DHCPv6 protocol violation occurs while
-/// processing a message (e.g. a mandatory option is missing)
-class RFCViolation : public isc::Exception {
-public:
-
-/// @brief constructor
-///
-/// @param file name of the file, where exception occurred
-/// @param line line of the file, where exception occurred
-/// @param what text description of the issue that caused exception
-RFCViolation(const char* file, size_t line, const char* what) :
-    isc::Exception(file, line, what) {}
-};
-
-
-
 /// @brief DHCPv6 server service.
 ///
 /// This class represents DHCPv6 server. It contains all
@@ -83,7 +67,7 @@ public:
     /// @param dbconfig Lease manager configuration string.  The default
     ///        of the "memfile" manager is used for testing.
     Dhcpv6Srv(uint16_t port = DHCP6_SERVER_PORT,
-            const char* dbconfig = "type=memfile");
+              const char* dbconfig = "type=memfile");
 
     /// @brief Destructor. Used during DHCPv6 service shutdown.
     virtual ~Dhcpv6Srv();
@@ -324,7 +308,7 @@ private:
     boost::shared_ptr<AllocEngine> alloc_engine_;
 
     /// Server DUID (to be sent in server-identifier option)
-    boost::shared_ptr<isc::dhcp::Option> serverid_;
+    OptionPtr serverid_;
 
     /// Indicates if shutdown is in progress. Setting it to true will
     /// initiate server shutdown procedure.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -29,6 +29,7 @@
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
+#include <dhcpsrv/utils.h>
 #include <util/buffer.h>
 #include <util/range_utilities.h>
 
@@ -72,8 +73,9 @@ public:
         subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 48, 1000,
                                          2000, 3000, 4000));
         pool_ = Pool6Ptr(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:1::"), 64));
-        subnet_->addPool6(pool_);
+        subnet_->addPool(pool_);
 
+        CfgMgr::instance().deleteSubnets6();
         CfgMgr::instance().addSubnet6(subnet_);
     }
 
@@ -607,7 +609,7 @@ TEST_F(Dhcpv6SrvTest, SolicitInvalidHint) {
 
 // This test checks that the server is offering different addresses to different
 // clients in ADVERTISEs. Please note that ADVERTISE is not a guarantee that such
-// and address will be assigned. Had the pool was very small and contained only
+// an address will be assigned. Had the pool was very small and contained only
 // 2 addresses, the third client would get the same advertise as the first one
 // and this is a correct behavior. It is REQUEST that will fail for the third
 // client. ADVERTISE is basically saying "if you send me a request, you will

src/bin/loadzone/tests/correct/include.db

@@ -1,8 +1,6 @@
 $ORIGIN include.   ; initialize origin
 $TTL 300
-; this needs #2500
-;@			IN SOA	ns hostmaster (
-@			IN SOA	ns.include. hostmaster.include. (
+@			IN SOA	ns hostmaster (
 				1        ; serial
 				3600
 				1800

src/bin/loadzone/tests/correct/mix1.db

@@ -1,7 +1,5 @@
 $ORIGIN mix1.
-; this needs #2500
-;@			IN SOA	ns hostmaster (
-@			IN SOA	ns.mix1. hostmaster.mix1. (
+@			IN SOA	ns hostmaster (
 				1        ; serial
 				3600
 				1800

src/bin/loadzone/tests/correct/mix2.db

@@ -1,7 +1,5 @@
 $ORIGIN mix2.
-; this needs #2500
-;@		1	IN SOA	ns hostmaster (
-@		1	IN SOA	ns.mix2. hostmaster.mix2. (
+@		1	IN SOA	ns hostmaster (
 				1        ; serial
 				3600
 				1800

src/bin/loadzone/tests/correct/ttl1.db

@@ -1,7 +1,5 @@
 $ORIGIN ttl1.
-; this needs #2500
-;@			IN SOA	ns hostmaster (
-@			IN SOA	ns.ttl1. hostmaster.ttl1. (
+@			IN SOA	ns hostmaster (
 				1        ; serial
 				3600
 				1800

src/bin/loadzone/tests/correct/ttl2.db

@@ -1,7 +1,5 @@
 $ORIGIN ttl2.
-; this needs #2500
-;@		1	IN SOA	ns hostmaster (
-@		1	IN SOA	ns.ttl2. hostmaster.ttl2 (
+@		1	IN SOA	ns hostmaster (
 				1        ; serial
 				3600
 				1800

src/bin/loadzone/tests/correct/ttlext.db

@@ -1,7 +1,5 @@
 $ORIGIN ttlext.
-; this needs #2500
-;@			IN SOA	ns hostmaster (
-@			IN SOA	ns.ttlext. hostmaster.ttlext. (
+@			IN SOA	ns hostmaster (
 				1        ; serial
 				3600
 				1800

src/bin/xfrin/tests/xfrin_test.py

@@ -60,7 +60,7 @@ TSIG_KEY = TSIGKey("example.com:SFuWd/q99SzF8Yzd1QbB9g==")
 
 # SOA intended to be used for the new SOA as a result of transfer.
 soa_rdata = Rdata(RRType.SOA(), TEST_RRCLASS,
-                  'master.example.com. admin.example.com ' +
+                  'master.example.com. admin.example.com. ' +
                   '1234 3600 1800 2419200 7200')
 soa_rrset = RRset(TEST_ZONE_NAME, TEST_RRCLASS, RRType.SOA(), RRTTL(3600))
 soa_rrset.add_rdata(soa_rdata)
@@ -68,7 +68,7 @@ soa_rrset.add_rdata(soa_rdata)
 # SOA intended to be used for the current SOA at the secondary side.
 # Note that its serial is smaller than that of soa_rdata.
 begin_soa_rdata = Rdata(RRType.SOA(), TEST_RRCLASS,
-                        'master.example.com. admin.example.com ' +
+                        'master.example.com. admin.example.com. ' +
                         '1230 3600 1800 2419200 7200')
 begin_soa_rrset = RRset(TEST_ZONE_NAME, TEST_RRCLASS, RRType.SOA(), RRTTL(3600))
 begin_soa_rrset.add_rdata(begin_soa_rdata)

src/bin/xfrout/tests/xfrout_test.py.in

@@ -249,7 +249,7 @@ class TestXfroutSessionBase(unittest.TestCase):
             # In the RDATA only the serial matters.
             for i in range(0, num_soa):
                 soa.add_rdata(Rdata(RRType.SOA(), soa_class,
-                                    'm r ' + str(ixfr) + ' 1 1 1 1'))
+                                    'm. r. ' + str(ixfr) + ' 1 1 1 1'))
             msg.add_rrset(Message.SECTION_AUTHORITY, soa)
 
         renderer = MessageRenderer()

src/lib/datasrc/tests/memory/rdata_serialization_unittest.cc

@@ -71,20 +71,20 @@ struct TestRdata {
 // unusual and corner cases).
 const TestRdata test_rdata_list[] = {
     {"IN", "A", "192.0.2.1", 0},
-    {"IN", "NS", "ns.example.com", 0},
-    {"IN", "CNAME", "cname.example.com", 0},
-    {"IN", "SOA", "ns.example.com root.example.com 0 0 0 0 0", 0},
-    {"IN", "PTR", "reverse.example.com", 0},
+    {"IN", "NS", "ns.example.com.", 0},
+    {"IN", "CNAME", "cname.example.com.", 0},
+    {"IN", "SOA", "ns.example.com. root.example.com. 0 0 0 0 0", 0},
+    {"IN", "PTR", "reverse.example.com.", 0},
     {"IN", "HINFO", "\"cpu-info\" \"OS-info\"", 1},
-    {"IN", "MINFO", "root.example.com mbox.example.com", 0},
-    {"IN", "MX", "10 mx.example.com", 0},
+    {"IN", "MINFO", "root.example.com. mbox.example.com.", 0},
+    {"IN", "MX", "10 mx.example.com.", 0},
     {"IN", "TXT", "\"test1\" \"test 2\"", 1},
-    {"IN", "RP", "root.example.com. rp-text.example.com", 0},
-    {"IN", "AFSDB", "1 afsdb.example.com", 0},
+    {"IN", "RP", "root.example.com. rp-text.example.com.", 0},
+    {"IN", "AFSDB", "1 afsdb.example.com.", 0},
     {"IN", "AAAA", "2001:db8::1", 0},
-    {"IN", "SRV", "1 0 10 target.example.com", 0},
-    {"IN", "NAPTR", "100 50 \"s\" \"http\" \"\" _http._tcp.example.com", 1},
-    {"IN", "DNAME", "dname.example.com", 0},
+    {"IN", "SRV", "1 0 10 target.example.com.", 0},
+    {"IN", "NAPTR", "100 50 \"s\" \"http\" \"\" _http._tcp.example.com.", 1},
+    {"IN", "DNAME", "dname.example.com.", 0},
     {"IN", "DS", "12892 5 2 5F0EB5C777586DE18DA6B5", 1},
     {"IN", "SSHFP", "1 1 dd465c09cfa51fb45020cc83316fff", 1},
     // We handle RRSIG separately, so it's excluded from the list
@@ -98,7 +98,7 @@ const TestRdata test_rdata_list[] = {
     {"IN", "TYPE65000", "\\# 3 010203", 1}, // some "custom" type
     {"IN", "TYPE65535", "\\# 0", 1},        // max RR type, 0-length RDATA
     {"CH", "A", "\\# 2 0102", 1}, // A RR for non-IN class; varlen data
-    {"CH", "NS", "ns.example.com", 0}, // class CH, generic data
+    {"CH", "NS", "ns.example.com.", 0}, // class CH, generic data
     {"CH", "TXT", "BIND10", 1},        // ditto
     {"HS", "A", "\\# 5 0102030405", 1}, // A RR for non-IN class; varlen data
     {NULL, NULL, NULL, 0}

src/lib/dhcp/Makefile.am

@@ -16,6 +16,7 @@ lib_LTLIBRARIES = libb10-dhcp++.la
 libb10_dhcp___la_SOURCES  =
 libb10_dhcp___la_SOURCES += dhcp6.h dhcp4.h
 libb10_dhcp___la_SOURCES += duid.cc duid.h
+libb10_dhcp___la_SOURCES += hwaddr.cc hwaddr.h
 libb10_dhcp___la_SOURCES += iface_mgr.cc iface_mgr.h
 libb10_dhcp___la_SOURCES += iface_mgr_bsd.cc
 libb10_dhcp___la_SOURCES += iface_mgr_linux.cc

src/lib/dhcp/duid.h

@@ -86,7 +86,7 @@ typedef boost::shared_ptr<DUID> DuidPtr;
 ///
 /// This class is intended to be a generic IPv4 client identifier. It can hold
 /// a client-id
-class ClientId : DUID {
+class ClientId : public DUID {
 public:
     /// @brief Maximum size of a client ID
     ///

src/lib/dhcp/hwaddr.cc

@@ -0,0 +1,62 @@
+// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <dhcp/hwaddr.h>
+#include <dhcp/dhcp4.h>
+#include <iomanip>
+#include <sstream>
+#include <vector>
+
+namespace isc {
+namespace dhcp {
+
+HWAddr::HWAddr()
+    :htype_(HTYPE_ETHER) {
+}
+
+HWAddr::HWAddr(const uint8_t* hwaddr, size_t len, uint8_t htype)
+    :hwaddr_(hwaddr, hwaddr + len), htype_(htype) {
+}
+
+HWAddr::HWAddr(const std::vector<uint8_t>& hwaddr, uint8_t htype)
+    :hwaddr_(hwaddr), htype_(htype) {
+}
+
+std::string HWAddr::toText() const {
+    std::stringstream tmp;
+    tmp << "hwtype=" << static_cast<int>(htype_) << " ";
+    tmp << std::hex;
+    bool delim = false;
+    for (std::vector<uint8_t>::const_iterator it = hwaddr_.begin();
+         it != hwaddr_.end(); ++it) {
+        if (delim) {
+            tmp << ":";
+        }
+        tmp << std::setw(2) << std::setfill('0') << static_cast<unsigned int>(*it);
+        delim = true;
+    }
+    return (tmp.str());
+}
+
+bool HWAddr::operator==(const HWAddr& other) const {
+    return ((this->htype_  == other.htype_) && 
+            (this->hwaddr_ == other.hwaddr_));
+}
+
+bool HWAddr::operator!=(const HWAddr& other) const {
+    return !(*this == other);
+}
+
+}; // end of isc::dhcp namespace
+}; // end of isc namespace

src/lib/dhcp/hwaddr.h

@@ -0,0 +1,67 @@
+// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef HWADDR_H
+#define HWADDR_H
+
+#include <vector>
+#include <stdint.h>
+#include <stddef.h>
+#include <dhcp/dhcp4.h>
+#include <boost/shared_ptr.hpp>
+
+namespace isc {
+namespace dhcp {
+
+/// @brief Hardware type that represents information from DHCPv4 packet
+struct HWAddr {
+public:
+
+    /// @brief default constructor
+    HWAddr();
+
+    /// @brief constructor, based on C-style pointer and length
+    /// @param hwaddr pointer to hardware address
+    /// @param len length of the address pointed by hwaddr
+    /// @param htype hardware type
+    HWAddr(const uint8_t* hwaddr, size_t len, uint8_t htype);
+
+    /// @brief constructor, based on C++ vector<uint8_t>
+    /// @param hwaddr const reference to hardware address
+    /// @param htype hardware type
+    HWAddr(const std::vector<uint8_t>& hwaddr, uint8_t htype);
+
+    // Vector that keeps the actual hardware address
+    std::vector<uint8_t> hwaddr_;
+
+    // Hardware type
+    uint8_t htype_;
+
+    /// @brief Returns textual representation of a client-id (e.g. 00:01:02:03)
+    std::string toText() const;
+
+    /// @brief Compares two hardware addresses for equality
+    bool operator==(const HWAddr& other) const;
+
+    /// @brief Compares two hardware addresses for inequality
+    bool operator!=(const HWAddr& other) const;
+};
+
+/// @brief Shared pointer to a hardware address structure
+typedef boost::shared_ptr<HWAddr> HWAddrPtr;
+
+}; // end of isc::dhcp namespace
+}; // end of isc namespace
+
+#endif // HWADDR_H

src/lib/dhcp/pkt4.cc

@@ -40,8 +40,7 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
       local_port_(DHCP4_SERVER_PORT),
       remote_port_(DHCP4_CLIENT_PORT),
       op_(DHCPTypeToBootpType(msg_type)),
-      htype_(HTYPE_ETHER),
-      hlen_(0),
+      hwaddr_(new HWAddr()),
       hops_(0),
       transid_(transid),
       secs_(0),
@@ -50,12 +49,12 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
       yiaddr_(DEFAULT_ADDRESS),
       siaddr_(DEFAULT_ADDRESS),
       giaddr_(DEFAULT_ADDRESS),
-      bufferOut_(DHCPV4_PKT_HDR_LEN),
-      msg_type_(msg_type)
+      bufferOut_(DHCPV4_PKT_HDR_LEN)
 {
-    memset(chaddr_, 0, MAX_CHADDR_LEN);
     memset(sname_, 0, MAX_SNAME_LEN);
     memset(file_, 0, MAX_FILE_LEN);
+
+    setType(msg_type);
 }
 
 Pkt4::Pkt4(const uint8_t* data, size_t len)
@@ -66,6 +65,7 @@ Pkt4::Pkt4(const uint8_t* data, size_t len)
       local_port_(DHCP4_SERVER_PORT),
       remote_port_(DHCP4_CLIENT_PORT),
       op_(BOOTREQUEST),
+      hwaddr_(new HWAddr()),
       transid_(0),
       secs_(0),
       flags_(0),
@@ -73,8 +73,7 @@ Pkt4::Pkt4(const uint8_t* data, size_t len)
       yiaddr_(DEFAULT_ADDRESS),
       siaddr_(DEFAULT_ADDRESS),
       giaddr_(DEFAULT_ADDRESS),
-      bufferOut_(0), // not used, this is RX packet
-      msg_type_(DHCPDISCOVER)
+      bufferOut_(0) // not used, this is RX packet
 {
     if (len < DHCPV4_PKT_HDR_LEN) {
         isc_throw(OutOfRange, "Truncated DHCPv4 packet (len=" << len
@@ -105,9 +104,15 @@ Pkt4::len() {
 
 bool
 Pkt4::pack() {
+    if (!hwaddr_) {
+        isc_throw(InvalidOperation, "Can't build Pkt4 packet. HWAddr not set.");
+    }
+
+    size_t hw_len = hwaddr_->hwaddr_.size();
+
     bufferOut_.writeUint8(op_);
-    bufferOut_.writeUint8(htype_);
-    bufferOut_.writeUint8(hlen_);
+    bufferOut_.writeUint8(hwaddr_->htype_);
+    bufferOut_.writeUint8(hw_len < MAX_CHADDR_LEN ? hw_len : MAX_CHADDR_LEN);
     bufferOut_.writeUint8(hops_);
     bufferOut_.writeUint32(transid_);
     bufferOut_.writeUint16(secs_);
@@ -116,7 +121,23 @@ Pkt4::pack() {
     bufferOut_.writeUint32(yiaddr_);
     bufferOut_.writeUint32(siaddr_);
     bufferOut_.writeUint32(giaddr_);
-    bufferOut_.writeData(chaddr_, MAX_CHADDR_LEN);
+
+
+    if (hw_len <= MAX_CHADDR_LEN) {
+        // write up to 16 bytes of the hardware address (CHADDR field is 16
+        // bytes long in DHCPv4 message).
+        bufferOut_.writeData(&hwaddr_->hwaddr_[0],
+                             (hw_len < MAX_CHADDR_LEN ? hw_len : MAX_CHADDR_LEN) );
+        hw_len = MAX_CHADDR_LEN - hw_len;
+    } else {
+        hw_len = MAX_CHADDR_LEN;
+    }
+
+    // write (len) bytes of padding
+    vector<uint8_t> zeros(hw_len, 0);
+    bufferOut_.writeData(&zeros[0], hw_len);
+    // bufferOut_.writeData(chaddr_, MAX_CHADDR_LEN);
+
     bufferOut_.writeData(sname_, MAX_SNAME_LEN);
     bufferOut_.writeData(file_, MAX_FILE_LEN);
 
@@ -145,8 +166,8 @@ Pkt4::unpack() {
     }
 
     op_ = bufferIn.readUint8();
-    htype_ = bufferIn.readUint8();
-    hlen_ = bufferIn.readUint8();
+    uint8_t htype = bufferIn.readUint8();
+    uint8_t hlen = bufferIn.readUint8();
     hops_ = bufferIn.readUint8();
     transid_ = bufferIn.readUint32();
     secs_ = bufferIn.readUint16();
@@ -155,10 +176,16 @@ Pkt4::unpack() {
     yiaddr_ = IOAddress(bufferIn.readUint32());
     siaddr_ = IOAddress(bufferIn.readUint32());
     giaddr_ = IOAddress(bufferIn.readUint32());
-    bufferIn.readData(chaddr_, MAX_CHADDR_LEN);
+
+    vector<uint8_t> hw_addr(MAX_CHADDR_LEN, 0);
+    bufferIn.readVector(hw_addr, MAX_CHADDR_LEN);
     bufferIn.readData(sname_, MAX_SNAME_LEN);
     bufferIn.readData(file_, MAX_FILE_LEN);
 
+    hw_addr.resize(hlen);
+
+    hwaddr_ = HWAddrPtr(new HWAddr(hw_addr, htype));
+
     if (bufferIn.getLength() == bufferIn.getPosition()) {
         // this is *NOT* DHCP packet. It does not have any DHCPv4 options. In
         // particular, it does not have magic cookie, a 4 byte sequence that
@@ -189,21 +216,44 @@ Pkt4::unpack() {
 }
 
 void Pkt4::check() {
+    uint8_t msg_type = getType();
+    if (msg_type > DHCPLEASEACTIVE) {
+        isc_throw(BadValue, "Invalid DHCP message type received: "
+                  << msg_type);
+    }
+}
+
+uint8_t Pkt4::getType() const {
+    OptionPtr generic = getOption(DHO_DHCP_MESSAGE_TYPE);
+    if (!generic) {
+        isc_throw(Unexpected, "Missing DHCP Message Type option");
+    }
+
+    // Check if Message Type is specified as OptionInt<uint8_t>
     boost::shared_ptr<OptionInt<uint8_t> > typeOpt =
-        boost::dynamic_pointer_cast<OptionInt<uint8_t> >(getOption(DHO_DHCP_MESSAGE_TYPE));
+        boost::dynamic_pointer_cast<OptionInt<uint8_t> >(generic);
     if (typeOpt) {
-        uint8_t msg_type = typeOpt->getValue();
-        if (msg_type > DHCPLEASEACTIVE) {
-            isc_throw(BadValue, "Invalid DHCP message type received: "
-                      << msg_type);
-        }
-        msg_type_ = msg_type;
+        return (typeOpt->getValue());
+    }
 
+    // Try to use it as generic option
+    return (generic->getUint8());
+}
+
+void Pkt4::setType(uint8_t dhcp_type) {
+    OptionPtr opt = getOption(DHO_DHCP_MESSAGE_TYPE);
+    if (opt) {
+        // There is message type option already, update it
+        opt->setUint8(dhcp_type);
     } else {
-        isc_throw(Unexpected, "Missing DHCP Message Type option");
+        // There is no message type option yet, add it
+        std::vector<uint8_t> tmp(1, dhcp_type);
+        opt = OptionPtr(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE, tmp));
+        addOption(opt);
     }
 }
 
+
 void Pkt4::repack() {
     bufferOut_.writeData(&data_[0], data_.size());
 }
@@ -213,7 +263,7 @@ Pkt4::toText() {
     stringstream tmp;
     tmp << "localAddr=" << local_addr_.toText() << ":" << local_port_
         << " remoteAddr=" << remote_addr_.toText()
-        << ":" << remote_port_ << ", msgtype=" << int(msg_type_)
+        << ":" << remote_port_ << ", msgtype=" << getType()
         << ", transid=0x" << hex << transid_ << dec << endl;
 
     for (isc::dhcp::Option::OptionCollection::iterator opt=options_.begin();
@@ -239,10 +289,15 @@ Pkt4::setHWAddr(uint8_t hType, uint8_t hlen,
         isc_throw(OutOfRange, "Invalid HW Address specified");
     }
 
-    htype_ = hType;
-    hlen_ = hlen;
-    std::copy(&mac_addr[0], &mac_addr[hlen], &chaddr_[0]);
-    std::fill(&chaddr_[hlen], &chaddr_[MAX_CHADDR_LEN], 0);
+    hwaddr_.reset(new HWAddr(mac_addr, hType));
+}
+
+void
+Pkt4::setHWAddr(const HWAddrPtr& addr) {
+    if (!addr) {
+        isc_throw(BadValue, "Setting hw address to NULL is forbidden");
+    }
+    hwaddr_ = addr;
 }
 
 void
@@ -302,6 +357,23 @@ Pkt4::DHCPTypeToBootpType(uint8_t dhcpType) {
     }
 }
 
+uint8_t
+Pkt4::getHtype() const {
+    if (!hwaddr_) {
+        isc_throw(InvalidOperation, "Can't get HType. HWAddr not defined");
+    }
+    return (hwaddr_->htype_);
+}
+
+uint8_t
+Pkt4::getHlen() const {
+    if (!hwaddr_) {
+        isc_throw(InvalidOperation, "Can't get HType. HWAddr not defined");
+    }
+    uint8_t len = hwaddr_->hwaddr_.size();
+    return (len <= MAX_CHADDR_LEN ? len : MAX_CHADDR_LEN);
+}
+
 void
 Pkt4::addOption(boost::shared_ptr<Option> opt) {
     // Check for uniqueness (DHCPv4 options must be unique)
@@ -313,7 +385,7 @@ Pkt4::addOption(boost::shared_ptr<Option> opt) {
 }
 
 boost::shared_ptr<isc::dhcp::Option>
-Pkt4::getOption(uint8_t type) {
+Pkt4::getOption(uint8_t type) const {
     Option::OptionCollection::const_iterator x = options_.find(type);
     if (x != options_.end()) {
         return (*x).second;
@@ -321,6 +393,16 @@ Pkt4::getOption(uint8_t type) {
     return boost::shared_ptr<isc::dhcp::Option>(); // NULL
 }
 
+bool
+Pkt4::delOption(uint8_t type) {
+    isc::dhcp::Option::OptionCollection::iterator x = options_.find(type);
+    if (x != options_.end()) {
+        options_.erase(x);
+        return (true); // delete successful
+    }
+    return (false); // can't find option to be deleted
+}
+
 void
 Pkt4::updateTimestamp() {
     timestamp_ = boost::posix_time::microsec_clock::universal_time();

src/lib/dhcp/pkt4.h

@@ -18,6 +18,7 @@
 #include <asiolink/io_address.h>
 #include <util/buffer.h>
 #include <dhcp/option.h>
+#include <dhcp/hwaddr.h>
 
 #include <boost/date_time/posix_time/posix_time.hpp>
 #include <boost/shared_ptr.hpp>
@@ -217,16 +218,15 @@ public:
     /// @return transaction-id
     uint32_t getTransid() const { return (transid_); };
 
-    /// @brief Returns message type (e.g. 1 = DHCPDISCOVER).
+    /// @brief Returns DHCP message type (e.g. 1 = DHCPDISCOVER).
     ///
     /// @return message type
-    uint8_t
-    getType() const { return (msg_type_); }
+    uint8_t getType() const;
 
-    /// @brief Sets message type (e.g. 1 = DHCPDISCOVER).
+    /// @brief Sets DHCP message type (e.g. 1 = DHCPDISCOVER).
     ///
     /// @param type message type to be set
-    void setType(uint8_t type) { msg_type_=type; };
+    void setType(uint8_t type);
 
     /// @brief Returns sname field
     ///
@@ -273,27 +273,28 @@ public:
     void setHWAddr(uint8_t hType, uint8_t hlen,
                    const std::vector<uint8_t>& mac_addr);
 
+    /// @brief Sets hardware address
+    ///
+    /// Sets hardware address, based on existing HWAddr structure
+    /// @param addr already filled in HWAddr structure
+    /// @throw BadValue if addr is null
+    void setHWAddr(const HWAddrPtr& addr);
+
     /// Returns htype field
     ///
     /// @return hardware type
     uint8_t
-    getHtype() const { return (htype_); };
+    getHtype() const;
 
     /// Returns hlen field
     ///
     /// @return hardware address length
     uint8_t
-    getHlen() const { return (hlen_); };
-
-    /// @brief Returns chaddr field.
-    ///
-    /// Note: This is 16 bytes long field. It doesn't have to be
-    /// null-terminated. Do no use strlen() or similar on it.
-    ///
-    /// @return pointer to hardware address
-    const uint8_t*
-    getChaddr() const { return (chaddr_); };
+    getHlen() const;
 
+    /// @brief returns hardware address information
+    /// @return hardware address structure
+    HWAddrPtr getHWAddr() const { return (hwaddr_); }
 
     /// @brief Returns reference to output buffer.
     ///
@@ -321,7 +322,12 @@ public:
     /// @return returns option of requested type (or NULL)
     ///         if no such option is present
     boost::shared_ptr<Option>
-    getOption(uint8_t opt_type);
+    getOption(uint8_t opt_type) const;
+
+    /// @brief Deletes specified option
+    /// @param type option type to be deleted
+    /// @return true if anything was deleted, false otherwise
+    bool delOption(uint8_t type);
 
     /// @brief Returns interface name.
     ///
@@ -454,11 +460,11 @@ protected:
     /// type is kept in message type option).
     uint8_t op_;
 
-    /// link-layer address type
-    uint8_t htype_;
-
-    /// link-layer address length
-    uint8_t hlen_;
+    /// @brief link-layer address and hardware information
+    /// represents 3 fields: htype (hardware type, 1 byte), hlen (length of the
+    /// hardware address, up to 16) and chaddr (hardware address field,
+    /// 16 bytes)
+    HWAddrPtr hwaddr_;
 
     /// Number of relay agents traversed
     uint8_t hops_;
@@ -484,9 +490,6 @@ protected:
     /// giaddr field (32 bits): Gateway IP address
     isc::asiolink::IOAddress giaddr_;
 
-    /// Hardware address field (16 bytes)
-    uint8_t chaddr_[MAX_CHADDR_LEN];
-
     /// sname field (64 bytes)
     uint8_t sname_[MAX_SNAME_LEN];
 
@@ -518,16 +521,11 @@ protected:
     /// data format change etc.
     std::vector<uint8_t> data_;
 
-    /// message type (e.g. 1=DHCPDISCOVER)
-    /// @todo this will eventually be replaced with DHCP Message Type
-    /// option (option 53)
-    uint8_t msg_type_;
-
     /// collection of options present in this message
     ///
     /// @warning This protected member is accessed by derived
     /// classes directly. One of such derived classes is
-    /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
+    /// @ref perfdhcp::PerfPkt4. The impact on derived classes'
     /// behavior must be taken into consideration before making
     /// changes to this member such as access scope restriction or
     /// data format change etc.

src/lib/dhcp/tests/Makefile.am

@@ -27,6 +27,7 @@ if HAVE_GTEST
 TESTS += libdhcp++_unittests
 
 libdhcp___unittests_SOURCES  = run_unittests.cc
+libdhcp___unittests_SOURCES += hwaddr_unittest.cc
 libdhcp___unittests_SOURCES += iface_mgr_unittest.cc
 libdhcp___unittests_SOURCES += libdhcp++_unittest.cc
 libdhcp___unittests_SOURCES += option4_addrlst_unittest.cc

src/lib/dhcp/tests/hwaddr_unittest.cc

@@ -0,0 +1,126 @@
+// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <config.h>
+
+#include <asiolink/io_address.h>
+#include <dhcp/hwaddr.h>
+#include <dhcp/dhcp4.h>
+#include <exceptions/exceptions.h>
+
+#include <boost/scoped_ptr.hpp>
+#include <gtest/gtest.h>
+
+#include <iostream>
+#include <sstream>
+
+using namespace std;
+using namespace isc;
+using namespace isc::dhcp;
+using namespace isc::asiolink;
+
+using boost::scoped_ptr;
+
+namespace {
+
+// This test verifies if the constructors are working as expected
+// and process passed parameters.
+TEST(HWAddrTest, constructor) {
+
+    const uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};
+    const uint8_t htype = HTYPE_ETHER;
+
+    vector<uint8_t> data2(data1, data1 + sizeof(data1));
+
+    scoped_ptr<HWAddr> hwaddr1(new HWAddr(data1, sizeof(data1), htype));
+    scoped_ptr<HWAddr> hwaddr2(new HWAddr(data2, htype));
+    scoped_ptr<HWAddr> hwaddr3(new HWAddr());
+
+    EXPECT_TRUE(data2 == hwaddr1->hwaddr_);
+    EXPECT_EQ(htype, hwaddr1->htype_);
+
+    EXPECT_TRUE(data2 == hwaddr2->hwaddr_);
+    EXPECT_EQ(htype, hwaddr2->htype_);
+
+    EXPECT_EQ(0, hwaddr3->hwaddr_.size());
+    EXPECT_EQ(htype, hwaddr3->htype_);
+}
+
+// This test checks if the comparison operators are sane.
+TEST(HWAddrTest, operators) {
+    uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};
+    uint8_t data2[] = {0, 1, 2, 3, 4};
+    uint8_t data3[] = {0, 1, 2, 3, 4, 5, 7}; // last digit different
+    uint8_t data4[] = {0, 1, 2, 3, 4, 5, 6}; // the same as 1
+
+    uint8_t htype1 = HTYPE_ETHER;
+    uint8_t htype2 = HTYPE_FDDI;
+
+    scoped_ptr<HWAddr> hw1(new HWAddr(data1, sizeof(data1), htype1));
+    scoped_ptr<HWAddr> hw2(new HWAddr(data2, sizeof(data2), htype1));
+    scoped_ptr<HWAddr> hw3(new HWAddr(data3, sizeof(data3), htype1));
+    scoped_ptr<HWAddr> hw4(new HWAddr(data4, sizeof(data4), htype1));
+
+    // MAC address the same as data1 and data4, but different hardware type
+    scoped_ptr<HWAddr> hw5(new HWAddr(data4, sizeof(data4), htype2));
+
+    EXPECT_TRUE(*hw1 == *hw4);
+    EXPECT_FALSE(*hw1 == *hw2);
+    EXPECT_FALSE(*hw1 == *hw3);
+
+    EXPECT_FALSE(*hw1 != *hw4);
+    EXPECT_TRUE(*hw1 != *hw2);
+    EXPECT_TRUE(*hw1 != *hw3);
+
+    EXPECT_FALSE(*hw1 == *hw5);
+    EXPECT_FALSE(*hw4 == *hw5);
+
+    EXPECT_TRUE(*hw1 != *hw5);
+    EXPECT_TRUE(*hw4 != *hw5);
+}
+
+// Checks that toText() method produces appropriate text representation
+TEST(HWAddrTest, toText) {
+    uint8_t data[] = {0, 1, 2, 3, 4, 5};
+    uint8_t htype = 15;
+
+    HWAddrPtr hw(new HWAddr(data, sizeof(data), htype));
+
+    EXPECT_EQ("hwtype=15 00:01:02:03:04:05", hw->toText());
+
+}
+
+TEST(HWAddrTest, stringConversion) {
+
+    // Check that an empty vector returns an appropriate string
+    HWAddr hwaddr;
+    std::string result = hwaddr.toText();
+    EXPECT_EQ(std::string("hwtype=1 "), result);
+
+    // ... that a single-byte string is OK
+    hwaddr.hwaddr_.push_back(0xc3);
+    result = hwaddr.toText();
+    EXPECT_EQ(std::string("hwtype=1 c3"), result);
+
+    // ... and that a multi-byte string works
+    hwaddr.hwaddr_.push_back(0x7);
+    hwaddr.hwaddr_.push_back(0xa2);
+    hwaddr.hwaddr_.push_back(0xe8);
+    hwaddr.hwaddr_.push_back(0x42);
+    result = hwaddr.toText();
+    EXPECT_EQ(std::string("hwtype=1 c3:07:a2:e8:42"), result);
+}
+
+
+} // end of anonymous namespace

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -702,11 +702,9 @@ TEST_F(IfaceMgrTest, sendReceive4) {
     sendPkt->setYiaddr(IOAddress("192.0.2.3"));
     sendPkt->setGiaddr(IOAddress("192.0.2.4"));
 
-    // unpack() now checks if mandatory DHCP_MESSAGE_TYPE is present
-    boost::shared_ptr<Option> msgType(new Option(Option::V4,
-           static_cast<uint16_t>(DHO_DHCP_MESSAGE_TYPE)));
-    msgType->setUint8(static_cast<uint8_t>(DHCPDISCOVER));
-    sendPkt->addOption(msgType);
+    // Unpack() now checks if mandatory DHCP_MESSAGE_TYPE is present.
+    // Workarounds (creating DHCP Message Type Option by hand) are no longer
+    // needed as setDhcpType() is called in constructor.
 
     uint8_t sname[] = "That's just a string that will act as SNAME";
     sendPkt->setSname(sname, strlen((const char*)sname));
@@ -744,7 +742,6 @@ TEST_F(IfaceMgrTest, sendReceive4) {
     EXPECT_EQ(sendPkt->getYiaddr(), rcvPkt->getYiaddr());
     EXPECT_EQ(sendPkt->getGiaddr(), rcvPkt->getGiaddr());
     EXPECT_EQ(sendPkt->getTransid(), rcvPkt->getTransid());
-    EXPECT_EQ(sendPkt->getType(), rcvPkt->getType());
     EXPECT_TRUE(sendPkt->getSname() == rcvPkt->getSname());
     EXPECT_TRUE(sendPkt->getFile() == rcvPkt->getFile());
     EXPECT_EQ(sendPkt->getHtype(), rcvPkt->getHtype());

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -69,8 +69,9 @@ TEST(Pkt4Test, constructor) {
         pkt = new Pkt4(DHCPDISCOVER, 0xffffffff);
     );
 
-    // DHCPv4 packet must be at least 236 bytes long
-    EXPECT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN), pkt->len());
+    // DHCPv4 packet must be at least 236 bytes long, with Message Type
+    // Option taking extra 3 bytes it is 239
+    EXPECT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN) + 3, pkt->len());
     EXPECT_EQ(DHCPDISCOVER, pkt->getType());
     EXPECT_EQ(0xffffffff, pkt->getTransid());
     EXPECT_NO_THROW(
@@ -219,8 +220,10 @@ TEST(Pkt4Test, fixedFields) {
     EXPECT_EQ(dummySiaddr.toText(), pkt->getSiaddr().toText());
     EXPECT_EQ(dummyGiaddr.toText(), pkt->getGiaddr().toText());
 
-    // chaddr is always 16 bytes long and contains link-layer addr (MAC)
-    EXPECT_EQ(0, memcmp(dummyChaddr, pkt->getChaddr(), 16));
+    // Chaddr contains link-layer addr (MAC). It is no longer always 16 bytes
+    // long and its length depends on hlen value (it is up to 16 bytes now).
+    ASSERT_EQ(pkt->getHWAddr()->hwaddr_.size(), dummyHlen);
+    EXPECT_EQ(0, memcmp(dummyChaddr, &pkt->getHWAddr()->hwaddr_[0], dummyHlen));
 
     EXPECT_EQ(0, memcmp(dummySname, &pkt->getSname()[0], 64));
 
@@ -237,7 +240,9 @@ TEST(Pkt4Test, fixedFieldsPack) {
         pkt->pack();
     );
 
-    ASSERT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN), pkt->len());
+    // Minimum packet size is 236 bytes + 3 bytes of mandatory
+    // DHCP Message Type Option
+    ASSERT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN) + 3, pkt->len());
 
     // redundant but MUCH easier for debug in gdb
     const uint8_t* exp = &expectedFormat[0];
@@ -282,7 +287,7 @@ TEST(Pkt4Test, fixedFieldsUnpack) {
     EXPECT_EQ(string("255.255.255.255"), pkt->getGiaddr().toText());
 
     // chaddr is always 16 bytes long and contains link-layer addr (MAC)
-    EXPECT_EQ(0, memcmp(dummyChaddr, pkt->getChaddr(), Pkt4::MAX_CHADDR_LEN));
+    EXPECT_EQ(0, memcmp(dummyChaddr, &pkt->getHWAddr()->hwaddr_[0], dummyHlen));
 
     ASSERT_EQ(static_cast<size_t>(Pkt4::MAX_SNAME_LEN), pkt->getSname().size());
     EXPECT_EQ(0, memcmp(dummySname, &pkt->getSname()[0], Pkt4::MAX_SNAME_LEN));
@@ -321,7 +326,7 @@ TEST(Pkt4Test, hwAddr) {
         pkt->setHWAddr(255-macLen*10, // just weird htype
                        macLen,
                        mac);
-        EXPECT_EQ(0, memcmp(expectedChaddr, pkt->getChaddr(),
+        EXPECT_EQ(0, memcmp(expectedChaddr, &pkt->getHWAddr()->hwaddr_[0],
                             Pkt4::MAX_CHADDR_LEN));
 
         EXPECT_NO_THROW(
@@ -469,7 +474,7 @@ TEST(Pkt4Test, file) {
 static uint8_t v4Opts[] = {
     12,  3, 0,   1,  2, // Hostname
     14,  3, 10, 11, 12, // Merit Dump File
-    53, 1, 1, // Message Type (required to not throw exception during unpack)
+    53, 1, 2, // Message Type (required to not throw exception during unpack)
     60,  3, 20, 21, 22, // Class Id
     128, 3, 30, 31, 32, // Vendor specific
     254, 3, 40, 41, 42, // Reserved
@@ -487,18 +492,15 @@ TEST(Pkt4Test, options) {
 
     boost::shared_ptr<Option> opt1(new Option(Option::V4, 12, payload[0]));
     boost::shared_ptr<Option> opt3(new Option(Option::V4, 14, payload[1]));
-    boost::shared_ptr<Option> optMsgType(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE));
     boost::shared_ptr<Option> opt2(new Option(Option::V4, 60, payload[2]));
     boost::shared_ptr<Option> opt5(new Option(Option::V4,128, payload[3]));
     boost::shared_ptr<Option> opt4(new Option(Option::V4,254, payload[4]));
-    optMsgType->setUint8(static_cast<uint8_t>(DHCPDISCOVER));
 
     pkt->addOption(opt1);
     pkt->addOption(opt2);
     pkt->addOption(opt3);
     pkt->addOption(opt4);
     pkt->addOption(opt5);
-    pkt->addOption(optMsgType);
 
     EXPECT_TRUE(pkt->getOption(12));
     EXPECT_TRUE(pkt->getOption(60));
@@ -530,6 +532,12 @@ TEST(Pkt4Test, options) {
     EXPECT_EQ(0, memcmp(ptr, v4Opts, sizeof(v4Opts)));
     EXPECT_EQ(DHO_END, static_cast<uint8_t>(*(ptr + sizeof(v4Opts))));
 
+    // delOption() checks
+    EXPECT_TRUE(pkt->getOption(12)); // Sanity check: option 12 is still there
+    EXPECT_TRUE(pkt->delOption(12)); // We should be able to remove it
+    EXPECT_FALSE(pkt->getOption(12)); // It should not be there anymore
+    EXPECT_FALSE(pkt->delOption(12)); // And removal should fail
+
     EXPECT_NO_THROW(
         delete pkt;
     );
@@ -643,6 +651,23 @@ TEST(Pkt4Test, Timestamp) {
     EXPECT_TRUE(ts_period.length().total_microseconds() >= 0);
 }
 
+TEST(Pkt4Test, hwaddr) {
+    scoped_ptr<Pkt4> pkt(new Pkt4(DHCPOFFER, 1234));
+    const uint8_t hw[] = { 2, 4, 6, 8, 10, 12 }; // MAC
+    const uint8_t hw_type = 123; // hardware type
+
+    HWAddrPtr hwaddr(new HWAddr(hw, sizeof(hw), hw_type));
 
+    // setting NULL hardware address is not allowed
+    EXPECT_THROW(pkt->setHWAddr(HWAddrPtr()), BadValue);
+
+    pkt->setHWAddr(hwaddr);
+
+    EXPECT_EQ(hw_type, pkt->getHtype());
+
+    EXPECT_EQ(sizeof(hw), pkt->getHlen());
+
+    EXPECT_TRUE(hwaddr == pkt->getHWAddr());
+}
 
 } // end of anonymous namespace

src/lib/dhcpsrv/Makefile.am

@@ -36,7 +36,6 @@ libb10_dhcpsrv_la_SOURCES += alloc_engine.cc alloc_engine.h
 libb10_dhcpsrv_la_SOURCES += dhcpsrv_log.cc dhcpsrv_log.h
 libb10_dhcpsrv_la_SOURCES += cfgmgr.cc cfgmgr.h
 libb10_dhcpsrv_la_SOURCES += dhcp_config_parser.h
-libb10_dhcpsrv_la_SOURCES += hwaddr.cc hwaddr.h
 libb10_dhcpsrv_la_SOURCES += lease_mgr.cc lease_mgr.h
 libb10_dhcpsrv_la_SOURCES += lease_mgr_factory.cc lease_mgr_factory.h
 libb10_dhcpsrv_la_SOURCES += memfile_lease_mgr.cc memfile_lease_mgr.h

src/lib/dhcpsrv/addr_utilities.cc

@@ -195,5 +195,15 @@ isc::asiolink::IOAddress lastAddrInPrefix(const isc::asiolink::IOAddress& prefix
     }
 }
 
+isc::asiolink::IOAddress getNetmask4(uint8_t len) {
+    if (len > 32) {
+        isc_throw(BadValue, "Invalid netmask size " << len << ", allowed range "
+                  "is 0..32");
+    }
+    uint32_t x = ~bitMask4[len];
+
+    return (IOAddress(x));
+}
+
 };
 };

src/lib/dhcpsrv/addr_utilities.h

@@ -52,6 +52,11 @@ isc::asiolink::IOAddress firstAddrInPrefix(const isc::asiolink::IOAddress& prefi
 isc::asiolink::IOAddress lastAddrInPrefix(const isc::asiolink::IOAddress& prefix,
                                            uint8_t len);
 
+/// @brief generates an IPv4 netmask of specified length
+/// @throw BadValue if len is greater than 32
+/// @return netmask
+isc::asiolink::IOAddress getNetmask4(uint8_t len);
+
 };
 };
 

src/lib/dhcpsrv/alloc_engine.cc

@@ -16,7 +16,7 @@
 #include <dhcpsrv/lease_mgr_factory.h>
 
 #include <cstring>
-
+#include <vector>
 #include <string.h>
 
 using namespace isc::asiolink;
@@ -58,7 +58,7 @@ AllocEngine::IterativeAllocator::increaseAddress(const isc::asiolink::IOAddress&
 
 
 isc::asiolink::IOAddress
-AllocEngine::IterativeAllocator::pickAddress(const Subnet6Ptr& subnet,
+AllocEngine::IterativeAllocator::pickAddress(const SubnetPtr& subnet,
                                              const DuidPtr&,
                                              const IOAddress&) {
 
@@ -67,14 +67,14 @@ AllocEngine::IterativeAllocator::pickAddress(const Subnet6Ptr& subnet,
     // perhaps restaring the server).
     IOAddress last = subnet->getLastAllocated();
 
-    const Pool6Collection& pools = subnet->getPools();
+    const PoolCollection& pools = subnet->getPools();
 
     if (pools.empty()) {
         isc_throw(AllocFailed, "No pools defined in selected subnet");
     }
 
     // first we need to find a pool the last address belongs to.
-    Pool6Collection::const_iterator it;
+    PoolCollection::const_iterator it;
     for (it = pools.begin(); it != pools.end(); ++it) {
         if ((*it)->inRange(last)) {
             break;
@@ -124,9 +124,9 @@ AllocEngine::HashedAllocator::HashedAllocator()
 
 
 isc::asiolink::IOAddress
-AllocEngine::HashedAllocator::pickAddress(const Subnet6Ptr&,
-                                             const DuidPtr&,
-                                             const IOAddress&) {
+AllocEngine::HashedAllocator::pickAddress(const SubnetPtr&,
+                                          const DuidPtr&,
+                                          const IOAddress&) {
     isc_throw(NotImplemented, "Hashed allocator is not implemented");
 }
 
@@ -137,9 +137,9 @@ AllocEngine::RandomAllocator::RandomAllocator()
 
 
 isc::asiolink::IOAddress
-AllocEngine::RandomAllocator::pickAddress(const Subnet6Ptr&,
-                                             const DuidPtr&,
-                                             const IOAddress&) {
+AllocEngine::RandomAllocator::pickAddress(const SubnetPtr&,
+                                          const DuidPtr&,
+                                          const IOAddress&) {
     isc_throw(NotImplemented, "Random allocator is not implemented");
 }
 
@@ -191,7 +191,7 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
             /// implemented
 
             // the hint is valid and not currently used, let's create a lease for it
-            Lease6Ptr lease = createLease(subnet, duid, iaid, hint, fake_allocation);
+            Lease6Ptr lease = createLease6(subnet, duid, iaid, hint, fake_allocation);
 
             // It can happen that the lease allocation failed (we could have lost
             // the race condition. That means that the hint is lo longer usable and
@@ -235,7 +235,7 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
         if (!existing) {
             // there's no existing lease for selected candidate, so it is
             // free. Let's allocate it.
-            Lease6Ptr lease = createLease(subnet, duid, iaid, candidate,
+            Lease6Ptr lease = createLease6(subnet, duid, iaid, candidate,
                                           fake_allocation);
             if (lease) {
                 return (lease);
@@ -260,6 +260,146 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
               << " tries");
 }
 
+Lease4Ptr
+AllocEngine::allocateAddress4(const SubnetPtr& subnet,
+                              const ClientIdPtr& clientid,
+                              const HWAddrPtr& hwaddr,
+                              const IOAddress& hint,
+                              bool fake_allocation /* = false */ ) {
+
+    // Allocator is always created in AllocEngine constructor and there is
+    // currently no other way to set it, so that check is not really necessary.
+    if (!allocator_) {
+        isc_throw(InvalidOperation, "No allocator selected");
+    }
+
+    // Check if there's existing lease for that subnet/clientid/hwaddr combination.
+    Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(hwaddr->hwaddr_, subnet->getID());
+    if (existing) {
+        // We have a lease already. This is a returning client, probably after
+        // its reboot.
+        existing = renewLease4(subnet, clientid, hwaddr, existing, fake_allocation);
+        if (existing) {
+            return (existing);
+        }
+
+        // If renewal failed (e.g. the lease no longer matches current configuration)
+        // let's continue the allocation process
+    }
+
+    if (clientid) {
+        existing = LeaseMgrFactory::instance().getLease4(*clientid, subnet->getID());
+        if (existing) {
+            // we have a lease already. This is a returning client, probably after
+            // its reboot.
+            existing = renewLease4(subnet, clientid, hwaddr, existing, fake_allocation);
+            // @todo: produce a warning. We haven't found him using MAC address, but
+            // we found him using client-id
+            if (existing) {
+                return (existing);
+            }
+        }
+    }
+
+    // check if the hint is in pool and is available
+    if (subnet->inPool(hint)) {
+        existing = LeaseMgrFactory::instance().getLease4(hint);
+        if (!existing) {
+            /// @todo: Check if the hint is reserved once we have host support
+            /// implemented
+
+            // The hint is valid and not currently used, let's create a lease for it
+            Lease4Ptr lease = createLease4(subnet, clientid, hwaddr, hint, fake_allocation);
+
+            // It can happen that the lease allocation failed (we could have lost
+            // the race condition. That means that the hint is lo longer usable and
+            // we need to continue the regular allocation path.
+            if (lease) {
+                return (lease);
+            }
+        } else {
+            if (existing->expired()) {
+                return (reuseExpiredLease(existing, subnet, clientid, hwaddr,
+                                          fake_allocation));
+            }
+
+        }
+    }
+
+    // Hint is in the pool but is not available. Search the pool until first of
+    // the following occurs:
+    // - we find a free address
+    // - we find an address for which the lease has expired
+    // - we exhaust the number of tries
+    //
+    // @todo: Current code does not handle pool exhaustion well. It will be
+    // improved. Current problems:
+    // 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
+    // 10 addresses), we will iterate over it 100 times before giving up
+    // 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
+    // 3. the whole concept of infinite attempts is just asking for infinite loop
+    // We may consider some form or reference counting (this pool has X addresses
+    // left), but this has one major problem. We exactly control allocation
+    // moment, but we currently do not control expiration time at all
+
+    unsigned int i = attempts_;
+    do {
+        IOAddress candidate = allocator_->pickAddress(subnet, clientid, hint);
+
+        /// @todo: check if the address is reserved once we have host support
+        /// implemented
+
+        Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(candidate);
+        if (!existing) {
+            // there's no existing lease for selected candidate, so it is
+            // free. Let's allocate it.
+            Lease4Ptr lease = createLease4(subnet, clientid, hwaddr, candidate,
+                                          fake_allocation);
+            if (lease) {
+                return (lease);
+            }
+
+            // Although the address was free just microseconds ago, it may have
+            // been taken just now. If the lease insertion fails, we continue
+            // allocation attempts.
+        } else {
+            if (existing->expired()) {
+                return (reuseExpiredLease(existing, subnet, clientid, hwaddr,
+                                          fake_allocation));
+            }
+        }
+
+        // Continue trying allocation until we run out of attempts
+        // (or attempts are set to 0, which means infinite)
+        --i;
+    } while ( i || !attempts_);
+
+    isc_throw(AllocFailed, "Failed to allocate address after " << attempts_
+              << " tries");
+}
+
+Lease4Ptr AllocEngine::renewLease4(const SubnetPtr& subnet,
+                                   const ClientIdPtr& clientid,
+                                   const HWAddrPtr& hwaddr,
+                                   const Lease4Ptr& lease,
+                                   bool fake_allocation /* = false */) {
+
+    lease->subnet_id_ = subnet->getID();
+    lease->hwaddr_ = hwaddr->hwaddr_;
+    lease->client_id_ = clientid;
+    lease->cltt_ = time(NULL);
+    lease->t1_ = subnet->getT1();
+    lease->t2_ = subnet->getT2();
+    lease->valid_lft_ = subnet->getValid();
+
+    if (!fake_allocation) {
+        // for REQUEST we do update the lease
+        LeaseMgrFactory::instance().updateLease4(lease);
+    }
+
+    return (lease);
+}
+
 Lease6Ptr AllocEngine::reuseExpiredLease(Lease6Ptr& expired,
                                          const Subnet6Ptr& subnet,
                                          const DuidPtr& duid,
@@ -300,11 +440,50 @@ Lease6Ptr AllocEngine::reuseExpiredLease(Lease6Ptr& expired,
     return (expired);
 }
 
-Lease6Ptr AllocEngine::createLease(const Subnet6Ptr& subnet,
-                                   const DuidPtr& duid,
-                                   uint32_t iaid,
-                                   const IOAddress& addr,
-                                   bool fake_allocation /*= false */ ) {
+Lease4Ptr AllocEngine::reuseExpiredLease(Lease4Ptr& expired,
+                                         const SubnetPtr& subnet,
+                                         const ClientIdPtr& clientid,
+                                         const HWAddrPtr& hwaddr,
+                                         bool fake_allocation /*= false */ ) {
+
+    if (!expired->expired()) {
+        isc_throw(BadValue, "Attempt to recycle lease that is still valid");
+    }
+
+    // address, lease type and prefixlen (0) stay the same
+    expired->client_id_ = clientid;
+    expired->hwaddr_ = hwaddr->hwaddr_;
+    expired->valid_lft_ = subnet->getValid();
+    expired->t1_ = subnet->getT1();
+    expired->t2_ = subnet->getT2();
+    expired->cltt_ = time(NULL);
+    expired->subnet_id_ = subnet->getID();
+    expired->fixed_ = false;
+    expired->hostname_ = std::string("");
+    expired->fqdn_fwd_ = false;
+    expired->fqdn_rev_ = false;
+
+    /// @todo: log here that the lease was reused (there's ticket #2524 for
+    /// logging in libdhcpsrv)
+
+    if (!fake_allocation) {
+        // for REQUEST we do update the lease
+        LeaseMgrFactory::instance().updateLease4(expired);
+    }
+
+    // We do nothing for SOLICIT. We'll just update database when
+    // the client gets back to us with REQUEST message.
+
+    // it's not really expired at this stage anymore - let's return it as
+    // an updated lease
+    return (expired);
+}
+
+Lease6Ptr AllocEngine::createLease6(const Subnet6Ptr& subnet,
+                                    const DuidPtr& duid,
+                                    uint32_t iaid,
+                                    const IOAddress& addr,
+                                    bool fake_allocation /*= false */ ) {
 
     Lease6Ptr lease(new Lease6(Lease6::LEASE_IA_NA, addr, duid, iaid,
                                subnet->getPreferred(), subnet->getValid(),
@@ -338,6 +517,53 @@ Lease6Ptr AllocEngine::createLease(const Subnet6Ptr& subnet,
     }
 }
 
+Lease4Ptr AllocEngine::createLease4(const SubnetPtr& subnet,
+                                    const DuidPtr& clientid,
+                                    const HWAddrPtr& hwaddr,
+                                    const IOAddress& addr,
+                                    bool fake_allocation /*= false */ ) {
+    if (!hwaddr) {
+        isc_throw(BadValue, "Can't create a lease with NULL HW address");
+    }
+    time_t now = time(NULL);
+
+    // @todo: remove this kludge after ticket #2590 is implemented
+    std::vector<uint8_t> local_copy;
+    if (clientid) {
+        local_copy = clientid->getDuid();
+    }
+
+    Lease4Ptr lease(new Lease4(addr, &hwaddr->hwaddr_[0], hwaddr->hwaddr_.size(),
+                               &local_copy[0], local_copy.size(), subnet->getValid(),
+                               subnet->getT1(), subnet->getT2(), now,
+                               subnet->getID()));
+
+    if (!fake_allocation) {
+        // That is a real (REQUEST) allocation
+        bool status = LeaseMgrFactory::instance().addLease(lease);
+        if (status) {
+            return (lease);
+        } else {
+            // One of many failures with LeaseMgr (e.g. lost connection to the
+            // database, database failed etc.). One notable case for that
+            // is that we are working in multi-process mode and we lost a race
+            // (some other process got that address first)
+            return (Lease4Ptr());
+        }
+    } else {
+        // That is only fake (DISCOVER) allocation
+
+        // It is for OFFER only. We should not insert the lease into LeaseMgr,
+        // but rather check that we could have inserted it.
+        Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(addr);
+        if (!existing) {
+            return (lease);
+        } else {
+            return (Lease4Ptr());
+        }
+    }
+}
+
 AllocEngine::~AllocEngine() {
     // no need to delete allocator. smart_ptr will do the trick for us
 }

src/lib/dhcpsrv/alloc_engine.h

@@ -17,6 +17,7 @@
 
 #include <asiolink/io_address.h>
 #include <dhcp/duid.h>
+#include <dhcp/hwaddr.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/lease_mgr.h>
 
@@ -65,8 +66,14 @@ protected:
         /// reserved - AllocEngine will check that and will call pickAddress
         /// again if necessary. The number of times this method is called will
         /// increase as the number of available leases will decrease.
+        ///
+        /// @param subnet next address will be returned from pool of that subnet
+        /// @param duid Client's DUID
+        /// @param hint client's hint
+        ///
+        /// @return the next address
         virtual isc::asiolink::IOAddress
-        pickAddress(const Subnet6Ptr& subnet, const DuidPtr& duid,
+        pickAddress(const SubnetPtr& subnet, const DuidPtr& duid,
                     const isc::asiolink::IOAddress& hint) = 0;
 
         /// @brief virtual destructor
@@ -96,7 +103,7 @@ protected:
         /// @param hint client's hint (ignored)
         /// @return the next address
         virtual isc::asiolink::IOAddress
-            pickAddress(const Subnet6Ptr& subnet,
+            pickAddress(const SubnetPtr& subnet,
                         const DuidPtr& duid,
                         const isc::asiolink::IOAddress& hint);
     private:
@@ -125,7 +132,7 @@ protected:
         /// @param duid Client's DUID
         /// @param hint a hint (last address that was picked)
         /// @return selected address
-        virtual isc::asiolink::IOAddress pickAddress(const Subnet6Ptr& subnet,
+        virtual isc::asiolink::IOAddress pickAddress(const SubnetPtr& subnet,
                                                      const DuidPtr& duid,
                                                      const isc::asiolink::IOAddress& hint);
     };
@@ -148,7 +155,7 @@ protected:
         /// @param hint the last address that was picked (ignored)
         /// @return a random address from the pool
         virtual isc::asiolink::IOAddress
-        pickAddress(const Subnet6Ptr& subnet, const DuidPtr& duid,
+        pickAddress(const SubnetPtr& subnet, const DuidPtr& duid,
                     const isc::asiolink::IOAddress& hint);
     };
 
@@ -174,6 +181,48 @@ protected:
     ///        we give up (0 means unlimited)
     AllocEngine(AllocType engine_type, unsigned int attempts);
 
+    /// @brief Allocates an IPv4 lease
+    ///
+    /// This method uses currently selected allocator to pick an address from
+    /// specified subnet, creates a lease for that address and then inserts
+    /// it into LeaseMgr (if this allocation is not fake).
+    ///
+    /// @param subnet subnet the allocation should come from
+    /// @param clientid Client identifier
+    /// @param hwaddr client's hardware address info
+    /// @param hint a hint that the client provided
+    /// @param fake_allocation is this real i.e. REQUEST (false) or just picking
+    ///        an address for DISCOVER that is not really allocated (true)
+    /// @return Allocated IPv4 lease (or NULL if allocation failed)
+    Lease4Ptr
+    allocateAddress4(const SubnetPtr& subnet,
+                     const ClientIdPtr& clientid,
+                     const HWAddrPtr& hwaddr,
+                     const isc::asiolink::IOAddress& hint,
+                     bool fake_allocation);
+
+    /// @brief Renews a IPv4 lease
+    ///
+    /// Since both request and renew are implemented in DHCPv4 as the sending of
+    /// a REQUEST packet, it is difficult to easily distinguish between those
+    /// cases. Therefore renew for DHCPv4 is done in the allocation engine.
+    /// This method is also used when client crashed/rebooted and tries
+    /// to get a new lease. It thinks that it gets a new lease, but in fact
+    /// we are only renewing the still valid lease for that client.
+    ///
+    /// @param subnet subnet the client is attached to
+    /// @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
+    Lease4Ptr
+    renewLease4(const SubnetPtr& subnet,
+                const ClientIdPtr& clientid,
+                const HWAddrPtr& hwaddr,
+                const Lease4Ptr& lease,
+                bool fake_allocation /* = false */);
+
     /// @brief Allocates an IPv6 lease
     ///
     /// This method uses currently selected allocator to pick an address from
@@ -181,7 +230,7 @@ protected:
     /// it into LeaseMgr (if this allocation is not fake).
     ///
     /// @param subnet subnet the allocation should come from
-    /// @param duid Client'd DUID
+    /// @param duid Client's DUID
     /// @param iaid iaid field from the IA_NA container that client sent
     /// @param hint a hint that the client provided
     /// @param fake_allocation is this real i.e. REQUEST (false) or just picking
@@ -198,6 +247,25 @@ protected:
     virtual ~AllocEngine();
 private:
 
+    /// @brief Creates a lease and inserts it in LeaseMgr if necessary
+    ///
+    /// Creates a lease based on specified parameters and tries to insert it
+    /// into the database. That may fail in some cases, e.g. when there is another
+    /// allocation process and we lost a race to a specific lease.
+    ///
+    /// @param subnet subnet the lease is allocated from
+    /// @param clientid client identifier
+    /// @param hwaddr client's hardware address
+    /// @param addr an address that was selected and is confirmed to be available
+    /// @param fake_allocation is this real i.e. REQUEST (false) or just picking
+    ///        an address for DISCOVER that is not really allocated (true)
+    /// @return allocated lease (or NULL in the unlikely case of the lease just
+    ///        becomed unavailable)
+    Lease4Ptr createLease4(const SubnetPtr& subnet, const DuidPtr& clientid,
+                           const HWAddrPtr& hwaddr,
+                           const isc::asiolink::IOAddress& addr,
+                           bool fake_allocation = false);
+
     /// @brief creates a lease and inserts it in LeaseMgr if necessary
     ///
     /// Creates a lease based on specified parameters and tries to insert it
@@ -212,11 +280,30 @@ private:
     ///        an address for SOLICIT that is not really allocated (true)
     /// @return allocated lease (or NULL in the unlikely case of the lease just
     ///        becomed unavailable)
-    Lease6Ptr createLease(const Subnet6Ptr& subnet, const DuidPtr& duid,
-                          uint32_t iaid, const isc::asiolink::IOAddress& addr,
-                          bool fake_allocation = false);
+    Lease6Ptr createLease6(const Subnet6Ptr& subnet, const DuidPtr& duid,
+                           uint32_t iaid, const isc::asiolink::IOAddress& addr,
+                           bool fake_allocation = false);
+
+    /// @brief Reuses expired IPv4 lease
+    ///
+    /// Updates existing expired lease with new information. Lease database
+    /// is updated if this is real (i.e. REQUEST, fake_allocation = false), not
+    /// dummy allocation request (i.e. DISCOVER, fake_allocation = true).
+    ///
+    /// @param expired old, expired lease
+    /// @param subnet subnet the lease is allocated from
+    /// @param clientid client identifier
+    /// @param hwaddr client's hardware address
+    /// @param fake_allocation is this real i.e. REQUEST (false) or just picking
+    ///        an address for DISCOVER that is not really allocated (true)
+    /// @return refreshed lease
+    /// @throw BadValue if trying to recycle lease that is still valid
+    Lease4Ptr reuseExpiredLease(Lease4Ptr& expired, const SubnetPtr& subnet,
+                                const ClientIdPtr& clientid,
+                                const HWAddrPtr& hwaddr,
+                                bool fake_allocation = false);
 
-    /// @brief reuses expired lease
+    /// @brief Reuses expired IPv6 lease
     ///
     /// Updates existing expired lease with new information. Lease database
     /// is updated if this is real (i.e. REQUEST, fake_allocation = false), not

src/lib/dhcpsrv/hwaddr.cc

@@ -1,42 +0,0 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
-//
-// Permission to use, copy, modify, and/or distribute this software for any
-// purpose with or without fee is hereby granted, provided that the above
-// copyright notice and this permission notice appear in all copies.
-//
-// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
-// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
-// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
-// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
-// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
-// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
-// PERFORMANCE OF THIS SOFTWARE.
-
-
-#include <dhcpsrv/hwaddr.h>
-
-#include <string>
-#include <iomanip>
-#include <iostream>
-#include <sstream>
-
-namespace isc {
-namespace dhcp {
-
-std::string
-hardwareAddressString(const HWAddr& hwaddr) {
-    std::ostringstream stream;
-
-    for (size_t i = 0; i < hwaddr.size(); ++i) {
-        if (i > 0) {
-            stream << ":";
-        }
-        stream << std::setw(2) << std::hex << std::setfill('0')
-               << static_cast<unsigned int>(hwaddr[i]);
-    }
-
-    return (stream.str());
-}
-
-};  // namespace dhcp
-};  // namespace isc

src/lib/dhcpsrv/lease_mgr.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -32,13 +32,18 @@ using namespace std;
 namespace isc {
 namespace dhcp {
 
+Lease::Lease(const isc::asiolink::IOAddress& addr, uint32_t t1, uint32_t t2,
+             uint32_t valid_lft, SubnetID subnet_id, time_t cltt)
+    :addr_(addr), t1_(t1), t2_(t2), valid_lft_(valid_lft), cltt_(cltt),
+     subnet_id_(subnet_id), fixed_(false), fqdn_fwd_(false), fqdn_rev_(false) {
+}
+
 Lease6::Lease6(LeaseType type, const isc::asiolink::IOAddress& addr,
                DuidPtr duid, uint32_t iaid, uint32_t preferred, uint32_t valid,
                uint32_t t1, uint32_t t2, SubnetID subnet_id, uint8_t prefixlen)
-    : addr_(addr), type_(type), prefixlen_(prefixlen), iaid_(iaid), duid_(duid),
-      preferred_lft_(preferred), valid_lft_(valid), t1_(t1), t2_(t2),
-      subnet_id_(subnet_id), fixed_(false), fqdn_fwd_(false),
-      fqdn_rev_(false) {
+    : Lease(addr, t1, t2, valid, subnet_id, 0/*cltt*/),
+      type_(type), prefixlen_(prefixlen), iaid_(iaid), duid_(duid),
+      preferred_lft_(preferred) {
     if (!duid) {
         isc_throw(InvalidOperation, "DUID must be specified for a lease");
     }
@@ -46,7 +51,7 @@ Lease6::Lease6(LeaseType type, const isc::asiolink::IOAddress& addr,
     cltt_ = time(NULL);
 }
 
-bool Lease6::expired() const {
+bool Lease::expired() const {
 
     // Let's use int64 to avoid problems with negative/large uint32 values
     int64_t expire_time = cltt_ + valid_lft_;
@@ -63,18 +68,6 @@ std::string LeaseMgr::getParameter(const std::string& name) const {
 }
 
 std::string
-Lease4::toText() const {
-    ostringstream stream;
-
-    stream << "Address:       " << addr_.toText() << "\n"
-           << "Valid life:    " << valid_lft_ << "\n"
-           << "Cltt:          " << cltt_ << "\n"
-           << "Subnet ID:     " << subnet_id_ << "\n";
-
-    return (stream.str());
-}
-
-std::string
 Lease6::toText() const {
     ostringstream stream;
 
@@ -103,6 +96,21 @@ Lease6::toText() const {
     return (stream.str());
 }
 
+std::string
+Lease4::toText() const {
+    ostringstream stream;
+
+    stream << "Address:       " << addr_.toText() << "\n"
+           << "Valid life:    " << valid_lft_ << "\n"
+           << "T1:            " << t1_ << "\n"
+           << "T2:            " << t2_ << "\n"
+           << "Cltt:          " << cltt_ << "\n"
+           << "Subnet ID:     " << subnet_id_ << "\n";
+
+    return (stream.str());
+}
+
+
 bool
 Lease4::operator==(const Lease4& other) const {
     return (

src/lib/dhcpsrv/lease_mgr.h

@@ -18,7 +18,7 @@
 #include <asiolink/io_address.h>
 #include <dhcp/duid.h>
 #include <dhcp/option.h>
-#include <dhcpsrv/hwaddr.h>
+#include <dhcp/hwaddr.h>
 #include <dhcpsrv/subnet.h>
 #include <exceptions/exceptions.h>
 
@@ -108,38 +108,19 @@ public:
         isc::Exception(file, line, what) {}
 };
 
-/// @brief Structure that holds a lease for IPv4 address
+/// @brief a common structure for IPv4 and IPv6 leases
 ///
-/// For performance reasons it is a simple structure, not a class. If we chose
-/// make it a class, all fields would have to made private and getters/setters
-/// would be required. As this is a critical part of the code that will be used
-/// extensively, direct access is warranted.
-struct Lease4 {
-    /// @brief Maximum size of a hardware address
-    static const size_t HWADDR_MAX = 20;
+/// This structure holds all information that is common between IPv4 and IPv6
+/// leases.
+struct Lease {
 
+    Lease(const isc::asiolink::IOAddress& addr, uint32_t t1, uint32_t t2,
+          uint32_t valid_lft, SubnetID subnet_id, time_t cltt);
 
-    /// IPv4 address
-    isc::asiolink::IOAddress addr_;
-
-    /// @brief Address extension
+    /// @brief IPv4 ot IPv6 address
     ///
-    /// It is envisaged that in some cases IPv4 address will be accompanied
-    /// with some additional data. One example of such use are Address + Port
-    /// solutions (or Port-restricted Addresses), where several clients may get
-    /// the same address, but different port ranges. This feature is not
-    /// expected to be widely used.  Under normal circumstances, the value
-    /// should be 0.
-    uint32_t ext_;
-
-    /// @brief Hardware address
-    std::vector<uint8_t> hwaddr_;
-
-    /// @brief Client identifier
-    ///
-    /// @todo Should this be a pointer to a client ID or the ID itself?
-    ///       Compare with the DUID in the Lease6 structure.
-    ClientIdPtr client_id_;
+    /// IPv4, IPv6 address or, in the case of a prefix delegation, the prefix.
+    isc::asiolink::IOAddress addr_;
 
     /// @brief Renewal timer
     ///
@@ -201,6 +182,46 @@ struct Lease4 {
     /// system administrator.
     std::string comments_;
 
+    /// @brief Convert Lease to Printable Form
+    ///
+    /// @return String form of the lease
+    virtual std::string toText() const = 0;
+
+    /// @brief returns true if the lease is expired
+    /// @return true if the lease is expired
+    bool expired() const;
+
+};
+
+/// @brief Structure that holds a lease for IPv4 address
+///
+/// For performance reasons it is a simple structure, not a class. If we chose
+/// make it a class, all fields would have to made private and getters/setters
+/// would be required. As this is a critical part of the code that will be used
+/// extensively, direct access is warranted.
+struct Lease4 : public Lease {
+    /// @brief Maximum size of a hardware address
+    static const size_t HWADDR_MAX = 20;
+
+    /// @brief Address extension
+    ///
+    /// It is envisaged that in some cases IPv4 address will be accompanied
+    /// with some additional data. One example of such use are Address + Port
+    /// solutions (or Port-restricted Addresses), where several clients may get
+    /// the same address, but different port ranges. This feature is not
+    /// expected to be widely used.  Under normal circumstances, the value
+    /// should be 0.
+    uint32_t ext_;
+
+    /// @brief Hardware address
+    std::vector<uint8_t> hwaddr_;
+
+    /// @brief Client identifier
+    ///
+    /// @todo Should this be a pointer to a client ID or the ID itself?
+    ///       Compare with the DUID in the Lease6 structure.
+    ClientIdPtr client_id_;
+
     /// @brief Constructor
     ///
     /// @param addr IPv4 address as unsigned 32-bit integer in network byte
@@ -212,26 +233,19 @@ struct Lease4 {
     /// @param valid_lft Lifetime of the lease
     /// @param cltt Client last transmission time
     /// @param subnet_id Subnet identification
-    Lease4(uint32_t addr, const uint8_t* hwaddr, size_t hwaddr_len,
+    Lease4(const isc::asiolink::IOAddress& addr, const uint8_t* hwaddr, size_t hwaddr_len,
            const uint8_t* clientid, size_t clientid_len, uint32_t valid_lft,
-           time_t cltt, uint32_t subnet_id)
-        : addr_(addr), ext_(0), hwaddr_(hwaddr, hwaddr + hwaddr_len),
-          client_id_(new ClientId(clientid, clientid_len)), t1_(0), t2_(0),
-          valid_lft_(valid_lft), cltt_(cltt), subnet_id_(subnet_id),
-          fixed_(false), hostname_(), fqdn_fwd_(false), fqdn_rev_(false),
-          comments_()
-    {}
+           uint32_t t1, uint32_t t2, time_t cltt, uint32_t subnet_id)
+        : Lease(addr, t1, t2, valid_lft, subnet_id, cltt),
+        ext_(0), hwaddr_(hwaddr, hwaddr + hwaddr_len),
+        client_id_(new ClientId(clientid, clientid_len)) {
+    }
 
     /// @brief Default constructor
     ///
     /// Initialize fields that don't have a default constructor.
-    Lease4() : addr_(0), fixed_(false), fqdn_fwd_(false), fqdn_rev_(false)
-    {}
-
-    /// @brief Convert lease to printable form
-    ///
-    /// @return Textual represenation of lease data
-    std::string toText() const;
+    Lease4() : Lease(0, 0, 0, 0, 0, 0) {
+    }
 
     /// @brief Compare two leases for equality
     ///
@@ -245,6 +259,11 @@ struct Lease4 {
         return (!operator==(other));
     }
 
+    /// @brief Convert lease to printable form
+    ///
+    /// @return Textual represenation of lease data
+    virtual std::string toText() const;
+
     /// @todo: Add DHCPv4 failover related fields here
 };
 
@@ -262,7 +281,7 @@ typedef std::vector<Lease4Ptr> Lease4Collection;
 /// make it a class, all fields would have to made private and getters/setters
 /// would be required. As this is a critical part of the code that will be used
 /// extensively, direct access is warranted.
-struct Lease6 {
+struct Lease6 : public Lease {
 
     /// @brief Type of lease contents
     typedef enum {
@@ -271,11 +290,6 @@ struct Lease6 {
         LEASE_IA_PD  /// the lease contains IPv6 prefix (for prefix delegation)
     } LeaseType;
 
-    /// @brief IPv6 address
-    ///
-    /// IPv6 address or, in the case of a prefix delegation, the prefix.
-    isc::asiolink::IOAddress addr_;
-
     /// @brief Lease type
     ///
     /// One of normal address, temporary address, or prefix.
@@ -302,69 +316,6 @@ struct Lease6 {
     /// assigned or renewed (cltt), expressed in seconds.
     uint32_t preferred_lft_;
 
-    /// @brief valid lifetime
-    ///
-    /// This parameter specifies the valid lifetime since the lease waa
-    /// assigned/renewed (cltt), expressed in seconds.
-    uint32_t valid_lft_;
-
-    /// @brief T1 timer
-    ///
-    /// Specifies renewal time. Although technically it is a property of the
-    /// IA container and not the address itself, since our data model does not
-    /// define a separate IA entity, we are keeping it in the lease. In the
-    /// case of multiple addresses/prefixes for the same IA, each must have
-    /// consistent T1 and T2 values. This is specified in seconds since cltt.
-    /// The value will also be useful for failover to calculate the next
-    /// expected client transmission time.
-    uint32_t t1_;
-
-    /// @brief T2 timer
-    ///
-    /// Specifies rebinding time. Although technically it is a property of the
-    /// IA container and not the address itself, since our data model does not
-    /// define a separate IA entity, we are keeping it in the lease. In the
-    /// case of multiple addresses/prefixes for the same IA, each must have
-    /// consistent T1 and T2 values. This is specified in seconds since cltt.
-    uint32_t t2_;
-
-    /// @brief Client last transmission time
-    ///
-    /// Specifies a timestamp giving the time when the last transmission from a
-    /// client was received.
-    time_t cltt_;
-
-    /// @brief Subnet identifier
-    ///
-    /// Specifies the identification of the subnet to which the lease belongs.
-    SubnetID subnet_id_;
-
-    /// @brief Fixed lease?
-    ///
-    /// Fixed leases are kept after they are released/expired.
-    bool fixed_;
-
-    /// @brief Client hostname
-    ///
-    /// This field may be empty
-    std::string hostname_;
-
-    /// @brief Forward zone updated?
-    ///
-    /// Set true if the DNS AAAA record for this lease has been updated.
-    bool fqdn_fwd_;
-
-    /// @brief Reverse zone updated?
-    ///
-    /// Set true if the DNS PTR record for this lease has been updated.
-    bool fqdn_rev_;
-
-    /// @brief Lease comments
-    ///
-    /// Currently not used. It may be used for keeping comments made by the
-    /// system administrator.
-    std::string comments_;
-
     /// @todo: Add DHCPv6 failover related fields here
 
     /// @brief Constructor
@@ -375,18 +326,9 @@ struct Lease6 {
     /// @brief Constructor
     ///
     /// Initialize fields that don't have a default constructor.
-    Lease6() : addr_("::"), type_(LEASE_IA_NA), fixed_(false), fqdn_fwd_(false),
-               fqdn_rev_(false)
-    {}
-
-    /// @brief Convert Lease6 to Printable Form
-    ///
-    /// @return String form of the lease
-    std::string toText() const;
-
-    /// @brief returns true if the lease is expired
-    /// @return true if the lease is expired
-    bool expired() const;
+    Lease6() : Lease(isc::asiolink::IOAddress("::"), 0, 0, 0, 0, 0),
+        type_(LEASE_IA_NA) {
+    }
 
     /// @brief Compare two leases for equality
     ///
@@ -399,6 +341,11 @@ struct Lease6 {
     bool operator!=(const Lease6& other) const {
         return (!operator==(other));
     }
+
+    /// @brief Convert Lease to Printable Form
+    ///
+    /// @return String form of the lease
+    virtual std::string toText() const;
 };
 
 /// @brief Pointer to a Lease6 structure.

src/lib/dhcpsrv/memfile_lease_mgr.cc

@@ -14,6 +14,7 @@
 
 #include <dhcpsrv/dhcpsrv_log.h>
 #include <dhcpsrv/memfile_lease_mgr.h>
+#include <exceptions/exceptions.h>
 
 #include <iostream>
 
@@ -31,7 +32,13 @@ Memfile_LeaseMgr::~Memfile_LeaseMgr() {
 bool Memfile_LeaseMgr::addLease(const Lease4Ptr& lease) {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
               DHCPSRV_MEMFILE_ADD_ADDR4).arg(lease->addr_.toText());
-    return (false);
+
+    if (getLease4(lease->addr_)) {
+        // there is a lease with specified address already
+        return (false);
+    }
+    storage4_.insert(lease);
+    return (true);
 }
 
 bool Memfile_LeaseMgr::addLease(const Lease6Ptr& lease) {
@@ -46,40 +53,63 @@ bool Memfile_LeaseMgr::addLease(const Lease6Ptr& lease) {
     return (true);
 }
 
-Lease4Ptr Memfile_LeaseMgr::getLease4(
-        const isc::asiolink::IOAddress& addr) const {
+Lease4Ptr Memfile_LeaseMgr::getLease4(const isc::asiolink::IOAddress& addr) const {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
               DHCPSRV_MEMFILE_GET_ADDR4).arg(addr.toText());
 
-    return (Lease4Ptr());
+    Lease4Storage::iterator l = storage4_.find(addr);
+    if (l == storage4_.end()) {
+        return (Lease4Ptr());
+    } else {
+        return (*l);
+    }
 }
 
 Lease4Collection Memfile_LeaseMgr::getLease4(const HWAddr& hwaddr) const {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
-              DHCPSRV_MEMFILE_GET_HWADDR).arg(hardwareAddressString(hwaddr));
+              DHCPSRV_MEMFILE_GET_HWADDR).arg(hwaddr.toText());
 
-    return (Lease4Collection());
+    isc_throw(NotImplemented, "getLease4(HWaddr x) method not implemented yet");
 }
 
 Lease4Ptr Memfile_LeaseMgr::getLease4(const HWAddr& hwaddr,
                                       SubnetID subnet_id) const {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
               DHCPSRV_MEMFILE_GET_SUBID_HWADDR).arg(subnet_id)
-              .arg(hardwareAddressString(hwaddr));
+        .arg(hwaddr.toText());
+
+    Lease4Storage::iterator l;
+    for (l = storage4_.begin(); l != storage4_.end(); ++l) {
+        if ( ((*l)->hwaddr_ == hwaddr.hwaddr_) &&
+             ((*l)->subnet_id_ == subnet_id)) {
+            return (*l);
+        }
+    }
+
+    // not found
     return (Lease4Ptr());
 }
 
 Lease4Collection Memfile_LeaseMgr::getLease4(const ClientId& clientid) const {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
               DHCPSRV_MEMFILE_GET_CLIENTID).arg(clientid.toText());
-    return (Lease4Collection());
+    isc_throw(NotImplemented, "getLease4(ClientId) not implemented");
 }
 
-Lease4Ptr Memfile_LeaseMgr::getLease4(const ClientId& clientid,
+Lease4Ptr Memfile_LeaseMgr::getLease4(const ClientId& client_id,
                                       SubnetID subnet_id) const {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
               DHCPSRV_MEMFILE_GET_SUBID_CLIENTID).arg(subnet_id)
-              .arg(clientid.toText());
+              .arg(client_id.toText());
+    Lease4Storage::iterator l;
+    for (l = storage4_.begin(); l != storage4_.end(); ++l) {
+        if ( (*(*l)->client_id_ == client_id) &&
+             ((*l)->subnet_id_ == subnet_id)) {
+            return (*l);
+        }
+    }
+
+    // not found
     return (Lease4Ptr());
 }
 
@@ -138,11 +168,18 @@ bool Memfile_LeaseMgr::deleteLease(const isc::asiolink::IOAddress& addr) {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
               DHCPSRV_MEMFILE_DELETE_ADDR).arg(addr.toText());
     if (addr.isV4()) {
-        // V4 not implemented yet
-        return (false);
+        // v4 lease
+        Lease4Storage::iterator l = storage4_.find(addr);
+        if (l == storage4_.end()) {
+            // No such lease
+            return (false);
+        } else {
+            storage4_.erase(l);
+            return (true);
+        }
 
     } else {
-        // V6 lease
+        // v6 lease
         Lease6Storage::iterator l = storage6_.find(addr);
         if (l == storage6_.end()) {
             // No such lease

src/lib/dhcpsrv/memfile_lease_mgr.h

@@ -15,7 +15,7 @@
 #ifndef MEMFILE_LEASE_MGR_H
 #define MEMFILE_LEASE_MGR_H
 
-#include <dhcpsrv/hwaddr.h>
+#include <dhcp/hwaddr.h>
 #include <dhcpsrv/lease_mgr.h>
 
 #include <boost/multi_index/indexed_by.hpp>
@@ -81,7 +81,7 @@ public:
     /// @param hwaddr hardware address of the client
     ///
     /// @return lease collection
-    virtual Lease4Collection getLease4(const HWAddr& hwaddr) const;
+    virtual Lease4Collection getLease4(const isc::dhcp::HWAddr& hwaddr) const;
 
     /// @brief Returns existing IPv4 leases for specified hardware address
     ///        and a subnet
@@ -226,12 +226,28 @@ protected:
             // IPv6 address that are unique. That particular key is a member
             // of the Lease6 structure, is of type IOAddress and can be accessed
             // by doing &Lease6::addr_
-            boost::multi_index::ordered_unique< 
-                boost::multi_index::member<Lease6, isc::asiolink::IOAddress, &Lease6::addr_> 
+            boost::multi_index::ordered_unique<
+                boost::multi_index::member<Lease, isc::asiolink::IOAddress, &Lease::addr_>
             >
         >
     > Lease6Storage; // Let the whole contraption be called Lease6Storage.
 
+    typedef boost::multi_index_container< // this is a multi-index container...
+    Lease4Ptr, // it will hold shared_ptr to leases6
+        boost::multi_index::indexed_by< // and will be sorted by
+            // IPv6 address that are unique. That particular key is a member
+            // of the Lease6 structure, is of type IOAddress and can be accessed
+            // by doing &Lease6::addr_
+            boost::multi_index::ordered_unique<
+                boost::multi_index::member<Lease, isc::asiolink::IOAddress, &Lease::addr_>
+            >
+        >
+    > Lease4Storage; // Let the whole contraption be called Lease6Storage.
+
+    /// @brief stores IPv4 leases
+    Lease4Storage storage4_;
+
+    /// @brief stores IPv6 leases
     Lease6Storage storage6_;
 };
 
@@ -239,4 +255,3 @@ protected:
 }; // end of isc namespace
 
 #endif // MEMFILE_LEASE_MGR
-

src/lib/dhcpsrv/mysql_lease_mgr.cc

@@ -16,6 +16,7 @@
 
 #include <asiolink/io_address.h>
 #include <dhcp/duid.h>
+#include <dhcp/hwaddr.h>
 #include <dhcpsrv/dhcpsrv_log.h>
 #include <dhcpsrv/mysql_lease_mgr.h>
 
@@ -451,9 +452,10 @@ public:
         time_t cltt = 0;
         MySqlLeaseMgr::convertFromDatabaseTime(expire_, valid_lifetime_, cltt);
 
+        // note that T1 and T2 are not stored
         return (Lease4Ptr(new Lease4(addr4_, hwaddr_buffer_, hwaddr_length_,
                                      client_id_buffer_, client_id_length_,
-                                     valid_lifetime_, cltt, subnet_id_)));
+                                     valid_lifetime_, 0, 0, cltt, subnet_id_)));
     }
 
     /// @brief Return columns in error
@@ -1289,7 +1291,7 @@ MySqlLeaseMgr::getLease4(const isc::asiolink::IOAddress& addr) const {
 Lease4Collection
 MySqlLeaseMgr::getLease4(const HWAddr& hwaddr) const {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
-              DHCPSRV_MYSQL_GET_HWADDR).arg(hardwareAddressString(hwaddr));
+              DHCPSRV_MYSQL_GET_HWADDR).arg(hwaddr.toText());
 
     // Set up the WHERE clause value
     MYSQL_BIND inbind[1];
@@ -1300,8 +1302,8 @@ MySqlLeaseMgr::getLease4(const HWAddr& hwaddr) const {
     // a "char*". (We could avoid the "const_cast" by copying the data to a
     // local variable, but as the data is only being read, this introduces
     // an unnecessary copy).
-    unsigned long hwaddr_length = hwaddr.size();
-    uint8_t* data = const_cast<uint8_t*>(&hwaddr[0]);
+    unsigned long hwaddr_length = hwaddr.hwaddr_.size();
+    uint8_t* data = const_cast<uint8_t*>(&hwaddr.hwaddr_[0]);
 
     inbind[0].buffer_type = MYSQL_TYPE_BLOB;
     inbind[0].buffer = reinterpret_cast<char*>(data);
@@ -1320,7 +1322,7 @@ Lease4Ptr
 MySqlLeaseMgr::getLease4(const HWAddr& hwaddr, SubnetID subnet_id) const {
     LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
               DHCPSRV_MYSQL_GET_SUBID_HWADDR)
-              .arg(subnet_id).arg(hardwareAddressString(hwaddr));
+        .arg(subnet_id).arg(hwaddr.toText());
 
     // Set up the WHERE clause value
     MYSQL_BIND inbind[2];
@@ -1331,8 +1333,8 @@ MySqlLeaseMgr::getLease4(const HWAddr& hwaddr, SubnetID subnet_id) const {
     // a "char*". (We could avoid the "const_cast" by copying the data to a
     // local variable, but as the data is only being read, this introduces
     // an unnecessary copy).
-    unsigned long hwaddr_length = hwaddr.size();
-    uint8_t* data = const_cast<uint8_t*>(&hwaddr[0]);
+    unsigned long hwaddr_length = hwaddr.hwaddr_.size();
+    uint8_t* data = const_cast<uint8_t*>(&hwaddr.hwaddr_[0]);
 
     inbind[0].buffer_type = MYSQL_TYPE_BLOB;
     inbind[0].buffer = reinterpret_cast<char*>(data);

src/lib/dhcpsrv/mysql_lease_mgr.h

@@ -15,7 +15,7 @@
 #ifndef MYSQL_LEASE_MGR_H
 #define MYSQL_LEASE_MGR_H
 
-#include <dhcpsrv/hwaddr.h>
+#include <dhcp/hwaddr.h>
 #include <dhcpsrv/lease_mgr.h>
 
 #include <boost/scoped_ptr.hpp>
@@ -132,7 +132,7 @@ public:
     ///        programming error.
     /// @throw isc::dhcp::DbOperationError An operation on the open database has
     ///        failed.
-    virtual Lease4Collection getLease4(const HWAddr& hwaddr) const;
+    virtual Lease4Collection getLease4(const isc::dhcp::HWAddr& hwaddr) const;
 
     /// @brief Returns existing IPv4 leases for specified hardware address
     ///        and a subnet
@@ -150,7 +150,7 @@ public:
     ///        programming error.
     /// @throw isc::dhcp::DbOperationError An operation on the open database has
     ///        failed.
-    virtual Lease4Ptr getLease4(const HWAddr& hwaddr,
+    virtual Lease4Ptr getLease4(const isc::dhcp::HWAddr& hwaddr,
                                 SubnetID subnet_id) const;
 
     /// @brief Returns existing IPv4 lease for specified client-id

src/lib/dhcpsrv/pool.h

@@ -179,6 +179,13 @@ typedef boost::shared_ptr<Pool6> Pool6Ptr;
 /// @brief a container for IPv6 Pools
 typedef std::vector<Pool6Ptr> Pool6Collection;
 
+/// @brief a pointer to either IPv4 or IPv6 Pool
+typedef boost::shared_ptr<Pool> PoolPtr;
+
+/// @brief a container for either IPv4 or IPv6 Pools
+typedef std::vector<PoolPtr> PoolCollection;
+
+
 } // end of isc::dhcp namespace
 } // end of isc namespace
 

src/lib/dhcpsrv/subnet.cc

@@ -120,12 +120,12 @@ Subnet4::Subnet4(const isc::asiolink::IOAddress& prefix, uint8_t length,
     }
 }
 
-void Subnet4::addPool4(const Pool4Ptr& pool) {
+void Subnet::addPool(const PoolPtr& pool) {
     IOAddress first_addr = pool->getFirstAddress();
     IOAddress last_addr = pool->getLastAddress();
 
     if (!inRange(first_addr) || !inRange(last_addr)) {
-        isc_throw(BadValue, "Pool4 (" << first_addr.toText() << "-" << last_addr.toText()
+        isc_throw(BadValue, "Pool (" << first_addr.toText() << "-" << last_addr.toText()
                   << " does not belong in this (" << prefix_.toText() << "/"
                   << static_cast<int>(prefix_len_) << ") subnet4");
     }
@@ -135,9 +135,10 @@ void Subnet4::addPool4(const Pool4Ptr& pool) {
     pools_.push_back(pool);
 }
 
-Pool4Ptr Subnet4::getPool4(const isc::asiolink::IOAddress& hint /* = IOAddress("::")*/ ) {
-    Pool4Ptr candidate;
-    for (Pool4Collection::iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
+PoolPtr Subnet::getPool(isc::asiolink::IOAddress hint) {
+
+    PoolPtr candidate;
+    for (PoolCollection::iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
 
         // if we won't find anything better, then let's just use the first pool
         if (!candidate) {
@@ -153,6 +154,7 @@ Pool4Ptr Subnet4::getPool4(const isc::asiolink::IOAddress& hint /* = IOAddress("
     return (candidate);
 }
 
+
 void
 Subnet4::validateOption(const OptionPtr& option) const {
     if (!option) {
@@ -162,14 +164,14 @@ Subnet4::validateOption(const OptionPtr& option) const {
     }
 }
 
-bool Subnet4::inPool(const isc::asiolink::IOAddress& addr) const {
+bool Subnet::inPool(const isc::asiolink::IOAddress& addr) const {
 
     // Let's start with checking if it even belongs to that subnet.
     if (!inRange(addr)) {
         return (false);
     }
 
-    for (Pool4Collection::const_iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
+    for (PoolCollection::const_iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
         if ((*pool)->inRange(addr)) {
             return (true);
         }
@@ -191,39 +193,6 @@ Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length,
     }
 }
 
-void Subnet6::addPool6(const Pool6Ptr& pool) {
-    IOAddress first_addr = pool->getFirstAddress();
-    IOAddress last_addr = pool->getLastAddress();
-
-    if (!inRange(first_addr) || !inRange(last_addr)) {
-        isc_throw(BadValue, "Pool6 (" << first_addr.toText() << "-" << last_addr.toText()
-                  << ") does not belong in this (" << prefix_.toText()
-                  << "/" << static_cast<int>(prefix_len_)
-                  << ") subnet6");
-    }
-    /// @todo: Check that pools do not overlap
-
-    pools_.push_back(pool);
-}
-
-Pool6Ptr Subnet6::getPool6(const isc::asiolink::IOAddress& hint /* = IOAddress("::")*/ ) {
-    Pool6Ptr candidate;
-    for (Pool6Collection::iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
-
-        // if we won't find anything better, then let's just use the first pool
-        if (!candidate) {
-            candidate = *pool;
-        }
-
-        // if the client provided a pool and there's a pool that hint is valid in,
-        // then let's use that pool
-        if ((*pool)->inRange(hint)) {
-            return (*pool);
-        }
-    }
-    return (candidate);
-}
-
 void
 Subnet6::validateOption(const OptionPtr& option) const {
     if (!option) {
@@ -233,21 +202,5 @@ Subnet6::validateOption(const OptionPtr& option) const {
     }
 }
 
-bool Subnet6::inPool(const isc::asiolink::IOAddress& addr) const {
-
-    // Let's start with checking if it even belongs to that subnet.
-    if (!inRange(addr)) {
-        return (false);
-    }
-
-    for (Pool6Collection::const_iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
-        if ((*pool)->inRange(addr)) {
-            return (true);
-        }
-    }
-    // there's no pool that address belongs to
-    return (false);
-}
-
 } // end of isc::dhcp namespace
 } // end of isc namespace

src/lib/dhcpsrv/subnet.h

@@ -242,7 +242,7 @@ public:
     /// @param addr this address will be checked if it belongs to any pools in
     ///        that subnet
     /// @return true if the address is in any of the pools
-    virtual bool inPool(const isc::asiolink::IOAddress& addr) const = 0;
+    bool inPool(const isc::asiolink::IOAddress& addr) const;
 
     /// @brief return valid-lifetime for addresses in that prefix
     Triplet<uint32_t> getValid() const {
@@ -315,6 +315,37 @@ public:
         return (std::make_pair(prefix_, prefix_len_));
     }
 
+    /// @brief Adds a new pool.
+    /// @param pool pool to be added
+    void addPool(const PoolPtr& pool);
+
+    /// @brief Returns a pool that specified address belongs to
+    ///
+    /// @param addr address that the returned pool should cover (optional)
+    /// @return Pointer to found Pool4 or Pool6 (or NULL)
+    PoolPtr getPool(isc::asiolink::IOAddress addr);
+
+    /// @brief Returns a pool without any address specified
+    /// @return returns one of the pools defined
+    PoolPtr getPool() {
+        return (getPool(default_pool()));
+    }
+
+    /// @brief Returns the default address that will be used for pool selection
+    ///
+    /// It must be implemented in derived classes (should return :: for Subnet6
+    /// and 0.0.0.0 for Subnet4)
+    virtual isc::asiolink::IOAddress default_pool() const = 0;
+
+    /// @brief returns all pools
+    ///
+    /// The reference is only valid as long as the object that returned it.
+    ///
+    /// @return a collection of all pools
+    const PoolCollection& getPools() const {
+        return pools_;
+    }
+
     /// @brief returns textual representation of the subnet (e.g. "2001:db8::/64")
     ///
     /// @return textual representation
@@ -355,6 +386,9 @@ protected:
     /// a Subnet4 or Subnet6.
     SubnetID id_;
 
+    /// @brief collection of pools in that list
+    PoolCollection pools_;
+
     /// @brief a prefix of the subnet
     isc::asiolink::IOAddress prefix_;
 
@@ -391,6 +425,9 @@ private:
     OptionSpacesPtr option_spaces_;
 };
 
+/// @brief A generic pointer to either Subnet4 or Subnet6 object
+typedef boost::shared_ptr<Subnet> SubnetPtr;
+
 /// @brief A configuration holder for IPv4 subnet.
 ///
 /// This class represents an IPv4 subnet.
@@ -409,34 +446,6 @@ public:
             const Triplet<uint32_t>& t2,
             const Triplet<uint32_t>& valid_lifetime);
 
-    /// @brief Returns a pool that specified address belongs to
-    ///
-    /// @param hint address that the returned pool should cover (optional)
-    /// @return Pointer to found pool4 (or NULL)
-    Pool4Ptr getPool4(const isc::asiolink::IOAddress& hint =
-                      isc::asiolink::IOAddress("0.0.0.0"));
-
-    /// @brief Adds a new pool.
-    /// @param pool pool to be added
-    void addPool4(const Pool4Ptr& pool);
-
-    /// @brief returns all pools
-    ///
-    /// The reference is only valid as long as the object that returned it.
-    ///
-    /// @return a collection of all pools
-    const Pool4Collection& getPools() const {
-        return pools_;
-    }
-
-    /// @brief checks if the specified address is in pools
-    ///
-    /// See the description in \ref Subnet::inPool().
-    ///
-    /// @param addr this address will be checked if it belongs to any pools in that subnet
-    /// @return true if the address is in any of the pools
-    bool inPool(const isc::asiolink::IOAddress& addr) const;
-
 protected:
 
     /// @brief Check if option is valid and can be added to a subnet.
@@ -446,8 +455,11 @@ protected:
     /// @throw isc::BadValue if provided option is invalid.
     virtual void validateOption(const OptionPtr& option) const;
 
-    /// @brief collection of pools in that list
-    Pool4Collection pools_;
+    /// @brief Returns default address for pool selection
+    /// @return ANY IPv4 address
+    virtual isc::asiolink::IOAddress default_pool() const {
+        return (isc::asiolink::IOAddress("0.0.0.0"));
+    }
 };
 
 /// @brief A pointer to a Subnet4 object
@@ -484,35 +496,6 @@ public:
         return (preferred_);
     }
 
-    /// @brief Returns a pool that specified address belongs to
-    ///
-    /// @param hint address that the returned pool should cover (optional)
-    /// @return Pointer to found pool6 (or NULL)
-    Pool6Ptr getPool6(const isc::asiolink::IOAddress& hint =
-                      isc::asiolink::IOAddress("::"));
-
-    /// @brief Adds a new pool.
-    /// @param pool pool to be added
-    void addPool6(const Pool6Ptr& pool);
-
-    /// @brief returns all pools
-    ///
-    /// The reference is only valid as long as the object that
-    /// returned it.
-    ///
-    /// @return a collection of all pools
-    const Pool6Collection& getPools() const {
-        return pools_;
-    }
-
-    /// @brief checks if the specified address is in pools
-    ///
-    /// See the description in \ref Subnet::inPool().
-    ///
-    /// @param addr this address will be checked if it belongs to any pools in that subnet
-    /// @return true if the address is in any of the pools
-    bool inPool(const isc::asiolink::IOAddress& addr) const;
-
 protected:
 
     /// @brief Check if option is valid and can be added to a subnet.
@@ -522,6 +505,12 @@ protected:
     /// @throw isc::BadValue if provided option is invalid.
     virtual void validateOption(const OptionPtr& option) const;
 
+    /// @brief Returns default address for pool selection
+    /// @return ANY IPv6 address
+    virtual isc::asiolink::IOAddress default_pool() const {
+        return (isc::asiolink::IOAddress("::"));
+    }
+
     /// @brief collection of pools in that list
     Pool6Collection pools_;
 

src/lib/dhcpsrv/tests/Makefile.am

@@ -30,7 +30,6 @@ libdhcpsrv_unittests_SOURCES  = run_unittests.cc
 libdhcpsrv_unittests_SOURCES += addr_utilities_unittest.cc
 libdhcpsrv_unittests_SOURCES += alloc_engine_unittest.cc
 libdhcpsrv_unittests_SOURCES += cfgmgr_unittest.cc
-libdhcpsrv_unittests_SOURCES += hwaddr_unittest.cc
 libdhcpsrv_unittests_SOURCES += lease_mgr_factory_unittest.cc
 libdhcpsrv_unittests_SOURCES += lease_mgr_unittest.cc
 libdhcpsrv_unittests_SOURCES += memfile_lease_mgr_unittest.cc

src/lib/dhcpsrv/tests/addr_utilities_unittest.cc

@@ -16,6 +16,7 @@
 #include <config.h>
 
 #include <dhcpsrv/addr_utilities.h>
+#include <exceptions/exceptions.h>
 
 #include <gtest/gtest.h>
 
@@ -28,6 +29,8 @@ using namespace std;
 using namespace isc::dhcp;
 using namespace isc::asiolink;
 
+namespace {
+
 // This test verifies that lastAddrInPrefix is able to handle IPv4 operations.
 TEST(AddrUtilitiesTest, lastAddrInPrefix4) {
     IOAddress addr1("192.0.2.1");
@@ -154,3 +157,47 @@ TEST(AddrUtilitiesTest, firstAddrInPrefix6) {
     EXPECT_EQ("2001::ff80", firstAddrInPrefix(addr2, 121).toText());
     EXPECT_EQ("2001::ff00", firstAddrInPrefix(addr2, 120).toText());
 }
+
+// Checks if IPv4 netmask is generated properly
+TEST(AddrUtilitiesTest, getNetmask4) {
+    EXPECT_EQ("0.0.0.0", getNetmask4(0).toText());
+    EXPECT_EQ("128.0.0.0", getNetmask4(1).toText());
+    EXPECT_EQ("192.0.0.0", getNetmask4(2).toText());
+    EXPECT_EQ("224.0.0.0", getNetmask4(3).toText());
+    EXPECT_EQ("240.0.0.0", getNetmask4(4).toText());
+    EXPECT_EQ("248.0.0.0", getNetmask4(5).toText());
+    EXPECT_EQ("252.0.0.0", getNetmask4(6).toText());
+    EXPECT_EQ("254.0.0.0", getNetmask4(7).toText());
+    EXPECT_EQ("255.0.0.0", getNetmask4(8).toText());
+
+    EXPECT_EQ("255.128.0.0", getNetmask4(9).toText());
+    EXPECT_EQ("255.192.0.0", getNetmask4(10).toText());
+    EXPECT_EQ("255.224.0.0", getNetmask4(11).toText());
+    EXPECT_EQ("255.240.0.0", getNetmask4(12).toText());
+    EXPECT_EQ("255.248.0.0", getNetmask4(13).toText());
+    EXPECT_EQ("255.252.0.0", getNetmask4(14).toText());
+    EXPECT_EQ("255.254.0.0", getNetmask4(15).toText());
+    EXPECT_EQ("255.255.0.0", getNetmask4(16).toText());
+
+    EXPECT_EQ("255.255.128.0", getNetmask4(17).toText());
+    EXPECT_EQ("255.255.192.0", getNetmask4(18).toText());
+    EXPECT_EQ("255.255.224.0", getNetmask4(19).toText());
+    EXPECT_EQ("255.255.240.0", getNetmask4(20).toText());
+    EXPECT_EQ("255.255.248.0", getNetmask4(21).toText());
+    EXPECT_EQ("255.255.252.0", getNetmask4(22).toText());
+    EXPECT_EQ("255.255.254.0", getNetmask4(23).toText());
+    EXPECT_EQ("255.255.255.0", getNetmask4(24).toText());
+
+    EXPECT_EQ("255.255.255.128", getNetmask4(25).toText());
+    EXPECT_EQ("255.255.255.192", getNetmask4(26).toText());
+    EXPECT_EQ("255.255.255.224", getNetmask4(27).toText());
+    EXPECT_EQ("255.255.255.240", getNetmask4(28).toText());
+    EXPECT_EQ("255.255.255.248", getNetmask4(29).toText());
+    EXPECT_EQ("255.255.255.252", getNetmask4(30).toText());
+    EXPECT_EQ("255.255.255.254", getNetmask4(31).toText());
+    EXPECT_EQ("255.255.255.255", getNetmask4(32).toText());
+
+    EXPECT_THROW(getNetmask4(33), isc::BadValue);
+}
+
+}; // end of anonymous namespace

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -16,6 +16,7 @@
 
 #include <asiolink/io_address.h>
 #include <dhcp/duid.h>
+#include <dhcp/dhcp4.h>
 #include <dhcpsrv/alloc_engine.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/lease_mgr.h>
@@ -30,7 +31,7 @@
 
 #include <iostream>
 #include <sstream>
-#include <map>
+#include <set>
 #include <time.h>
 
 using namespace std;
@@ -41,20 +42,33 @@ using namespace isc::dhcp::test;
 
 namespace {
 
+/// @brief Allocation engine with some internal methods exposed
 class NakedAllocEngine : public AllocEngine {
 public:
+
+    /// @brief the sole constructor
+    /// @param engine_type specifies engine type (e.g. iterative)
+    /// @param attempts number of lease selection attempts before giving up
     NakedAllocEngine(AllocEngine::AllocType engine_type, unsigned int attempts)
         :AllocEngine(engine_type, attempts) {
     }
+
+    // Expose internal classes for testing purposes
     using AllocEngine::Allocator;
     using AllocEngine::IterativeAllocator;
 };
 
-// empty class for now, but may be extended once Addr6 becomes bigger
-class AllocEngineTest : public ::testing::Test {
+/// @brief Used in Allocation Engine tests for IPv6
+class AllocEngine6Test : public ::testing::Test {
 public:
-    AllocEngineTest() {
-        duid_ = boost::shared_ptr<DUID>(new DUID(vector<uint8_t>(8, 0x42)));
+
+    /// @brief Default constructor
+    ///
+    /// Sets duid_, iaid_, subnet_, pool_ fields to example values used
+    /// in many tests, initializes cfg_mgr configuration and creates
+    /// lease database.
+    AllocEngine6Test() {
+        duid_ = DuidPtr(new DUID(vector<uint8_t>(8, 0x42)));
         iaid_ = 42;
 
         // instantiate cfg_mgr
@@ -63,12 +77,15 @@ public:
         subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));
         pool_ = Pool6Ptr(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1::10"),
                                    IOAddress("2001:db8:1::20")));
-        subnet_->addPool6(pool_);
+        subnet_->addPool(pool_);
         cfg_mgr.addSubnet6(subnet_);
 
         factory_.create("type=memfile");
     }
 
+    /// @brief checks if Lease6 matches expected configuration
+    ///
+    /// @param lease lease to be checked
     void checkLease6(const Lease6Ptr& lease) {
         // that is belongs to the right subnet
         EXPECT_EQ(lease->subnet_id_, subnet_->getID());
@@ -88,20 +105,80 @@ public:
         // @todo: check cltt
      }
 
-    ~AllocEngineTest() {
+    ~AllocEngine6Test() {
         factory_.destroy();
     }
 
-    DuidPtr duid_;
-    uint32_t iaid_;
-    Subnet6Ptr subnet_;
-    Pool6Ptr pool_;
-    LeaseMgrFactory factory_;
+    DuidPtr duid_;            ///< client-identifier (value used in tests)
+    uint32_t iaid_;           ///< IA identifier (value used in tests)
+    Subnet6Ptr subnet_;       ///< subnet6 (used in tests)
+    Pool6Ptr pool_;           ///< pool belonging to subnet_
+    LeaseMgrFactory factory_; ///< pointer to LeaseMgr factory
+};
+
+/// @brief Used in Allocation Engine tests for IPv4
+class AllocEngine4Test : public ::testing::Test {
+public:
+
+    /// @brief Default constructor
+    ///
+    /// Sets clientid_, hwaddr_, subnet_, pool_ fields to example values
+    /// used in many tests, initializes cfg_mgr configuration and creates
+    /// lease database.
+    AllocEngine4Test() {
+        clientid_ = ClientIdPtr(new ClientId(vector<uint8_t>(8, 0x44)));
+        static uint8_t mac[] = { 0, 1, 22, 33, 44, 55};
+
+        // Let's use odd hardware type to check if there is no Ethernet
+        // hardcoded anywhere.
+        hwaddr_ = HWAddrPtr(new HWAddr(mac, sizeof(mac), HTYPE_FDDI));
+
+        // instantiate cfg_mgr
+        CfgMgr& cfg_mgr = CfgMgr::instance();
+
+        subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3));
+        pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"),
+                                   IOAddress("192.0.2.109")));
+        subnet_->addPool(pool_);
+        cfg_mgr.addSubnet4(subnet_);
+
+        factory_.create("type=memfile");
+    }
+
+    /// @brief checks if Lease4 matches expected configuration
+    ///
+    /// @param lease lease to be checked
+    void checkLease4(const Lease4Ptr& lease) {
+        // that is belongs to the right subnet
+        EXPECT_EQ(lease->subnet_id_, subnet_->getID());
+        EXPECT_TRUE(subnet_->inRange(lease->addr_));
+        EXPECT_TRUE(subnet_->inPool(lease->addr_));
+
+        // that it have proper parameters
+        EXPECT_EQ(subnet_->getValid(), lease->valid_lft_);
+        EXPECT_EQ(subnet_->getT1(), lease->t1_);
+        EXPECT_EQ(subnet_->getT2(), lease->t2_);
+        EXPECT_TRUE(false == lease->fqdn_fwd_);
+        EXPECT_TRUE(false == lease->fqdn_rev_);
+        EXPECT_TRUE(*lease->client_id_ == *clientid_);
+        EXPECT_TRUE(lease->hwaddr_ == hwaddr_->hwaddr_);
+        // @todo: check cltt
+     }
+
+    ~AllocEngine4Test() {
+        factory_.destroy();
+    }
+
+    ClientIdPtr clientid_;    ///< client-identifier (value used in tests)
+    HWAddrPtr hwaddr_;        ///< hardware address (value used in tests)
+    Subnet4Ptr subnet_;       ///< subnet4 (used in tests)
+    Pool4Ptr pool_;           ///< pool belonging to subnet_
+    LeaseMgrFactory factory_; ///< pointer to LeaseMgr factory
 };
 
 // This test checks if the Allocation Engine can be instantiated and that it
 // parses parameters string properly.
-TEST_F(AllocEngineTest, constructor) {
+TEST_F(AllocEngine6Test, constructor) {
     boost::scoped_ptr<AllocEngine> x;
 
     // Hashed and random allocators are not supported yet
@@ -112,7 +189,7 @@ TEST_F(AllocEngineTest, constructor) {
 }
 
 // This test checks if the simple allocation can succeed
-TEST_F(AllocEngineTest, simpleAlloc) {
+TEST_F(AllocEngine6Test, simpleAlloc6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -135,7 +212,7 @@ TEST_F(AllocEngineTest, simpleAlloc) {
 }
 
 // This test checks if the fake allocation (for SOLICIT) can succeed
-TEST_F(AllocEngineTest, fakeAlloc) {
+TEST_F(AllocEngine6Test, fakeAlloc6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -156,7 +233,7 @@ TEST_F(AllocEngineTest, fakeAlloc) {
 
 // This test checks if the allocation with a hint that is valid (in range,
 // in pool and free) can succeed
-TEST_F(AllocEngineTest, allocWithValidHint) {
+TEST_F(AllocEngine6Test, allocWithValidHint6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -184,15 +261,16 @@ TEST_F(AllocEngineTest, allocWithValidHint) {
 
 // This test checks if the allocation with a hint that is in range,
 // in pool, but is currently used) can succeed
-TEST_F(AllocEngineTest, allocWithUsedHint) {
+TEST_F(AllocEngine6Test, allocWithUsedHint6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
 
     // let's create a lease and put it in the LeaseMgr
     DuidPtr duid2 = boost::shared_ptr<DUID>(new DUID(vector<uint8_t>(8, 0xff)));
+    time_t now = time(NULL);
     Lease6Ptr used(new Lease6(Lease6::LEASE_IA_NA, IOAddress("2001:db8:1::1f"),
-                              duid2, 1, 2, 3, 4, 5, subnet_->getID()));
+                              duid2, 1, 2, 3, 4, now, subnet_->getID()));
     ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
 
     // another client comes in and request an address that is in pool, but
@@ -223,7 +301,7 @@ TEST_F(AllocEngineTest, allocWithUsedHint) {
 
 // This test checks if the allocation with a hint that is out the blue
 // can succeed. The invalid hint should be ignored completely.
-TEST_F(AllocEngineTest, allocBogusHint) {
+TEST_F(AllocEngine6Test, allocBogusHint6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -253,7 +331,7 @@ TEST_F(AllocEngineTest, allocBogusHint) {
 
 // This test verifies that the allocator picks addresses that belong to the
 // pool
-TEST_F(AllocEngineTest, IterativeAllocator) {
+TEST_F(AllocEngine6Test, IterativeAllocator) {
     boost::scoped_ptr<NakedAllocEngine::Allocator>
         alloc(new NakedAllocEngine::IterativeAllocator());
 
@@ -267,7 +345,7 @@ TEST_F(AllocEngineTest, IterativeAllocator) {
 // This test verifies that the iterative allocator really walks over all addresses
 // in all pools in specified subnet. It also must not pick the same address twice
 // unless it runs out of pool space and must start over.
-TEST_F(AllocEngineTest, IterativeAllocator_manyPools) {
+TEST_F(AllocEngine6Test, IterativeAllocator_manyPools6) {
     NakedAllocEngine::IterativeAllocator* alloc = new NakedAllocEngine::IterativeAllocator();
 
     // let's start from 2, as there is 2001:db8:1::10 - 2001:db8:1::20 pool already.
@@ -280,14 +358,14 @@ TEST_F(AllocEngineTest, IterativeAllocator_manyPools) {
         Pool6Ptr pool(new Pool6(Pool6::TYPE_IA, IOAddress(min.str()),
                                 IOAddress(max.str())));
         // cout << "Adding pool: " << min.str() << "-" << max.str() << endl;
-        subnet_->addPool6(pool);
+        subnet_->addPool(pool);
     }
 
     int total = 17 + 8*9; // first pool (::10 - ::20) has 17 addresses in it,
                           // there are 8 extra pools with 9 addresses in each.
 
     // Let's keep picked addresses here and check their uniqueness.
-    std::map<IOAddress, int> generated_addrs;
+    std::set<IOAddress> generated_addrs;
     int cnt = 0;
     while (++cnt) {
         IOAddress candidate = alloc->pickAddress(subnet_, duid_, IOAddress("::"));
@@ -300,7 +378,7 @@ TEST_F(AllocEngineTest, IterativeAllocator_manyPools) {
 
         if (generated_addrs.find(candidate) == generated_addrs.end()) {
             // we haven't had this
-            generated_addrs[candidate] = 0;
+            generated_addrs.insert(candidate);
         } else {
             // we have seen this address before. That should mean that we
             // iterated over all addresses.
@@ -322,7 +400,7 @@ TEST_F(AllocEngineTest, IterativeAllocator_manyPools) {
 }
 
 // This test checks if really small pools are working
-TEST_F(AllocEngineTest, smallPool) {
+TEST_F(AllocEngine6Test, smallPool6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -334,7 +412,7 @@ TEST_F(AllocEngineTest, smallPool) {
     // Create configuration similar to other tests, but with a single address pool
     subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));
     pool_ = Pool6Ptr(new Pool6(Pool6::TYPE_IA, addr, addr)); // just a single address
-    subnet_->addPool6(pool_);
+    subnet_->addPool(pool_);
     cfg_mgr.addSubnet6(subnet_);
 
     Lease6Ptr lease = engine->allocateAddress6(subnet_, duid_, iaid_, IOAddress("::"),
@@ -358,7 +436,7 @@ TEST_F(AllocEngineTest, smallPool) {
 
 // This test checks if all addresses in a pool are currently used, the attempt
 // to find out a new lease fails.
-TEST_F(AllocEngineTest, outOfAddresses) {
+TEST_F(AllocEngine6Test, outOfAddresses6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -370,7 +448,7 @@ TEST_F(AllocEngineTest, outOfAddresses) {
     // Create configuration similar to other tests, but with a single address pool
     subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));
     pool_ = Pool6Ptr(new Pool6(Pool6::TYPE_IA, addr, addr)); // just a single address
-    subnet_->addPool6(pool_);
+    subnet_->addPool(pool_);
     cfg_mgr.addSubnet6(subnet_);
 
     // Just a different duid
@@ -389,7 +467,7 @@ TEST_F(AllocEngineTest, outOfAddresses) {
 }
 
 // This test checks if an expired lease can be reused in SOLICIT (fake allocation)
-TEST_F(AllocEngineTest, solicitReuseExpiredLease) {
+TEST_F(AllocEngine6Test, solicitReuseExpiredLease6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -401,7 +479,7 @@ TEST_F(AllocEngineTest, solicitReuseExpiredLease) {
     // Create configuration similar to other tests, but with a single address pool
     subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));
     pool_ = Pool6Ptr(new Pool6(Pool6::TYPE_IA, addr, addr)); // just a single address
-    subnet_->addPool6(pool_);
+    subnet_->addPool(pool_);
     cfg_mgr.addSubnet6(subnet_);
 
     // Just a different duid
@@ -413,6 +491,9 @@ TEST_F(AllocEngineTest, solicitReuseExpiredLease) {
     lease->valid_lft_ = 495; // Lease was valid for 495 seconds
     ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
 
+    // Make sure that we really created expired lease
+    ASSERT_TRUE(lease->expired());
+
     // CASE 1: Asking for any address
     lease = engine->allocateAddress6(subnet_, duid_, iaid_, IOAddress("::"),
                                      true);
@@ -432,7 +513,7 @@ TEST_F(AllocEngineTest, solicitReuseExpiredLease) {
 }
 
 // This test checks if an expired lease can be reused in REQUEST (actual allocation)
-TEST_F(AllocEngineTest, requestReuseExpiredLease) {
+TEST_F(AllocEngine6Test, requestReuseExpiredLease6) {
     boost::scoped_ptr<AllocEngine> engine;
     ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
     ASSERT_TRUE(engine);
@@ -444,7 +525,7 @@ TEST_F(AllocEngineTest, requestReuseExpiredLease) {
     // Create configuration similar to other tests, but with a single address pool
     subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));
     pool_ = Pool6Ptr(new Pool6(Pool6::TYPE_IA, addr, addr)); // just a single address
-    subnet_->addPool6(pool_);
+    subnet_->addPool(pool_);
     cfg_mgr.addSubnet6(subnet_);
 
     // Let's create an expired lease
@@ -473,4 +554,412 @@ TEST_F(AllocEngineTest, requestReuseExpiredLease) {
     detailCompareLease(lease, from_mgr);
 }
 
+// --- IPv4 ---
+
+// This test checks if the simple IPv4 allocation can succeed
+TEST_F(AllocEngine4Test, simpleAlloc4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
+                                               IOAddress("0.0.0.0"), false);
+
+    // Check that we got a lease
+    ASSERT_TRUE(lease);
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is indeed in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+// This test checks if the fake allocation (for DISCOVER) can succeed
+TEST_F(AllocEngine4Test, fakeAlloc4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
+                                               IOAddress("0.0.0.0"), true);
+
+    // Check that we got a lease
+    ASSERT_TRUE(lease);
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is NOT in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_FALSE(from_mgr);
+}
+
+
+// This test checks if the allocation with a hint that is valid (in range,
+// in pool and free) can succeed
+TEST_F(AllocEngine4Test, allocWithValidHint4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
+                                               IOAddress("192.0.2.105"),
+                                               false);
+
+    // Check that we got a lease
+    ASSERT_TRUE(lease);
+
+    // We should get what we asked for
+    EXPECT_EQ(lease->addr_.toText(), "192.0.2.105");
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is indeed in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+
+// This test checks if the allocation with a hint that is in range,
+// in pool, but is currently used) can succeed
+TEST_F(AllocEngine4Test, allocWithUsedHint4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    // 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 };
+    time_t now = time(NULL);
+    Lease4Ptr used(new Lease4(IOAddress("192.0.2.106"), hwaddr2, sizeof(hwaddr2),
+                              clientid2, sizeof(clientid2), 1, 2, 3, now, subnet_->getID()));
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
+
+    // Another client comes in and request an address that is in pool, but
+    // unfortunately it is used already. The same address must not be allocated
+    // twice.
+    Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
+                                               IOAddress("192.0.2.106"),
+                                               false);
+    // Check that we got a lease
+    ASSERT_TRUE(lease);
+
+    // Allocated address must be different
+    EXPECT_TRUE(used->addr_.toText() != lease->addr_.toText());
+
+    // We should NOT get what we asked for, because it is used already
+    EXPECT_TRUE(lease->addr_.toText() != "192.0.2.106");
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is indeed in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+
+// This test checks if the allocation with a hint that is out the blue
+// can succeed. The invalid hint should be ignored completely.
+TEST_F(AllocEngine4Test, allocBogusHint4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    // Client would like to get a 3000::abc lease, which does not belong to any
+    // supported lease. Allocation engine should ignore it and carry on
+    // with the normal allocation
+    Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
+                                               IOAddress("10.1.1.1"),
+                                               false);
+    // Check that we got a lease
+    ASSERT_TRUE(lease);
+
+    // We should NOT get what we asked for, because it is used already
+    EXPECT_TRUE(lease->addr_.toText() != "10.1.1.1");
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is indeed in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+
+// This test verifies that the allocator picks addresses that belong to the
+// pool
+TEST_F(AllocEngine4Test, IterativeAllocator) {
+    boost::scoped_ptr<NakedAllocEngine::Allocator>
+        alloc(new NakedAllocEngine::IterativeAllocator());
+
+    for (int i = 0; i < 1000; ++i) {
+        IOAddress candidate = alloc->pickAddress(subnet_, clientid_,
+                                                 IOAddress("0.0.0.0"));
+        EXPECT_TRUE(subnet_->inPool(candidate));
+    }
+}
+
+
+// This test verifies that the iterative allocator really walks over all addresses
+// in all pools in specified subnet. It also must not pick the same address twice
+// unless it runs out of pool space and must start over.
+TEST_F(AllocEngine4Test, IterativeAllocator_manyPools4) {
+    NakedAllocEngine::IterativeAllocator* alloc = new NakedAllocEngine::IterativeAllocator();
+
+    // Let's start from 2, as there is 2001:db8:1::10 - 2001:db8:1::20 pool already.
+    for (int i = 2; i < 10; ++i) {
+        stringstream min, max;
+
+        min << "192.0.2." << i * 10 + 1;
+        max << "192.0.2." << i * 10 + 9;
+
+        Pool4Ptr pool(new Pool4(IOAddress(min.str()),
+                                IOAddress(max.str())));
+        // cout << "Adding pool: " << min.str() << "-" << max.str() << endl;
+        subnet_->addPool(pool);
+    }
+
+    int total = 10 + 8 * 9; // first pool (.100 - .109) has 10 addresses in it,
+                            // there are 8 extra pools with 9 addresses in each.
+
+    // Let's keep picked addresses here and check their uniqueness.
+    std::set<IOAddress> generated_addrs;
+    int cnt = 0;
+    while (++cnt) {
+        IOAddress candidate = alloc->pickAddress(subnet_, clientid_, IOAddress("0.0.0.0"));
+        EXPECT_TRUE(subnet_->inPool(candidate));
+
+        // One way to easily verify that the iterative allocator really works is
+        // to uncomment the following line and observe its output that it
+        // covers all defined subnets.
+        // cout << candidate.toText() << endl;
+
+        if (generated_addrs.find(candidate) == generated_addrs.end()) {
+            // We haven't had this
+            generated_addrs.insert(candidate);
+        } else {
+            // we have seen this address before. That should mean that we
+            // iterated over all addresses.
+            if (generated_addrs.size() == total) {
+                // we have exactly the number of address in all pools
+                break;
+            }
+            ADD_FAILURE() << "Too many or not enough unique addresses generated.";
+            break;
+        }
+
+        if ( cnt>total ) {
+            ADD_FAILURE() << "Too many unique addresses generated.";
+            break;
+        }
+    }
+
+    delete alloc;
+}
+
+
+// This test checks if really small pools are working
+TEST_F(AllocEngine4Test, smallPool4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    IOAddress addr("192.0.2.17");
+    CfgMgr& cfg_mgr = CfgMgr::instance();
+    cfg_mgr.deleteSubnets4(); // Get rid of the default test configuration
+
+    // Create configuration similar to other tests, but with a single address pool
+    subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3));
+    pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address
+    subnet_->addPool(pool_);
+    cfg_mgr.addSubnet4(subnet_);
+
+    Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"),
+                                               false);
+
+    // Check that we got that single lease
+    ASSERT_TRUE(lease);
+
+    EXPECT_EQ("192.0.2.17", lease->addr_.toText());
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is indeed in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+// This test checks if all addresses in a pool are currently used, the attempt
+// to find out a new lease fails.
+TEST_F(AllocEngine4Test, outOfAddresses4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    IOAddress addr("192.0.2.17");
+    CfgMgr& cfg_mgr = CfgMgr::instance();
+    cfg_mgr.deleteSubnets4(); // Get rid of the default test configuration
+
+    // Create configuration similar to other tests, but with a single address pool
+    subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3));
+    pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address
+    subnet_->addPool(pool_);
+    cfg_mgr.addSubnet4(subnet_);
+
+    // Just a different hw/client-id for the second client
+    uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
+    time_t now = time(NULL);
+    Lease4Ptr lease(new Lease4(addr, hwaddr2, sizeof(hwaddr2), clientid2, sizeof(clientid2),
+                               501, 502, 503, now, subnet_->getID()));
+    lease->cltt_ = time(NULL) - 10; // Allocated 10 seconds ago
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
+
+    // There is just a single address in the pool and allocated it to someone
+    // else, so the allocation should fail
+
+    EXPECT_THROW(engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"),false),
+                 AllocFailed);
+}
+
+// This test checks if an expired lease can be reused in DISCOVER (fake allocation)
+TEST_F(AllocEngine4Test, discoverReuseExpiredLease4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    IOAddress addr("192.0.2.15");
+    CfgMgr& cfg_mgr = CfgMgr::instance();
+    cfg_mgr.deleteSubnets4(); // Get rid of the default test configuration
+
+    // Create configuration similar to other tests, but with a single address pool
+    subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3));
+    pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address
+    subnet_->addPool(pool_);
+    cfg_mgr.addSubnet4(subnet_);
+
+    // Just a different hw/client-id for the second client
+    uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
+    time_t now = time(NULL) - 500; // Allocated 500 seconds ago
+    Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, sizeof(hwaddr2),
+                               495, 100, 200, now, subnet_->getID()));
+    // Lease was assigned 500 seconds ago, but its valid lifetime is 495, so it
+    // is expired already
+    ASSERT_TRUE(lease->expired());
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
+
+    // CASE 1: Asking for any address
+    lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"),
+                                     true);
+    // Check that we got that single lease
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(addr.toText(), lease->addr_.toText());
+
+    // Do all checks on the lease (if subnet-id, preferred/valid times are ok etc.)
+    checkLease4(lease);
+
+    // CASE 2: Asking specifically for this address
+    lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress(addr.toText()),
+                                     true);
+    // Check that we got that single lease
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(addr.toText(), lease->addr_.toText());
+}
+
+// This test checks if an expired lease can be reused in REQUEST (actual allocation)
+TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    IOAddress addr("192.0.2.105");
+
+    // Just a different hw/client-id for the second client
+    uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
+    time_t now = time(NULL) - 500; // Allocated 500 seconds ago
+    Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, sizeof(hwaddr2),
+                               495, 100, 200, now, subnet_->getID()));
+    // lease was assigned 500 seconds ago, but its valid lifetime is 495, so it
+    // is expired already
+    ASSERT_TRUE(lease->expired());
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
+
+    // A client comes along, asking specifically for this address
+    lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
+                                     IOAddress(addr.toText()), false);
+
+    // Check that he got that single lease
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(addr.toText(), lease->addr_.toText());
+
+    // Check that the lease is indeed updated in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(addr);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+// This test checks if a lease is really renewed when renewLease4 method is
+// called
+TEST_F(AllocEngine4Test, renewLease4) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    IOAddress addr("192.0.2.102");
+    const uint32_t old_lifetime = 100;
+    const uint32_t old_t1 = 50;
+    const uint32_t old_t2 = 75;
+    const time_t old_timestamp = time(NULL) - 45; // Allocated 45 seconds ago
+
+    // Just a different hw/client-id for the second client
+    const uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
+    const uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
+    Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2,
+                               sizeof(hwaddr2), old_lifetime, old_t1, old_t2,
+                               old_timestamp, subnet_->getID()));
+    ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
+
+    // Lease was assigned 45 seconds ago and is valid for 100 seconds. Let's
+    // renew it.
+    ASSERT_FALSE(lease->expired());
+    lease = engine->renewLease4(subnet_, clientid_, hwaddr_, lease, false);
+    // Check that he got that single lease
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(addr.toText(), lease->addr_.toText());
+
+    // Check that the lease matches subnet_, hwaddr_,clientid_ parameters
+    checkLease4(lease);
+
+    // Check that the lease is indeed updated in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(addr);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
 }; // end of anonymous namespace

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -39,9 +39,13 @@ namespace {
 class CfgMgrTest : public ::testing::Test {
 public:
     CfgMgrTest() {
+        // make sure we start with a clean configuration
+        CfgMgr::instance().deleteSubnets4();
+        CfgMgr::instance().deleteSubnets6();
     }
 
     ~CfgMgrTest() {
+        // clean up after the test
         CfgMgr::instance().deleteSubnets4();
         CfgMgr::instance().deleteSubnets6();
         CfgMgr::instance().deleteOptionDefs();

src/lib/dhcpsrv/tests/hwaddr_unittest.cc

@@ -1,46 +0,0 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
-//
-// Permission to use, copy, modify, and/or distribute this software for any
-// purpose with or without fee is hereby granted, provided that the above
-// copyright notice and this permission notice appear in all copies.
-//
-// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
-// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
-// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
-// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
-// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
-// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
-// PERFORMANCE OF THIS SOFTWARE.
-
-#include <dhcpsrv/hwaddr.h>
-
-#include <gtest/gtest.h>
-
-#include <string>
-
-using namespace isc::dhcp;
-
-namespace {
-
-TEST(HwaddrTest, stringConversion) {
-
-    // Check that an empty vector returns an appropriate string
-    HWAddr hwaddr;
-    std::string result = hardwareAddressString(hwaddr);
-    EXPECT_EQ(std::string(""), result);
-
-    // ... that a single-byte string is OK
-    hwaddr.push_back(0xc3);
-    result = hardwareAddressString(hwaddr);
-    EXPECT_EQ(std::string("c3"), result);
-
-    // ... and that a multi-byte string works
-    hwaddr.push_back(0x7);
-    hwaddr.push_back(0xa2);
-    hwaddr.push_back(0xe8);
-    hwaddr.push_back(0x42);
-    result = hardwareAddressString(hwaddr);
-    EXPECT_EQ(std::string("c3:07:a2:e8:42"), result);
-}
-
-};  // Anonymous namespace

src/lib/dhcpsrv/tests/lease_mgr_unittest.cc

@@ -272,7 +272,7 @@ TEST(Lease4, Lease4Constructor) {
 
         // Create the lease
         Lease4 lease(ADDRESS[i], HWADDR, sizeof(HWADDR),
-                     CLIENTID, sizeof(CLIENTID), VALID_LIFETIME, current_time,
+                     CLIENTID, sizeof(CLIENTID), VALID_LIFETIME, 0, 0, current_time,
                      SUBNET_ID);
 
         EXPECT_EQ(ADDRESS[i], static_cast<uint32_t>(lease.addr_));
@@ -312,10 +312,10 @@ TEST(Lease4, OperatorEquals) {
 
     // Check when the leases are equal.
     Lease4 lease1(ADDRESS, HWADDR, sizeof(HWADDR),
-                  CLIENTID, sizeof(CLIENTID), VALID_LIFETIME, current_time,
+                  CLIENTID, sizeof(CLIENTID), VALID_LIFETIME, current_time, 0, 0,
                   SUBNET_ID);
     Lease4 lease2(ADDRESS, HWADDR, sizeof(HWADDR),
-                  CLIENTID, sizeof(CLIENTID), VALID_LIFETIME, current_time,
+                  CLIENTID, sizeof(CLIENTID), VALID_LIFETIME, current_time, 0, 0,
                   SUBNET_ID);
     EXPECT_TRUE(lease1 == lease2);
     EXPECT_FALSE(lease1 != lease2);

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -770,7 +770,9 @@ TEST_F(MySqlLeaseMgrTest, getLease4Hwaddr) {
     }
 
     // Get the leases matching the hardware address of lease 1
-    Lease4Collection returned = lmptr_->getLease4(leases[1]->hwaddr_);
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    HWAddr tmp(leases[1]->hwaddr_, HTYPE_ETHER);
+    Lease4Collection returned = lmptr_->getLease4(tmp);
 
     // Should be three leases, matching leases[1], [3] and [5].
     ASSERT_EQ(3, returned.size());
@@ -787,13 +789,15 @@ TEST_F(MySqlLeaseMgrTest, getLease4Hwaddr) {
     EXPECT_EQ(straddress4_[5], addresses[2]);
 
     // Repeat test with just one expected match
-    returned = lmptr_->getLease4(leases[2]->hwaddr_);
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    returned = lmptr_->getLease4(HWAddr(leases[2]->hwaddr_, HTYPE_ETHER));
     EXPECT_EQ(1, returned.size());
     detailCompareLease(leases[2], *returned.begin());
 
     // Check that an empty vector is valid
     EXPECT_TRUE(leases[7]->hwaddr_.empty());
-    returned = lmptr_->getLease4(leases[7]->hwaddr_);
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    returned = lmptr_->getLease4(HWAddr(leases[7]->hwaddr_, HTYPE_ETHER));
     EXPECT_EQ(1, returned.size());
     detailCompareLease(leases[7], *returned.begin());
 
@@ -816,7 +820,8 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSize) {
     for (uint8_t i = 0; i <= Lease4::HWADDR_MAX; ++i) {
         leases[1]->hwaddr_.resize(i, i);
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
-        Lease4Collection returned = lmptr_->getLease4(leases[1]->hwaddr_);
+        // @todo: Simply use HWAddr directly once 2589 is implemented
+        Lease4Collection returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER));
         ASSERT_EQ(1, returned.size());
         detailCompareLease(leases[1], *returned.begin());
         (void) lmptr_->deleteLease(leases[1]->addr_);
@@ -831,7 +836,8 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSize) {
     //       be any indication in the C API.
     leases[1]->hwaddr_.resize(Lease4::HWADDR_MAX + 100, 42);
     EXPECT_TRUE(lmptr_->addLease(leases[1]));
-    Lease4Collection returned = lmptr_->getLease4(leases[1]->hwaddr_);
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    Lease4Collection returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER));
     EXPECT_EQ(0, returned.size());
 }
 
@@ -848,27 +854,31 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSubnetId) {
 
     // Get the leases matching the hardware address of lease 1 and
     // subnet ID of lease 1.  Result should be a single lease - lease 1.
-    Lease4Ptr returned = lmptr_->getLease4(leases[1]->hwaddr_,
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
                                            leases[1]->subnet_id_);
     ASSERT_TRUE(returned);
     detailCompareLease(leases[1], returned);
 
     // Try for a match to the hardware address of lease 1 and the wrong
     // subnet ID.
-    returned = lmptr_->getLease4(leases[1]->hwaddr_,
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
                                  leases[1]->subnet_id_ + 1);
     EXPECT_FALSE(returned);
 
     // Try for a match to the subnet ID of lease 1 (and lease 4) but
     // the wrong hardware address.
     vector<uint8_t> invalid_hwaddr(15, 0x77);
-    returned = lmptr_->getLease4(invalid_hwaddr,
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    returned = lmptr_->getLease4(HWAddr(invalid_hwaddr, HTYPE_ETHER),
                                  leases[1]->subnet_id_);
     EXPECT_FALSE(returned);
 
     // Try for a match to an unknown hardware address and an unknown
     // subnet ID.
-    returned = lmptr_->getLease4(invalid_hwaddr,
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    returned = lmptr_->getLease4(HWAddr(invalid_hwaddr, HTYPE_ETHER),
                                  leases[1]->subnet_id_ + 1);
     EXPECT_FALSE(returned);
 
@@ -880,7 +890,8 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSubnetId) {
     EXPECT_TRUE(lmptr_->deleteLease(leases[2]->addr_));
     leases[1]->addr_ = leases[2]->addr_;
     EXPECT_TRUE(lmptr_->addLease(leases[1]));
-    EXPECT_THROW(returned = lmptr_->getLease4(leases[1]->hwaddr_,
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    EXPECT_THROW(returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
                                               leases[1]->subnet_id_),
                  isc::dhcp::MultipleRecords);
 
@@ -905,7 +916,8 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSubnetIdSize) {
     for (uint8_t i = 0; i <= Lease4::HWADDR_MAX; ++i) {
         leases[1]->hwaddr_.resize(i, i);
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
-        Lease4Ptr returned = lmptr_->getLease4(leases[1]->hwaddr_,
+        // @todo: Simply use HWAddr directly once 2589 is implemented
+        Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
                                                leases[1]->subnet_id_);
         ASSERT_TRUE(returned);
         detailCompareLease(leases[1], returned);
@@ -919,7 +931,8 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSubnetIdSize) {
     //       be any indication in the C API.
     leases[1]->hwaddr_.resize(Lease4::HWADDR_MAX + 100, 42);
     EXPECT_TRUE(lmptr_->addLease(leases[1]));
-    Lease4Ptr returned = lmptr_->getLease4(leases[1]->hwaddr_,
+    // @todo: Simply use HWAddr directly once 2589 is implemented
+    Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
                                            leases[1]->subnet_id_);
     EXPECT_FALSE(returned);
 }

src/lib/dhcpsrv/tests/subnet_unittest.cc

@@ -61,28 +61,28 @@ TEST(Subnet4Test, Pool4InSubnet4) {
 
     Subnet4Ptr subnet(new Subnet4(IOAddress("192.1.2.0"), 24, 1, 2, 3));
 
-    Pool4Ptr pool1(new Pool4(IOAddress("192.1.2.0"), 25));
-    Pool4Ptr pool2(new Pool4(IOAddress("192.1.2.128"), 26));
-    Pool4Ptr pool3(new Pool4(IOAddress("192.1.2.192"), 30));
+    PoolPtr pool1(new Pool4(IOAddress("192.1.2.0"), 25));
+    PoolPtr pool2(new Pool4(IOAddress("192.1.2.128"), 26));
+    PoolPtr pool3(new Pool4(IOAddress("192.1.2.192"), 30));
 
-    subnet->addPool4(pool1);
+    subnet->addPool(pool1);
 
     // If there's only one pool, get that pool
-    Pool4Ptr mypool = subnet->getPool4();
+    PoolPtr mypool = subnet->getPool();
     EXPECT_EQ(mypool, pool1);
 
 
-    subnet->addPool4(pool2);
-    subnet->addPool4(pool3);
+    subnet->addPool(pool2);
+    subnet->addPool(pool3);
 
     // If there are more than one pool and we didn't provide hint, we
     // should get the first pool
-    mypool = subnet->getPool4();
+    mypool = subnet->getPool();
 
     EXPECT_EQ(mypool, pool1);
 
     // If we provide a hint, we should get a pool that this hint belongs to
-    mypool = subnet->getPool4(IOAddress("192.1.2.195"));
+    mypool = subnet->getPool(IOAddress("192.1.2.195"));
 
     EXPECT_EQ(mypool, pool3);
 
@@ -94,16 +94,16 @@ TEST(Subnet4Test, Subnet4_Pool4_checks) {
 
     // this one is in subnet
     Pool4Ptr pool1(new Pool4(IOAddress("192.255.0.0"), 16));
-    subnet->addPool4(pool1);
+    subnet->addPool(pool1);
 
     // this one is larger than the subnet!
     Pool4Ptr pool2(new Pool4(IOAddress("193.0.0.0"), 24));
 
-    EXPECT_THROW(subnet->addPool4(pool2), BadValue);
+    EXPECT_THROW(subnet->addPool(pool2), BadValue);
 
     // this one is totally out of blue
     Pool4Ptr pool3(new Pool4(IOAddress("1.2.3.4"), 16));
-    EXPECT_THROW(subnet->addPool4(pool3), BadValue);
+    EXPECT_THROW(subnet->addPool(pool3), BadValue);
 }
 
 TEST(Subnet4Test, addInvalidOption) {
@@ -132,7 +132,7 @@ TEST(Subnet4Test, inRangeinPool) {
 
     // this one is in subnet
     Pool4Ptr pool1(new Pool4(IOAddress("192.2.0.0"), 16));
-    subnet->addPool4(pool1);
+    subnet->addPool(pool1);
 
     // 192.1.1.1 belongs to the subnet...
     EXPECT_TRUE(subnet->inRange(IOAddress("192.1.1.1")));
@@ -207,28 +207,28 @@ TEST(Subnet6Test, Pool6InSubnet6) {
 
     Subnet6Ptr subnet(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));
 
-    Pool6Ptr pool1(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:1::"), 64));
-    Pool6Ptr pool2(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:2::"), 64));
-    Pool6Ptr pool3(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:3::"), 64));
+    PoolPtr pool1(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:1::"), 64));
+    PoolPtr pool2(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:2::"), 64));
+    PoolPtr pool3(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:3::"), 64));
 
-    subnet->addPool6(pool1);
+    subnet->addPool(pool1);
 
     // If there's only one pool, get that pool
-    Pool6Ptr mypool = subnet->getPool6();
+    PoolPtr mypool = subnet->getPool();
     EXPECT_EQ(mypool, pool1);
 
 
-    subnet->addPool6(pool2);
-    subnet->addPool6(pool3);
+    subnet->addPool(pool2);
+    subnet->addPool(pool3);
 
     // If there are more than one pool and we didn't provide hint, we
     // should get the first pool
-    mypool = subnet->getPool6();
+    mypool = subnet->getPool();
 
     EXPECT_EQ(mypool, pool1);
 
     // If we provide a hint, we should get a pool that this hint belongs to
-    mypool = subnet->getPool6(IOAddress("2001:db8:1:3::dead:beef"));
+    mypool = subnet->getPool(IOAddress("2001:db8:1:3::dead:beef"));
 
     EXPECT_EQ(mypool, pool3);
 }
@@ -239,21 +239,21 @@ TEST(Subnet6Test, Subnet6_Pool6_checks) {
 
     // this one is in subnet
     Pool6Ptr pool1(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:1::"), 64));
-    subnet->addPool6(pool1);
+    subnet->addPool(pool1);
 
     // this one is larger than the subnet!
     Pool6Ptr pool2(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8::"), 48));
 
-    EXPECT_THROW(subnet->addPool6(pool2), BadValue);
+    EXPECT_THROW(subnet->addPool(pool2), BadValue);
 
 
     // this one is totally out of blue
     Pool6Ptr pool3(new Pool6(Pool6::TYPE_IA, IOAddress("3000::"), 16));
-    EXPECT_THROW(subnet->addPool6(pool3), BadValue);
+    EXPECT_THROW(subnet->addPool(pool3), BadValue);
 
 
     Pool6Ptr pool4(new Pool6(Pool6::TYPE_IA, IOAddress("4001:db8:1::"), 80));
-    EXPECT_THROW(subnet->addPool6(pool4), BadValue);
+    EXPECT_THROW(subnet->addPool(pool4), BadValue);
 }
 
 TEST(Subnet6Test, addOptions) {
@@ -464,7 +464,7 @@ TEST(Subnet6Test, inRangeinPool) {
     // this one is in subnet
     Pool6Ptr pool1(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8::10"),
                              IOAddress("2001:db8::20")));
-    subnet->addPool6(pool1);
+    subnet->addPool(pool1);
 
     // 192.1.1.1 belongs to the subnet...
     EXPECT_TRUE(subnet->inRange(IOAddress("2001:db8::1")));

src/lib/dhcpsrv/hwaddr.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -12,33 +12,29 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#ifndef HWADDR_H
-#define HWADDR_H
+#ifndef DHCPSRV_UTILS_H
+#define DHCPSRV_UTILS_H
 
-#include <string>
-#include <vector>
-#include <stdint.h>
+#include <exceptions/exceptions.h>
 
 namespace isc {
 namespace dhcp {
 
-/// @brief Hardware Address
-typedef std::vector<uint8_t> HWAddr;
+/// An exception that is thrown if a DHCPv6 protocol violation occurs while
+/// processing a message (e.g. a mandatory option is missing)
+class RFCViolation : public isc::Exception {
+public:
 
-/// @brief Produce string representation of hardware address
+/// @brief constructor
 ///
-/// Returns a string containing the hardware address. This is only used for
-/// logging.
-///
-/// @todo Create a "hardware address" class of which this will be a member.
-///
-/// @param hwaddr Hardware address to convert to string form
-///
-/// @return String form of the hardware address.
-std::string
-hardwareAddressString(const HWAddr& hwaddr);
+/// @param file name of the file, where exception occurred
+/// @param line line of the file, where exception occurred
+/// @param what text description of the issue that caused exception
+RFCViolation(const char* file, size_t line, const char* what) :
+    isc::Exception(file, line, what) {}
+};
 
-};  // namespace dhcp
-};  // namespace isc
+}; // namespace isc::dhcp
+}; // namespace isc
 
-#endif // HWADDR_H
+#endif // DHCPSRV_UTILS_H

src/lib/dns/Makefile.am

@@ -23,6 +23,7 @@ EXTRA_DIST += rdata/generic/cname_5.cc
 EXTRA_DIST += rdata/generic/cname_5.h
 EXTRA_DIST += rdata/generic/detail/char_string.cc
 EXTRA_DIST += rdata/generic/detail/char_string.h
+EXTRA_DIST += rdata/generic/detail/lexer_util.h
 EXTRA_DIST += rdata/generic/detail/nsec_bitmap.cc
 EXTRA_DIST += rdata/generic/detail/nsec_bitmap.h
 EXTRA_DIST += rdata/generic/detail/nsec3param_common.cc

src/lib/dns/gen-rdatacode.py.in

@@ -32,8 +32,8 @@ import sys
 #
 # Example:
 #     new_rdata_factory_users = [('a', 'in'), ('a', 'ch'), ('soa', 'generic')]
-new_rdata_factory_users = [('aaaa', 'in'), ('txt', 'generic'),
-                           ('spf', 'generic')]
+new_rdata_factory_users = [('soa', 'generic'), ('txt', 'generic'),
+                           ('aaaa', 'in'), ('spf', 'generic')]
 
 re_typecode = re.compile('([\da-z]+)_(\d+)')
 classcode2txt = {}

src/lib/dns/rdata/generic/detail/lexer_util.h

@@ -0,0 +1,70 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef DNS_RDATA_LEXER_UTIL_H
+#define DNS_RDATA_LEXER_UTIL_H 1
+
+#include <dns/name.h>
+#include <dns/master_lexer.h>
+
+/// \file lexer_util.h
+/// \brief Utilities for extracting RDATA fields from lexer.
+///
+/// This file intends to define convenient small routines that can be
+/// commonly used in the RDATA implementation to build RDATA fields from
+/// a \c MasterLexer.
+
+namespace isc {
+namespace dns {
+namespace rdata {
+namespace generic {
+namespace detail {
+
+/// \brief Construct a Name object using a master lexer and optional origin.
+///
+/// This is a convenient shortcut of commonly used code pattern that would
+/// be used to build RDATA that contain a domain name field.
+///
+/// Note that this function throws an exception against invalid input.
+/// The (direct or indirect) caller's responsibility needs to expect and
+/// handle exceptions appropriately.
+///
+/// \throw MasterLexer::LexerError The next token from lexer is not string.
+/// \throw Other Exceptions from the \c Name class constructor if the next
+/// string token from the lexer does not represent a valid name.
+///
+/// \param lexer A \c MasterLexer object.  Its next token is expected to be
+/// a string that represent a domain name.
+/// \param origin If non NULL, specifies the origin of the name to be
+/// constructed.
+///
+/// \return A new Name object that corresponds to the next string token of
+/// the \c lexer.
+inline Name
+createNameFromLexer(MasterLexer& lexer, const Name* origin) {
+    const MasterToken::StringRegion& str_region =
+        lexer.getNextToken(MasterToken::STRING).getStringRegion();
+    return (Name(str_region.beg, str_region.len, origin));
+}
+
+} // namespace detail
+} // namespace generic
+} // namespace rdata
+} // namespace dns
+} // namespace isc
+#endif  // DNS_RDATA_LEXER_UTIL_H
+
+// Local Variables:
+// mode: c++
+// End:

src/lib/dns/rdata/generic/soa_6.cc

@@ -14,22 +14,29 @@
 
 #include <config.h>
 
-#include <string>
-
-#include <boost/static_assert.hpp>
-#include <boost/lexical_cast.hpp>
-
 #include <exceptions/exceptions.h>
 
 #include <util/buffer.h>
 #include <dns/name.h>
+#include <dns/master_lexer.h>
+#include <dns/master_loader.h>
+#include <dns/master_loader_callbacks.h>
 #include <dns/messagerenderer.h>
 #include <dns/rdata.h>
 #include <dns/rdataclass.h>
 
+#include <dns/rdata/generic/detail/lexer_util.h>
+
+#include <boost/static_assert.hpp>
+#include <boost/lexical_cast.hpp>
+
+#include <string>
+#include <sstream>
+
 using namespace std;
 using boost::lexical_cast;
 using namespace isc::util;
+using isc::dns::rdata::generic::detail::createNameFromLexer;
 
 // BEGIN_ISC_NAMESPACE
 // BEGIN_RDATA_NAMESPACE
@@ -42,35 +49,85 @@ SOA::SOA(InputBuffer& buffer, size_t) :
     buffer.readData(numdata_, sizeof(numdata_));
 }
 
+namespace {
+void
+fillParameters(MasterLexer& lexer, uint8_t numdata[20]) {
+    // Copy serial, refresh, retry, expire, minimum.  We accept the extended
+    // TTL-compatible style for the latter four.
+    OutputBuffer buffer(20);
+    buffer.writeUint32(lexer.getNextToken(MasterToken::NUMBER).getNumber());
+    for (int i = 0; i < 4; ++i) {
+        buffer.writeUint32(RRTTL(lexer.getNextToken(MasterToken::STRING).
+                                 getString()).getValue());
+    }
+    memcpy(numdata,  buffer.getData(), buffer.getLength());
+}
+}
+
+/// \brief Constructor from string.
+///
+/// The given string must represent a valid SOA RDATA.  There can be extra
+/// space characters at the beginning or end of the text (which are simply
+/// ignored), but other extra text, including a new line, will make the
+/// construction fail with an exception.
+///
+/// The MNAME and RNAME must be absolute since there's no parameter that
+/// specifies the origin name; if these are not absolute, \c MissingNameOrigin
+/// exception will be thrown.  These must not be represented as a quoted
+/// string.
+///
+/// See the construction that takes \c MasterLexer for other fields.
+///
+/// \throw Others Exception from the Name and RRTTL constructors.
+/// \throw InvalidRdataText Other general syntax errors.
 SOA::SOA(const std::string& soastr) :
-    mname_("."), rname_(".")    // quick hack workaround
+    // Fill in dummy name and replace them soon below.
+    mname_(Name::ROOT_NAME()), rname_(Name::ROOT_NAME())
 {
-    istringstream iss(soastr);
-    string token;
-
-    iss >> token;
-    if (iss.bad() || iss.fail()) {
-        isc_throw(InvalidRdataText, "Invalid SOA MNAME");
+    try {
+        std::istringstream ss(soastr);
+        MasterLexer lexer;
+        lexer.pushSource(ss);
+
+        mname_ = createNameFromLexer(lexer, NULL);
+        rname_ = createNameFromLexer(lexer, NULL);
+        fillParameters(lexer, numdata_);
+
+        if (lexer.getNextToken().getType() != MasterToken::END_OF_FILE) {
+            isc_throw(InvalidRdataText, "extra input text for SOA: "
+                      << soastr);
+        }
+    } catch (const MasterLexer::LexerError& ex) {
+        isc_throw(InvalidRdataText, "Failed to construct SOA from '" <<
+                  soastr << "': " << ex.what());
     }
-    mname_ = Name(token);
-    iss >> token;
-    if (iss.bad() || iss.fail()) {
-        isc_throw(InvalidRdataText, "Invalid SOA RNAME");
-    }
-    rname_ = Name(token);
+}
 
-    uint32_t serial, refresh, retry, expire, minimum;
-    iss >> serial >> refresh >> retry >> expire >> minimum;
-    if (iss.rdstate() != ios::eofbit) {
-        isc_throw(InvalidRdataText, "Invalid SOA format");
-    }
-    OutputBuffer buffer(20);
-    buffer.writeUint32(serial);
-    buffer.writeUint32(refresh);
-    buffer.writeUint32(retry);
-    buffer.writeUint32(expire);
-    buffer.writeUint32(minimum);
-    memcpy(numdata_,  buffer.getData(), buffer.getLength());
+/// \brief Constructor with a context of MasterLexer.
+///
+/// The \c lexer should point to the beginning of valid textual representation
+/// of an SOA RDATA.  The MNAME and RNAME fields can be non absolute if
+/// \c origin is non NULL, in which case \c origin is used to make them
+/// absolute.  These must not be represented as a quoted string.
+///
+/// The REFRESH, RETRY, EXPIRE, and MINIMUM fields can be either a valid
+/// decimal representation of an unsigned 32-bit integer or other
+/// valid textual representation of \c RRTTL such as "1H" (which means 3600).
+///
+/// \throw MasterLexer::LexerError General parsing error such as missing field.
+/// \throw Other Exceptions from the Name and RRTTL constructors if
+/// construction of textual fields as these objects fail.
+///
+/// \param lexer A \c MasterLexer object parsing a master file for the
+/// RDATA to be created
+/// \param origin If non NULL, specifies the origin of MNAME and RNAME when
+/// they are non absolute.
+SOA::SOA(MasterLexer& lexer, const Name* origin,
+         MasterLoader::Options, MasterLoaderCallbacks&) :
+    mname_(createNameFromLexer(lexer, origin)),
+    rname_(createNameFromLexer(lexer, origin))
+{
+    fillParameters(lexer, numdata_);
 }
 
 SOA::SOA(const Name& mname, const Name& rname, uint32_t serial,

src/lib/dns/tests/rdata_soa_unittest.cc

@@ -33,15 +33,129 @@ using namespace isc::dns::rdata;
 namespace {
 class Rdata_SOA_Test : public RdataTest {
 protected:
-    Rdata_SOA_Test() : rdata_soa(Name("ns.example.com"),
-                                 Name("root.example.com"),
-                                 2010012601, 3600, 300, 3600000, 1200)
+    Rdata_SOA_Test() :
+        rdata_soa(Name("ns.example.com"),
+                  Name("root.example.com"),
+                  2010012601, 3600, 300, 3600000, 1200)
     {}
+
+    // Common check to see if the given text can be used to construct SOA
+    // Rdata that is identical rdata_soa.
+    void checkFromText(const char* soa_txt, const Name* origin = NULL) {
+        std::stringstream ss(soa_txt);
+        MasterLexer lexer;
+        lexer.pushSource(ss);
+
+        if (origin == NULL) {
+            // from-string constructor works correctly only when origin
+            // is NULL (by its nature).
+            EXPECT_EQ(0, generic::SOA(soa_txt).compare(rdata_soa));
+        }
+        EXPECT_EQ(0, generic::SOA(lexer, origin, MasterLoader::DEFAULT,
+                                  loader_cb).compare(rdata_soa));
+    }
+
+    // Common check if given text (which is invalid as SOA RDATA) is rejected
+    // with the specified type of exception: ExForString is the expected
+    // exception for the "from string" constructor; ExForLexer is for the
+    // constructor with master lexer.
+    template <typename ExForString, typename ExForLexer>
+    void checkFromBadTexxt(const char* soa_txt, const Name* origin = NULL) {
+        EXPECT_THROW(generic::SOA soa(soa_txt), ExForString);
+
+        std::stringstream ss(soa_txt);
+        MasterLexer lexer;
+        lexer.pushSource(ss);
+        EXPECT_THROW(generic::SOA soa(lexer, origin, MasterLoader::DEFAULT,
+                                      loader_cb), ExForLexer);
+    }
+
     const generic::SOA rdata_soa;
 };
 
 TEST_F(Rdata_SOA_Test, createFromText) {
-    //TBD
+    // A simple case.
+    checkFromText("ns.example.com. root.example.com. "
+                  "2010012601 3600 300 3600000 1200");
+
+    // Beginning and trailing space are ignored.
+    checkFromText("  ns.example.com. root.example.com. "
+                  "2010012601 3600 300 3600000 1200  ");
+
+    // using extended TTL-like form for some parameters.
+    checkFromText("ns.example.com. root.example.com. "
+                  "2010012601 1H 5M 1000H 20M");
+
+    // multi-line.
+    checkFromText("ns.example.com. (root.example.com.\n"
+                  "2010012601 1H 5M 1000H) 20M");
+
+    // relative names for MNAME and RNAME with a separate origin (lexer
+    // version only)
+    const Name origin("example.com");
+    checkFromText("ns root 2010012601 1H 5M 1000H 20M", &origin);
+
+    // with the '@' notation with a separate origin (lexer version only)
+    const Name full_mname("ns.example.com");
+    checkFromText("@ root.example.com. 2010012601 1H 5M 1000H 20M",
+                  &full_mname);
+
+    // bad MNAME/RNAMEs
+    checkFromBadTexxt<EmptyLabel, EmptyLabel>(
+        "bad..example. . 2010012601 1H 5M 1000H 20M");
+    checkFromBadTexxt<EmptyLabel, EmptyLabel>(
+        ". bad..example. 2010012601 1H 5M 1000H 20M");
+
+    // Names shouldn't be quoted. (Note: on completion of #2534, the resulting
+    // exception will be different).
+    checkFromBadTexxt<MissingNameOrigin, MissingNameOrigin>(
+        "\".\" . 0 0 0 0 0");
+    checkFromBadTexxt<MissingNameOrigin, MissingNameOrigin>(
+        ". \".\" 0 0 0 0 0");
+
+    // Missing MAME or RNAME: for the string version, the serial would be
+    // tried as RNAME and result in "not absolute".  For the lexer version,
+    // it reaches the end-of-line, missing min TTL.
+    checkFromBadTexxt<MissingNameOrigin, MasterLexer::LexerError>(
+        ". 2010012601 0 0 0 0", &Name::ROOT_NAME());
+
+    // bad serial.  the string version converts lexer error to
+    // InvalidRdataText.
+    checkFromBadTexxt<InvalidRdataText, MasterLexer::LexerError>(
+        ". . bad 0 0 0 0");
+
+    // bad serial; exceeding the uint32_t range (4294967296 = 2^32)
+    checkFromBadTexxt<InvalidRdataText, MasterLexer::LexerError>(
+        ". . 4294967296 0 0 0 0");
+
+    // Bad format for other numeric parameters.  These will be tried as a TTL,
+    // and result in an exception there.
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(". . 2010012601 bad 0 0 0");
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(
+        ". . 2010012601 4294967296 0 0 0");
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(". . 2010012601 0 bad 0 0");
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(
+        ". . 2010012601 0 4294967296 0 0");
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(". . 2010012601 0 0 bad 0");
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(
+        ". . 2010012601 0 0 4294967296 0");
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(". . 2010012601 0 0 0 bad");
+    checkFromBadTexxt<InvalidRRTTL, InvalidRRTTL>(
+        ". . 2010012601 0 0 0 4294967296");
+
+    // No space between RNAME and serial.  This case is the same as missing
+    // M/RNAME.
+    checkFromBadTexxt<MissingNameOrigin, MasterLexer::LexerError>(
+        ". example.0 0 0 0 0", &Name::ROOT_NAME());
+
+    // Extra parameter.  string version immediately detects the error.
+    EXPECT_THROW(generic::SOA soa(". . 0 0 0 0 0 extra"), InvalidRdataText);
+    // Likewise.  Redundant newline is also considered an error.
+    EXPECT_THROW(generic::SOA soa(". . 0 0 0 0 0\n"), InvalidRdataText);
+    EXPECT_THROW(generic::SOA soa("\n. . 0 0 0 0 0"), InvalidRdataText);
+    // lexer version defers the check to the upper layer (we pass origin
+    // to skip the check with the string version).
+    checkFromText("ns root 2010012601 1H 5M 1000H 20M extra", &origin);
 }
 
 TEST_F(Rdata_SOA_Test, createFromWire) {
@@ -97,4 +211,42 @@ TEST_F(Rdata_SOA_Test, getMinimum) {
                                         0, 0, 0, 0, 0x80706050).getMinimum());
 }
 
+void
+compareCheck(const generic::SOA& small, const generic::SOA& large) {
+    EXPECT_GT(0, small.compare(large));
+    EXPECT_LT(0, large.compare(small));
+}
+
+TEST_F(Rdata_SOA_Test, compare) {
+    // Check simple equivalence
+    EXPECT_EQ(0, rdata_soa.compare(generic::SOA(
+                                       "ns.example.com. root.example.com. "
+                                       "2010012601 3600 300 3600000 1200")));
+    // Check name comparison is case insensitive
+    EXPECT_EQ(0, rdata_soa.compare(generic::SOA(
+                                       "NS.example.com. root.EXAMPLE.com. "
+                                       "2010012601 3600 300 3600000 1200")));
+
+    // Check names are compared in the RDATA comparison semantics (different
+    // from DNSSEC ordering for owner names)
+    compareCheck(generic::SOA("a.example. . 0 0 0 0 0"),
+                 generic::SOA("example. . 0 0 0 0 0"));
+    compareCheck(generic::SOA(". a.example. 0 0 0 0 0"),
+                 generic::SOA(". example. 0 0 0 0 0"));
+
+    // Compare other numeric fields: 1076895760 = 0x40302010,
+    // 270544960 = 0x10203040.  These are chosen to make sure that machine
+    // endian doesn't confuse the comparison results.
+    compareCheck(generic::SOA(". . 270544960 0 0 0 0"),
+                 generic::SOA(". . 1076895760 0 0 0 0"));
+    compareCheck(generic::SOA(". . 0 270544960 0 0 0"),
+                 generic::SOA(". . 0 1076895760 0 0 0"));
+    compareCheck(generic::SOA(". . 0 0 270544960 0 0"),
+                 generic::SOA(". . 0 0 1076895760 0 0"));
+    compareCheck(generic::SOA(". . 0 0 0 270544960 0"),
+                 generic::SOA(". . 0 0 0 1076895760 0"));
+    compareCheck(generic::SOA(". . 0 0 0 0 270544960"),
+                 generic::SOA(". . 0 0 0 0 1076895760"));
+}
+
 }

src/lib/dns/tests/rdata_txt_like_unittest.cc

@@ -57,7 +57,6 @@ template<class TXT_LIKE>
 class Rdata_TXT_LIKE_Test : public RdataTest {
 protected:
     Rdata_TXT_LIKE_Test() :
-        loader_cb(MasterLoaderCallbacks::getNullCallbacks()),
         wiredata_longesttxt(256, 'a'),
         rdata_txt_like("Test-String"),
         rdata_txt_like_empty("\"\""),
@@ -67,7 +66,6 @@ protected:
     }
 
 protected:
-    MasterLoaderCallbacks loader_cb;
     vector<uint8_t> wiredata_longesttxt;
     const TXT_LIKE rdata_txt_like;
     const TXT_LIKE rdata_txt_like_empty;

src/lib/dns/tests/rdata_unittest.cc

@@ -41,7 +41,8 @@ namespace isc {
 namespace dns {
 namespace rdata {
 RdataTest::RdataTest() :
-    obuffer(0), rdata_nomatch(createRdata(RRType(0), RRClass(1), "\\# 0"))
+    obuffer(0), rdata_nomatch(createRdata(RRType(0), RRClass(1), "\\# 0")),
+    loader_cb(MasterLoaderCallbacks::getNullCallbacks())
 {}
 
 RdataPtr

src/lib/dns/tests/rdata_unittest.h

@@ -42,6 +42,7 @@ protected:
     /// used to test the compare() method against a well-known RR type.
     RdataPtr rdata_nomatch;
     MasterLexer lexer;
+    MasterLoaderCallbacks loader_cb;
 };
 
 namespace test {

tests/tools/perfdhcp/perf_pkt4.cc

@@ -13,7 +13,7 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <dhcp/libdhcp++.h>
-#include <dhcp/dhcp6.h>
+#include <dhcp/dhcp4.h>
 
 #include "perf_pkt4.h"
 

tests/tools/perfdhcp/test_control.cc

@@ -1284,6 +1284,10 @@ TestControl::sendDiscover4(const TestControlSocket& socket,
     if (!pkt4) {
         isc_throw(Unexpected, "failed to create DISCOVER packet");
     }
+
+    // Delete the default Message Type option set by Pkt4
+    pkt4->delOption(DHO_DHCP_MESSAGE_TYPE);
+
     // Set options: DHCP_MESSAGE_TYPE and DHCP_PARAMETER_REQUEST_LIST
     OptionBuffer buf_msg_type;
     buf_msg_type.push_back(DHCPDISCOVER);
@@ -1371,11 +1375,7 @@ TestControl::sendRequest4(const TestControlSocket& socket,
                           const dhcp::Pkt4Ptr& offer_pkt4) {
     const uint32_t transid = generateTransid();
     Pkt4Ptr pkt4(new Pkt4(DHCPREQUEST, transid));
-    OptionBuffer buf_msg_type;
-    buf_msg_type.push_back(DHCPREQUEST);
-    OptionPtr opt_msg_type = Option::factory(Option::V4, DHO_DHCP_MESSAGE_TYPE,
-                                             buf_msg_type);
-    pkt4->addOption(opt_msg_type);
+
     // Use first flags indicates that we want to use the server
     // id captured in first packet.
     if (CommandOptions::instance().isUseFirst() &&
@@ -1414,9 +1414,7 @@ TestControl::sendRequest4(const TestControlSocket& socket,
     setDefaults4(socket, pkt4);
 
     // Set hardware address
-    const uint8_t* chaddr = offer_pkt4->getChaddr();
-    std::vector<uint8_t> mac_address(chaddr, chaddr + HW_ETHER_LEN);
-    pkt4->setHWAddr(HTYPE_ETHER, mac_address.size(), mac_address);
+    pkt4->setHWAddr(offer_pkt4->getHWAddr());
     // Set elapsed time.
     uint32_t elapsed_time = getElapsedTime<Pkt4Ptr>(discover_pkt4, offer_pkt4);
     pkt4->setSecs(static_cast<uint16_t>(elapsed_time / 1000));
@@ -1461,8 +1459,10 @@ TestControl::sendRequest4(const TestControlSocket& socket,
                                   transid));
 
      // Set hardware address from OFFER packet received.
-    const uint8_t* chaddr = offer_pkt4->getChaddr();
-    std::vector<uint8_t> mac_address(chaddr, chaddr + HW_ETHER_LEN);
+    HWAddrPtr hwaddr = offer_pkt4->getHWAddr();
+    std::vector<uint8_t> mac_address(HW_ETHER_LEN, 0);
+    uint8_t hw_len = hwaddr->hwaddr_.size();
+    memcpy(&mac_address[0], &hwaddr->hwaddr_[0], hw_len > 16 ? 16 : hw_len);
     pkt4->writeAt(rand_offset, mac_address.begin(), mac_address.end());
 
     // Set elapsed time.

tests/tools/perfdhcp/tests/test_control_unittest.cc

@@ -630,13 +630,10 @@ private:
     boost::shared_ptr<Pkt4>
     createOfferPkt4(uint32_t transid) const {
         boost::shared_ptr<Pkt4> offer(new Pkt4(DHCPOFFER, transid));
-        OptionPtr opt_msg_type = Option::factory(Option::V4, DHO_DHCP_MESSAGE_TYPE,
-                                                 OptionBuffer(DHCPOFFER));
         OptionPtr opt_serverid = Option::factory(Option::V4,
                                                  DHO_DHCP_SERVER_IDENTIFIER,
                                                  OptionBuffer(4, 1));
         offer->setYiaddr(asiolink::IOAddress("127.0.0.1"));
-        offer->addOption(opt_msg_type);
         offer->addOption(opt_serverid);
         offer->updateTimestamp();
         return (offer);