[trac878] Changes after Jinmei's pre-review:

- coding standards improved
- better exception handling

src/bin/dhcp6/addr6.cc

@@ -19,7 +19,8 @@
 
 #include "dhcp6/addr6.h"
 
-std::ostream & isc::operator << (std::ostream & out, const isc::Addr6& addr) {
+std::ostream&
+isc::operator << (std::ostream & out, const isc::Addr6& addr) {
     out << addr.getPlain();
     return out;
 }
@@ -27,7 +28,7 @@ std::ostream & isc::operator << (std::ostream & out, const isc::Addr6& addr) {
 using namespace std;
 using namespace isc;
 
-Addr6::Addr6(const char *addr, bool plain /*=false*/) {
+Addr6::Addr6(const char* addr, bool plain /*=false*/) {
     if (plain) {
         inet_pton(AF_INET6, addr, addr_);
     } else {
@@ -47,37 +48,44 @@ Addr6::Addr6(sockaddr_in6* addr) {
     memcpy(addr_, &addr->sin6_addr, 16);
 }
 
-bool Addr6::linkLocal() const {
+bool
+Addr6::linkLocal() const {
     if ( ( (unsigned char)addr_[0]==0xfe) &&
          ( (unsigned char)addr_[1]==0x80) ) {
-        return true;
+	return (true);
     } else {
-        return false;
+        return (false);
     }
 }
 
-bool Addr6::multicast() const {
+bool
+Addr6::multicast() const {
     if ( (unsigned char)addr_[0]==0xff) {
-        return true;
+        return (true);
     } else {
-        return false;
+        return (false);
     }
 }
 
-std::string Addr6::getPlain() const {
+std::string
+Addr6::getPlain() const {
     char buf[MAX_ADDRESS_STRING_LEN];
 
     inet_ntop(AF_INET6, addr_, buf, MAX_ADDRESS_STRING_LEN);
-    return string(buf);
+    return (string(buf));
 }
 
-
-bool Addr6::operator==(const Addr6& other) const {
+bool
+Addr6::equals(const Addr6& other) const {
     if (!memcmp(addr_, other.addr_, 16)) {
-        return true;
+        return (true);
     } else {
-        return false;
+        return (false);
     }
-
     // return !memcmp() would be shorter, but less readable
 }
+
+bool
+Addr6::operator==(const Addr6& other) const {
+    return (equals(other));
+}

src/bin/dhcp6/addr6.h

@@ -24,15 +24,26 @@ namespace isc {
     static const int MAX_ADDRESS_STRING_LEN =
         sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255");
 
+
+    /// \brief The implementation class for IPv6 address.
+    ///
+    /// There are no virtual methods to avoid virtual funtions
+    /// table. There are no extra fields, other than the address
+    /// itself. As a result, instances of this class are memory
+    /// optimal (sizeof(Addr6) == 16).
+    ///
+    /// Extra care should be taken, when extending this class
+    /// to keep low memory footprint.
     class Addr6 {
     public:
-        Addr6(const char * addr, bool plain=false);
+        Addr6(const char* addr, bool plain=false);
         Addr6(struct in6_addr* addr);
-        Addr6(struct sockaddr_in6 * addr);
+        Addr6(struct sockaddr_in6* addr);
         Addr6();
         inline const char * get() const { return addr_; }
         std::string getPlain() const;
-        char * get() { return addr_; }
+        char* getAddr() { return addr_; }
+        bool equals(const Addr6& other) const;
         bool operator==(const Addr6& other) const;
 
         bool linkLocal() const;
@@ -40,14 +51,13 @@ namespace isc {
 
         // no dtor necessary (no allocations done)
     private:
-        char addr_[MAX_ADDRESS_STRING_LEN];
+        char addr_[16];
     };
 
-    std::ostream & operator << (std::ostream & out, const Addr6& addr);
+    std::ostream& operator << (std::ostream & out, const Addr6& addr);
 
+    // TODO may need to also define map for faster access
     typedef std::list<Addr6> Addr6Lst;
-
 };
 
-
 #endif

src/bin/dhcp6/dhcp6.h

@@ -126,8 +126,8 @@ extern const int dhcpv6_type_name_max;
 /* 
  * DHCPv6 well-known multicast addressess, from section 5.1 of RFC 3315 
  */
-#define ALL_DHCP_RELAY_AGENTS_AND_SERVERS "FF02::1:2"
-#define ALL_DHCP_SERVERS "FF05::1:3"
+#define ALL_DHCP_RELAY_AGENTS_AND_SERVERS "ff02::1:2"
+#define ALL_DHCP_SERVERS "ff05::1:3"
 
 #define DHCP6_CLIENT_PORT 546
 #define DHCP6_SERVER_PORT 547

src/bin/dhcp6/dhcp6_srv.cc

@@ -22,29 +22,36 @@ using namespace isc;
 
 Dhcpv6Srv::Dhcpv6Srv() {
     cout << "Initialization" << endl;
+
+    // first call to instance() will create IfaceMgr (it's a singleton)
+    // it may throw something if things go wrong
+    IfaceMgr::instance();
 }
 
 Dhcpv6Srv::~Dhcpv6Srv() {
     cout << "DHCPv6 Srv shutdown." << endl;
 }
 
-bool Dhcpv6Srv::run() {
+bool
+Dhcpv6Srv::run() {
     while (true) {
-        Pkt6 * pkt;
+        Pkt6* pkt;
 
         pkt = IfaceMgr::instance().receive();
 
         if (pkt) {
-            Addr6 client = pkt->remoteAddr;
-            cout << "Received " << pkt->dataLen_ << " bytes, echoing back."
+            Addr6 client = pkt->remote_addr_;
+            cout << "Received " << pkt->data_len_ << " bytes, echoing back."
                  << endl;
             IfaceMgr::instance().send(*pkt);
             delete pkt;
         }
 
+	// TODO add support for config session (see src/bin/auth/main.cc)
+	//      so this daemon can be controlled from bob
         sleep(1);
 
     }
 
-    return true;
+    return (true);
 }

src/bin/dhcp6/iface_mgr.h

@@ -34,34 +34,37 @@ namespace isc {
             int ifindex_;
             Addr6Lst addrs_;
             char mac_[20]; //
-            int macLen_;
+            int mac_len_;
 
             Iface();
-            Iface(const std::string name, int ifindex);
+            Iface(const std::string& name, int ifindex);
             std::string getFullName() const;
             std::string getPlainMac() const;
 
             // next field is not needed, let's keep it in cointainers
         };
+
+	// TODO performance improvement: we may change this into
+	//      2 maps (ifindex-indexed and name-indexed)
         typedef std::list<Iface> IfaceLst;
 
         static IfaceMgr& instance();
         static void instanceCreate();
 
         Iface * getIface(int ifindex);
-        Iface * getIface(const std::string &ifname);
+        Iface * getIface(const std::string& ifname);
 
         bool openSockets();
         void printIfaces();
 
-        int openSocket(const std::string &ifname,
-                       const Addr6 &addr,
+        int openSocket(const std::string& ifname,
+                       const Addr6& addr,
                        int port, bool multicast);
-        bool joinMcast(int sock, const std::string &ifname,
+        bool joinMcast(int sock, const std::string& ifname,
                        const std::string& mcast);
 
-        bool send(Pkt6 &pkt);
-        Pkt6 * receive();
+        bool send(Pkt6& pkt);
+        Pkt6* receive();
 
 	// don't use private, we need derived classes in tests
     protected:

src/bin/dhcp6/main.cc

@@ -88,13 +88,11 @@ main(int argc, char* argv[]) {
 
     int ret = 0;
 
-    // XXX: we should eventually pass io_service here.
+    // TODO remainder of auth to dhcp6 code copy. We need to enable this in
+    //      dhcp6 eventually
 #if 0
     Session* cc_session = NULL;
-    Session* xfrin_session = NULL;
     Session* statistics_session = NULL;
-    bool xfrin_session_established = false; // XXX (see Trac #287)
-    bool statistics_session_established = false; // XXX (see Trac #287)
     ModuleCCSession* config_session = NULL;
 #endif
     try {
@@ -110,9 +108,8 @@ main(int argc, char* argv[]) {
         // auth_server->setVerbose(verbose_mode);
         cout << "[b10-dhcp6] Initiating DHCPv6 operation." << endl;
 
-        Dhcpv6Srv *srv = new Dhcpv6Srv();
+        Dhcpv6Srv* srv = new Dhcpv6Srv();
 
-        //srv->init();
         srv->run();
 
     } catch (const std::exception& ex) {

src/bin/dhcp6/pkt6.cc

@@ -15,32 +15,57 @@
 
 #include "dhcp6/dhcp6.h"
 #include "dhcp6/pkt6.h"
+#include <iostream>
 
 namespace isc {
 
-    /**
-     * constructor.
-     *
-     * Note: Pkt6 will take ownership of any data passed
-     *
-     * @param data
-     * @param dataLen
-     */
-    Pkt6::Pkt6(char * data, int dataLen) {
-	data_ = data;
-	dataLen_ = dataLen;
-    }
+///
+/// constructor
+///
+/// This constructor is used during packet reception.
+///
+/// Note: Pkt6 will take ownership of any data passed
+/// (due to performance reasons). Copying data on creation
+/// would be more elegant, but slower.
+///
+/// \param data
+/// \param dataLen
+///
+Pkt6::Pkt6(char * data, int dataLen) {
+    data_ = data;
+    data_len_ = dataLen;
+}
 
-    Pkt6::Pkt6(int dataLen) {
+///
+/// constructor
+///
+/// This constructor is used for generated packets.
+///
+/// Note: Pkt6 will take ownership of any data passed
+/// (due to performance reasons). Copying data on creation
+/// would be more elegant, but slower.
+///
+/// \param dataLen - length of the data to be allocated
+///
+Pkt6::Pkt6(int dataLen) {
+    try {
 	data_ = new char[dataLen];
-	dataLen_ = dataLen;
+	data_len_ = dataLen;
+    } catch (const std::exception& ex) {
+	// TODO move to LOG_FATAL()
+	// let's continue with empty pkt for now
+        std::cout << "Failed to allocate " << dataLen << " bytes."
+                  << std::endl;
+        data_ = 0;
+        data_len_ = 0;
     }
+}
 
-    Pkt6::~Pkt6() {
-	if (data_) {
-	    delete [] data_;
-	}
-
+Pkt6::~Pkt6() {
+    if (data_) {
+        delete [] data_;
+        data_ = 0;
     }
+}
 
 };

src/bin/dhcp6/pkt6.h

@@ -29,16 +29,16 @@ namespace isc {
         // XXX: probably need getter/setter wrappers
         //      and hide fields as protected
         char * data_;
-        int dataLen_;
+        int data_len_;
 
-        Addr6 localAddr;
-        Addr6 remoteAddr;
+        Addr6 local_addr_;
+        Addr6 remote_addr_;
 
-        std::string iface;
-        int ifindex;
+        std::string iface_;
+        int ifindex_;
 
-        int localPort;
-        int remotePort;
+        int local_port_;
+        int remote_port_;
 
         // XXX: add *a lot* here
 

src/bin/dhcp6/tests/addr6_unittest.cc

@@ -25,6 +25,7 @@
 using namespace std;
 using namespace isc;
 
+namespace {
 // empty class for now, but may be extended once Addr6 becomes bigger
 class Addr6Test : public ::testing::Test {
 public:
@@ -86,3 +87,5 @@ TEST_F(Addr6Test, stream) {
 
     EXPECT_STREQ( tmp.str().c_str(), plain.c_str() );
 }
+
+}

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -25,6 +25,7 @@
 using namespace std;
 using namespace isc;
 
+namespace {
 class Dhcpv6SrvTest : public ::testing::Test {
 public:
     Dhcpv6SrvTest() {
@@ -36,11 +37,17 @@ TEST_F(Dhcpv6SrvTest, basic) {
     // that is just a proof of concept and will be removed soon
     // No need to thoroughly test it
 
+    // srv has stubbed interface detection. It will read
+    // interfaces.txt instead. It will pretend to have detected
+    // fe80::1234 link-local address on eth0 interface. Obviously
+
+    // an attempt to bind this socket will fail.
     EXPECT_NO_THROW( {
         Dhcpv6Srv * srv = new Dhcpv6Srv();
 
 	delete srv;
-    });
+	});
     
 }
 
+}

src/bin/dhcp6/tests/iface_mgr_unittest.cc

@@ -27,6 +27,7 @@
 using namespace std;
 using namespace isc;
 
+namespace {
 class NakedIfaceMgr: public IfaceMgr {
     // "naked" Interface Manager, exposes internal fields 
 public:
@@ -169,10 +170,10 @@ TEST_F(IfaceMgrTest, sendReceive) {
 	sendPkt.data_[i] = i;
     }
 
-    sendPkt.remotePort = 10547;
-    sendPkt.remoteAddr = Addr6("::1", true);
-    sendPkt.ifindex = 1;
-    sendPkt.iface = "lo";
+    sendPkt.remote_port_ = 10547;
+    sendPkt.remote_addr_ = Addr6("::1", true);
+    sendPkt.ifindex_ = 1;
+    sendPkt.iface_ = "lo";
     
     Pkt6 * rcvPkt;
 
@@ -183,14 +184,15 @@ TEST_F(IfaceMgrTest, sendReceive) {
     ASSERT_TRUE( rcvPkt != NULL ); // received our own packet
 
     // let's check that we received what was sent
-    EXPECT_EQ(sendPkt.dataLen_, rcvPkt->dataLen_);
-    EXPECT_EQ(0, memcmp(sendPkt.data_, rcvPkt->data_, rcvPkt->dataLen_) );
+    EXPECT_EQ(sendPkt.data_len_, rcvPkt->data_len_);
+    EXPECT_EQ(0, memcmp(sendPkt.data_, rcvPkt->data_, rcvPkt->data_len_) );
 
-    EXPECT_EQ(sendPkt.remoteAddr, rcvPkt->remoteAddr);
-    EXPECT_EQ(rcvPkt->remotePort, 10546);
+    EXPECT_EQ(sendPkt.remote_addr_, rcvPkt->remote_addr_);
+    EXPECT_EQ(rcvPkt->remote_port_, 10546);
 
     delete rcvPkt;
 
     delete ifacemgr;
 }
 
+}

src/bin/dhcp6/tests/pkt6_unittest.cc

@@ -25,6 +25,7 @@
 using namespace std;
 using namespace isc;
 
+namespace {
 // empty class for now, but may be extended once Addr6 becomes bigger
 class Pkt6Test : public ::testing::Test {
 public:
@@ -35,17 +36,18 @@ public:
 TEST_F(Pkt6Test, constructor) {
     Pkt6 * pkt1 = new Pkt6(17);
     
-    ASSERT_EQ(pkt1->dataLen_, 17);
+    ASSERT_EQ(pkt1->data_len_, 17);
 
     char * buf = new char[23];
     // can't use char buf[23], as Pkt6 takes ownership of the data
 
     Pkt6 * pkt2 = new Pkt6(buf, 23);
 
-    ASSERT_EQ(pkt2->dataLen_, 23);
+    ASSERT_EQ(pkt2->data_len_, 23);
     ASSERT_EQ(pkt2->data_, buf);
 
     delete pkt1;
     delete pkt2;
 }
 
+}