[1186] Part 7 of review changes.

- test clean-up and improvements

src/bin/dhcp6/tests/iface_mgr_unittest.cc

@@ -142,6 +142,9 @@ TEST_F(IfaceMgrTest, dhcp6Sniffer) {
              << pkt->local_addr_.toText() << "\");" << endl;
         cout << "    pkt->ifindex_ = " << pkt->ifindex_ << ";" << endl;
         cout << "    pkt->iface_ = \"" << pkt->iface_ << "\";" << endl;
+
+        // TODO it is better to declare an array and then memcpy it to
+        // packet.
         for (int i=0; i< pkt->data_len_; i++) {
             cout << "    pkt->data_[" << i << "]="
                  << (int)(unsigned char)pkt->data_[i] << "; ";

src/lib/dhcp/pkt6.cc

@@ -99,6 +99,7 @@ Pkt6::packUDP() {
         data_len_ = length;
     }
 
+    data_len_ = length;
     try {
         // DHCPv6 header: message-type (1 octect) + transaction id (3 octets)
         data_[0] = msg_type_;

src/lib/dhcp/tests/libdhcp_unittest.cc

@@ -33,12 +33,20 @@ public:
     }
 };
 
+static const uint8_t packed[] = {
+    0, 12, 0, 5, 100, 101, 102, 103, 104, // opt1 (9 bytes)
+    0, 13, 0, 3, 105, 106, 107, // opt2 (7 bytes)
+    0, 14, 0, 2, 108, 109, // opt3 (6 bytes)
+    1,  0, 0, 4, 110, 111, 112, 113, // opt4 (8 bytes)
+    1,  1, 0, 1, 114 // opt5 (5 bytes)
+};
+
 TEST_F(LibDhcpTest, packOptions6) {
     boost::shared_array<uint8_t> buf(new uint8_t[512]);
     isc::dhcp::Option::Option6Collection opts; // list of options
 
     // generate content for options
-    for (int i=0;i<64;i++) {
+    for (int i = 0; i < 64; i++) {
         buf[i]=i+100;
     }
 
@@ -48,14 +56,6 @@ TEST_F(LibDhcpTest, packOptions6) {
     boost::shared_ptr<Option> opt4(new Option(Option::V6,256, buf,10, 4));
     boost::shared_ptr<Option> opt5(new Option(Option::V6,257, buf,14, 1));
 
-    char expected[] = {
-        0, 12, 0, 5, 100, 101, 102, 103, 104, // opt1
-        0, 13, 0, 3, 105, 106, 107, // opt2
-        0, 14, 0, 2, 108, 109, // opt3
-        1,  0, 0, 4, 110, 111, 112, 113, // opt4
-        1,  1, 0, 1, 114
-    };
-
     opts.insert(pair<int, boost::shared_ptr<Option> >(opt1->getType(), opt1));
     opts.insert(pair<int, boost::shared_ptr<Option> >(opt1->getType(), opt2));
     opts.insert(pair<int, boost::shared_ptr<Option> >(opt1->getType(), opt3));
@@ -67,23 +67,15 @@ TEST_F(LibDhcpTest, packOptions6) {
          offset = LibDHCP::packOptions6(buf, 512, 100, opts);
     });
     EXPECT_EQ(135, offset); // options should take 35 bytes
-    EXPECT_EQ(0, memcmp(&buf[100], expected, 35) );
+    EXPECT_EQ(0, memcmp(&buf[100], packed, 35) );
 }
 
 TEST_F(LibDhcpTest, unpackOptions6) {
 
     // just couple of random options
-    char packed[] = {
-        0, 12, 0, 5, 100, 101, 102, 103, 104, // opt1 (9 bytes)
-        0, 13, 0, 3, 105, 106, 107, // opt2 (7 bytes)
-        0, 14, 0, 2, 108, 109, // opt3 (6 bytes)
-        1,  0, 0, 4, 110, 111, 112, 113, // opt4 (8 bytes)
-        1,  1, 0, 1, 114 // opt5 (5 bytes)
-    };
     // Option is used as a simple option implementation
     // More advanced uses are validated in tests dedicated for
     // specific derived classes.
-
     isc::dhcp::Option::Option6Collection options; // list of options
 
     // we can't use packed directly, as shared_array would try to

src/lib/dhcp/tests/option6_addrlst_unittest.cc

@@ -38,6 +38,13 @@ public:
 
 TEST_F(Option6AddrLstTest, basic) {
 
+    // limiting tests to just a 2001:db8::/32 as is *wrong*.
+    // Good tests check corner cases as well.
+    // ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff checks
+    // for integer overflow
+    // ff02::face:b00c checks if multicast addresses
+    // can be represented properly.
+
     uint8_t sampledata[] = {
         // 2001:db8:1::dead:beef
         0x20, 0x01, 0x0d, 0xb8, 0x00, 0x01, 0, 0,
@@ -93,7 +100,7 @@ TEST_F(Option6AddrLstTest, basic) {
     };
 
     boost::shared_array<uint8_t> buf(new uint8_t[300]);
-    for (int i=0; i<300; i++)
+    for (int i = 0; i < 300; i++)
         buf[i] = 0;
 
     memcpy(&buf[0], sampledata, 48);

src/lib/dhcp/tests/option6_ia_unittest.cc

@@ -39,22 +39,22 @@ public:
 TEST_F(Option6IATest, basic) {
 
     boost::shared_array<uint8_t> simple_buf(new uint8_t[128]);
-    for (int i=0; i<128; i++)
+    for (int i = 0; i < 128; i++)
         simple_buf[i] = 0;
-    simple_buf[0]=0xa1; // iaid
-    simple_buf[1]=0xa2;
-    simple_buf[2]=0xa3;
-    simple_buf[3]=0xa4;
+    simple_buf[0] = 0xa1; // iaid
+    simple_buf[1] = 0xa2;
+    simple_buf[2] = 0xa3;
+    simple_buf[3] = 0xa4;
 
-    simple_buf[4]=0x81; // T1
-    simple_buf[5]=0x02;
-    simple_buf[6]=0x03;
-    simple_buf[7]=0x04;
+    simple_buf[4] = 0x81; // T1
+    simple_buf[5] = 0x02;
+    simple_buf[6] = 0x03;
+    simple_buf[7] = 0x04;
 
-    simple_buf[8]=0x84; // T2
-    simple_buf[9]=0x03;
-    simple_buf[10]=0x02;
-    simple_buf[11]=0x01;
+    simple_buf[8] = 0x84; // T2
+    simple_buf[9] = 0x03;
+    simple_buf[10] = 0x02;
+    simple_buf[11] = 0x01;
 
     // create an option
     // unpack() is called from constructor
@@ -104,12 +104,14 @@ TEST_F(Option6IATest, basic) {
                           (simple_buf[74] << 8) +
                           (simple_buf[75]) );
 
-    delete opt;
+    EXPECT_NO_THROW(
+        delete opt;
+    );
 }
 
 TEST_F(Option6IATest, simple) {
     boost::shared_array<uint8_t> simple_buf(new uint8_t[128]);
-    for (int i=0; i<128; i++)
+    for (int i = 0; i < 128; i++)
         simple_buf[i] = 0;
 
     Option6IA * ia = new Option6IA(D6O_IA_NA, 1234);
@@ -121,7 +123,9 @@ TEST_F(Option6IATest, simple) {
     EXPECT_EQ(2345, ia->getT1());
     EXPECT_EQ(3456, ia->getT2());
 
-    delete ia;
+    EXPECT_NO_THROW(
+        delete ia;
+    );
 }
 
 // test if option can build suboptions
@@ -176,7 +180,9 @@ TEST_F(Option6IATest, suboptions_pack) {
 
     EXPECT_EQ(0, memcmp(&buf[10], expected, 48));
 
-    delete ia;
+    EXPECT_NO_THROW(
+        delete ia;
+    );
 }
 
 // test if option can parse suboptions
@@ -204,7 +210,7 @@ TEST_F(Option6IATest, suboptions_unpack) {
     };
 
     boost::shared_array<uint8_t> buf(new uint8_t[128]);
-    for (int i=0; i<128; i++)
+    for (int i = 0; i < 128; i++)
         buf[i] = 0;
     memcpy(&buf[0], expected, 48);
 
@@ -235,16 +241,18 @@ TEST_F(Option6IATest, suboptions_unpack) {
 
     // checks for dummy option
     subopt = ia->getOption(0xcafe);
-    ASSERT_FALSE(subopt == boost::shared_ptr<Option>()); // non-NULL
+    ASSERT_TRUE(subopt); // should be non-NULL
 
     EXPECT_EQ(0xcafe, subopt->getType());
     EXPECT_EQ(4, subopt->len());
     EXPECT_EQ(NULL, subopt->getData());
 
     subopt = ia->getOption(1); // get option 1
-    ASSERT_EQ(subopt, boost::shared_ptr<Option>()); // NULL
+    ASSERT_FALSE(subopt); // should be NULL
 
-    delete ia;
+    EXPECT_NO_THROW(
+        delete ia;
+    );
 }
 
 }

src/lib/dhcp/tests/option6_iaaddr_unittest.cc

@@ -37,36 +37,36 @@ public:
 TEST_F(Option6IAAddrTest, basic) {
 
     boost::shared_array<uint8_t> simple_buf(new uint8_t[128]);
-    for (int i=0; i<128; i++)
+    for (int i = 0; i < 128; i++)
         simple_buf[i] = 0;
-    simple_buf[0]=0x20;
-    simple_buf[1]=0x01;
-    simple_buf[2]=0x0d;
-    simple_buf[3]=0xb8;
-    simple_buf[4]=0x00;
-    simple_buf[5]=0x01;
-    simple_buf[12]=0xde;
-    simple_buf[13]=0xad;
-    simple_buf[14]=0xbe;
-    simple_buf[15]=0xef; // 2001:db8:1::dead:beef
-
-    simple_buf[16]=0x00;
-    simple_buf[17]=0x00;
-    simple_buf[18]=0x03;
-    simple_buf[19]=0xe8; // 1000
-
-    simple_buf[20]=0xb2;
-    simple_buf[21]=0xd0;
-    simple_buf[22]=0x5e;
-    simple_buf[23]=0x00; // 3.000.000.000 
+    simple_buf[0] = 0x20;
+    simple_buf[1] = 0x01;
+    simple_buf[2] = 0x0d;
+    simple_buf[3] = 0xb8;
+    simple_buf[4] = 0x00;
+    simple_buf[5] = 0x01;
+    simple_buf[12] = 0xde;
+    simple_buf[13] = 0xad;
+    simple_buf[14] = 0xbe;
+    simple_buf[15] = 0xef; // 2001:db8:1::dead:beef
+
+    simple_buf[16] = 0x00;
+    simple_buf[17] = 0x00;
+    simple_buf[18] = 0x03;
+    simple_buf[19] = 0xe8; // 1000
+
+    simple_buf[20] = 0xb2;
+    simple_buf[21] = 0xd0;
+    simple_buf[22] = 0x5e;
+    simple_buf[23] = 0x00; // 3,000,000,000
 
     // create an option (unpack content)
     Option6IAAddr* opt = new Option6IAAddr(D6O_IAADDR,
                                            simple_buf,
                                            128,
-                                           0, 
+                                           0,
                                            24);
-    
+
     // pack this option again in the same buffer, but in
     // different place
     int offset = opt->pack(simple_buf, 128, 50);
@@ -80,17 +80,22 @@ TEST_F(Option6IAAddrTest, basic) {
 
     EXPECT_EQ(D6O_IAADDR, opt->getType());
 
+    EXPECT_EQ(Option::OPTION6_HDR_LEN + Option6IAAddr::OPTION6_IAADDR_LEN,
+              opt->len());
+
     // check if pack worked properly:
     // if option type is correct
     EXPECT_EQ(D6O_IAADDR, simple_buf[50]*256 + simple_buf[51]);
 
     // if option length is correct
     EXPECT_EQ(24, simple_buf[52]*256 + simple_buf[53]);
-    
+
     // if option content is correct
     EXPECT_EQ(0, memcmp(&simple_buf[0], &simple_buf[54],24));
 
-    delete opt;
+    EXPECT_NO_THROW(
+        delete opt;
+    );
 }
 
 }

src/lib/dhcp/tests/option_unittest.cc

@@ -43,7 +43,9 @@ TEST_F(OptionTest, basic4) {
     EXPECT_EQ(NULL, opt->getData());
     EXPECT_EQ(2, opt->len()); // just v4 header
 
-    delete opt;
+    EXPECT_NO_THROW(
+        delete opt;
+    );
 }
 
 // tests simple constructor
@@ -55,14 +57,16 @@ TEST_F(OptionTest, basic6) {
     EXPECT_EQ(NULL, opt->getData());
     EXPECT_EQ(4, opt->len()); // just v6 header
 
-    delete opt;
+    EXPECT_NO_THROW(
+        delete opt;
+    );
 }
 
 // tests contructor used in pkt reception
 // option contains actual data
 TEST_F(OptionTest, data1) {
     boost::shared_array<uint8_t> buf(new uint8_t[32]);
-    for (int i=0; i<32; i++)
+    for (int i = 0; i < 32; i++)
         buf[i] = 100+i;
     Option* opt = new Option(Option::V6, 333, //type
                              buf,
@@ -85,7 +89,9 @@ TEST_F(OptionTest, data1) {
     // payload
     EXPECT_EQ(0, memcmp(&buf[3], &buf[24], 7) );
 
-    delete opt;
+    EXPECT_NO_THROW(
+        delete opt;
+    );
 }
 
 // another text that tests the same thing, just
@@ -93,20 +99,20 @@ TEST_F(OptionTest, data1) {
 TEST_F(OptionTest, data2) {
 
     boost::shared_array<uint8_t> simple_buf(new uint8_t[128]);
-    for (int i=0; i<128; i++)
+    for (int i = 0; i < 128; i++)
         simple_buf[i] = 0;
-    simple_buf[0]=0xa1;
-    simple_buf[1]=0xa2;
-    simple_buf[2]=0xa3;
-    simple_buf[3]=0xa4;
+    simple_buf[0] = 0xa1;
+    simple_buf[1] = 0xa2;
+    simple_buf[2] = 0xa3;
+    simple_buf[3] = 0xa4;
 
     // create an option (unpack content)
-    Option* opt = new Option(Option::V6, 
+    Option* opt = new Option(Option::V6,
                              D6O_CLIENTID,
                              simple_buf,
-                             0, 
+                             0,
                              4);
-    
+
     // pack this option again in the same buffer, but in
     // different place
     int offset18 = opt->pack(simple_buf, 128, 10);
@@ -123,11 +129,13 @@ TEST_F(OptionTest, data2) {
 
     // if option length is correct
     EXPECT_EQ(4, simple_buf[12]*256 + simple_buf[13]);
-    
+
     // if option content is correct
     EXPECT_EQ(0, memcmp(&simple_buf[0], &simple_buf[14],4));
 
-    delete opt;
+    EXPECT_NO_THROW(
+        delete opt;
+    );
 }
 
 // check that an option can contain 2 suboptions:
@@ -171,7 +179,9 @@ TEST_F(OptionTest, suboptions1) {
     // payload
     EXPECT_EQ(0, memcmp(&buf[20], expected, 20) );
 
-    delete opt1;
+    EXPECT_NO_THROW(
+        delete opt1;
+    );
 }
 
 // check that an option can contain 2 suboptions:
@@ -211,7 +221,9 @@ TEST_F(OptionTest, suboptions2) {
     // payload
     EXPECT_EQ(0, memcmp(&buf[20], expected, 20) );
 
-    delete opt1;
+    EXPECT_NO_THROW(
+        delete opt1;
+    );
 }
 
 TEST_F(OptionTest, addgetdel) {

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -39,7 +39,7 @@ public:
 TEST_F(Pkt6Test, constructor) {
     Pkt6 * pkt1 = new Pkt6(17);
 
-    ASSERT_EQ(pkt1->data_len_, 17);
+    EXPECT_EQ(pkt1->data_len_, 17);
 
     delete pkt1;
 }
@@ -85,30 +85,85 @@ Pkt6 *capture1() {
     return (pkt);
 }
 
-TEST_F(Pkt6Test, parse_solicit1) {
+TEST_F(Pkt6Test, unpack_solicit1) {
     Pkt6 * sol = capture1();
 
     ASSERT_EQ(true, sol->unpack());
 
-    boost::shared_ptr<isc::dhcp::Option> null;
+    // check for length
+    EXPECT_EQ(98, sol->len() );
+
+    // check for type
+    EXPECT_EQ(DHCPV6_SOLICIT, sol->getType() );
 
     // check that all present options are returned
-    EXPECT_NE(null, sol->getOption(D6O_CLIENTID)); // client-id is present
-    EXPECT_NE(null, sol->getOption(D6O_IA_NA));    // IA_NA is present
-    EXPECT_NE(null, sol->getOption(D6O_ELAPSED_TIME));  // elapsed is present
-    EXPECT_NE(null, sol->getOption(D6O_NAME_SERVERS));
-    EXPECT_NE(null, sol->getOption(D6O_ORO));
+    EXPECT_TRUE(sol->getOption(D6O_CLIENTID)); // client-id is present
+    EXPECT_TRUE(sol->getOption(D6O_IA_NA));    // IA_NA is present
+    EXPECT_TRUE(sol->getOption(D6O_ELAPSED_TIME));  // elapsed is present
+    EXPECT_TRUE(sol->getOption(D6O_NAME_SERVERS));
+    EXPECT_TRUE(sol->getOption(D6O_ORO));
 
     // let's check that non-present options are not returned
-    EXPECT_EQ(null, sol->getOption(D6O_SERVERID)); // server-id is missing
-    EXPECT_EQ(null, sol->getOption(D6O_IA_TA));
-    EXPECT_EQ(null, sol->getOption(D6O_IAADDR));
+    EXPECT_FALSE(sol->getOption(D6O_SERVERID)); // server-id is missing
+    EXPECT_FALSE(sol->getOption(D6O_IA_TA));
+    EXPECT_FALSE(sol->getOption(D6O_IAADDR));
 
     std::cout << sol->toText();
 
     delete sol;
 }
 
+TEST_F(Pkt6Test, packUnpack) {
+
+    Pkt6 * parent = new Pkt6(100);
+
+    parent->setType(DHCPV6_SOLICIT);
+
+    boost::shared_ptr<Option> opt1(new Option(Option::V6, 1));
+    boost::shared_ptr<Option> opt2(new Option(Option::V6, 2));
+    boost::shared_ptr<Option> opt3(new Option(Option::V6, 100));
+    // let's not use zero-length option type 3 as it is IA_NA
+
+    parent->addOption(opt1);
+    parent->addOption(opt2);
+    parent->addOption(opt3);
+
+    EXPECT_EQ(DHCPV6_SOLICIT, parent->getType());
+    int transid = parent->getTransid();
+    // transaction-id was randomized, let's remember it
+
+    // calculated length should be 16
+    EXPECT_EQ( Pkt6::DHCPV6_PKT_HDR_LEN + 3*Option::OPTION6_HDR_LEN, 
+               parent->len() );
+
+    EXPECT_TRUE( parent->pack() );
+
+    //
+    EXPECT_EQ( Pkt6::DHCPV6_PKT_HDR_LEN + 3*Option::OPTION6_HDR_LEN, 
+               parent->len() );
+
+    // let's delete options from options_ collection
+    // they still be defined in packed 
+    parent->options_.clear();
+
+    // that that removed options are indeed are gone
+    EXPECT_EQ( 4, parent->len() );
+
+    // now recreate options list
+    EXPECT_TRUE( parent->unpack() );
+
+    // transid, message-type should be the same as before
+    EXPECT_EQ(transid, parent->getTransid());
+    EXPECT_EQ(DHCPV6_SOLICIT, parent->getType());
+    
+    EXPECT_TRUE( parent->getOption(1));
+    EXPECT_TRUE( parent->getOption(2));
+    EXPECT_TRUE( parent->getOption(100));
+    EXPECT_FALSE( parent->getOption(4));
+    
+    delete parent;
+}
+
 TEST_F(Pkt6Test, addGetDelOptions) {
     Pkt6 * parent = new Pkt6(100);