[1186] Changes after second review

- added passing by reference in many places
- started using writeUint16/readUint16 functions

src/lib/dhcp/libdhcp.cc

@@ -40,10 +40,10 @@ LibDHCP::unpackOptions6(const boost::shared_array<uint8_t> buf,
     }
     unsigned int end = offset + parse_len;
 
-    while ( offset +4 <= end ) {
-        unsigned int opt_type = buf[offset]*256 + buf[offset+1];
+    while (offset +4 <= end) {
+        uint16_t opt_type = buf[offset]*256 + buf[offset+1];
         offset += 2;
-        unsigned int opt_len = buf[offset]*256 + buf[offset+1];
+        uint16_t opt_len = buf[offset]*256 + buf[offset+1];
         offset += 2;
 
         if (offset + opt_len > end ) {

src/lib/dhcp/option.cc

@@ -19,12 +19,14 @@
 #include <iomanip>
 #include <boost/shared_array.hpp>
 #include "exceptions/exceptions.h"
+#include "util/io_utilities.h"
 
-#include "option.h"
-#include "libdhcp.h"
+#include "dhcp/option.h"
+#include "dhcp/libdhcp.h"
 
 using namespace std;
 using namespace isc::dhcp;
+using namespace isc::util;
 
 Option::Option(Universe u, unsigned short type)
     :universe_(u), type_(type), data_len_(0) {
@@ -33,7 +35,7 @@ Option::Option(Universe u, unsigned short type)
 }
 
 Option::Option(Universe u, unsigned short type,
-               boost::shared_array<uint8_t> buf,
+               const boost::shared_array<uint8_t>& buf,
                unsigned int offset, unsigned int len)
     :universe_(u), type_(type), data_(buf),
      data_len_(len), offset_(offset)
@@ -44,7 +46,7 @@ Option::Option(Universe u, unsigned short type,
 }
 
 unsigned int
-Option::pack(boost::shared_array<uint8_t> buf,
+Option::pack(boost::shared_array<uint8_t>& buf,
              unsigned int buf_len,
              unsigned int offset) {
     switch (universe_) {
@@ -59,7 +61,7 @@ Option::pack(boost::shared_array<uint8_t> buf,
 
 
 unsigned int
-Option::pack4(boost::shared_array<uint8_t> buf,
+Option::pack4(boost::shared_array<uint8_t>& buf,
              unsigned int buf_len,
              unsigned int offset) {
     if ( offset+len() > buf_len ) {
@@ -76,7 +78,7 @@ Option::pack4(boost::shared_array<uint8_t> buf,
 }
 
 unsigned int
-Option::pack6(boost::shared_array<uint8_t> buf,
+Option::pack6(boost::shared_array<uint8_t>& buf,
              unsigned int buf_len,
              unsigned int offset) {
     if ( offset+len() > buf_len ) {
@@ -87,20 +89,22 @@ Option::pack6(boost::shared_array<uint8_t> buf,
     int length = len() - getHeaderLen();
 
     uint8_t * ptr = &buf[offset];
-    *(uint16_t*)ptr = htons(type_);
-    ptr += 2;
-    *(uint16_t*)ptr = htons(length);
-    ptr += 2;
+    writeUint16(type_, ptr);
+    ptr += sizeof(uint16_t);
+
+    writeUint16(length, ptr);
+    ptr += sizeof(uint16_t);
+
     if (data_len_)
         memcpy(ptr, &data_[offset_], data_len_);
 
-    offset += 4 + data_len_; // end of this option
+    offset += OPTION6_HDR_LEN + data_len_; // end of this option
 
     return LibDHCP::packOptions6(buf, buf_len, offset, options_);
 }
 
 unsigned int
-Option::unpack(boost::shared_array<uint8_t> buf,
+Option::unpack(const boost::shared_array<uint8_t>& buf,
                unsigned int buf_len,
                unsigned int offset,
                unsigned int parse_len) {
@@ -117,7 +121,7 @@ Option::unpack(boost::shared_array<uint8_t> buf,
 }
 
 unsigned int
-Option::unpack4(boost::shared_array<uint8_t>,
+Option::unpack4(const boost::shared_array<uint8_t>&,
                 unsigned int ,
                 unsigned int ,
                 unsigned int ) {
@@ -126,7 +130,7 @@ Option::unpack4(boost::shared_array<uint8_t>,
 }
 
 unsigned int
-Option::unpack6(boost::shared_array<uint8_t> buf,
+Option::unpack6(const boost::shared_array<uint8_t>& buf,
                 unsigned int buf_len,
                 unsigned int offset,
                 unsigned int parse_len) {

src/lib/dhcp/option.h

@@ -52,7 +52,7 @@ public:
     /// @return a pointer to a created option object
     typedef boost::shared_ptr<Option> Factory(Option::Universe u,
                                               unsigned short type,
-                                              boost::shared_array<uint8_t> buf,
+                                              boost::shared_array<uint8_t>& buf,
                                               unsigned int offset,
                                               unsigned int len);
 
@@ -75,8 +75,8 @@ public:
     /// @param buf pointer to a buffer
     /// @param offset offset in a buffer pointing to first byte of data
     /// @param len length of the option data
-    Option(Universe u, unsigned short type, boost::shared_array<uint8_t> buf,
-           unsigned int offset,
+    Option(Universe u, unsigned short type,
+           const boost::shared_array<uint8_t>& buf, unsigned int offset,
            unsigned int len);
 
     /// @brief writes option in wire-format to buf
@@ -92,7 +92,7 @@ public:
     /// @return offset to first unused byte after stored option
     ///
     virtual unsigned int
-    pack(boost::shared_array<uint8_t> buf,
+    pack(boost::shared_array<uint8_t>& buf,
          unsigned int buf_len,
          unsigned int offset);
 
@@ -108,7 +108,7 @@ public:
     ///
     /// @return offset after last parsed octet
     virtual unsigned int
-    unpack(boost::shared_array<uint8_t> buf,
+    unpack(const boost::shared_array<uint8_t>& buf,
            unsigned int buf_len,
            unsigned int offset,
            unsigned int parse_len);
@@ -157,6 +157,12 @@ public:
     /// Some DHCPv6 options can have suboptions. This method allows adding
     /// options within options.
     ///
+    /// Note: option is passed by value. That is very convenient as it allows
+    /// downcasting from any derived classes, e.g. shared_ptr<Option6_IA> type
+    /// can be passed directly, without any casts. That would not be possible
+    /// with passing by reference. addOption() is expected to be used in
+    /// many places. Requiring casting is not feasible.
+    ///
     /// @param opt shared pointer to a suboption that is going to be added.
     void
     addOption(boost::shared_ptr<Option> opt);
@@ -192,7 +198,7 @@ protected:
     ///
     /// @return offset to the next byte after last used byte
     virtual unsigned int
-    pack4(boost::shared_array<uint8_t> buf,
+    pack4(boost::shared_array<uint8_t>& buf,
           unsigned int buf_len,
           unsigned int offset);
 
@@ -205,7 +211,7 @@ protected:
     ///
     /// @return offset to the next byte after last used byte
     virtual unsigned int
-    pack6(boost::shared_array<uint8_t> buf,
+    pack6(boost::shared_array<uint8_t>& buf,
           unsigned int buf_len,
           unsigned int offset);
 
@@ -217,7 +223,7 @@ protected:
     ///
     /// @return offset to the next byte after last parsed byte
     virtual unsigned int
-    unpack4(boost::shared_array<uint8_t> buf,
+    unpack4(const boost::shared_array<uint8_t>& buf,
             unsigned int buf_len,
             unsigned int offset,
             unsigned int parse_len);
@@ -230,7 +236,7 @@ protected:
     ///
     /// @return offset to the next byte after last parsed byte
     virtual unsigned int
-    unpack6(boost::shared_array<uint8_t> buf,
+    unpack6(const boost::shared_array<uint8_t>& buf,
             unsigned int buf_len,
             unsigned int offset,
             unsigned int parse_len);

src/lib/dhcp/option6_addrlst.cc

@@ -18,6 +18,7 @@
 #include "exceptions/exceptions.h"
 
 #include "asiolink/io_address.h"
+#include "util/io_utilities.h"
 #include "dhcp/libdhcp.h"
 #include "dhcp/option6_addrlst.h"
 #include "dhcp/dhcp6.h"
@@ -26,7 +27,7 @@ using namespace std;
 using namespace isc;
 using namespace isc::dhcp;
 using namespace isc::asiolink;
-
+using namespace isc::util;
 
 Option6AddrLst::Option6AddrLst(unsigned short type,
                                const AddressContainer& addrs)
@@ -59,7 +60,7 @@ Option6AddrLst::setAddresses(const AddressContainer& addrs) {
 }
 
 unsigned int
-Option6AddrLst::pack(boost::shared_array<uint8_t> buf,
+Option6AddrLst::pack(boost::shared_array<uint8_t>& buf,
                     unsigned int buf_len,
                     unsigned int offset) {
     if (len() > buf_len) {
@@ -67,13 +68,16 @@ Option6AddrLst::pack(boost::shared_array<uint8_t> buf,
                   << ", buffer=" << buf_len << ": too small buffer.");
     }
 
-    *(uint16_t*)&buf[offset] = htons(type_);
-    offset += 2;
-    *(uint16_t*)&buf[offset] = htons(len()-4); // len() returns complete option
-    // length. len field contains length without 4-byte option header
-    offset += 2;
+    writeUint16(type_, &buf[offset]);
+    offset += sizeof(uint16_t);
+
+    // len() returns complete option length.
+    // len field contains length without 4-byte option header
+    writeUint16(len() - OPTION6_HDR_LEN, &buf[offset]);
+    offset += sizeof(uint16_t);
 
-    for (std::vector<IOAddress>::const_iterator addr=addrs_.begin();
+    // this wrapping is *ugly*. I wish there was a a
+    for (AddressContainer::const_iterator addr=addrs_.begin();
          addr!=addrs_.end();
          ++addr) {
         memcpy(&buf[offset],
@@ -86,10 +90,10 @@ Option6AddrLst::pack(boost::shared_array<uint8_t> buf,
 }
 
 unsigned int
-Option6AddrLst::unpack(boost::shared_array<uint8_t> buf,
-                  unsigned int buf_len,
-                  unsigned int offset,
-                  unsigned int option_len) {
+Option6AddrLst::unpack(const boost::shared_array<uint8_t>& buf,
+                       unsigned int buf_len,
+                       unsigned int offset,
+                       unsigned int option_len) {
     if (offset+option_len > buf_len) {
         isc_throw(OutOfRange, "Option " << type_
                   << " truncated.");

src/lib/dhcp/option6_addrlst.h

@@ -71,7 +71,7 @@ public:
     /// @return offset to the next unused char (just after stored option)
     ///
     unsigned int
-    pack(boost::shared_array<uint8_t> buf, unsigned int buf_len,
+    pack(boost::shared_array<uint8_t>& buf, unsigned int buf_len,
          unsigned int offset);
 
     /// @brief Parses received data
@@ -84,7 +84,7 @@ public:
     /// @return offset to the next unparsed char (just after parsed option)
     ///
     virtual unsigned int
-    unpack(boost::shared_array<uint8_t> buf,
+    unpack(const boost::shared_array<uint8_t>& buf,
            unsigned int buf_len,
            unsigned int offset,
            unsigned int parse_len);

src/lib/dhcp/option6_ia.cc

@@ -17,29 +17,31 @@
 #include <sstream>
 #include "exceptions/exceptions.h"
 
-#include "libdhcp.h"
-#include "option6_ia.h"
-#include "dhcp6.h"
+#include "dhcp/libdhcp.h"
+#include "dhcp/option6_ia.h"
+#include "dhcp/dhcp6.h"
+#include "util/io_utilities.h"
 
 using namespace std;
 using namespace isc;
 using namespace isc::dhcp;
+using namespace isc::util;
 
 Option6IA::Option6IA(unsigned short type, unsigned int iaid)
     :Option(Option::V6, type), iaid_(iaid) {
 }
 
 Option6IA::Option6IA(unsigned short type,
-                   boost::shared_array<uint8_t> buf,
-                   unsigned int buf_len,
-                   unsigned int offset,
-                   unsigned int option_len)
+                     const boost::shared_array<uint8_t>& buf,
+                     unsigned int buf_len,
+                     unsigned int offset,
+                     unsigned int option_len)
     :Option(Option::V6, type) {
     unpack(buf, buf_len, offset, option_len);
 }
 
 unsigned int
-Option6IA::pack(boost::shared_array<uint8_t> buf,
+Option6IA::pack(boost::shared_array<uint8_t>& buf,
                 unsigned int buf_len,
                 unsigned int offset) {
     if (offset + len() > buf_len) {
@@ -52,12 +54,14 @@ Option6IA::pack(boost::shared_array<uint8_t> buf,
                   << len() << " is too small (at least 16 is required).");
     }
 
+    writeUint16(type_, &buf[offset]);
+    offset += sizeof(uint16_t);
+
+    writeUint16(len() - OPTION6_HDR_LEN, &buf[offset]);
+    offset += sizeof(uint16_t);
+
+    /// TODO start using writeUint32 once such function is implemented
     uint8_t* ptr = &buf[offset];
-    *(uint16_t*)ptr = htons(type_);
-    ptr += sizeof(uint16_t);
-    *(uint16_t*)ptr = htons(len() - 4); // len() returns complete option length
-    // len field contains length without 4-byte option header
-    ptr += sizeof(uint16_t);
 
     *(uint32_t*)ptr = htonl(iaid_);
     ptr += sizeof(uint32_t);
@@ -68,12 +72,12 @@ Option6IA::pack(boost::shared_array<uint8_t> buf,
     *(uint32_t*)ptr = htonl(t2_);
     ptr += sizeof(uint32_t);
 
-    offset = LibDHCP::packOptions6(buf, buf_len, offset+16, options_);
+    offset = LibDHCP::packOptions6(buf, buf_len, offset+12, options_);
     return offset;
 }
 
 unsigned int
-Option6IA::unpack(boost::shared_array<uint8_t> buf,
+Option6IA::unpack(const boost::shared_array<uint8_t>& buf,
                   unsigned int buf_len,
                   unsigned int offset,
                   unsigned int parse_len) {

src/lib/dhcp/option6_ia.h

@@ -15,6 +15,7 @@
 #ifndef OPTION_IA_H_
 #define OPTION_IA_H_
 
+#include <stdint.h>
 #include "option.h"
 
 namespace isc {
@@ -30,7 +31,7 @@ public:
     ///
     /// @param type option type (usually 4 for IA_NA, 25 for IA_PD)
     /// @param iaid identity association identifier (id of IA)
-    Option6IA(unsigned short type, unsigned int iaid);
+    Option6IA(uint16_t type, unsigned int iaid);
 
     /// @brief ctor, used for received options
     ///
@@ -45,7 +46,7 @@ public:
     /// @param buf_len buffer length
     /// @param offset offset in buffer
     /// @param len number of bytes to parse
-    Option6IA(unsigned short type, boost::shared_array<uint8_t> buf,
+    Option6IA(uint16_t type, const boost::shared_array<uint8_t>& buf,
               unsigned int buf_len, unsigned int offset, unsigned int len);
 
     /// Writes option in wire-format to buf, returns pointer to first unused
@@ -57,7 +58,7 @@ public:
     ///
     /// @return offset to the first unused byte after stored option
     unsigned int
-    pack(boost::shared_array<uint8_t> buf, unsigned int buf_len,
+    pack(boost::shared_array<uint8_t>& buf, unsigned int buf_len,
          unsigned int offset);
 
     /// @brief Parses received buffer
@@ -72,7 +73,7 @@ public:
     ///
     /// @return offset after last parsed octet
     virtual unsigned int
-    unpack(boost::shared_array<uint8_t> buf, unsigned int buf_len,
+    unpack(const boost::shared_array<uint8_t>& buf, unsigned int buf_len,
            unsigned int offset, unsigned int parse_len);
 
     /// Provides human readable text representation

src/lib/dhcp/option6_iaaddr.cc

@@ -21,11 +21,13 @@
 #include "dhcp/option6_iaaddr.h"
 #include "dhcp/dhcp6.h"
 #include "asiolink/io_address.h"
+#include "util/io_utilities.h"
 
 using namespace std;
 using namespace isc;
 using namespace isc::dhcp;
 using namespace isc::asiolink;
+using namespace isc::util;
 
 Option6IAAddr::Option6IAAddr(unsigned short type,
                              const isc::asiolink::IOAddress& addr,
@@ -43,7 +45,7 @@ Option6IAAddr::Option6IAAddr(unsigned short type,
 }
 
 unsigned int
-Option6IAAddr::pack(boost::shared_array<uint8_t> buf,
+Option6IAAddr::pack(boost::shared_array<uint8_t>& buf,
                     unsigned int buf_len,
                     unsigned int offset) {
     if (len() > buf_len) {
@@ -71,7 +73,7 @@ Option6IAAddr::pack(boost::shared_array<uint8_t> buf,
 }
 
 unsigned int
-Option6IAAddr::unpack(boost::shared_array<uint8_t> buf,
+Option6IAAddr::unpack(const boost::shared_array<uint8_t>& buf,
                   unsigned int buf_len,
                   unsigned int offset,
                   unsigned int parse_len) {

src/lib/dhcp/option6_iaaddr.h

@@ -61,7 +61,7 @@ public:
     ///
     /// @return offset to first unused byte after stored option
     unsigned int
-    pack(boost::shared_array<uint8_t> buf, unsigned int buf_len,
+    pack(boost::shared_array<uint8_t>& buf, unsigned int buf_len,
          unsigned int offset);
 
     /// @brief Parses buffer.
@@ -76,7 +76,7 @@ public:
     ///
     /// @return offset after last parsed octet
     virtual unsigned int
-    unpack(boost::shared_array<uint8_t> buf,
+    unpack(const boost::shared_array<uint8_t>& buf,
            unsigned int buf_len,
            unsigned int offset,
            unsigned int parse_len);

src/lib/dhcp/pkt6.cc

@@ -114,7 +114,6 @@ Pkt6::packUDP() {
                                                       4/*offset*/,
                                                       options_);
 
-
         // sanity check
         if (offset != length) {
             isc_throw(OutOfRange, "Packet build failed: expected size="