[3546] Changes after review

src/lib/dhcp/pkt.cc

@@ -18,9 +18,34 @@
 namespace isc {
 namespace dhcp {
 
-void
-Pkt::addOption(const OptionPtr& opt) {
-    options_.insert(std::pair<int, OptionPtr>(opt->getType(), opt));
+Pkt::Pkt(uint32_t transid, const isc::asiolink::IOAddress& local_addr,
+         const isc::asiolink::IOAddress& remote_addr, uint16_t local_port,
+         uint16_t remote_port)
+    :transid_(transid),
+     iface_(""),
+     ifindex_(-1),
+     local_addr_(local_addr),
+     remote_addr_(remote_addr),
+     local_port_(local_port),
+     remote_port_(remote_port),
+     buffer_out_(0)
+{
+}
+
+Pkt::Pkt(const uint8_t* buf, uint32_t len, const isc::asiolink::IOAddress& local_addr,
+         const isc::asiolink::IOAddress& remote_addr, uint16_t local_port,
+         uint16_t remote_port)
+    :transid_(0),
+     iface_(""),
+     ifindex_(-1),
+     local_addr_(local_addr),
+     remote_addr_(remote_addr),
+     local_port_(local_port),
+     remote_port_(remote_port),
+     buffer_out_(0)
+{
+    data_.resize(len);
+    memcpy(&data_[0], buf, len);
 }
 
 OptionPtr
@@ -96,8 +121,8 @@ Pkt::getMAC(uint32_t hw_addr_src) {
         if (mac) {
             return (mac);
         } else if (hw_addr_src == MAC_SOURCE_RAW) {
-            // If we're interested only in RAW sockets, no bother trying
-            // other options.
+            // If we're interested only in RAW sockets as source of that info,
+            // there's no point in trying other options.
             return (HWAddrPtr());
         }
     }

src/lib/dhcp/pkt.h

@@ -27,16 +27,19 @@ namespace isc {
 
 namespace dhcp {
 
-/// @brief Base class for DHCPv4 (Pkt4) and DHCPv6 (Pkt6) classes
+/// @brief Base class for classes representing DHCP messages.
 ///
-/// This is a base class that holds all common information (e.g. source and
-/// destination ports) and operations (e.g. add, get, delete options).
-/// This class is purely virtual. Please instantiate Pkt4, Pkt6 or any
-/// other derived classes.
+/// This is a base class that holds common information (e.g. source
+/// and destination ports) and operations (e.g. add, get, delete options)
+/// for derived classes representing both DHCPv4 and DHCPv6 messages.
+/// The @c Pkt4 and @c Pkt6 classes derive from it.
+///
+/// @note This is abstract class. Please instantiate derived classes
+/// such as @c Pkt4 or @c Pkt6.
 class Pkt {
 public:
 
-    /// @defgroup Specifies where a given MAC address was obtained
+    /// @defgroup mac_sources Specifies where a given MAC address was obtained.
     ///
     /// @brief The list covers all possible MAC sources.
     ///
@@ -45,32 +48,33 @@ public:
     ///
     /// @{
 
-    /// Not really a type, only used in getMAC() calls
+    /// Not really a type, only used in getMAC() calls.
     static const uint32_t MAC_SOURCE_ANY = 0xffff;
 
-    /// obtained first hand from raw socket (100% reliable)
+    /// Obtained first hand from raw socket (100% reliable).
     static const uint32_t MAC_SOURCE_RAW = 0x0001;
 
-    /// extracted from DUID-LL or DUID-LLT (not reliable as the client
-    /// can send fake DUID)
+    /// Extracted from DUID-LL or DUID-LLT (not 100% reliable as the client
+    /// can send fake DUID).
     static const uint32_t MAC_SOURCE_DUID = 0x0002;
 
-    /// extracted from IPv6 link-local address. Client can use different
-    /// IID other than EUI-64, e.g. Windows supports RFC4941 and uses
-    /// random values instead of EUI-64.
+    /// Extracted from IPv6 link-local address. Not 100% reliable, as the
+    /// client can use different IID other than EUI-64, e.g. Windows supports
+    /// RFC4941 and uses random values instead of EUI-64.
     static const uint32_t MAC_SOURCE_IPV6_LINK_LOCAL = 0x0004;
 
-    /// RFC6939 (a relay agent can insert client link layer address option)
-    /// Note that a skilled attacker can fake that by sending his request
-    /// relayed, so the legitimate relay will think it's a second relay.
+    /// Get it from RFC6939 option. (A relay agent can insert client link layer
+    /// address option). Note that a skilled attacker can fake that by sending
+    /// his request relayed, so the legitimate relay will think it's a second
+    /// relay.
     static const uint32_t MAC_SOURCE_CLIENT_ADDR_RELAY_OPTION = 0x0008;
 
     /// A relay can insert remote-id. In some deployments it contains a MAC
-    /// address.
+    /// address (RFC4649).
     static const uint32_t MAC_SOURCE_REMOTE_ID = 0x0010;
 
     /// A relay can insert a subscriber-id option. In some deployments it
-    /// contains a MAC address.
+    /// contains a MAC address (RFC4580).
     static const uint32_t MAC_SOURCE_SUBSCRIBER_ID = 0x0020;
 
     /// A CMTS (acting as DHCP relay agent) that supports DOCSIS standard
@@ -80,64 +84,73 @@ public:
 
     /// @}
 
-
 protected:
-Pkt(uint32_t transid, const isc::asiolink::IOAddress& local_addr,
-    const isc::asiolink::IOAddress& remote_addr, uint16_t local_port,
-    uint16_t remote_port)
-        :transid_(transid),
-        iface_(""),
-        ifindex_(-1),
-        local_addr_(local_addr),
-        remote_addr_(remote_addr),
-        local_port_(local_port),
-        remote_port_(remote_port),
-        buffer_out_(0)
-        {
-        }
-
-    Pkt(const uint8_t* buf, uint32_t len, const isc::asiolink::IOAddress& local_addr,
+
+    /// @brief Constructor.
+    ///
+    /// This constructor is typically used for transmitted messages as it
+    /// creates an empty (no options) packet. The constructor is protected,
+    /// so only derived classes can call it. Pkt class cannot be instantiated
+    /// anyway, because it is an abstract class.
+    ///
+    /// @param transid transaction-id
+    /// @param local_addr local IPv4 or IPv6 address
+    /// @param remote_addr remote IPv4 or IPv6 address
+    /// @param local_port local UDP (one day also TCP) port
+    /// @param remote_port remote UDP (one day also TCP) port
+    Pkt(uint32_t transid, const isc::asiolink::IOAddress& local_addr,
         const isc::asiolink::IOAddress& remote_addr, uint16_t local_port,
-        uint16_t remote_port)
-       :transid_(0),
-        iface_(""),
-        ifindex_(-1),
-        local_addr_(local_addr),
-        remote_addr_(remote_addr),
-        local_port_(local_port),
-        remote_port_(remote_port),
-        buffer_out_(0)
-    {
-        data_.resize(len);
-        memcpy(&data_[0], buf, len);
-    }
+        uint16_t remote_port);
+
+    /// @brief Constructor.
+    ///
+    /// This constructor is typically used for received messages as it takes
+    /// a buffer that's going to be parsed as one of arguments. The constructor
+    /// is protected, so only derived classes can call it. Pkt class cannot be
+    /// instantiated anyway, because it is an abstract class.
+    ///
+    /// @param buf pointer to a buffer that contains on-wire data
+    /// @param len length of the pointer specified in buf
+    /// @param local_addr local IPv4 or IPv6 address
+    /// @param remote_addr remote IPv4 or IPv6 address
+    /// @param local_port local UDP (one day also TCP) port
+    /// @param remote_port remote UDP (one day also TCP) port
+    Pkt(const uint8_t* buf, uint32_t len,
+        const isc::asiolink::IOAddress& local_addr,
+        const isc::asiolink::IOAddress& remote_addr, uint16_t local_port,
+        uint16_t remote_port);
 
 public:
 
     /// @brief Prepares on-wire format of DHCP (either v4 or v6) packet.
     ///
     /// Prepares on-wire format of message and all its options.
-    /// Options must be stored in options_ field.
+    /// A caller must ensure that options are stored in options_ field
+    /// prior to calling this method.
+    ///
     /// Output buffer will be stored in buffer_out_.
-    /// The buffer_out_ is cleared before writting to the buffer.
+    /// The buffer_out_ should be cleared before writting to the buffer
+    /// in the derived classes.
     ///
-    /// @note This is a pure virtual method. Actual implementations are in
-    /// Pkt4 and Pkt6 class.
+    /// @note This is a pure virtual method and must be implemented in
+    /// the derived classes. The @c Pkt4 and @c Pkt6 class have respective
+    /// implementations of this method.
     ///
     /// @throw InvalidOperation if packing fails
     virtual void pack() = 0;
 
     /// @brief Parses on-wire form of DHCP (either v4 or v6) packet.
     ///
-    /// Parses received packet, stored in on-wire format in bufferIn_.
+    /// Parses received packet, stored in on-wire format in data_.
     ///
     /// Will create a collection of option objects that will
     /// be stored in options_ container.
     ///
-    /// @note This is a pure virtual method. Actual implementations are in
-    /// Pkt4 and Pkt6 class.
+    /// @note This is a pure virtual method and must be implemented in
+    /// the derived classes. The @c Pkt4 and @c Pkt6 class have respective
+    /// implementations of this method.
     ///
-    /// Method with throw exception if packet parsing fails.
+    /// Method will throw exception if packet parsing fails.
     ///
     /// @todo Pkt4 throws exceptions when unpacking fails, while Pkt6
     ///       catches exceptions and returns false. We need to unify that
@@ -160,15 +173,19 @@ public:
     /// @return reference to output buffer
     isc::util::OutputBuffer& getBuffer() { return (buffer_out_); };
 
-    /// Adds an option to this packet.
+    /// @brief Adds an option to this packet.
+    ///
+    /// @note This is a pure virtual method and must be implemented in
+    /// the derived classes. The @c Pkt4 and @c Pkt6 class have respective
+    /// implementations of this method.
     ///
-    /// Sadly, we need to have 2 versions of that method. One that
-    /// accepts duplicates (DHCPv6) and one that does not (DHCPv4)
+    /// We can't have a unified method, as DHCPv6 allows option duplicates,
+    /// while DHCPv4 doesn't.
     ///
     /// @param opt option to be added.
-    virtual void addOption(const OptionPtr& opt);
+    virtual void addOption(const OptionPtr& opt) = 0;
 
-    /// Attempts to delete first suboption of requested type
+    /// @brief Attempts to delete first suboption of requested type.
     ///
     /// @param type Type of option to be deleted.
     ///
@@ -179,20 +196,26 @@ public:
     ///
     /// This function is useful mainly for debugging.
     ///
-    /// @note This is pure virtual method. Actual implementations are in
-    ///       Pkt4 and Pkt6 classes.
+    /// @note This is a pure virtual method and must be implemented in
+    /// the derived classes. The @c Pkt4 and @c Pkt6 class have respective
+    /// implementations of this method.
     ///
     /// @return string with text representation
     virtual std::string toText() = 0;
 
-    /// Returns packet type
+    /// @brief Returns packet size in binary format.
     ///
-    /// For DHCPv6, this is a straightforward operation (read packet field), but
-    /// for DHCPv4 it requires finding appropriate option (that may or may not
-    /// be present).
+    /// Returns size of the packet in on-wire format or size needed to store
+    /// it in on-wire format.
+    ///
+    /// @note This is a pure virtual method and must be implemented in
+    /// the derived classes. The @c Pkt4 and @c Pkt6 class have respective
+    /// implementations of this method.
+    ///
+    /// @return packet size in bytes
     virtual size_t len() = 0;
 
-    /// Returns message type (e.g. 1 = SOLICIT)
+    /// @brief Returns message type (e.g. 1 = SOLICIT).
     ///
     /// This is a pure virtual method. DHCPv4 stores type in option 53 and
     /// DHCPv6 stores it in the header.
@@ -200,7 +223,7 @@ public:
     /// @return message type
     virtual uint8_t getType() const = 0;
 
-    /// Sets message type (e.g. 1 = SOLICIT)
+    /// @brief Sets message type (e.g. 1 = SOLICIT).
     ///
     /// This is a pure virtual method. DHCPv4 stores type in option 53 and
     /// DHCPv6 stores it in the header.
@@ -208,24 +231,16 @@ public:
     /// @param type message type to be set
     virtual void setType(uint8_t type) = 0;
 
-    /// @brief Sets transaction-id value
+    /// @brief Sets transaction-id value.
     ///
     /// @param transid transaction-id to be set.
     void setTransid(uint32_t transid) { transid_ = transid; }
 
-    /// Returns value of transaction-id field
+    /// @brief Returns value of transaction-id field.
     ///
     /// @return transaction-id
     uint32_t getTransid() const { return (transid_); };
 
-    /// @brief Classes this packet belongs to.
-    ///
-    /// This field is public, so the code outside of Pkt4 or Pkt6 class can
-    /// iterate over existing classes. Having it public also solves the problem
-    /// of returned reference lifetime. It is preferred to use @ref inClass and
-    /// @ref addClass should be used to operate on this field.
-    ClientClasses classes_;
-
     /// @brief Checks whether a client belongs to a given class
     ///
     /// @param client_class name of the class
@@ -291,21 +306,11 @@ public:
     /// @brief Copies content of input buffer to output buffer.
     ///
     /// This is mostly a diagnostic function. It is being used for sending
-    /// received packet. Received packet is stored in bufferIn_, but
+    /// received packet. Received packet is stored in data_, but
     /// transmitted data is stored in buffer_out_. If we want to send packet
     /// that we just received, a copy between those two buffers is necessary.
     void repack();
 
-    /// collection of options present in this message
-    ///
-    /// @warning This public member is accessed by derived
-    /// classes directly. One of such derived classes is
-    /// @ref perfdhcp::PerfPkt6. The impact on derived clasess'
-    /// behavior must be taken into consideration before making
-    /// changes to this member such as access scope restriction or
-    /// data format change etc.
-    isc::dhcp::OptionCollection options_;
-
     /// @brief Set callback function to be used to parse options.
     ///
     /// @param callback An instance of the callback function or NULL to
@@ -449,13 +454,18 @@ public:
     /// varied sources: raw sockets, client-id, link-local IPv6 address,
     /// and various relay options.
     ///
+    /// @note Technically the proper term for this information is a link layer
+    /// address, but it is frequently referred to MAC or hardware address.
+    /// Since we're calling the feature "MAC addresses in DHCPv6", we decided
+    /// to keep the name of getMAC().
+    ///
     /// hw_addr_src takes a combination of bit values specified in
     /// MAC_SOURCE_* constants.
     ///
     /// @param hw_addr_src a bitmask that specifies hardware address source
     HWAddrPtr getMAC(uint32_t hw_addr_src);
 
-    /// @brief virtual desctructor
+    /// @brief Virtual desctructor.
     ///
     /// There is nothing to clean up here, but since there are virtual methods,
     /// we define virtual destructor to ensure that derived classes will have
@@ -463,6 +473,24 @@ public:
     virtual ~Pkt() {
     }
 
+    /// @brief Classes this packet belongs to.
+    ///
+    /// This field is public, so the code outside of Pkt4 or Pkt6 class can
+    /// iterate over existing classes. Having it public also solves the problem
+    /// of returned reference lifetime. It is preferred to use @ref inClass and
+    /// @ref addClass should be used to operate on this field.
+    ClientClasses classes_;
+
+    /// @brief Collection of options present in this message.
+    ///
+    /// @warning This public member is accessed by derived
+    /// classes directly. One of such derived classes is
+    /// @ref perfdhcp::PerfPkt6. The impact on derived clasess'
+    /// behavior must be taken into consideration before making
+    /// changes to this member such as access scope restriction or
+    /// data format change etc.
+    isc::dhcp::OptionCollection options_;
+
 protected:
 
     /// transaction-id (32 bits for v4, 24 bits for v6)

src/lib/dhcp/pkt4.cc

@@ -45,7 +45,6 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
       siaddr_(DEFAULT_ADDRESS),
       giaddr_(DEFAULT_ADDRESS)
 {
-    // buffer_out_.resize(DHCPV4_PKT_HDR_LEN);
     memset(sname_, 0, MAX_SNAME_LEN);
     memset(file_, 0, MAX_FILE_LEN);
 
@@ -134,7 +133,6 @@ Pkt4::pack() {
         // write (len) bytes of padding
         vector<uint8_t> zeros(hw_len, 0);
         buffer_out_.writeData(&zeros[0], hw_len);
-        // buffer_out_.writeData(chaddr_, MAX_CHADDR_LEN);
 
         buffer_out_.writeData(sname_, MAX_SNAME_LEN);
         buffer_out_.writeData(file_, MAX_FILE_LEN);

src/lib/dhcp/pkt4.h

@@ -287,7 +287,7 @@ public:
 
     /// @brief Add an option.
     ///
-    /// Throws BadValue if option with that type is already present.
+    /// @throw BadValue if option with that type is already present.
     ///
     /// @param opt option to be added
     virtual void

src/lib/dhcp/pkt6.cc

@@ -113,6 +113,10 @@ OptionPtr Pkt6::getAnyRelayOption(uint16_t opt_type, RelaySearchOrder order) {
     return (OptionPtr());
 }
 
+void
+Pkt6::addOption(const OptionPtr& opt) {
+    options_.insert(std::pair<int, OptionPtr>(opt->getType(), opt));
+}
 
 OptionPtr Pkt6::getRelayOption(uint16_t opt_type, uint8_t relay_level) {
     if (relay_level >= relay_info_.size()) {

src/lib/dhcp/pkt6.h

@@ -132,15 +132,23 @@ public:
     /// @return true if parsing was successful
     virtual bool unpack();
 
+    /// @brief Adds an option.
+    ///
+    /// @param opt option to be added
+    virtual void
+    addOption(const OptionPtr& opt);
+
     /// @brief Returns protocol of this packet (UDP or TCP).
     ///
     /// @return protocol type
     DHCPv6Proto getProto();
 
-    /// Sets protocol of this packet.
+    /// @brief Sets protocol of this packet.
     ///
     /// @param proto protocol (UDP or TCP)
-    void setProto(DHCPv6Proto proto = UDP) { proto_ = proto; }
+    void setProto(DHCPv6Proto proto = UDP) {
+        proto_ = proto;
+    }
 
     /// @brief Returns text representation of the packet.
     ///
@@ -161,12 +169,12 @@ public:
     /// @return number of bytes required to assemble this packet
     virtual size_t len();
 
-    /// Returns message type (e.g. 1 = SOLICIT)
+    /// @brief Returns message type (e.g. 1 = SOLICIT).
     ///
     /// @return message type
     virtual uint8_t getType() const { return (msg_type_); }
 
-    /// Sets message type (e.g. 1 = SOLICIT)
+    /// @brief Sets message type (e.g. 1 = SOLICIT).
     ///
     /// @param type message type to be set
     virtual void setType(uint8_t type) { msg_type_=type; };
@@ -264,14 +272,14 @@ public:
     std::vector<RelayInfo> relay_info_;
 
 protected:
-    /// Builds on wire packet for TCP transmission.
+    /// @brief Builds on wire packet for TCP transmission.
     ///
-    /// TODO This function is not implemented yet.
+    /// @todo This function is not implemented yet.
     ///
     /// @throw NotImplemented, IPv6 over TCP is not yet supported.
     void packTCP();
 
-    /// Builds on wire packet for UDP transmission.
+    /// @brief Builds on wire packet for UDP transmission.
     ///
     /// @throw InvalidOperation if packing fails
     void packUDP();
@@ -283,7 +291,7 @@ protected:
     /// Will create a collection of option objects that will
     /// be stored in options_ container.
     ///
-    /// TODO This function is not implemented yet.
+    /// @todo This function is not implemented yet.
     ///
     /// @return true, if build was successful
     bool unpackTCP();
@@ -298,7 +306,7 @@ protected:
     /// @return true, if build was successful
     bool unpackUDP();
 
-    /// @brief unpacks direct (non-relayed) message
+    /// @brief Unpacks direct (non-relayed) message.
     ///
     /// This method unpacks specified buffer range as a direct
     /// (e.g. solicit or request) message. This method is called from
@@ -310,7 +318,7 @@ protected:
     bool unpackMsg(OptionBuffer::const_iterator begin,
                    OptionBuffer::const_iterator end);
 
-    /// @brief unpacks relayed message (RELAY-FORW or RELAY-REPL)
+    /// @brief Unpacks relayed message (RELAY-FORW or RELAY-REPL).
     ///
     /// This method is called from unpackUDP() when received message
     /// is detected to be relay-message. It goes iteratively over
@@ -319,18 +327,18 @@ protected:
     /// @return true if parsing was successful
     bool unpackRelayMsg();
 
-    /// @brief calculates overhead introduced in specified relay
+    /// @brief Calculates overhead introduced in specified relay.
     ///
     /// It is used when calculating message size and packing message
     /// @param relay RelayInfo structure that holds information about relay
     /// @return number of bytes needed to store relay information
     uint16_t getRelayOverhead(const RelayInfo& relay) const;
 
-    /// @brief calculates overhead for all relays defined for this message
+    /// @brief Calculates overhead for all relays defined for this message.
     /// @return number of bytes needed to store all relay information
     uint16_t calculateRelaySizes();
 
-    /// @brief calculates size of the message as if it was not relayed at all
+    /// @brief Calculates size of the message as if it was not relayed at all.
     ///
     /// This is equal to len() if the message was not relayed.
     /// @return number of bytes required to store the message