[2902] Pass packet object to the function creating ethernet header.

src/lib/dhcp/pkt4.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-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

src/lib/dhcp/pkt4.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-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

src/lib/dhcp/pkt_filter_lpf.cc

@@ -130,7 +130,7 @@ PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) {
     // decode IP stack and find actual offset of the DHCP packet.
     // 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 making this
+    // ports from the dummy packet. We should consider doing it
     // in some more elegant way.
     Pkt4Ptr dummy_pkt = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 0));
 
@@ -164,15 +164,10 @@ PktFilterLPF::send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) {
 
     OutputBuffer buf(14);
 
-    // Ethernet frame header
-    HWAddrPtr hwaddr = pkt->getRemoteHWAddr();
-    std::vector<uint8_t> dest_addr;
-    if (!hwaddr) {
-        dest_addr.resize(HWAddr::ETHERNET_HWADDR_LEN);
-    } else {
-        dest_addr = pkt->getRemoteHWAddr()->hwaddr_;
-    }
-    writeEthernetHeader(iface.getMac(), &dest_addr[0], buf);
+    // Ethernet frame header.
+    // Note that we don't validate whether HW addresses in 'pkt'
+    // are valid because they are validated be the function called.
+    writeEthernetHeader(pkt, buf);
 
     // It is likely that the local address in pkt object is set to
     // broadcast address. This is the case if server received the
@@ -204,7 +199,8 @@ PktFilterLPF::send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) {
                         reinterpret_cast<const struct sockaddr*>(&sa),
                         sizeof(sockaddr_ll));
     if (result < 0) {
-        isc_throw(SocketWriteError, "pkt4 send failed");
+        isc_throw(SocketWriteError, "failed to send DHCPv4 packet, errno="
+                  << errno << " (check errno.h)");
     }
 
     return (0);

src/lib/dhcp/protocol_util.cc

@@ -111,11 +111,34 @@ decodeIpUdpHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
 }
 
 void
-writeEthernetHeader(const uint8_t* src_hw_addr, const uint8_t* dest_hw_addr,
-                    OutputBuffer& out_buf) {
+writeEthernetHeader(const Pkt4Ptr& pkt, OutputBuffer& out_buf) {
+    HWAddrPtr remote_addr = pkt->getRemoteHWAddr();
+    HWAddrPtr local_addr = pkt->getLocalHWAddr();
+    if (!remote_addr) {
+        isc_throw(BadValue, "remote HW address must be set to construct"
+                  " an ethernet frame header");
+
+    } else if (!local_addr) {
+        isc_throw(BadValue, "local HW address must be set to construct"
+                  " an ethernet frame header");
+
+    } else if (remote_addr->hwaddr_.size() != HWAddr::ETHERNET_HWADDR_LEN) {
+        isc_throw(BadValue, "invalid size of the remote HW address "
+                  << remote_addr->hwaddr_.size() << " when constructing"
+                  << " an ethernet frame header; expected size is"
+                  << " " << HWAddr::ETHERNET_HWADDR_LEN);
+
+    } else if (local_addr->hwaddr_.size() != HWAddr::ETHERNET_HWADDR_LEN) {
+        isc_throw(BadValue, "invalid size of the local HW address "
+                  << local_addr->hwaddr_.size() << " when constructing"
+                  << " an ethernet frame header; expected size is"
+                  << " " << HWAddr::ETHERNET_HWADDR_LEN);
+
+    }
+
     // Write destination and source address.
-    out_buf.writeData(dest_hw_addr, HWAddr::ETHERNET_HWADDR_LEN);
-    out_buf.writeData(src_hw_addr, HWAddr::ETHERNET_HWADDR_LEN);
+    out_buf.writeData(&remote_addr->hwaddr_[0], HWAddr::ETHERNET_HWADDR_LEN);
+    out_buf.writeData(&local_addr->hwaddr_[0], HWAddr::ETHERNET_HWADDR_LEN);
     // Type IP.
     out_buf.writeUint16(0x0800);
 }

src/lib/dhcp/protocol_util.h

@@ -50,6 +50,10 @@ static const size_t IP_SRC_ADDR_OFFSET = 12;
 /// a 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'
+/// pointer is valid. Caller must make sure that pointer is
+/// allocated.
+///
 /// @param buf input buffer holding header to be parsed.
 /// @param [out] pkt packet object receiving HW source address read from header.
 ///
@@ -64,6 +68,10 @@ void decodeEthernetHeader(util::InputBuffer& buf, Pkt4Ptr& pkt);
 /// addresses and ports and read from these headers and stored in
 /// the appropriate members of pkt object.
 ///
+/// @warning This function does not check that the provided 'pkt'
+/// pointer is valid. Caller must make sure that pointer is
+/// allocated.
+///
 /// @param buf input buffer holding headers to be parsed.
 /// @param [out] pkt packet object where IP addresses and ports
 /// are stored.
@@ -74,11 +82,13 @@ void decodeIpUdpHeader(util::InputBuffer& buf, Pkt4Ptr& pkt);
 
 /// @brief Writes ethernet frame header into a buffer.
 ///
-/// @param src_hw_addr source HW address.
-/// @param dst_hw_addr destination HW address.
+/// @warning This function does not check that the provided 'pkt'
+/// pointer is valid. Caller must make sure that pointer is
+/// allocated.
+///
+/// @param pkt packet object holding source and destination HW address.
 /// @param [out] out_buf buffer where a header is written.
-void writeEthernetHeader(const uint8_t* src_hw_addr,
-                         const uint8_t* dest_hw_addr,
+void writeEthernetHeader(const Pkt4Ptr& pkt,
                          util::OutputBuffer& out_buf);
 
 /// @brief Writes both IP and UDP header into output buffer
@@ -88,6 +98,10 @@ void writeEthernetHeader(const uint8_t* src_hw_addr,
 /// ports stored in the Pkt4 object are copied as source and destination
 /// addresses and ports into IP/UDP headers.
 ///
+/// @warning This function does not check that the provided 'pkt'
+/// pointer is valid. Caller must make sure that pointer is
+/// allocated.
+///
 /// @param pkt DHCPv4 packet to be sent in IP packet
 /// @param [out] out_buf buffer where an IP header is written
 void writeIpUdpHeader(const Pkt4Ptr& pkt, util::OutputBuffer& out_buf);

src/lib/dhcp/tests/protocol_util_unittest.cc

@@ -210,11 +210,33 @@ TEST(ProtocolUtilTest, writeEthernetHeader) {
     const uint8_t dest_hw_addr[6] = {
         0x20, 0x31, 0x42, 0x53, 0x64, 0x75
     };
+
     // Create output buffer.
     OutputBuffer buf(1);
+    Pkt4Ptr pkt(new Pkt4(DHCPDISCOVER, 0));
+
+    // HW addresses not set yet. It should fail.
+    EXPECT_THROW(writeEthernetHeader(pkt, buf), BadValue);
+
+    HWAddrPtr local_hw_addr(new HWAddr(src_hw_addr, 6, 1));
+    ASSERT_NO_THROW(pkt->setLocalHWAddr(local_hw_addr));
+
+    // Remote address still not set. It should fail again.
+    EXPECT_THROW(writeEthernetHeader(pkt, buf), BadValue);
+
+    // Set invalid length (7) of the hw address.
+    HWAddrPtr remote_hw_addr(new HWAddr(&std::vector<uint8_t>(1, 7)[0], 7, 1));
+    ASSERT_NO_THROW(pkt->setRemoteHWAddr(remote_hw_addr));
+    // HW address is too long, so it should fail again.
+    EXPECT_THROW(writeEthernetHeader(pkt, buf), BadValue);
+
+    // Finally, set a valid HW address.
+    remote_hw_addr.reset(new HWAddr(dest_hw_addr, 6, 1));
+    ASSERT_NO_THROW(pkt->setRemoteHWAddr(remote_hw_addr));
 
-    // Construct the ethernet header using HW addresses.
-    writeEthernetHeader(src_hw_addr, dest_hw_addr, buf);
+    // Construct the ethernet header using HW addresses stored
+    // in the pkt object.
+    writeEthernetHeader(pkt, buf);
 
     // The resulting ethernet header consists of destination
     // and src HW address (each 6 bytes long) and two bytes