[3180] Addressed review comments.

src/bin/dhcp4/dhcp4_srv.cc

@@ -1181,9 +1181,9 @@ Dhcpv4Srv::unpackOptions(const OptionBuffer& buf,
     size_t offset = 0;
 
     OptionDefContainer option_defs;
-    if (option_space == "dhcp6") {
+    if (option_space == "dhcp4") {
         // Get the list of stdandard option definitions.
-        option_defs = LibDHCP::getOptionDefs(Option::V6);
+        option_defs = LibDHCP::getOptionDefs(Option::V4);
     } else if (!option_space.empty()) {
         OptionDefContainerPtr option_defs_ptr =
             CfgMgr::instance().getOptionDefs(option_space);

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -1664,6 +1664,7 @@ TEST_F(Dhcpv4SrvTest, Hooks) {
 // - option (option space 'foobar')
 //   - sub option (option space 'foo')
 //      - sub option (option space 'bar')
+// @todo Add more thorough unit tests for unpackOptions.
 TEST_F(Dhcpv4SrvTest, unpackOptions) {
     // Create option definition for each level of encapsulation. Each option
     // definition is for the option code 1. Options may have the same

src/lib/dhcp/option.h

@@ -45,12 +45,32 @@ typedef boost::shared_ptr<OptionBuffer> OptionBufferPtr;
 class Option;
 typedef boost::shared_ptr<Option> OptionPtr;
 
-/// A collection of DHCPv6 options
+/// A collection of DHCP (v4 or v6) options
 typedef std::multimap<unsigned int, OptionPtr> OptionCollection;
 
-/// This type describes a callback function to parse options from buffer.
-typedef boost::function< size_t(const OptionBuffer&, const std::string,
-                                OptionCollection&, size_t*, size_t*)
+/// @brief This type describes a callback function to parse options from buffer.
+///
+/// @note The last two parameters should be specified in the callback function
+/// parameters list only if DHCPv6 options are parsed. Exclude these parameters
+/// from the callback function defined to parse DHCPv4 options.
+///
+/// @param buffer A buffer holding options to be parsed.
+/// @param encapsulated_space A name of the option space to which options being
+/// parsed belong.
+/// @param [out] options A container to which parsed options should be appended.
+/// @param relay_msg_offset A pointer to a size_t value. It indicates the
+/// offset to beginning of relay_msg option. This parameter should be specified
+/// for DHCPv6 options only.
+/// @param relay_msg_len A pointer to a size_t value. It holds the length of
+/// of the relay_msg option. This parameter should be specified for DHCPv6
+/// options only.
+///
+/// @return An offset to the first byte after last parsed option.
+typedef boost::function< size_t(const OptionBuffer& buffer,
+                                const std::string encapsulated_space,
+                                OptionCollection& options,
+                                size_t* relay_msg_offset,
+                                size_t* relay_msg_len)
                          > UnpackOptionsCallback;
 
 

src/lib/dhcp/option_int.h

@@ -47,6 +47,7 @@ public:
     ///
     /// @throw isc::dhcp::InvalidDataType if data field type provided
     /// as template parameter is not a supported integer type.
+    /// @todo Extend constructor to set encapsulated option space name.
     OptionInt(Option::Universe u, uint16_t type, T value)
         : Option(u, type), value_(value) {
         if (!OptionDataTypeTraits<T>::integer_type) {
@@ -69,6 +70,7 @@ public:
     /// @throw isc::OutOfRange if provided buffer is shorter than data size.
     /// @throw isc::dhcp::InvalidDataType if data field type provided
     /// as template parameter is not a supported integer type.
+    /// @todo Extend constructor to set encapsulated option space name.
     OptionInt(Option::Universe u, uint16_t type, OptionBufferConstIter begin,
                OptionBufferConstIter end)
         : Option(u, type) {

src/lib/dhcp/pkt4.cc

@@ -162,61 +162,66 @@ void
 Pkt4::unpack() {
 
     // input buffer (used during message reception)
-    isc::util::InputBuffer bufferIn(&data_[0], data_.size());
+    isc::util::InputBuffer buffer_in(&data_[0], data_.size());
 
-    if (bufferIn.getLength() < DHCPV4_PKT_HDR_LEN) {
+    if (buffer_in.getLength() < DHCPV4_PKT_HDR_LEN) {
         isc_throw(OutOfRange, "Received truncated DHCPv4 packet (len="
-                  << bufferIn.getLength() << " received, at least "
+                  << buffer_in.getLength() << " received, at least "
                   << DHCPV4_PKT_HDR_LEN << "is expected");
     }
 
-    op_ = bufferIn.readUint8();
-    uint8_t htype = bufferIn.readUint8();
-    uint8_t hlen = bufferIn.readUint8();
-    hops_ = bufferIn.readUint8();
-    transid_ = bufferIn.readUint32();
-    secs_ = bufferIn.readUint16();
-    flags_ = bufferIn.readUint16();
-    ciaddr_ = IOAddress(bufferIn.readUint32());
-    yiaddr_ = IOAddress(bufferIn.readUint32());
-    siaddr_ = IOAddress(bufferIn.readUint32());
-    giaddr_ = IOAddress(bufferIn.readUint32());
+    op_ = buffer_in.readUint8();
+    uint8_t htype = buffer_in.readUint8();
+    uint8_t hlen = buffer_in.readUint8();
+    hops_ = buffer_in.readUint8();
+    transid_ = buffer_in.readUint32();
+    secs_ = buffer_in.readUint16();
+    flags_ = buffer_in.readUint16();
+    ciaddr_ = IOAddress(buffer_in.readUint32());
+    yiaddr_ = IOAddress(buffer_in.readUint32());
+    siaddr_ = IOAddress(buffer_in.readUint32());
+    giaddr_ = IOAddress(buffer_in.readUint32());
 
     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);
+    buffer_in.readVector(hw_addr, MAX_CHADDR_LEN);
+    buffer_in.readData(sname_, MAX_SNAME_LEN);
+    buffer_in.readData(file_, MAX_FILE_LEN);
 
     hw_addr.resize(hlen);
 
     hwaddr_ = HWAddrPtr(new HWAddr(hw_addr, htype));
 
-    if (bufferIn.getLength() == bufferIn.getPosition()) {
+    if (buffer_in.getLength() == buffer_in.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
         // differentiates between DHCP and BOOTP packets.
         isc_throw(InvalidOperation, "Received BOOTP packet. BOOTP is not supported.");
     }
 
-    if (bufferIn.getLength() - bufferIn.getPosition() < 4) {
+    if (buffer_in.getLength() - buffer_in.getPosition() < 4) {
       // there is not enough data to hold magic DHCP cookie
       isc_throw(Unexpected, "Truncated or no DHCP packet.");
     }
 
-    uint32_t magic = bufferIn.readUint32();
+    uint32_t magic = buffer_in.readUint32();
     if (magic != DHCP_OPTIONS_COOKIE) {
       isc_throw(Unexpected, "Invalid or missing DHCP magic cookie");
     }
 
-    size_t opts_len = bufferIn.getLength() - bufferIn.getPosition();
+    size_t opts_len = buffer_in.getLength() - buffer_in.getPosition();
     vector<uint8_t> opts_buffer;
 
-    // First use of readVector.
-    bufferIn.readVector(opts_buffer, opts_len);
+    // Use readVector because a function which parses option requires
+    // a vector as an input.
+    buffer_in.readVector(opts_buffer, opts_len);
     if (callback_.empty()) {
         LibDHCP::unpackOptions4(opts_buffer, options_);
     } else {
-        callback_(opts_buffer, "dhcp4", options_, 0, 0);
+        // The last two arguments are set to NULL because they are
+        // specific to DHCPv6 options parsing. They are unused for
+        // DHCPv4 case. In DHCPv6 case they hold are the relay message
+        // offset and length.
+        callback_(opts_buffer, "dhcp4", options_, NULL, NULL);
     }
 
     // @todo check will need to be called separately, so hooks can be called

src/lib/dhcp/pkt6.cc

@@ -330,6 +330,9 @@ Pkt6::unpackMsg(OptionBuffer::const_iterator begin,
         if (callback_.empty()) {
             LibDHCP::unpackOptions6(opt_buffer, options_);
         } else {
+            // The last two arguments hold the DHCPv6 Relay message offset and
+            // length. Setting them to NULL because we are dealing with the
+            // not-relayed message.
             callback_(opt_buffer, "dhcp6", options_, 0, 0);
         }
     } catch (const Exception& e) {

src/lib/dhcp/tests/option_unittest.cc

@@ -591,11 +591,11 @@ TEST_F(OptionTest, unpackCallback) {
     // Create a buffer which holds two sub options.
     const char opt_data[] = {
         0x00, 0x01,  // sub option code  = 1
-        0x00, 0x02,  // sub option length = 1
-        0x00, 0x01,  // sub option data
+        0x00, 0x02,  // sub option length = 2
+        0x00, 0x01,  // sub option data (2 bytes)
         0x00, 0x02,  // sub option code = 2
-        0x00, 0x02,  // sub option length
-        0x00, 0x01   // sub option data
+        0x00, 0x02,  // sub option length = 2
+        0x01, 0x01   // sub option data (2 bytes)
     };
     OptionBuffer opt_buf(opt_data, opt_data + sizeof(opt_data));
 
@@ -605,6 +605,9 @@ TEST_F(OptionTest, unpackCallback) {
     ASSERT_FALSE(cb.executed_);
     // Create an option and install a callback.
     NakedOption option;
+    // Parameters from _1 to _5 are placeholders for the actual values
+    // to be passed to the callback function. See: boost::bind documentation
+    // at http://www.boost.org/doc/libs/1_54_0/libs/bind/bind.html.
     option.setCallback(boost::bind(&CustomUnpackCallback::execute, &cb,
                                    _1, _2, _3, _4, _5));
     // Parse options. It should result in a call to our callback function.

tests/tools/perfdhcp/pkt_transform.cc

@@ -32,7 +32,7 @@ namespace perfdhcp {
 bool
 PktTransform::pack(const Option::Universe universe,
                    const OptionBuffer& in_buffer,
-                   const Option::OptionCollection& options,
+                   const OptionCollection& options,
                    const size_t transid_offset,
                    const uint32_t transid,
                    util::OutputBuffer& out_buffer) {
@@ -75,7 +75,7 @@ PktTransform::pack(const Option::Universe universe,
 bool
 PktTransform::unpack(const Option::Universe universe,
                      const OptionBuffer& in_buffer,
-                     const Option::OptionCollection& options,
+                     const OptionCollection& options,
                      const size_t transid_offset,
                      uint32_t& transid) {
 
@@ -113,13 +113,13 @@ PktTransform::unpack(const Option::Universe universe,
 
 void
 PktTransform::packOptions(const OptionBuffer& in_buffer,
-                          const Option::OptionCollection& options,
+                          const OptionCollection& options,
                           util::OutputBuffer& out_buffer) {
     try {
         // If there are any options on the list, we will use provided
         // options offsets to override them in the output buffer
         // with new contents.
-        for (Option::OptionCollection::const_iterator it = options.begin();
+        for (OptionCollection::const_iterator it = options.begin();
              it != options.end(); ++it) {
             // Get options with their position (offset).
             boost::shared_ptr<LocalizedOption> option =
@@ -157,8 +157,8 @@ PktTransform::packOptions(const OptionBuffer& in_buffer,
 
 void
 PktTransform::unpackOptions(const OptionBuffer& in_buffer,
-                            const Option::OptionCollection& options) {
-    for (Option::OptionCollection::const_iterator it = options.begin();
+                            const OptionCollection& options) {
+    for (OptionCollection::const_iterator it = options.begin();
          it != options.end(); ++it) {
 
         boost::shared_ptr<LocalizedOption> option =

tests/tools/perfdhcp/pkt_transform.h

@@ -66,7 +66,7 @@ public:
     /// \return false, if pack operation failed.
     static bool pack(const dhcp::Option::Universe universe,
                      const dhcp::OptionBuffer& in_buffer,
-                     const dhcp::Option::OptionCollection& options,
+                     const dhcp::OptionCollection& options,
                      const size_t transid_offset,
                      const uint32_t transid,
                      util::OutputBuffer& out_buffer);
@@ -88,7 +88,7 @@ public:
     /// \return false, if unpack operation failed.
     static bool unpack(const dhcp::Option::Universe universe,
                        const dhcp::OptionBuffer& in_buffer,
-                       const dhcp::Option::OptionCollection& options,
+                       const dhcp::OptionCollection& options,
                        const size_t transid_offset,
                        uint32_t& transid);
 
@@ -135,7 +135,7 @@ private:
     ///
     /// \throw isc::Unexpected if options update failed.
     static void packOptions(const dhcp::OptionBuffer& in_buffer,
-                            const dhcp::Option::OptionCollection& options,
+                            const dhcp::OptionCollection& options,
                             util::OutputBuffer& out_buffer);
 
     /// \brief Reads contents of specified options from buffer.
@@ -159,7 +159,7 @@ private:
     ///
     /// \throw isc::Unexpected if options unpack failed.
     static void unpackOptions(const dhcp::OptionBuffer& in_buffer,
-                              const dhcp::Option::OptionCollection& options);
+                              const dhcp::OptionCollection& options);
 
 };