[3200] Fix the bogus processDiscover and processRequest tests.

Also, clear the output buffer in the Pkt4 and Pkt6 every time when pack()
is called.

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -274,6 +274,45 @@ void Dhcpv4SrvTest::checkClientId(const Pkt4Ptr& rsp, const OptionPtr& expected_
     EXPECT_TRUE(expected_clientid->getData() == opt->getData());
 }
 
+::testing::AssertionResult
+Dhcpv4SrvTest::createPacketFromBuffer(const Pkt4Ptr& src_pkt,
+                                      Pkt4Ptr& dst_pkt) {
+    // Create on-wire format of the packet. If pack() has been called
+    // on this instance of the packet already, the next call to pack()
+    // should remove all contents of the output buffer.
+    try {
+        src_pkt->pack();
+    } catch (const Exception& ex) {
+        return (::testing::AssertionFailure() <<
+                "Failed to parse source packet: "
+                << ex.what());
+    }
+    // Get the output buffer from the source packet.
+    const util::OutputBuffer& buf = src_pkt->getBuffer();
+    // Create a copy of the packet using the output buffer from the source
+    // packet.
+    try {
+        dst_pkt.reset(new Pkt4(static_cast<const uint8_t*>(buf.getData()),
+                               buf.getLength()));
+    } catch (const Exception& ex) {
+        return (::testing::AssertionFailure()
+                << "Failed to create a destination packet from the buffer: "
+                << ex.what());
+    }
+
+    try {
+        // Parse the new packet and return to the caller.
+        dst_pkt->unpack();
+    } catch (const Exception& ex) {
+        return (::testing::AssertionFailure()
+                << "Failed to parse a destination packet: "
+                << ex.what());
+    }
+
+    return (::testing::AssertionSuccess());
+}
+
+
 void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     // Create an instance of the tested class.
     boost::scoped_ptr<NakedDhcpv4Srv> srv(new NakedDhcpv4Srv(0));
@@ -318,9 +357,14 @@ void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     // are returned when requested.
     configureRequestedOptions();
 
+    // Create a copy of the original packet by parsing its wire format.
+    // This simulates the real life scenario when we process the packet
+    // which was parsed from its wire format.
+    Pkt4Ptr received;
+    ASSERT_TRUE(createPacketFromBuffer(req, received));
     if (msg_type == DHCPDISCOVER) {
         ASSERT_NO_THROW(
-            rsp = srv->processDiscover(req);
+            rsp = srv->processDiscover(received);
         );
 
         // Should return OFFER
@@ -328,7 +372,7 @@ void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
         EXPECT_EQ(DHCPOFFER, rsp->getType());
 
     } else {
-        ASSERT_NO_THROW(rsp = srv->processRequest(req););
+        ASSERT_NO_THROW(rsp = srv->processRequest(received));
 
         // Should return ACK
         ASSERT_TRUE(rsp);
@@ -336,7 +380,7 @@ void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
 
     }
 
-    messageCheck(req, rsp);
+    messageCheck(received, rsp);
 
     // We did not request any options so these should not be present
     // in the RSP.
@@ -348,15 +392,18 @@ void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     // Add 'Parameter Request List' option.
     addPrlOption(req);
 
+    ASSERT_TRUE(createPacketFromBuffer(req, received));
+    ASSERT_TRUE(received->getOption(DHO_DHCP_PARAMETER_REQUEST_LIST));
+
     if (msg_type == DHCPDISCOVER) {
-        ASSERT_NO_THROW(rsp = srv->processDiscover(req););
+        ASSERT_NO_THROW(rsp = srv->processDiscover(received));
 
         // Should return non-NULL packet.
         ASSERT_TRUE(rsp);
         EXPECT_EQ(DHCPOFFER, rsp->getType());
 
     } else {
-        ASSERT_NO_THROW(rsp = srv->processRequest(req););
+        ASSERT_NO_THROW(rsp = srv->processRequest(received));
 
         // Should return non-NULL packet.
         ASSERT_TRUE(rsp);

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -183,6 +183,32 @@ public:
     /// @return relayed DISCOVER
     Pkt4Ptr captureRelayedDiscover();
 
+    /// @brief Create packet from output buffer of another packet.
+    ///
+    /// This function creates a packet using an output buffer from another
+    /// packet. This imitates reception of a packet from the wire. The
+    /// unpack function is then called to parse packet contents and to
+    /// create a collection of the options carried by this packet.
+    ///
+    /// This function is useful for unit tests which verify that the received
+    /// packet is parsed correctly. In those cases it is usually inappropriate
+    /// to create an instance of the packet, add options, set packet
+    /// fields and use such packet as an input to processDiscover or
+    /// processRequest function. This is because, such a packet has certain
+    /// options already initialized and there is no way to verify that they
+    /// have been initialized when packet instance was created or wire buffer
+    /// processing. By creating a packet from the buffer we guarantee that the
+    /// new packet is entirely initialized during wire data parsing.
+    ///
+    /// @param src_pkt A source packet, to be copied.
+    /// @param [out] dst_pkt A destination packet.
+    ///
+    /// @return assertion result indicating if a function completed with
+    /// success or failure.
+    static ::testing::AssertionResult
+    createPacketFromBuffer(const Pkt4Ptr& src_pkt,
+                           Pkt4Ptr& dst_pkt);
+
     /// @brief generates a DHCPv4 packet based on provided hex string
     ///
     /// @return created packet

src/lib/dhcp/pkt4.cc

@@ -108,6 +108,10 @@ Pkt4::pack() {
         isc_throw(InvalidOperation, "Can't build Pkt4 packet. HWAddr not set.");
     }
 
+    // Clear the output buffer to make sure that consecutive calls to pack()
+    // will not result in concatenation of multiple packet copies.
+    buffer_out_.clear();
+
     try {
         size_t hw_len = hwaddr_->hwaddr_.size();
 

src/lib/dhcp/pkt4.h

@@ -72,6 +72,7 @@ public:
     /// Prepares on-wire format of message and all its options.
     /// Options must be stored in options_ field.
     /// Output buffer will be stored in buffer_out_.
+    /// The buffer_out_ is cleared before writting to the buffer.
     ///
     /// @throw InvalidOperation if packing fails
     void

src/lib/dhcp/pkt6.cc

@@ -43,7 +43,7 @@ Pkt6::Pkt6(const uint8_t* buf, uint32_t buf_len, DHCPv6Proto proto /* = UDP */)
     remote_addr_("::"),
     local_port_(0),
     remote_port_(0),
-    bufferOut_(0) {
+    buffer_out_(0) {
     data_.resize(buf_len);
     memcpy(&data_[0], buf, buf_len);
 }
@@ -58,7 +58,7 @@ Pkt6::Pkt6(uint8_t msg_type, uint32_t transid, DHCPv6Proto proto /*= UDP*/) :
     remote_addr_("::"),
     local_port_(0),
     remote_port_(0),
-    bufferOut_(0) {
+    buffer_out_(0) {
 }
 
 uint16_t Pkt6::len() {
@@ -199,6 +199,8 @@ Pkt6::pack() {
 void
 Pkt6::packUDP() {
     try {
+        // Make sure that the buffer is empty before we start writting to it.
+        buffer_out_.clear();
 
         // is this a relayed packet?
         if (!relay_info_.empty()) {
@@ -215,11 +217,11 @@ Pkt6::packUDP() {
                  relay != relay_info_.end(); ++relay) {
 
                 // build relay-forw/relay-repl header (see RFC3315, section 7)
-                bufferOut_.writeUint8(relay->msg_type_);
-                bufferOut_.writeUint8(relay->hop_count_);
-                bufferOut_.writeData(&(relay->linkaddr_.toBytes()[0]),
+                buffer_out_.writeUint8(relay->msg_type_);
+                buffer_out_.writeUint8(relay->hop_count_);
+                buffer_out_.writeData(&(relay->linkaddr_.toBytes()[0]),
                                      isc::asiolink::V6ADDRESS_LEN);
-                bufferOut_.writeData(&relay->peeraddr_.toBytes()[0],
+                buffer_out_.writeData(&relay->peeraddr_.toBytes()[0],
                                      isc::asiolink::V6ADDRESS_LEN);
 
                 // store every option in this relay scope. Usually that will be
@@ -230,28 +232,28 @@ Pkt6::packUDP() {
                 for (OptionCollection::const_iterator opt =
                          relay->options_.begin();
                      opt != relay->options_.end(); ++opt) {
-                    (opt->second)->pack(bufferOut_);
+                    (opt->second)->pack(buffer_out_);
                 }
 
                 // and include header relay-msg option. Its payload will be
                 // generated in the next iteration (if there are more relays)
                 // or outside the loop (if there are no more relays and the
                 // payload is a direct message)
-                bufferOut_.writeUint16(D6O_RELAY_MSG);
-                bufferOut_.writeUint16(relay->relay_msg_len_);
+                buffer_out_.writeUint16(D6O_RELAY_MSG);
+                buffer_out_.writeUint16(relay->relay_msg_len_);
             }
 
         }
 
         // DHCPv6 header: message-type (1 octect) + transaction id (3 octets)
-        bufferOut_.writeUint8(msg_type_);
+        buffer_out_.writeUint8(msg_type_);
         // store 3-octet transaction-id
-        bufferOut_.writeUint8( (transid_ >> 16) & 0xff );
-        bufferOut_.writeUint8( (transid_ >> 8) & 0xff );
-        bufferOut_.writeUint8( (transid_) & 0xff );
+        buffer_out_.writeUint8( (transid_ >> 16) & 0xff );
+        buffer_out_.writeUint8( (transid_ >> 8) & 0xff );
+        buffer_out_.writeUint8( (transid_) & 0xff );
 
         // the rest are options
-        LibDHCP::packOptions(bufferOut_, options_);
+        LibDHCP::packOptions(buffer_out_, options_);
     }
     catch (const Exception& e) {
        // An exception is thrown and message will be written to Logger
@@ -502,7 +504,7 @@ Pkt6::delOption(uint16_t type) {
 }
 
 void Pkt6::repack() {
-    bufferOut_.writeData(&data_[0], data_.size());
+    buffer_out_.writeData(&data_[0], data_.size());
 }
 
 void

src/lib/dhcp/pkt6.h

@@ -115,6 +115,7 @@ public:
     /// Options must be stored in options_ field.
     /// Output buffer will be stored in data_. Length
     /// will be set in data_len_.
+    /// The output buffer is cleared before new data is written to it.
     ///
     /// @throw BadValue if packet protocol is invalid, InvalidOperation
     /// if packing fails, or NotImplemented if protocol is TCP (IPv6 over TCP is
@@ -139,7 +140,7 @@ public:
     /// zero length
     ///
     /// @return reference to output buffer
-    const isc::util::OutputBuffer& getBuffer() const { return (bufferOut_); };
+    const isc::util::OutputBuffer& getBuffer() const { return (buffer_out_); };
 
     /// @brief Returns protocol of this packet (UDP or TCP).
     ///
@@ -530,7 +531,7 @@ protected:
     /// remote TCP or UDP port
     uint16_t remote_port_;
 
-    /// output buffer (used during message transmission)
+    /// Output buffer (used during message transmission)
     ///
     /// @warning This protected member is accessed by derived
     /// classes directly. One of such derived classes is
@@ -538,7 +539,7 @@ protected:
     /// behavior must be taken into consideration before making
     /// changes to this member such as access scope restriction or
     /// data format change etc.
-    isc::util::OutputBuffer bufferOut_;
+    isc::util::OutputBuffer buffer_out_;
 
     /// packet timestamp
     boost::posix_time::ptime timestamp_;

tests/tools/perfdhcp/perf_pkt6.cc

@@ -43,7 +43,7 @@ PerfPkt6::rawPack() {
                                options_,
                                getTransidOffset(),
                                getTransid(),
-                               bufferOut_));
+                               buffer_out_));
 }
 
 bool