[1224] Pkt4 improvements after review

 - dhcp4.h added to Makefile.am
 - Pkt4.cc: address fields initialized in more efficient way
 - Pkt4.h: many improvements in comments
 - Minor improvements in Pkt4 unittests

src/lib/dhcp/Makefile.am

@@ -14,7 +14,7 @@ libdhcp_la_SOURCES += option.cc option.h
 libdhcp_la_SOURCES += option6_ia.cc option6_ia.h
 libdhcp_la_SOURCES += option6_iaaddr.cc option6_iaaddr.h
 libdhcp_la_SOURCES += option6_addrlst.cc option6_addrlst.h
-libdhcp_la_SOURCES += dhcp6.h
+libdhcp_la_SOURCES += dhcp6.h dhcp4.h
 libdhcp_la_SOURCES += pkt6.cc pkt6.h
 libdhcp_la_SOURCES += pkt4.cc pkt4.h
 

src/lib/dhcp/dhcp4.h

@@ -1,7 +1,3 @@
-/* dhcp.h
-
-   Protocol structures... */
-
 /*
  * Copyright (c) 2004-2011 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 1995-2003 by Internet Software Consortium
@@ -30,7 +26,7 @@
  * To learn more about Vixie Enterprises, see ``http://www.vix.com''.
  */
 
-/* 
+/*
  * NOTE: This files is imported from ISC DHCP. It uses C notation.
  *       Format kept for easier merge.
  */
@@ -38,154 +34,151 @@
 #ifndef DHCP_H
 #define DHCP_H
 
-#define DHCP_UDP_OVERHEAD	(20 + /* IP header */			\
-			        8)   /* UDP header */
-#define DHCP_SNAME_LEN		64
-#define DHCP_FILE_LEN		128
-#define DHCP_FIXED_NON_UDP	236
-#define DHCP_FIXED_LEN		(DHCP_FIXED_NON_UDP + DHCP_UDP_OVERHEAD)
-						/* Everything but options. */
-#define BOOTP_MIN_LEN		300
-
-#define DHCP_MTU_MAX		1500
-#define DHCP_MTU_MIN            576
+#include <stdint.h>
 
-#define DHCP_MAX_OPTION_LEN	(DHCP_MTU_MAX - DHCP_FIXED_LEN)
-#define DHCP_MIN_OPTION_LEN     (DHCP_MTU_MIN - DHCP_FIXED_LEN)
+namespace isc {
+namespace dhcp {
 
 /* BOOTP (rfc951) message types */
-#define	BOOTREQUEST	1
-#define BOOTREPLY	2
+    enum BOOTPTypes {
+        BOOTREQUEST = 1,
+        BOOTREPLY = 2 };
 
 /* Possible values for flags field... */
-#define BOOTP_BROADCAST 32768L
+    static const uint16_t BOOTP_BROADCAST = 32768L;
 
 /* Possible values for hardware type (htype) field... */
-#define HTYPE_ETHER	1               /* Ethernet 10Mbps              */
-#define HTYPE_IEEE802	6               /* IEEE 802.2 Token Ring...	*/
-#define HTYPE_FDDI	8		/* FDDI...			*/
+    enum HType {
+        HTYPE_ETHER = 1,   /* Ethernet 10Mbps */
+        HTYPE_IEEE802 = 6, /* IEEE 802.2 Token Ring */
+        HTYPE_FDDI = 8     /* FDDI */
+        /// TODO Add infiniband here
+    };
 
 /* Magic cookie validating dhcp options field (and bootp vendor
    extensions field). */
-#define DHCP_OPTIONS_COOKIE	"\143\202\123\143"
+#define DHCP_OPTIONS_COOKIE     "\143\202\123\143"
 
 /* DHCP Option codes: */
-
-#define DHO_PAD					0
-#define DHO_SUBNET_MASK				1
-#define DHO_TIME_OFFSET				2
-#define DHO_ROUTERS				3
-#define DHO_TIME_SERVERS			4
-#define DHO_NAME_SERVERS			5
-#define DHO_DOMAIN_NAME_SERVERS			6
-#define DHO_LOG_SERVERS				7
-#define DHO_COOKIE_SERVERS			8
-#define DHO_LPR_SERVERS				9
-#define DHO_IMPRESS_SERVERS			10
-#define DHO_RESOURCE_LOCATION_SERVERS		11
-#define DHO_HOST_NAME				12
-#define DHO_BOOT_SIZE				13
-#define DHO_MERIT_DUMP				14
-#define DHO_DOMAIN_NAME				15
-#define DHO_SWAP_SERVER				16
-#define DHO_ROOT_PATH				17
-#define DHO_EXTENSIONS_PATH			18
-#define DHO_IP_FORWARDING			19
-#define DHO_NON_LOCAL_SOURCE_ROUTING		20
-#define DHO_POLICY_FILTER			21
-#define DHO_MAX_DGRAM_REASSEMBLY		22
-#define DHO_DEFAULT_IP_TTL			23
-#define DHO_PATH_MTU_AGING_TIMEOUT		24
-#define DHO_PATH_MTU_PLATEAU_TABLE		25
-#define DHO_INTERFACE_MTU			26
-#define DHO_ALL_SUBNETS_LOCAL			27
-#define DHO_BROADCAST_ADDRESS			28
-#define DHO_PERFORM_MASK_DISCOVERY		29
-#define DHO_MASK_SUPPLIER			30
-#define DHO_ROUTER_DISCOVERY			31
-#define DHO_ROUTER_SOLICITATION_ADDRESS		32
-#define DHO_STATIC_ROUTES			33
-#define DHO_TRAILER_ENCAPSULATION		34
-#define DHO_ARP_CACHE_TIMEOUT			35
-#define DHO_IEEE802_3_ENCAPSULATION		36
-#define DHO_DEFAULT_TCP_TTL			37
-#define DHO_TCP_KEEPALIVE_INTERVAL		38
-#define DHO_TCP_KEEPALIVE_GARBAGE		39
-#define DHO_NIS_DOMAIN				40
-#define DHO_NIS_SERVERS				41
-#define DHO_NTP_SERVERS				42
-#define DHO_VENDOR_ENCAPSULATED_OPTIONS		43
-#define DHO_NETBIOS_NAME_SERVERS		44
-#define DHO_NETBIOS_DD_SERVER			45
-#define DHO_NETBIOS_NODE_TYPE			46
-#define DHO_NETBIOS_SCOPE			47
-#define DHO_FONT_SERVERS			48
-#define DHO_X_DISPLAY_MANAGER			49
-#define DHO_DHCP_REQUESTED_ADDRESS		50
-#define DHO_DHCP_LEASE_TIME			51
-#define DHO_DHCP_OPTION_OVERLOAD		52
-#define DHO_DHCP_MESSAGE_TYPE			53
-#define DHO_DHCP_SERVER_IDENTIFIER		54
-#define DHO_DHCP_PARAMETER_REQUEST_LIST		55
-#define DHO_DHCP_MESSAGE			56
-#define DHO_DHCP_MAX_MESSAGE_SIZE		57
-#define DHO_DHCP_RENEWAL_TIME			58
-#define DHO_DHCP_REBINDING_TIME			59
-#define DHO_VENDOR_CLASS_IDENTIFIER		60
-#define DHO_DHCP_CLIENT_IDENTIFIER		61
-#define DHO_NWIP_DOMAIN_NAME			62
-#define DHO_NWIP_SUBOPTIONS			63
-#define DHO_USER_CLASS				77
-#define DHO_FQDN				81
-#define DHO_DHCP_AGENT_OPTIONS			82
-#define DHO_AUTHENTICATE			90  /* RFC3118, was 210 */
-#define DHO_CLIENT_LAST_TRANSACTION_TIME	91
-#define DHO_ASSOCIATED_IP			92
-#define DHO_SUBNET_SELECTION			118 /* RFC3011! */
-#define DHO_DOMAIN_SEARCH			119 /* RFC3397 */
-#define DHO_VIVCO_SUBOPTIONS			124
-#define DHO_VIVSO_SUBOPTIONS			125
-
-#define DHO_END					255
+    enum DHCPOptionType {
+        DHO_PAD                          = 0,
+        DHO_SUBNET_MASK                  = 1,
+        DHO_TIME_OFFSET                  = 2,
+        DHO_ROUTERS                      = 3,
+        DHO_TIME_SERVERS                 = 4,
+        DHO_NAME_SERVERS                 = 5,
+        DHO_DOMAIN_NAME_SERVERS          = 6,
+        DHO_LOG_SERVERS                  = 7,
+        DHO_COOKIE_SERVERS               = 8,
+        DHO_LPR_SERVERS                  = 9,
+        DHO_IMPRESS_SERVERS              = 10,
+        DHO_RESOURCE_LOCATION_SERVERS    = 11,
+        DHO_HOST_NAME                    = 12,
+        DHO_BOOT_SIZE                    = 13,
+        DHO_MERIT_DUMP                   = 14,
+        DHO_DOMAIN_NAME                  = 15,
+        DHO_SWAP_SERVER                  = 16,
+        DHO_ROOT_PATH                    = 17,
+        DHO_EXTENSIONS_PATH              = 18,
+        DHO_IP_FORWARDING                = 19,
+        DHO_NON_LOCAL_SOURCE_ROUTING     = 20,
+        DHO_POLICY_FILTER                = 21,
+        DHO_MAX_DGRAM_REASSEMBLY         = 22,
+        DHO_DEFAULT_IP_TTL               = 23,
+        DHO_PATH_MTU_AGING_TIMEOUT       = 24,
+        DHO_PATH_MTU_PLATEAU_TABLE       = 25,
+        DHO_INTERFACE_MTU                = 26,
+        DHO_ALL_SUBNETS_LOCAL            = 27,
+        DHO_BROADCAST_ADDRESS            = 28,
+        DHO_PERFORM_MASK_DISCOVERY       = 29,
+        DHO_MASK_SUPPLIER                = 30,
+        DHO_ROUTER_DISCOVERY             = 31,
+        DHO_ROUTER_SOLICITATION_ADDRESS  = 32,
+        DHO_STATIC_ROUTES                = 33,
+        DHO_TRAILER_ENCAPSULATION        = 34,
+        DHO_ARP_CACHE_TIMEOUT            = 35,
+        DHO_IEEE802_3_ENCAPSULATION      = 36,
+        DHO_DEFAULT_TCP_TTL              = 37,
+        DHO_TCP_KEEPALIVE_INTERVAL       = 38,
+        DHO_TCP_KEEPALIVE_GARBAGE        = 39,
+        DHO_NIS_DOMAIN                   = 40,
+        DHO_NIS_SERVERS                  = 41,
+        DHO_NTP_SERVERS                  = 42,
+        DHO_VENDOR_ENCAPSULATED_OPTIONS  = 43,
+        DHO_NETBIOS_NAME_SERVERS         = 44,
+        DHO_NETBIOS_DD_SERVER            = 45,
+        DHO_NETBIOS_NODE_TYPE            = 46,
+        DHO_NETBIOS_SCOPE                = 47,
+        DHO_FONT_SERVERS                 = 48,
+        DHO_X_DISPLAY_MANAGER            = 49,
+        DHO_DHCP_REQUESTED_ADDRESS       = 50,
+        DHO_DHCP_LEASE_TIME              = 51,
+        DHO_DHCP_OPTION_OVERLOAD         = 52,
+        DHO_DHCP_MESSAGE_TYPE            = 53,
+        DHO_DHCP_SERVER_IDENTIFIER       = 54,
+        DHO_DHCP_PARAMETER_REQUEST_LIST  = 55,
+        DHO_DHCP_MESSAGE                 = 56,
+        DHO_DHCP_MAX_MESSAGE_SIZE        = 57,
+        DHO_DHCP_RENEWAL_TIME            = 58,
+        DHO_DHCP_REBINDING_TIME          = 59,
+        DHO_VENDOR_CLASS_IDENTIFIER      = 60,
+        DHO_DHCP_CLIENT_IDENTIFIER       = 61,
+        DHO_NWIP_DOMAIN_NAME             = 62,
+        DHO_NWIP_SUBOPTIONS              = 63,
+        DHO_USER_CLASS                   = 77,
+        DHO_FQDN                         = 81,
+        DHO_DHCP_AGENT_OPTIONS           = 82,
+        DHO_AUTHENTICATE                 = 90,  /* RFC3118, was 210 */
+        DHO_CLIENT_LAST_TRANSACTION_TIME = 91,
+        DHO_ASSOCIATED_IP                = 92,
+        DHO_SUBNET_SELECTION             = 118, /* RFC3011! */
+        DHO_DOMAIN_SEARCH                = 119, /* RFC3397 */
+        DHO_VIVCO_SUBOPTIONS             = 124,
+        DHO_VIVSO_SUBOPTIONS             = 125,
+
+        DHO_END                          = 255
+    };
 
 /* DHCP message types. */
-#define DHCPDISCOVER		1
-#define DHCPOFFER		2
-#define DHCPREQUEST		3
-#define DHCPDECLINE		4
-#define DHCPACK			5
-#define DHCPNAK			6
-#define DHCPRELEASE		7
-#define DHCPINFORM		8
-#define DHCPLEASEQUERY		10
-#define DHCPLEASEUNASSIGNED	11
-#define DHCPLEASEUNKNOWN	12
-#define DHCPLEASEACTIVE		13
-
+    enum DHCPMessageType {
+        DHCPDISCOVER        =  1,
+        DHCPOFFER           =  2,
+        DHCPREQUEST         =  3,
+        DHCPDECLINE         =  4,
+        DHCPACK             =  5,
+        DHCPNAK             =  6,
+        DHCPRELEASE         =  7,
+        DHCPINFORM          =  8,
+        DHCPLEASEQUERY      =  10,
+        DHCPLEASEUNASSIGNED =  11,
+        DHCPLEASEUNKNOWN    =  12,
+        DHCPLEASEACTIVE     =  13
+    };
 
 /* Relay Agent Information option subtypes: */
-#define RAI_CIRCUIT_ID	1
-#define RAI_REMOTE_ID	2
-#define RAI_AGENT_ID	3
-#define RAI_LINK_SELECT	5
+#define RAI_CIRCUIT_ID  1
+#define RAI_REMOTE_ID   2
+#define RAI_AGENT_ID    3
+#define RAI_LINK_SELECT 5
 
 /* FQDN suboptions: */
-#define FQDN_NO_CLIENT_UPDATE		1
-#define FQDN_SERVER_UPDATE		2
-#define FQDN_ENCODED			3
-#define FQDN_RCODE1			4
-#define FQDN_RCODE2			5
-#define FQDN_HOSTNAME			6
-#define FQDN_DOMAINNAME			7
-#define FQDN_FQDN			8
-#define FQDN_SUBOPTION_COUNT		8
+#define FQDN_NO_CLIENT_UPDATE           1
+#define FQDN_SERVER_UPDATE              2
+#define FQDN_ENCODED                    3
+#define FQDN_RCODE1                     4
+#define FQDN_RCODE2                     5
+#define FQDN_HOSTNAME                   6
+#define FQDN_DOMAINNAME                 7
+#define FQDN_FQDN                       8
+#define FQDN_SUBOPTION_COUNT            8
 
 /* Enterprise Suboptions: */
-#define VENDOR_ISC_SUBOPTIONS		2495
+#define VENDOR_ISC_SUBOPTIONS           2495
 
-#define DHCP4_CLIENT_PORT 68
-#define DHCP4_SERVER_PORT 67
+    static const uint16_t DHCP4_CLIENT_PORT = 68;
+    static const uint16_t DHCP4_SERVER_PORT = 67;
 
+} // end of isc::dhcp namespace
+} // end of isc namespace
 
 #endif /* DHCP_H */
-

src/lib/dhcp/pkt4.cc

@@ -26,6 +26,8 @@ using namespace isc::asiolink;
 
 namespace isc {
 
+const IOAddress DEFAULT_ADDRESS("0.0.0.0");
+
 Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
      :local_addr_(IOAddress("0.0.0.0")),
       remote_addr_(IOAddress("0.0.0.0")),
@@ -40,10 +42,10 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
       transid_(transid),
       secs_(0),
       flags_(0),
-      ciaddr_(IOAddress("0.0.0.0")),
-      yiaddr_(IOAddress("0.0.0.0")),
-      siaddr_(IOAddress("0.0.0.0")),
-      giaddr_(IOAddress("0.0.0.0")),
+      ciaddr_(DEFAULT_ADDRESS),
+      yiaddr_(DEFAULT_ADDRESS),
+      siaddr_(DEFAULT_ADDRESS),
+      giaddr_(DEFAULT_ADDRESS),
       bufferIn_(0), // not used, this is TX packet
       bufferOut_(DHCPV4_PKT_HDR_LEN),
       msg_type_(msg_type)
@@ -66,10 +68,10 @@ Pkt4::Pkt4(const uint8_t* data, size_t len)
       transid_(transid_),
       secs_(0),
       flags_(0),
-      ciaddr_(IOAddress("0.0.0.0")),
-      yiaddr_(IOAddress("0.0.0.0")),
-      siaddr_(IOAddress("0.0.0.0")),
-      giaddr_(IOAddress("0.0.0.0")),
+      ciaddr_(DEFAULT_ADDRESS),
+      yiaddr_(DEFAULT_ADDRESS),
+      siaddr_(DEFAULT_ADDRESS),
+      giaddr_(DEFAULT_ADDRESS),
       bufferIn_(0), // not used, this is TX packet
       bufferOut_(DHCPV4_PKT_HDR_LEN),
       msg_type_(DHCPDISCOVER)
@@ -123,6 +125,10 @@ Pkt4::setHWAddr(uint8_t hType, uint8_t hlen, const uint8_t* macAddr) {
         isc_throw(OutOfRange, "Hardware address (len=" << hlen
                   << " too long. Max " << MAX_CHADDR_LEN << " supported.");
     }
+    if ( (!macAddr) && (hlen > 0) ) {
+        isc_throw(OutOfRange, "Invalid HW Address specified");
+    }
+
     htype_ = hType;
     hlen_ = hlen;
     memset(chaddr_, 0, MAX_CHADDR_LEN);
@@ -150,7 +156,7 @@ Pkt4::setFile(const uint8_t* file, size_t fileLen /*= MAX_FILE_LEN*/) {
     memset(file_, 0, MAX_FILE_LEN);
     memcpy(file_, file, fileLen);
 
-    // no need to store snameLen as any empty space is filled with 0s
+    // no need to store fileLen as any empty space is filled with 0s
 }
 
 uint8_t

src/lib/dhcp/pkt4.h

@@ -16,6 +16,7 @@
 #define PKT4_H
 
 #include <iostream>
+#include <vector>
 #include <boost/shared_ptr.hpp>
 #include <boost/shared_array.hpp>
 #include "asiolink/io_address.h"
@@ -29,52 +30,52 @@ namespace dhcp {
 class Pkt4 {
 public:
 
-    // length of the CHADDR field in DHCPv4 message
+    /// length of the CHADDR field in DHCPv4 message
     const static size_t MAX_CHADDR_LEN = 16;
 
-    // length of the SNAME field in DHCPv4 message
+    /// length of the SNAME field in DHCPv4 message
     const static size_t MAX_SNAME_LEN = 64;
 
-    // length of the FILE field in DHCPv4 message
+    /// length of the FILE field in DHCPv4 message
     const static size_t MAX_FILE_LEN = 128;
 
-    /// specifes DHCPv4 packet header length (fixed part)
+    /// specifies DHCPv4 packet header length (fixed part)
     const static size_t DHCPV4_PKT_HDR_LEN = 236;
 
-    /// Constructor, used in replying to a message
+    /// Constructor, used in replying to a message.
     ///
     /// @param msg_type type of message (e.g. DHCPDISOVER=1)
     /// @param transid transaction-id
     Pkt4(uint8_t msg_type, uint32_t transid);
 
-    /// Constructor, used in message transmission
+    /// @brief Constructor, used in message reception.
     ///
-    /// Creates new message. Transaction-id will randomized.
+    /// Creates new message. Pkt4 will copy data to bufferIn_
+    /// buffer on creation.
     ///
     /// @param data pointer to received data
     /// @param len size of buffer to be allocated for this packet.
     Pkt4(const uint8_t* data, size_t len);
 
-    /// @brief Prepares on-wire format.
+    /// @brief Prepares on-wire format of DHCPv4 packet.
     ///
     /// Prepares on-wire format of message and all its options.
     /// Options must be stored in options_ field.
-    /// Output buffer will be stored in data_. Length
-    /// will be set in data_len_.
+    /// Output buffer will be stored in bufferOut_.
     ///
     /// @return true if packing procedure was successful
     bool
     pack();
 
-    /// @brief Parses on-wire form of UDP DHCPv6 packet.
+    /// @brief Parses on-wire form of DHCPv4 packet.
+    ///
+    /// Parses received packet, stored in on-wire format in bufferIn_.
     ///
-    /// Parses received packet, stored in on-wire format in data_.
-    /// data_len_ must be set to indicate data length.
     /// Will create a collection of option objects that will
     /// be stored in options_ container.
     ///
-    /// @return true, if build was successful
-    bool 
+    /// @return true, if parsing was successful
+    bool
     unpack();
 
     /// @brief Returns text representation of the packet.
@@ -85,10 +86,10 @@ public:
     std::string
     toText();
 
-    /// @brief Returns calculated length of the packet.
+    /// @brief Returns the size of the required buffer to build the packet.
     ///
-    /// This function returns size of required buffer to buld this packet.
-    /// To use that function, options_ field must be set.
+    /// Returns the size of the required buffer to build the packet with
+    /// the current set of packet options.
     ///
     /// @return number of bytes required to build this packet
     size_t
@@ -210,11 +211,11 @@ public:
     /// @brief Returns sname field
     ///
     /// Note: This is 64 bytes long field. It doesn't have to be
-    /// null-terminated. Do no use strlen() or similar on it.
+    /// null-terminated. Do not use strlen() or similar on it.
     ///
     /// @return sname field
-    const uint8_t*
-    getSname() { return (sname_); };
+    const std::vector<uint8_t>
+    getSname() { return (std::vector<uint8_t>(sname_, &sname_[MAX_SNAME_LEN])); };
 
     /// Sets sname field
     ///
@@ -225,11 +226,11 @@ public:
     /// @brief Returns file field
     ///
     /// Note: This is 128 bytes long field. It doesn't have to be
-    /// null-terminated. Do no use strlen() or similar on it.
+    /// null-terminated. Do not use strlen() or similar on it.
     ///
     /// @return pointer to file field
-    const uint8_t*
-    getFile() { return (file_); };
+    const std::vector<uint8_t>
+    getFile() { return (std::vector<uint8_t>(file_, &file_[MAX_FILE_LEN])); };
 
     /// Sets file field
     ///
@@ -237,7 +238,13 @@ public:
     void
     setFile(const uint8_t* file, size_t fileLen = MAX_FILE_LEN);
 
-    /// Sets hardware address
+    /// @brief Sets hardware address.
+    ///
+    /// Sets parameters of hardware address. hlen specifies
+    /// length of macAddr buffer. Content of macAddr buffer
+    /// will be copied to appropriate field.
+    ///
+    /// Note: macAddr must be a buffer of at least hlen bytes.
     ///
     /// @param hwType hardware type (will be sent in htype field)
     /// @param hlen hardware length (will be sent in hlen field)
@@ -288,10 +295,9 @@ protected:
 
     /// @brief interface index
     ///
-    /// interface index (each network interface has assigned unique ifindex
-    /// it is functional equvalent of name, but sometimes more useful, e.g.
-    /// when using crazy systems that allow spaces in interface names
-    /// e.g. windows
+    /// Each network interface has assigned unique ifindex. It is functional
+    /// equvalent of name, but sometimes more useful, e.g. when using crazy
+    /// systems that allow spaces in interface names e.g. MS Windows)
     int ifindex_;
 
     /// local UDP port
@@ -300,10 +306,13 @@ protected:
     /// remote UDP port
     int remote_port_;
 
-    /// message operation code (kept due to BOOTP format, this is NOT DHCPv4 type)
+    /// @brief message operation code
     ///
-    /// Note: This is legacy BOOTP field. There's no need to manipulate it 
-    /// directly. Its value is set based on DHCP message type.
+    /// Note: This is legacy BOOTP field. There's no need to manipulate it
+    /// directly. Its value is set based on DHCP message type. Note that
+    /// DHCPv4 protocol reuses BOOTP message format, so this field is
+    /// kept due to BOOTP format. This is NOT DHCPv4 type (DHCPv4 message
+    /// type is kept in message type option).
     uint8_t op_;
 
     /// link-layer address type
@@ -324,26 +333,26 @@ protected:
     /// flags
     uint16_t flags_;
 
-    // ciaddr field (32 bits): Client's IP address
+    /// ciaddr field (32 bits): Client's IP address
     isc::asiolink::IOAddress ciaddr_;
 
-    // yiaddr field (32 bits): Client's IP address ("your"), set by server
+    /// yiaddr field (32 bits): Client's IP address ("your"), set by server
     isc::asiolink::IOAddress yiaddr_;
 
-    // siaddr field (32 bits): next server IP address in boot process(e.g.TFTP)
+    /// siaddr field (32 bits): next server IP address in boot process(e.g.TFTP)
     isc::asiolink::IOAddress siaddr_;
 
-    // giaddr field (32 bits): Gateway IP address
+    /// giaddr field (32 bits): Gateway IP address
     isc::asiolink::IOAddress giaddr_;
 
-    // ciaddr field (32 bits): Client's IP address
-    uint8_t chaddr_[16]; 
-    
-    // sname 64 bytes
-    uint8_t sname_[64];
+    /// Hardware address field (16 bytes)
+    uint8_t chaddr_[MAX_CHADDR_LEN];
+
+    /// sname field (64 bytes)
+    uint8_t sname_[MAX_SNAME_LEN];
 
-    // file
-    uint8_t file_[128];
+    /// file field (128 bytes)
+    uint8_t file_[MAX_FILE_LEN];
 
     // end of real DHCPv4 fields
 
@@ -352,11 +361,11 @@ protected:
     /// thus OutputBuffer, not InputBuffer
     isc::util::OutputBuffer bufferIn_;
 
-    /// output buffer (used during message 
+    /// output buffer (used during message
     isc::util::OutputBuffer bufferOut_;
 
     /// message type (e.g. 1=DHCPDISCOVER)
-    /// TODO: this will eventually be replaced with DHCP Message Type 
+    /// TODO: this will eventually be replaced with DHCP Message Type
     /// option (option 53)
     uint8_t msg_type_;
 

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -42,12 +42,13 @@ namespace {
 TEST(Pkt4Test, constructor) {
 
     ASSERT_EQ(236U, DHCPV4_PKT_HDR_LEN);
-    Pkt4 * pkt = 0;
+    Pkt4* pkt = 0;
 
     // minimal 
     uint8_t testData[250];
-    for (int i=0 ; i < 250; i++)
+    for (int i = 0; i < 250; i++) {
         testData[i]=i; 
+    }
 
     // positive case1. Normal received packet
     EXPECT_NO_THROW(
@@ -81,7 +82,8 @@ TEST(Pkt4Test, constructor) {
         OutOfRange
     );
     if (pkt) {
-        // test failed anyway. Exception should have been thrown
+        // test failed. Exception should have been thrown, but
+        // object was created instead. Let's clean this up
         delete pkt;
     }
 }
@@ -127,7 +129,7 @@ generateTestPacket1() {
     pkt->setHops(13); // 13 relays. Wow!
     // transaction-id is already set
     pkt->setSecs(42);
-    pkt->setFlags( 0xffffU ); // all flags set
+    pkt->setFlags(0xffffU); // all flags set
     pkt->setCiaddr(IOAddress("192.0.2.1"));
     pkt->setYiaddr(IOAddress("1.2.3.4"));
     pkt->setSiaddr(IOAddress("192.0.2.255"));
@@ -202,11 +204,11 @@ TEST(Pkt4Test, fixedFields) {
     EXPECT_EQ(string("255.255.255.255"), pkt->getGiaddr().toText());
 
     // chaddr is always 16 bytes long and contains link-layer addr (MAC)
-    EXPECT_FALSE( memcmp(dummyChaddr, pkt->getChaddr(), 16) );
+    EXPECT_EQ(0, memcmp(dummyChaddr, pkt->getChaddr(), 16));
 
-    EXPECT_FALSE( memcmp(dummySname, pkt->getSname(), 64) );
+    EXPECT_EQ(0, memcmp(dummySname, &pkt->getSname()[0], 64));
 
-    EXPECT_FALSE( memcmp(dummyFile, pkt->getFile(), 128) );
+    EXPECT_EQ(0, memcmp(dummyFile, &pkt->getFile()[0], 128));
 
     EXPECT_EQ(DHCPDISCOVER, pkt->getType());
 }
@@ -248,11 +250,11 @@ TEST(Pkt4Test, fixedFieldsUnpack) {
     EXPECT_EQ(string("255.255.255.255"), pkt->getGiaddr.toText());
 
     // chaddr is always 16 bytes long and contains link-layer addr (MAC)
-    EXPECT_FALSE( memcmp(expectedChaddr, pkt->getChaddr(), 16) );
+    EXPECT_EQ(0, memcmp(expectedChaddr, pkt->getChaddr(), 16));
 
-    EXPECT_FALSE( memcmp(expectedSname, pkt->getSname(), 64) );
+    EXPECT_EQ(0, memcmp(expectedSname, pkt->getSname(), 64));
 
-    EXPECT_FALSE( memcmp(expectedFile, pkt->getFile(), 128) );
+    EXPECT_EQ(0, memcmp(expectedFile, pkt->getFile(), 128));
 
     EXPECT_EQ(DHCPSOLICIT, pkt->getType());
 }
@@ -367,7 +369,7 @@ TEST(Pkt4Test, sname) {
         pkt = new Pkt4(DHCPOFFER, 1234);
         pkt->setSname(sname, snameLen);
 
-        EXPECT_EQ(0, memcmp(expectedSname, pkt->getSname(), Pkt4::MAX_SNAME_LEN));
+        EXPECT_EQ(0, memcmp(expectedSname, &pkt->getSname()[0], Pkt4::MAX_SNAME_LEN));
 
 #if 0
         /// TODO Uncomment when ticket #1227 is implemented)
@@ -404,7 +406,7 @@ TEST(Pkt4Test, file) {
         pkt = new Pkt4(DHCPOFFER, 1234);
         pkt->setFile(file, fileLen);
 
-        EXPECT_EQ(0, memcmp(expectedFile, pkt->getFile(), Pkt4::MAX_FILE_LEN));
+        EXPECT_EQ(0, memcmp(expectedFile, &pkt->getFile()[0], Pkt4::MAX_FILE_LEN));
 
 #if 0
         /// TODO Uncomment when ticket #1227 is implemented)