[2902] Changes as a result of the review.

src/bin/dhcp4/dhcp4_srv.cc

@@ -63,7 +63,7 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig, const bool use_bcast,
     try {
         // First call to instance() will create IfaceMgr (it's a singleton)
         // it may throw something if things go wrong.
-        // The 'true' value of in the call to setMatchingPacketFilter imposes
+        // The 'true' value of the call to setMatchingPacketFilter imposes
         // that IfaceMgr will try to use the mechanism to respond directly
         // to the client which doesn't have address assigned. This capability
         // may be lacking on some OSes, so there is no guarantee that server
@@ -373,7 +373,12 @@ Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     }
 
     // If src/dest HW addresses are used by the packet filtering class
-    // we need to copy them as well.
+    // we need to copy them as well. There is a need to check that the
+    // address being set is not-NULL because an attempt to set the NULL
+    // HW would result in exception. If these values are not set, the
+    // the default HW addresses (zeroed) should be generated by the
+    // packet filtering class when creating Ethernet header for
+    // outgoing packet.
     HWAddrPtr src_hw_addr = question->getLocalHWAddr();
     if (src_hw_addr) {
         answer->setLocalHWAddr(src_hw_addr);

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -48,12 +48,26 @@ public:
 
     /// @brief Constructor.
     ///
-    /// It disables configuration of broadcast options on
-    /// sockets that are opened by the Dhcpv4Srv constructor.
-    /// Also, disables the Direct V4 traffic as it requires
-    /// use of raw sockets. Use of broadcast as well as raw
-    /// sockets require root privileges, thus can't be used
-    /// in unit testing.
+    /// This constructor disables default modes of operation used by the
+    /// Dhcpv4Srv class:
+    /// - Send/receive broadcast messages through sockets on interfaces
+    /// which support broadcast traffic.
+    /// - Direct DHCPv4 traffic - communication with clients which do not
+    /// have IP address assigned yet.
+    ///
+    /// Enabling these modes requires root privilges so they must be
+    /// disabled for unit testing.
+    ///
+    /// Note, that disabling broadcast options on sockets does not impact
+    /// the operation of these tests because they use local loopback
+    /// interface which doesn't have broadcast capability anyway. It rather
+    /// prevents setting broadcast options on other (broadcast capable)
+    /// sockets which are opened on other interfaces in Dhcpv4Srv constructor.
+    ///
+    /// The Direct DHCPv4 Traffic capability can be disabled here because
+    /// it is tested with PktFilterLPFTest unittest. The tests which belong
+    /// to PktFilterLPFTest can be enabled on demand when root privileges can
+    /// be guaranteed.
     NakedDhcpv4Srv(uint16_t port = 0)
         : Dhcpv4Srv(port, "type=memfile", false, false) {
     }
@@ -373,26 +387,41 @@ public:
                              const IOAddress& client_addr,
                              const IOAddress& relay_addr) {
 
+        // Create an instance of the tested class.
         boost::scoped_ptr<NakedDhcpv4Srv> srv(new NakedDhcpv4Srv(0));
+
+        // Initialize the source HW address.
         vector<uint8_t> mac(6);
         for (int i = 0; i < 6; i++) {
-            mac[i] = i*10;
+            mac[i] = i * 10;
         }
-
+        // Initialized the destination HW address.
         vector<uint8_t> dst_mac(6);
-        for (int i = 0; i < 6; i++) {
+        for (int i = 0; i < 6; ++i) {
             dst_mac[i] = i * 20;
         }
-
+        // Create a DHCP message. It will be used to simulate the
+        // incoming message.
         boost::shared_ptr<Pkt4> req(new Pkt4(msg_type, 1234));
+        // Create a response message. It will hold a reponse packet.
+        // Initially, set it to NULL.
         boost::shared_ptr<Pkt4> rsp;
-
+        // Set the name of the interface on which packet is received.
         req->setIface("eth0");
+        // Set the interface index. It is just a dummy value and will
+        // not be interprented.
         req->setIndex(17);
+        // Set the target HW address. This value is normally used to
+        // construct the data link layer header.
         req->setRemoteHWAddr(1, 6, dst_mac);
+        // Set the HW address. This value is set on DHCP level (in chaddr).
         req->setHWAddr(1, 6, mac);
+        // Set local HW address. It is used to construct the data link layer
+        // header.
         req->setLocalHWAddr(1, 6, mac);
+        // Set target IP address.
         req->setRemoteAddr(IOAddress(client_addr));
+        // Set relay address.
         req->setGiaddr(relay_addr);
 
         // We are going to test that certain options are returned

src/lib/dhcp/iface_mgr.cc

@@ -220,9 +220,10 @@ IfaceMgr::setPacketFilter(const boost::shared_ptr<PktFilter>& packet_filter) {
         const Iface::SocketCollection& sockets = iface->getSockets();
         for (Iface::SocketCollection::const_iterator sock = sockets.begin();
              sock != sockets.end(); ++sock) {
-            // There is at least one socket open, so we have to fail.
             if (sock->family_ == AF_INET) {
-                isc_throw(PacketFilterChangeDenied, "it is not allowed to set new packet"
+            // There is at least one socket open, so we have to fail.
+                isc_throw(PacketFilterChangeDenied,
+                          "it is not allowed to set new packet"
                           << " filter when there are open IPv4 sockets - need"
                           << " to close them first");
             }

src/lib/dhcp/iface_mgr.h

@@ -88,7 +88,7 @@ struct SocketInfo {
 };
 
 
-/// @brief represents a single network interface
+/// @brief Represents a single network interface
 ///
 /// Iface structure represents network interface with all useful
 /// information, like name, interface index, MAC address and
@@ -96,13 +96,20 @@ struct SocketInfo {
 class Iface {
 public:
 
-    /// maximum MAC address length (Infiniband uses 20 bytes)
+    /// Maximum MAC address length (Infiniband uses 20 bytes)
     static const unsigned int MAX_MAC_LEN = 20;
 
-    /// type that defines list of addresses
+    /// Type that defines list of addresses
     typedef std::vector<isc::asiolink::IOAddress> AddressCollection;
 
-    /// type that holds a list of socket informations
+    /// @brief Type that holds a list of socket information.
+    ///
+    /// @warning The type of the container used here must guarantee
+    /// that the iterators do not invalidate when erase() is called.
+    /// This is because, the \ref closeSockets function removes
+    /// elements selectively by calling erase on the element to be
+    /// removed and further iterates through remaining elements.
+    ///
     /// @todo: Add SocketCollectionConstIter type
     typedef std::list<SocketInfo> SocketCollection;
 
@@ -167,7 +174,7 @@ public:
 
     /// @brief Sets flag_*_ fields based on bitmask value returned by OS
     ///
-    /// Note: Implementation of this method is OS-dependent as bits have
+    /// @note Implementation of this method is OS-dependent as bits have
     /// different meaning on each OS.
     ///
     /// @param flags bitmask value returned by OS in interface detection
@@ -258,53 +265,53 @@ public:
     const SocketCollection& getSockets() const { return sockets_; }
 
 protected:
-    /// socket used to sending data
+    /// Socket used to send data.
     SocketCollection sockets_;
 
-    /// network interface name
+    /// Network interface name.
     std::string name_;
 
-    /// interface index (a value that uniquely indentifies an interface)
+    /// Interface index (a value that uniquely indentifies an interface).
     int ifindex_;
 
-    /// list of assigned addresses
+    /// List of assigned addresses.
     AddressCollection addrs_;
 
-    /// link-layer address
+    /// Link-layer address.
     uint8_t mac_[MAX_MAC_LEN];
 
-    /// length of link-layer address (usually 6)
+    /// Length of link-layer address (usually 6).
     size_t mac_len_;
 
-    /// hardware type
+    /// Hardware type.
     uint16_t hardware_type_;
 
 public:
     /// @todo: Make those fields protected once we start supporting more
     /// than just Linux
 
-    /// specifies if selected interface is loopback
+    /// Specifies if selected interface is loopback.
     bool flag_loopback_;
 
-    /// specifies if selected interface is up
+    /// Specifies if selected interface is up.
     bool flag_up_;
 
-    /// flag specifies if selected interface is running
-    /// (e.g. cable plugged in, wifi associated)
+    /// Flag specifies if selected interface is running
+    /// (e.g. cable plugged in, wifi associated).
     bool flag_running_;
 
-    /// flag specifies if selected interface is multicast capable
+    /// Flag specifies if selected interface is multicast capable.
     bool flag_multicast_;
 
-   /// flag specifies if selected interface is broadcast capable
+    /// Flag specifies if selected interface is broadcast capable.
     bool flag_broadcast_;
 
-    /// interface flags (this value is as is returned by OS,
-    /// it may mean different things on different OSes)
+    /// Interface flags (this value is as is returned by OS,
+    /// it may mean different things on different OSes).
     uint32_t flags_;
 };
 
-/// @brief handles network interfaces, transmission and reception
+/// @brief Handles network interfaces, transmission and reception.
 ///
 /// IfaceMgr is an interface manager class that detects available network
 /// interfaces, configured addresses, link-local addresses, and provides
@@ -312,7 +319,7 @@ public:
 ///
 class IfaceMgr : public boost::noncopyable {
 public:
-    /// defines callback used when commands are received over control session
+    /// Defines callback used when commands are received over control session.
     typedef void (*SessionCallback) (void);
 
     /// @brief Packet reception buffer size
@@ -328,7 +335,7 @@ public:
     //      2 maps (ifindex-indexed and name-indexed) and
     //      also hide it (make it public make tests easier for now)
 
-    /// type that holds a list of interfaces
+    /// Type that holds a list of interfaces.
     typedef std::list<Iface> IfaceCollection;
 
     /// IfaceMgr is a singleton class. This method returns reference
@@ -401,11 +408,10 @@ public:
     /// @return a socket descriptor
     uint16_t getSocket(const isc::dhcp::Pkt4& pkt);
 
-    /// debugging method that prints out all available interfaces
+    /// Debugging method that prints out all available interfaces.
     ///
     /// @param out specifies stream to print list of interfaces to
-    void
-    printIfaces(std::ostream& out = std::cout);
+    void printIfaces(std::ostream& out = std::cout);
 
     /// @brief Sends an IPv6 packet.
     ///
@@ -563,7 +569,7 @@ public:
                       const bool use_bcast = true);
 
     /// @brief Closes all open sockets.
-    /// Is used in destructor, but also from Dhcpv4_srv and Dhcpv6_srv classes.
+    /// Is used in destructor, but also from Dhcpv4Srv and Dhcpv6Srv classes.
     void closeSockets();
 
     /// @brief Closes all IPv4 or IPv6 sockets.
@@ -587,7 +593,7 @@ public:
     /// @throw BadValue if family value is different than AF_INET or AF_INET6.
     void closeSockets(const uint16_t family);
 
-    /// @brief returns number of detected interfaces
+    /// @brief Returns number of detected interfaces.
     ///
     /// @return number of detected interfaces
     uint16_t countIfaces() { return ifaces_.size(); }
@@ -618,7 +624,7 @@ public:
     ///
     /// @throw InvalidPacketFilter if provided packet filter object is NULL.
     /// @throw PacketFilterChangeDenied if there are open IPv4 sockets
-    void setPacketFilter(const boost::shared_ptr<PktFilter>& packet_filter);
+    void setPacketFilter(const PktFilterPtr& packet_filter);
 
     /// @brief Set Packet Filter object to handle send/receive packets.
     ///
@@ -718,14 +724,14 @@ protected:
     //int recvsock_; // TODO: should be fd_set eventually, but we have only
     //int sendsock_; // 2 sockets for now. Will do for until next release
 
-    // we can't use the same socket, as receiving socket
+    // We can't use the same socket, as receiving socket
     // is bound to multicast address. And we all know what happens
     // to people who try to use multicast as source address.
 
-    /// length of the control_buf_ array
+    /// Length of the control_buf_ array
     size_t control_buf_len_;
 
-    /// control-buffer, used in transmission and reception
+    /// Control-buffer, used in transmission and reception.
     boost::scoped_array<char> control_buf_;
 
     /// @brief A wrapper for OS-specific operations before sending IPv4 packet
@@ -745,10 +751,10 @@ protected:
     /// @return true if successful, false otherwise
     bool os_receive4(struct msghdr& m, Pkt4Ptr& pkt);
 
-    /// socket descriptor of the session socket
+    /// Socket descriptor of the session socket.
     int session_socket_;
 
-    /// a callback that will be called when data arrives over session_socket_
+    /// A callback that will be called when data arrives over session_socket_.
     SessionCallback session_callback_;
 private:
 
@@ -795,7 +801,7 @@ private:
     /// families, e.g. AF_INET, AF_PACKET etc. Another possible type of
     /// Packet Filter is the one used for unit testing, which doesn't
     /// open sockets but rather mimics their behavior (mock object).
-    boost::shared_ptr<PktFilter> packet_filter_;
+    PktFilterPtr packet_filter_;
 };
 
 }; // namespace isc::dhcp

src/lib/dhcp/iface_mgr_bsd.cc

@@ -54,8 +54,7 @@ void
 IfaceMgr::setMatchingPacketFilter(const bool /* direct_response_desired */) {
     // @todo Currently we ignore the preference to use direct traffic
     // because it hasn't been implemented for BSD systems.
-    boost::shared_ptr<PktFilter> pkt_filter(new PktFilterInet());
-    setPacketFilter(pkt_filter);
+    setPacketFilter(PktFilterPtr(new PktFilterInet()));
 }
 
 

src/lib/dhcp/iface_mgr_linux.cc

@@ -515,12 +515,10 @@ void Iface::setFlags(uint32_t flags) {
 void
 IfaceMgr::setMatchingPacketFilter(const bool direct_response_desired) {
     if (direct_response_desired) {
-        boost::shared_ptr<PktFilter> pkt_filter(new PktFilterLPF());
-        setPacketFilter(pkt_filter);
+        setPacketFilter(PktFilterPtr(new PktFilterLPF()));
 
     } else {
-        boost::shared_ptr<PktFilter> pkt_filter(new PktFilterInet());
-        setPacketFilter(pkt_filter);
+        setPacketFilter(PktFilterPtr(new PktFilterInet()));
 
     }
 }

src/lib/dhcp/iface_mgr_sun.cc

@@ -54,8 +54,7 @@ void
 IfaceMgr::setMatchingPacketFilter(const bool /* direct_response_desired */) {
     // @todo Currently we ignore the preference to use direct traffic
     // because it hasn't been implemented for Solaris.
-    boost::shared_ptr<PktFilter> pkt_filter(new PktFilterInet());
-    setPacketFilter(pkt_filter);
+    setPacketFilter(PktFilterPtr(new PktFilterInet()));
 }
 
 } // end of isc::dhcp namespace

src/lib/dhcp/pkt4.cc

@@ -284,7 +284,8 @@ Pkt4::setHWAddr(uint8_t htype, uint8_t hlen,
 void
 Pkt4::setHWAddr(const HWAddrPtr& addr) {
     if (!addr) {
-        isc_throw(BadValue, "Setting hw address to NULL is forbidden");
+        isc_throw(BadValue, "Setting DHCPv4 chaddr field to NULL"
+                  << " is forbidden");
     }
     hwaddr_ = addr;
 }
@@ -315,7 +316,8 @@ Pkt4::setLocalHWAddr(const uint8_t htype, const uint8_t hlen,
 void
 Pkt4::setLocalHWAddr(const HWAddrPtr& addr) {
     if (!addr) {
-        isc_throw(BadValue, "Setting HW address to NULL is forbidden");
+        isc_throw(BadValue, "Setting local HW address to NULL is"
+                  << " forbidden.");
     }
     local_hwaddr_ = addr;
 }
@@ -329,7 +331,8 @@ Pkt4::setRemoteHWAddr(const uint8_t htype, const uint8_t hlen,
 void
 Pkt4::setRemoteHWAddr(const HWAddrPtr& addr) {
     if (!addr) {
-        isc_throw(BadValue, "Setting HW address to NULL is forbidden");
+        isc_throw(BadValue, "Setting remote HW address to NULL is"
+                  << " forbidden.");
     }
     remote_hwaddr_ = addr;
 }

src/lib/dhcp/pkt_filter.h

@@ -29,6 +29,7 @@ public:
         isc::Exception(file, line, what) { };
 };
 
+/// Forward declaration to the structure describing a socket.
 struct SocketInfo;
 
 /// Forward declaration to the class representing interface

src/lib/dhcp/pkt_filter_inet.cc

@@ -53,7 +53,7 @@ int PktFilterInet::openSocket(const Iface& iface,
     }
 
 #ifdef SO_BINDTODEVICE
-    if (receive_bcast) {
+    if (receive_bcast && iface.flag_broadcast_) {
         // Bind to device so as we receive traffic on a specific interface.
         if (setsockopt(sock, SOL_SOCKET, SO_BINDTODEVICE, iface.getName().c_str(),
                        iface.getName().length() + 1) < 0) {

src/lib/dhcp/pkt_filter_lpf.cc

@@ -26,45 +26,66 @@
 
 namespace {
 
-/// Socket filter program, used to filter out all traffic other
-/// then DHCP. In particular, it allows receipt of UDP packets
+using namespace isc::dhcp;
+
+/// The socket filter program, used to filter out all traffic other
+/// than DHCP. In particular, it allows receipt of UDP packets
 /// on a specific (customizable) port. It does not allow fragmented
 /// packets.
 ///
 /// Socket filter program is platform independent code which is
 /// executed on the kernel level when new packet arrives. This concept
-/// origins from the Berkeley Packet Filtering supported on BSD systems.
+/// originates from the Berkeley Packet Filtering supported on BSD systems.
 ///
 /// @todo We may want to extend the filter to receive packets sent
 /// to the particular IP address assigned to the interface or
 /// broadcast address.
 struct sock_filter dhcp_sock_filter [] = {
-    // Make sure this is an IP packet...
-    BPF_STMT (BPF_LD + BPF_H + BPF_ABS, 12),
-    BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 8),
-
-    // Make sure it's a UDP packet...
-    BPF_STMT (BPF_LD + BPF_B + BPF_ABS, 23),
-    BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 0, 6),
-
-    // Make sure this isn't a fragment...
-    BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 20),
+    // Make sure this is an IP packet: check the half-word (two bytes)
+    // at offset 12 in the packet (the Ethernet packet type).  If it
+    // is, advance to the next instruction.  If not, advance 8
+    // instructions (which takes execution to the last instruction in
+    // the sequence: "drop it").
+    BPF_STMT(BPF_LD + BPF_H + BPF_ABS, ETHERNET_PACKET_TYPE_OFFSET),
+    BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 8),
+
+    // Make sure it's a UDP packet.  The IP protocol is at offset
+    // 9 in the IP header so, adding the Ethernet packet header size
+    // of 14 bytes gives an absolute byte offset in the packet of 23.
+    BPF_STMT(BPF_LD + BPF_B + BPF_ABS, ETHERNET_HEADER_LEN + IP_PROTO_TYPE_OFFSET),
+    BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 0, 6),
+
+    // Make sure this isn't a fragment by checking that the fragment
+    // offset field in the IP header is zero.  This field is the
+    // least-significant 13 bits in the bytes at offsets 6 and 7 in
+    // the IP header, so the half-word at offset 20 (6 + size of
+    // Ethernet header) is loaded and an appropriate mask applied.
+    BPF_STMT(BPF_LD + BPF_H + BPF_ABS, ETHERNET_HEADER_LEN + IP_FLAGS_OFFSET),
     BPF_JUMP(BPF_JMP + BPF_JSET + BPF_K, 0x1fff, 4, 0),
 
-    // Get the IP header length...
-    BPF_STMT (BPF_LDX + BPF_B + BPF_MSH, 14),
-
-    // Make sure it's to the right port...
-    BPF_STMT (BPF_LD + BPF_H + BPF_IND, 16),
-    // Use default DHCP server port, but it can be
-    // replaced if neccessary.
-    BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, isc::dhcp::DHCP4_SERVER_PORT, 0, 1),
+    // Get the IP header length.  This is achieved by the following
+    // (special) instruction that, given the offset of the start
+    // of the IP header (offset 14) loads the IP header length.
+    BPF_STMT(BPF_LDX + BPF_B + BPF_MSH, ETHERNET_HEADER_LEN),
+
+    // Make sure it's to the right port.  The following instruction
+    // adds the previously extracted IP header length to the given
+    // offset to locate the correct byte.  The given offset of 16
+    // comprises the length of the Ethernet header (14) plus the offset
+    // of the UDP destination port (2) within the UDP header.
+    BPF_STMT(BPF_LD + BPF_H + BPF_IND, ETHERNET_HEADER_LEN + UDP_DEST_PORT),
+    // The following instruction tests against the default DHCP server port,
+    // but the action port is actually set in PktFilterLPF::openSocket().
+    // N.B. The code in that method assumes that this instruction is at
+    // offset 8 in the program.  If this is changed, openSocket() must be
+    // updated.
+    BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, DHCP4_SERVER_PORT, 0, 1),
 
     // If we passed all the tests, ask for the whole packet.
-    BPF_STMT(BPF_RET+BPF_K, (u_int)-1),
+    BPF_STMT(BPF_RET + BPF_K, (u_int)-1),
 
     // Otherwise, drop it.
-    BPF_STMT(BPF_RET+BPF_K, 0),
+    BPF_STMT(BPF_RET + BPF_K, 0),
 };
 
 }
@@ -107,6 +128,10 @@ PktFilterLPF::openSocket(const Iface& iface, const isc::asiolink::IOAddress&,
     sa.sll_family = AF_PACKET;
     sa.sll_ifindex = iface.getIndex();
 
+    // For raw sockets we construct IP headers on our own, so we don't bind
+    // socket to IP address but to the interface. We will later use the
+    // Linux Packet Filtering to filter out these packets that we are
+    // interested in.
     if (bind(sock, reinterpret_cast<const struct sockaddr*>(&sa),
              sizeof(sa)) < 0) {
         close(sock);
@@ -120,9 +145,13 @@ PktFilterLPF::openSocket(const Iface& iface, const isc::asiolink::IOAddress&,
 
 Pkt4Ptr
 PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) {
-    // @todo: implement this function
     uint8_t raw_buf[IfaceMgr::RCVBUFSIZE];
     int data_len = read(socket_info.sockfd_, raw_buf, sizeof(raw_buf));
+    // If negative value is returned by read(), it indicates that an
+    // error occured. If returned value is 0, no data was read from the
+    // socket. In both cases something has gone wrong, because we expect
+    // that a chunk of data is there. We signal the lack of data by
+    // returing an empty packet.
     if (data_len <= 0) {
         return Pkt4Ptr();
     }
@@ -131,9 +160,12 @@ PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) {
 
     // @todo: This is awkward way to solve the chicken and egg problem
     // whereby we don't know the offset where DHCP data start in the
-    // received buffer when we create the packet object. The dummy
-    // object is created so as we can pass it to the functions which
-    // decode IP stack and find actual offset of the DHCP packet.
+    // received buffer when we create the packet object. In general case,
+    // the IP header has variable length. The information about its length
+    // is stored in one of its fields. Therefore, we have to decode the
+    // packet to get the offset of the DHCP data. The dummy object is
+    // created so as we can pass it to the functions which decode IP stack
+    // and find actual offset of the DHCP data.
     // Once we find the offset we can create another Pkt4 object from
     // the reminder of the input buffer and set the IP addresses and
     // ports from the dummy packet. We should consider doing it
@@ -180,11 +212,17 @@ PktFilterLPF::send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) {
     // are valid because they are checked by the function called.
     writeEthernetHeader(pkt, buf);
 
+    // This object represents broadcast address. We will compare the
+    // local packet address with it a few lines below. Having static
+    // variable guarantees that this object is created only once, not
+    // every time this function is called.
+    static const isc::asiolink::IOAddress bcast_addr("255.255.255.255");
+
     // It is likely that the local address in pkt object is set to
     // broadcast address. This is the case if server received the
     // client's packet on broadcast address. Therefore, we need to
     // correct it here and assign the actual source address.
-    if (pkt->getLocalAddr().toText() == "255.255.255.255") {
+    if (pkt->getLocalAddr() == bcast_addr) {
         const Iface::SocketCollection& sockets = iface.getSockets();
         for (Iface::SocketCollection::const_iterator it = sockets.begin();
              it != sockets.end(); ++it) {

src/lib/dhcp/protocol_util.cc

@@ -16,6 +16,7 @@
 #include <dhcp/dhcp6.h>
 #include <dhcp/protocol_util.h>
 #include <boost/static_assert.hpp>
+#include <net/ethernet.h>
 // in_systm.h is required on some some BSD systems
 // complaining that n_time is undefined but used
 // in ip.h.
@@ -34,7 +35,8 @@ decodeEthernetHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
     // then the size of the Ethernet frame header.
     if (buf.getLength() - buf.getPosition() < ETHERNET_HEADER_LEN) {
         isc_throw(InvalidPacketHeader, "size of ethernet header in received "
-                  << "packet is invalid, expected at least 14 bytes, received "
+                  << "packet is invalid, expected at least "
+                  << ETHERNET_HEADER_LEN << " bytes, received "
                   << buf.getLength() - buf.getPosition() << " bytes");
     }
     // Packet object must not be NULL. We want to output some values
@@ -92,7 +94,9 @@ decodeIpUdpHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
     // IP length is the number of 4 byte chunks that construct IPv4 header.
     // It must not be lower than 5 because first 20 bytes are fixed.
     if (ip_len < 5) {
-        ip_len = 5;
+        isc_throw(InvalidPacketHeader, "Value of the length of the IP header must not be"
+                  << " lower than 5 words. The length of the received header is "
+                  << ip_len << ".");
     }
 
     // Seek to the position of source IP address.
@@ -102,7 +106,8 @@ decodeIpUdpHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
     // Read destination address.
     pkt->setLocalAddr(IOAddress(buf.readUint32()));
 
-    // Skip IP header options (if any).
+    // Skip IP header options (if any) to start of the
+    // UDP header.
     buf.setPosition(start_pos + ip_len * 4);
 
     // Read source port from UDP header.
@@ -110,7 +115,8 @@ decodeIpUdpHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
     // Read destination port from UDP header.
     pkt->setLocalPort(buf.readUint16());
 
-    // Set the pointer position to the tail of UDP header.
+    // Set the pointer position to the first byte o the
+    // UDP payload (DHCP packet).
     buf.setPosition(start_pos + ip_len * 4 + UDP_HEADER_LEN);
 }
 
@@ -156,7 +162,7 @@ writeEthernetHeader(const Pkt4Ptr& pkt, OutputBuffer& out_buf) {
     }
 
     // Type IP.
-    out_buf.writeUint16(0x0800);
+    out_buf.writeUint16(ETHERTYPE_IP);
 }
 
 void
@@ -216,7 +222,7 @@ uint16_t
 calcChecksum(const uint8_t* buf, const uint32_t buf_size, uint32_t sum) {
     uint32_t i;
     for (i = 0; i < (buf_size & ~1U); i += 2) {
-        uint16_t chunk = buf[i] << 8 | buf[i+1];
+        uint16_t chunk = buf[i] << 8 | buf[i + 1];
         sum += chunk;
         if (sum > 0xFFFF) {
             sum -= 0xFFFF;

src/lib/dhcp/protocol_util.h

@@ -35,19 +35,31 @@ public:
 
 /// Size of the Ethernet frame header.
 static const size_t ETHERNET_HEADER_LEN = 14;
+/// Offset of the 2-byte word in the Ethernet packet which
+/// holds the type of the protocol it encapsulates.
+static const size_t ETHERNET_PACKET_TYPE_OFFSET = 12;
+
 /// Minimal IPv4 header length.
 static const size_t MIN_IP_HEADER_LEN = 20;
-/// UDP header length.
-static const size_t UDP_HEADER_LEN = 8;
+/// Offset in the IP header where the flags field starts.
+static const size_t IP_FLAGS_OFFSET = 6;
+/// Offset of the byte in IP header which holds the type
+/// of the protocol it encapsulates.
+static const size_t IP_PROTO_TYPE_OFFSET = 9;
 /// Offset of source address in the IPv4 header.
 static const size_t IP_SRC_ADDR_OFFSET = 12;
 
+/// UDP header length.
+static const size_t UDP_HEADER_LEN = 8;
+/// Offset within UDP header where destination port is held.
+static const size_t UDP_DEST_PORT = 2;
+
 /// @brief Decode the Ethernet header.
 ///
 /// This function reads Ethernet frame header from the provided
 /// buffer at the current read position. The source HW address
 /// is read from the header and assigned as client address in
-/// a pkt object. The buffer read pointer is set to the end
+/// the pkt object. The buffer read pointer is set to the end
 /// of the Ethernet frame header if read was successful.
 ///
 /// @warning This function does not check that the provided 'pkt'
@@ -66,7 +78,7 @@ void decodeEthernetHeader(util::InputBuffer& buf, Pkt4Ptr& pkt);
 /// This function reads IP and UDP headers from the provided buffer
 /// at the current read position. The source and destination IP
 /// addresses and ports and read from these headers and stored in
-/// the appropriate members of pkt object.
+/// the appropriate members of the pkt object.
 ///
 /// @warning This function does not check that the provided 'pkt'
 /// pointer is valid. Caller must make sure that pointer is
@@ -95,7 +107,7 @@ void writeEthernetHeader(const Pkt4Ptr& pkt,
 ///
 /// This utility function assembles IP and UDP packet headers for the
 /// provided DHCPv4 message. The source and destination addreses and
-/// ports stored in the Pkt4 object are copied as source and destination
+/// ports stored in the pkt object are copied as source and destination
 /// addresses and ports into IP/UDP headers.
 ///
 /// @warning This function does not check that the provided 'pkt'
@@ -117,6 +129,9 @@ void writeIpUdpHeader(const Pkt4Ptr& pkt, util::OutputBuffer& out_buf);
 /// of the summed values. It must be calculated outside of this function
 /// before writing the value to the packet buffer.
 ///
+/// The IP header checksum calculation algorithm has been defined in
+/// <a href="https://tools.ietf.org/html/rfc791#page-14">RFC 791</a>
+///
 /// @param buf buffer for which the checksum is calculated.
 /// @param buf_size size of the buffer for which checksum is calculated.
 /// @param sum initial checksum value, other values will be added to it.

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -818,7 +818,6 @@ TEST_F(IfaceMgrTest, sendReceive4) {
     );
 
     EXPECT_GE(socket1, 0);
-    //    EXPECT_GE(socket2, 0);
 
     boost::shared_ptr<Pkt4> sendPkt(new Pkt4(DHCPDISCOVER, 1234) );
 

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -686,8 +686,8 @@ TEST(Pkt4Test, hwaddrSrcRemote) {
     EXPECT_TRUE(dst_hwaddr == pkt->getLocalHWAddr());
 
     // Check that we can set the remote address.
-    EXPECT_NO_THROW(pkt->setRemoteHWAddr(dst_hwaddr));
-    EXPECT_TRUE(dst_hwaddr == pkt->getRemoteHWAddr());
+    EXPECT_NO_THROW(pkt->setRemoteHWAddr(src_hwaddr));
+    EXPECT_TRUE(src_hwaddr == pkt->getRemoteHWAddr());
 
     // Can't set the NULL addres.
     EXPECT_THROW(pkt->setRemoteHWAddr(HWAddrPtr()), BadValue);