[2095] tested multi-RDATA cases.

the implementation already supported this case, so only need to adjust tests.
encoding parts of the temporary hack is now unnecessary and removed.
tests were refactored so we can share the same test logic in multiple
scenarios.

src/lib/datasrc/memory/rdata_encoder.cc

@@ -257,19 +257,6 @@ getRdataEncodeSpec(RRClass rrclass, RRType rrtype) {
     return (generic_data_spec);
 }
 
-// A temporary helper of temporary encodeRdata(): it calculates the length
-// of the data portion of a NAPTR RDATA (i.e., the RDATA fields before the
-// "replacement" name).
-size_t
-getNAPTRDataLen(const rdata::Rdata& rdata) {
-    const rdata::generic::NAPTR& naptr_rdata =
-        dynamic_cast<const rdata::generic::NAPTR&>(rdata);
-
-    util::OutputBuffer buffer(0);
-    rdata.toWire(buffer);
-    return (buffer.getLength() - naptr_rdata.getReplacement().getLength());
-}
-
 // This class is used to divide the content of RDATA into \c RdataField
 // fields via message rendering logic.
 // The idea is to identify domain name fields in the writeName() method,
@@ -437,56 +424,8 @@ RdataEncoder::encode(void* buf, size_t buf_len) const {
 
 namespace testing {
 void
-encodeRdata(const rdata::Rdata& rdata, RRClass rrclass, RRType rrtype,
-            vector<uint8_t>& data_result, vector<uint16_t>& len_result)
-{
-    util::OutputBuffer buffer(0);
-    rdata.toWire(buffer);
-    util::InputBuffer ibuffer(buffer.getData(), buffer.getLength());
-    vector<uint8_t> tmp;        // used as temporary placeholder below
-
-    const RdataEncodeSpec& encode_spec = getRdataEncodeSpec(rrclass, rrtype);
-    for (size_t i = 0; i < encode_spec.field_count; ++i) {
-        const RdataFieldSpec& field_spec = encode_spec.fields[i];
-        switch (field_spec.type) {
-        case RdataFieldSpec::FIXEDLEN_DATA:
-            tmp.resize(field_spec.fixeddata_len);
-            ibuffer.readData(&tmp[0], tmp.size());
-            data_result.insert(data_result.end(), tmp.begin(), tmp.end());
-            break;
-        case RdataFieldSpec::VARLEN_DATA:
-        {
-            // In the vast majority cases of our supported RR types,
-            // variable-length data fields are placed at the end of RDATA,
-            // so the length of the field should be the remaining length
-            // of the output buffer.  The only exception is NAPTR, for which
-            // we use an ad hoc workaround (remember this function is for
-            // initial testing only, and will be cleaned up eventually).
-            const size_t pos = ibuffer.getPosition();
-            const size_t data_len = rrtype == RRType::NAPTR() ?
-                getNAPTRDataLen(rdata) : (ibuffer.getLength() - pos);
-            tmp.resize(data_len);
-            ibuffer.readData(&tmp[0], tmp.size());
-            data_result.insert(data_result.end(), tmp.begin(), tmp.end());
-            len_result.push_back(data_len);
-            break;
-        }
-        case RdataFieldSpec::DOMAIN_NAME:
-        {
-            const Name name(ibuffer);
-            const LabelSequence labels(name);
-            uint8_t labels_holder[LabelSequence::MAX_SERIALIZED_LENGTH];
-            labels.serialize(labels_holder, sizeof(labels_holder));
-            data_result.insert(data_result.end(), labels_holder,
-                               labels_holder + labels.getSerializedLength());
-            break;
-        }
-        }
-    }
-}
-
-void
 foreachRdataField(RRClass rrclass, RRType rrtype,
+                  size_t rdata_count,
                   const vector<uint8_t>& encoded_data,
                   const vector<uint16_t>& varlen_list,
                   NameCallback name_callback, DataCallback data_callback)
@@ -496,40 +435,42 @@ foreachRdataField(RRClass rrclass, RRType rrtype,
     size_t off = 0;
     size_t varlen_count = 0;
     size_t name_count = 0;
-    for (size_t i = 0; i < encode_spec.field_count; ++i) {
-        const RdataFieldSpec& field_spec = encode_spec.fields[i];
-        switch (field_spec.type) {
-        case RdataFieldSpec::FIXEDLEN_DATA:
-            if (data_callback) {
-                data_callback(&encoded_data.at(off), field_spec.fixeddata_len);
+    for (size_t count = 0; count < rdata_count; ++count) {
+        for (size_t i = 0; i < encode_spec.field_count; ++i) {
+            const RdataFieldSpec& field_spec = encode_spec.fields[i];
+            switch (field_spec.type) {
+            case RdataFieldSpec::FIXEDLEN_DATA:
+                if (data_callback) {
+                    data_callback(&encoded_data.at(off),
+                                  field_spec.fixeddata_len);
+                }
+                off += field_spec.fixeddata_len;
+                break;
+            case RdataFieldSpec::VARLEN_DATA:
+            {
+                const size_t varlen = varlen_list.at(varlen_count);
+                if (data_callback && varlen > 0) {
+                    data_callback(&encoded_data.at(off), varlen);
+                }
+                off += varlen;
+                ++varlen_count;
+                break;
             }
-            off += field_spec.fixeddata_len;
-            break;
-        case RdataFieldSpec::VARLEN_DATA:
-        {
-            const size_t varlen = varlen_list.at(varlen_count);
-            if (data_callback && varlen > 0) {
-                data_callback(&encoded_data.at(off), varlen);
+            case RdataFieldSpec::DOMAIN_NAME:
+            {
+                ++name_count;
+                const LabelSequence labels(&encoded_data.at(off));
+                if (name_callback) {
+                    name_callback(labels, field_spec.name_attributes);
+                }
+                off += labels.getSerializedLength();
+                break;
             }
-            off += varlen;
-            ++varlen_count;
-            break;
-        }
-        case RdataFieldSpec::DOMAIN_NAME:
-        {
-            ++name_count;
-            const LabelSequence labels(&encoded_data.at(off));
-            if (name_callback) {
-                name_callback(labels, field_spec.name_attributes);
             }
-            off += labels.getSerializedLength();
-            break;
-        }
         }
     }
-
-    assert(name_count == encode_spec.name_count);
-    assert(varlen_count == encode_spec.varlen_count);
+    assert(name_count == encode_spec.name_count * rdata_count);
+    assert(varlen_count == encode_spec.varlen_count * rdata_count);
 }
 } // namespace testing
 

src/lib/datasrc/memory/rdata_encoder.h

@@ -85,34 +85,20 @@ private:
 // update the test cases that use these functions with the complete
 // functions/classes, and then remove the entire namespace.
 namespace testing {
-// "Encode" given RDATA of given RR class and type.
-//
-// Fixed/variable-length data fields are encoded in their wire-format;
-// domain names are encoded in the form of:
-//  - nlen: name data length (1 byte)
-//  - olen: offset data length (1 byte)
-//  - name data (nlen bytes)
-//  - offset data (olen bytes)
-//
-// The encoded results are appended to data_result.
-// If the RDATA contain variable-length data fields, the lengths of the
-// these fields will be appended in len_result, in the order of appearance.
-void encodeRdata(const dns::rdata::Rdata& rdata, dns::RRClass rrclass,
-                 dns::RRType rrtype, std::vector<uint8_t>& data_result,
-                 std::vector<uint16_t>& len_result);
-
 // Callbacks used in foreachRdataField.
 typedef boost::function<void(const dns::LabelSequence&,
                              RdataNameAttributes)> NameCallback;
 typedef boost::function<void(const uint8_t*, size_t)> DataCallback;
 
-// Iterate over each RDATA field (in terms of the internal encoding) stored
-// in encoded_data, and call the given callback for each data (for
-// domain name fields, name_callback will be called; for normal data fields
-// data_callback will be called).  If the encoded data contain variable-length
-// data fields, varlen_list should store a sequence of their lengths, in the
-// of the appearance.
+// Iterate over each field (in terms of the internal encoding) of each
+// RDATA stored in encoded_data, and call the given callback for each
+// data (for domain name fields, name_callback will be called; for
+// normal data fields data_callback will be called).  rdata_count is
+// the number of RDATAs.  If the encoded data contain variable-length
+// data fields, varlen_list should store a sequence of their lengths,
+// in the of the appearance.
 void foreachRdataField(dns::RRClass rrclass, dns::RRType rrtype,
+                       size_t rdata_count,
                        const std::vector<uint8_t>& encoded_data,
                        const std::vector<uint16_t>& varlen_list,
                        NameCallback name_callback, DataCallback data_callback);

src/lib/datasrc/memory/tests/rdata_encoder_unittest.cc

@@ -26,6 +26,7 @@
 #include <gtest/gtest.h>
 
 #include <boost/bind.hpp>
+#include <boost/foreach.hpp>
 
 #include <cstring>
 #include <set>
@@ -50,10 +51,8 @@ struct TestRdata {
     const size_t n_varlen_fields; // expected # of variable-len fields
 };
 
-// This test data consist of all supported types of RDATA (+ some
-// unusual and corner cases).  We'll construct corresponding Rdata
-// object from this, and compare its wire format data both generated
-// by normal libdns++ interface and via encoding conversion.
+// This test data consist of (almost) all supported types of RDATA (+ some
+// unusual and corner cases).
 const TestRdata test_rdata_list[] = {
     {"IN", "A", "192.0.2.1", 0},
     {"IN", "NS", "ns.example.com", 0},
@@ -72,11 +71,7 @@ const TestRdata test_rdata_list[] = {
     {"IN", "DNAME", "dname.example.com", 0},
     {"IN", "DS", "12892 5 2 5F0EB5C777586DE18DA6B5", 1},
     {"IN", "SSHFP", "1 1 dd465c09cfa51fb45020cc83316fff", 1},
-    // Note: in our implementation RRSIG is treated as opaque data (including
-    // the signer name).  We use "com" for signer so it won't be a compress
-    // target in the test.
-    //{"IN", "RRSIG", "SOA 5 2 3600 20120814220826 20120715220826 12345 "
-    //"com. FAKEFAKEFAKE", 1},
+    // We handle RRSIG separately, so it's excluded from the list
     {"IN", "NSEC", "next.example.com. A AAAA NSEC RRSIG", 1},
     {"IN", "DNSKEY", "256 3 5 FAKEFAKE", 1},
     {"IN", "DHCID", "FAKEFAKE", 1},
@@ -115,13 +110,27 @@ class RdataEncoderTest : public ::testing::Test {
 protected:
     RdataEncoderTest() {}
 
+    // This helper test method constructs encodes the given list of RDATAs
+    // (in rdata_list), and then iterates over the data, rendering the fields
+    // in the wire format.  It then compares the wire data with the one
+    // generated by the normal libdns++ interface to see the encoding/decoding
+    // works as intended.
+    void checkEncode(RRClass rrclass, RRType rrtype,
+                     const vector<ConstRdataPtr>& rdata_list,
+                     size_t expected_varlen_fields);
+
     RdataEncoder encoder_;
     vector<uint8_t> encoded_data_;
     MessageRenderer expected_renderer_;
     MessageRenderer actual_renderer_;
+    vector<ConstRdataPtr> rdata_list_;
 };
 
-TEST_F(RdataEncoderTest, addRdata) {
+void
+RdataEncoderTest::checkEncode(RRClass rrclass, RRType rrtype,
+                              const vector<ConstRdataPtr>& rdata_list,
+                              size_t expected_varlen_fields)
+{
     // These two names will be rendered before and after the test RDATA,
     // to check in case the RDATA contain a domain name whether it's
     // compressed or not correctly.  The names in the RDATA should basically
@@ -139,55 +148,119 @@ TEST_F(RdataEncoderTest, addRdata) {
     need_additionals.insert(RRType::MX());
     need_additionals.insert(RRType::SRV());
 
+    expected_renderer_.clear();
+    actual_renderer_.clear();
+    expected_renderer_.writeName(dummy_name);
+    actual_renderer_.writeName(dummy_name);
+    encoded_data_.clear();
+
+    const bool additional_required =
+        (need_additionals.find(rrtype) != need_additionals.end());
+
+    BOOST_FOREACH(const ConstRdataPtr& rdata, rdata_list) {
+        rdata->toWire(expected_renderer_);
+    }
+    expected_renderer_.writeName(dummy_name2);
+
+    encoder_.start(rrclass, rrtype);
+    BOOST_FOREACH(const ConstRdataPtr& rdata, rdata_list) {
+        encoder_.addRdata(*rdata);
+    }
+    encoded_data_.resize(encoder_.getStorageLength());
+    encoder_.encode(&encoded_data_[0], encoded_data_.size());
+
+    // If this type of RDATA is expected to contain variable-length fields,
+    // we brute force the encoded data, exploiting our knowledge of actual
+    // encoding, then adjust the encoded data excluding the list of length
+    // fields.  This is ugly, but we should be able to eliminate this hack
+    // at #2096.
+    vector<uint16_t> varlen_list;
+    if (expected_varlen_fields > 0) {
+        const size_t varlen_list_size =
+            rdata_list.size() * expected_varlen_fields * sizeof(uint16_t);
+        ASSERT_LE(varlen_list_size, encoded_data_.size());
+        varlen_list.resize(rdata_list.size() * expected_varlen_fields);
+        std::memcpy(&varlen_list[0], &encoded_data_[0], varlen_list_size);
+        encoded_data_.assign(encoded_data_.begin() + varlen_list_size,
+                             encoded_data_.end());
+    }
+    foreachRdataField(rrclass, rrtype, rdata_list.size(), encoded_data_,
+                      varlen_list,
+                      boost::bind(renderNameField, &actual_renderer_,
+                                  additional_required, _1, _2),
+                      boost::bind(renderDataField, &actual_renderer_, _1, _2));
+
+    actual_renderer_.writeName(dummy_name2);
+    matchWireData(expected_renderer_.getData(), expected_renderer_.getLength(),
+                  actual_renderer_.getData(), actual_renderer_.getLength());
+}
+
+TEST_F(RdataEncoderTest, addRdata) {
+    // Basic check on the encoded data for (most of) all supported RR types,
+    // in a comprehensive manner.
     for (size_t i = 0; test_rdata_list[i].rrclass != NULL; ++i) {
         SCOPED_TRACE(string(test_rdata_list[i].rrclass) + "/" +
                      test_rdata_list[i].rrtype);
-
-        expected_renderer_.clear();
-        actual_renderer_.clear();
-        expected_renderer_.writeName(dummy_name);
-        actual_renderer_.writeName(dummy_name);
-        encoded_data_.clear();
-
         const RRClass rrclass(test_rdata_list[i].rrclass);
         const RRType rrtype(test_rdata_list[i].rrtype);
         const ConstRdataPtr rdata = createRdata(rrtype, rrclass,
                                                 test_rdata_list[i].rdata);
-        const bool additional_required =
-            (need_additionals.find(rrtype) != need_additionals.end());
+        rdata_list_.clear();
+        rdata_list_.push_back(rdata);
+        checkEncode(rrclass, rrtype, rdata_list_,
+                    test_rdata_list[i].n_varlen_fields);
+    }
+}
 
-        rdata->toWire(expected_renderer_);
-        expected_renderer_.writeName(dummy_name2);
+TEST_F(RdataEncoderTest, addRdataMulti) {
+    // Similar to addRdata(), but test with multiple RDATAs.
+    // Four different cases are tested: a single fixed-len RDATA (A),
+    // fixed-len data + domain name (MX), variable-len data only (TXT),
+    // variable-len data + domain name (NAPTR).
+    ConstRdataPtr a_rdata1 = createRdata(RRType::A(), RRClass::IN(),
+                                        "192.0.2.53");
+    ConstRdataPtr a_rdata2 = createRdata(RRType::A(), RRClass::IN(),
+                                         "192.0.2.54");
+    rdata_list_.clear();
+    rdata_list_.push_back(a_rdata1);
+    rdata_list_.push_back(a_rdata2);
+    checkEncode(RRClass::IN(), RRType::A(), rdata_list_, 0);
 
-        encoder_.start(rrclass, rrtype);
-        encoder_.addRdata(*rdata);
+    ConstRdataPtr mx_rdata1 = createRdata(RRType::MX(), RRClass::IN(),
+                                          "5 mx1.example.com");
+    ConstRdataPtr mx_rdata2 = createRdata(RRType::MX(), RRClass::IN(),
+                                          "10 mx2.example.com");
+    rdata_list_.clear();
+    rdata_list_.push_back(mx_rdata1);
+    rdata_list_.push_back(mx_rdata2);
+    checkEncode(RRClass::IN(), RRType::MX(), rdata_list_, 0);
 
-        vector<uint16_t> varlen_list;
-        encoded_data_.resize(encoder_.getStorageLength());
-        encoder_.encode(&encoded_data_[0], encoded_data_.size());
-        if (test_rdata_list[i].n_varlen_fields > 0) {
-            const size_t varlen_list_size =
-                test_rdata_list[i].n_varlen_fields * sizeof(uint16_t);
-            ASSERT_LE(varlen_list_size, encoded_data_.size());
-            varlen_list.resize(test_rdata_list[i].n_varlen_fields);
-            std::memcpy(&varlen_list[0], &encoded_data_[0], varlen_list_size);
-            encoded_data_.assign(encoded_data_.begin() + varlen_list_size,
-                                 encoded_data_.end());
-        }
-        foreachRdataField(rrclass, rrtype, encoded_data_, varlen_list,
-                          boost::bind(renderNameField, &actual_renderer_,
-                                      additional_required, _1, _2),
-                          boost::bind(renderDataField, &actual_renderer_,
-                                      _1, _2));
-
-        actual_renderer_.writeName(dummy_name2);
-        matchWireData(expected_renderer_.getData(),
-                      expected_renderer_.getLength(),
-                      actual_renderer_.getData(),
-                      actual_renderer_.getLength());
-    }
+    ConstRdataPtr txt_rdata1 = createRdata(RRType::TXT(), RRClass::IN(),
+                                           "foo bar baz");
+    ConstRdataPtr txt_rdata2 = createRdata(RRType::TXT(), RRClass::IN(),
+                                          "another text data");
+    rdata_list_.clear();
+    rdata_list_.push_back(txt_rdata1);
+    rdata_list_.push_back(txt_rdata2);
+    checkEncode(RRClass::IN(), RRType::TXT(), rdata_list_, 1);
+
+    ConstRdataPtr naptr_rdata1 =
+        createRdata(RRType::NAPTR(), RRClass::IN(),
+                    "100 50 \"s\" \"http\" \"\" _http._tcp.example.com");
+    ConstRdataPtr naptr_rdata2 =
+        createRdata(RRType::NAPTR(), RRClass::IN(),
+                    "200 100 \"s\" \"http\" \"\" _http._tcp.example.com");
+    rdata_list_.clear();
+    rdata_list_.push_back(naptr_rdata1);
+    rdata_list_.push_back(naptr_rdata2);
+    checkEncode(RRClass::IN(), RRType::NAPTR(), rdata_list_, 1);
 }
 
 // TODO: add before start
 
+// Note: in our implementation RRSIG is treated as opaque data (including
+// the signer name).  We use "com" for signer so it won't be a compress
+// target in the test.
+//{"IN", "RRSIG", "SOA 5 2 3600 20120814220826 20120715220826 12345 "
+//"com. FAKEFAKEFAKE", 1},
 }