[trac404] Unify the interface a little bit

And hint more by a little bit that the user isn't really expected to dig
inside the data.

src/lib/dns/benchmarks/rdatarender_bench.cc

@@ -67,28 +67,30 @@ public:
     RdataFieldsStore(ConstRdataPtr rdata) {
         const RdataFields fields(*rdata);
 
-        nspecs_ = fields.getFieldCount();
-        spec_store_.resize(nspecs_ * sizeof(RdataFields::FieldSpec));
+        spec_size_ = fields.getFieldDataSize();
+        spec_store_.resize(spec_size_);
         void* cp_spec = &spec_store_[0];
         memcpy(cp_spec, fields.getFieldSpecData(), spec_store_.size());
-        spec_ptr_ = static_cast<RdataFields::FieldSpec*>(cp_spec);
+        spec_ptr_ = cp_spec;
 
         data_length_ = fields.getDataLength();
-        data_store_.assign(fields.getData(), fields.getData() + data_length_);
+        data_store_.resize(data_length_);
+        void* cp_data = &data_store_[0];
+        memcpy(cp_data, fields.getData(), data_store_.size());
         // Vector guarantees that the elements are stored in continuous array
         // in memory, so this is actually correct by the standard
-        data_ptr_ = &data_store_[0];
+        data_ptr_ = cp_data;
     }
     void toWire(MessageRenderer& renderer) const {
-        RdataFields(spec_ptr_, nspecs_,
+        RdataFields(spec_ptr_, spec_size_,
                     data_ptr_, data_length_).toWire(renderer);
     }
 private:
     vector<unsigned char> spec_store_;
-    const RdataFields::FieldSpec* spec_ptr_;
-    unsigned int nspecs_;
-    vector<uint8_t> data_store_;
-    const uint8_t* data_ptr_;
+    vector<unsigned char> data_store_;
+    const void* spec_ptr_;
+    const void* data_ptr_;
+    unsigned int spec_size_;
     size_t data_length_;
 };
 

src/lib/dns/rdatafields.cc

@@ -158,16 +158,19 @@ RdataFields::RdataFields(const Rdata& rdata) {
     }
 }
 
-RdataFields::RdataFields(const FieldSpec* fields, const unsigned int nfields,
-                         const uint8_t* data, const size_t data_length) :
-    fields_(fields), nfields_(nfields), data_(data), data_length_(data_length),
+RdataFields::RdataFields(const void* fields, const unsigned int fields_length,
+                         const void* data, const size_t data_length) :
+    fields_(static_cast<const FieldSpec*>(fields)),
+    nfields_(fields_length / sizeof(*fields_)),
+    data_(static_cast<const uint8_t*>(data)),
+    data_length_(data_length),
     detail_(NULL)
 {
     if ((fields_ == NULL && nfields_ > 0) ||
         (fields_ != NULL && nfields_ == 0)) {
         isc_throw(InvalidParameter,
-                  "Inconsistent parameters for RdataFields: nfields ("
-                  << nfields_ << ") and fields conflict each other");
+                  "Inconsistent parameters for RdataFields: fields_length ("
+                  << fields_length << ") and fields conflict each other");
     }
     if ((data_ == NULL && data_length_ > 0) ||
         (data_ != NULL && data_length_ == 0)) {

src/lib/dns/rdatafields.h

@@ -107,20 +107,20 @@ getFieldSpecData()-> { compressible name { compressible name { other data
 /// scenario:
 /// \code // assume "rdata" is a reference type to Rdata
 /// const RdataFields fields(rdata);
-/// const unsigned int nfields = fields.getFieldCount();
-/// memcpy(some_place, fields.getFieldSpecData(), nfields*sizeof(FieldSpec));
+/// const unsigned int fields_size = fields.getFieldDataSize();
+/// memcpy(some_place, fields.getFieldSpecData(), fields_size);
 /// const size_t data_length = fields.getDataLength();
 /// memcpy(other_place, fields.getData(), data_length);
-/// // (nfields and data_length should be stored somewhere, too)
+/// // (fields_size and data_length should be stored somewhere, too)
 /// \endcode
 ///
 /// Another typical usage is to render the stored data in the wire format
 /// as efficiently as possible.  The following code is an example of such
 /// usage:
 /// \code // assume "renderer" is of type MessageRenderer
-/// // retrieve data_length and nfields from the storage
-/// RdataFields(static_cast<const FieldSpec*>(some_place),
-///             nfields, other_place, data_length).toWire(renderer);
+/// // retrieve data_length and fields_size from the storage
+/// RdataFields(some_place, fields_size, other_place,
+///             data_length).toWire(renderer);
 /// \endcode
 ///
 /// <b>Notes to Users</b>
@@ -128,7 +128,7 @@ getFieldSpecData()-> { compressible name { compressible name { other data
 /// The main purposes of this class is to help efficient operation
 /// for some (limited classes of) performance sensitive application.
 /// For this reason the interface and implementation rely on relatively
-/// lower-level, riskier primitives such as passing around bear pointers.
+/// lower-level, riskier primitives such as passing around bare pointers.
 ///
 /// It is therefore discouraged to use this class for general purpose
 /// applications that do not need to maximize performance in terms of either
@@ -278,7 +278,9 @@ public:
     /// memory region is a valid representation of domain name.
     /// Otherwise, a subsequent method call such as
     /// <code>toWire(AbstractMessageRenderer&) const</code>
-    /// may trigger an unexpected exception.
+    /// may trigger an unexpected exception. It also expects the fields reside
+    /// on address that is valid for them (eg. it has valid alignment), see
+    /// getFieldSpecData() for details.
     ///
     /// It is the caller's responsibility to ensure this assumption.
     /// In general, this constructor is expected to be used for serialized data
@@ -296,8 +298,8 @@ public:
     /// \param data A pointer to memory region for the entire RDATA.  This can
     /// be NULL.
     /// \param data_length The length of \c data in bytes.
-    RdataFields(const FieldSpec* fields, const unsigned int nfields,
-                const uint8_t* data, const size_t data_length);
+    RdataFields(const void* fields, const unsigned int fields_length,
+                const void* data, const size_t data_length);
 
     /// The destructor.
     ~RdataFields();
@@ -315,26 +317,40 @@ public:
 
     /// \brief Return a pointer to the RDATA encoded in the \c RdataFields.
     ///
+    /// The RdataFields holds ownership of the data.
+    ///
     /// This method never throws an exception.
-    const uint8_t* getData() const { return (data_); }
+    const void* getData() const { return (data_); }
 
-    /// \brief Return the number of fields encoded in the RdataFields.
+    /// \brief Return the number of bytes the buffer returned by
+    ///      getFieldSpecData() will occupy.
     ///
     /// This method never throws an exception.
-    unsigned int getFieldCount() const { return (nfields_); }
+    unsigned int getFieldDataSize() const { return (nfields_ *
+                                                    sizeof *fields_); }
 
     /// \brief Return a pointer to a sequence of \c FieldSpec for the
     /// \c RdataFields.
     ///
-    /// The data is just opaque internal representation, as a sequence
-    /// of bytes (unsigned char * because of C/C++ aliasing rules).
+    /// This should be treated as an opaque internal representation you can
+    /// just store off somewhere and use it to construct a new RdataFields.
+    /// from it. If you are really interested, you can typecast it to
+    /// FieldSpec * (which is what it really is internally).
+    ///
+    /// The RdataFields holds ownership of the data.
+    ///
+    /// \note You should, however, be aware of alignment issues. The pointer
+    ///     you pass to the constructor must be an address where the FieldSpec
+    ///     can live. If you store it at a wrong address (eg. even one with
+    ///     current implementation on most architectures), it might lead bad
+    ///     things from slow access to SIGBUS. The easiest way is not to
+    ///     interleave the fields with data from getData(). It is OK to place
+    ///     all the fields first (even from multiple RdataFields) and then
+    ///     place all the data after them.
     ///
     /// This method never throws an exception.
-    const uint8_t* getFieldSpecData() const {
-        // Nasty cast, because C++ authors didn't read the C specification
-        // about pointers. We can't return void* from it, that would break
-        // aliasing rules.
-        return ((const uint8_t*) fields_);
+    const void* getFieldSpecData() const {
+        return (fields_);
     }
 
     /// \brief Return the specification of the field identified by the given

src/lib/dns/tests/rdatafields_unittest.cc

@@ -71,7 +71,7 @@ RdataFieldsTest::constructCommonTests(const RdataFields& fields,
     EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, expected_data,
                         expected_data_len, fields.getData(),
                         fields.getDataLength());
-    EXPECT_EQ(1, fields.getFieldCount());
+    EXPECT_EQ(sizeof(RdataFields::FieldSpec), fields.getFieldDataSize());
     EXPECT_EQ(RdataFields::DATA, fields.getFieldSpec(0).type);
     EXPECT_EQ(4, fields.getFieldSpec(0).len);
 
@@ -93,7 +93,8 @@ TEST_F(RdataFieldsTest, constructFromRdata) {
 
 TEST_F(RdataFieldsTest, constructFromParams) {
     const RdataFields::FieldSpec spec(RdataFields::DATA, 4);
-    const RdataFields fields(&spec, 1, in_a_data, sizeof(in_a_data));
+    const RdataFields fields(&spec, sizeof(spec), in_a_data,
+                             sizeof(in_a_data));
     constructCommonTests(fields, in_a_data, sizeof(in_a_data));
 }
 
@@ -102,7 +103,7 @@ TEST_F(RdataFieldsTest, constructFromParams) {
 //
 void
 RdataFieldsTest::constructCommonTestsNS(const RdataFields& fields) {
-    EXPECT_EQ(1, fields.getFieldCount());
+    EXPECT_EQ(sizeof(RdataFields::FieldSpec), fields.getFieldDataSize());
     EXPECT_EQ(RdataFields::COMPRESSIBLE_NAME, fields.getFieldSpec(0).type);
     EXPECT_EQ(ns_name.getLength(), fields.getFieldSpec(0).len);
 
@@ -131,7 +132,7 @@ TEST_F(RdataFieldsTest, constructFromRdataNS) {
 TEST_F(RdataFieldsTest, constructFromParamsNS) {
     const RdataFields::FieldSpec spec(RdataFields::COMPRESSIBLE_NAME,
                                       sizeof(ns_data));
-    const RdataFields fields_ns(&spec, 1, ns_data, sizeof(ns_data));
+    const RdataFields fields_ns(&spec, sizeof(spec), ns_data, sizeof(ns_data));
     constructCommonTestsNS(fields_ns);
 }
 
@@ -142,7 +143,7 @@ void
 RdataFieldsTest::constructCommonTestsTXT(const RdataFields& fields) {
     // Since all fields are plain data, they are handled as a single data
     // field.
-    EXPECT_EQ(1, fields.getFieldCount());
+    EXPECT_EQ(sizeof(RdataFields::FieldSpec), fields.getFieldDataSize());
     EXPECT_EQ(RdataFields::DATA, fields.getFieldSpec(0).type);
     EXPECT_EQ(expected_wire.size(), fields.getFieldSpec(0).len);
 
@@ -173,8 +174,8 @@ TEST_F(RdataFieldsTest, constructFromParamsTXT) {
     UnitTestUtil::readWireData("rdatafields3.wire", expected_wire);
     expected_wire.erase(expected_wire.begin(), expected_wire.begin() + 2);
     const RdataFields::FieldSpec spec(RdataFields::DATA, expected_wire.size());
-    const RdataFields fields(&spec, 1, &expected_wire[0],
-                             expected_wire.size()); 
+    const RdataFields fields(&spec, sizeof(spec), &expected_wire[0],
+                             expected_wire.size());
     constructCommonTestsTXT(fields);
 }
 
@@ -189,7 +190,7 @@ RdataFieldsTest::constructCommonTestsRRSIG(const RdataFields& fields) {
     //   this is a variable length field.  In this test it's a 13-byte field.
     // - a variable-length data field for the signature.  In this tests
     //   it's a 15-byte field.
-    ASSERT_EQ(3, fields.getFieldCount());
+    ASSERT_EQ(3 * sizeof(RdataFields::FieldSpec), fields.getFieldDataSize());
     EXPECT_EQ(RdataFields::DATA, fields.getFieldSpec(0).type);
     EXPECT_EQ(18, fields.getFieldSpec(0).len);
     EXPECT_EQ(RdataFields::INCOMPRESSIBLE_NAME, fields.getFieldSpec(1).type);
@@ -239,7 +240,8 @@ TEST_F(RdataFieldsTest, constructFromParamsRRSIG) {
         RdataFields::FieldSpec(RdataFields::INCOMPRESSIBLE_NAME, 13),
         RdataFields::FieldSpec(RdataFields::DATA, 15)
     };
-    const RdataFields fields(specs, 3, &fields_wire[0], fields_wire.size());
+    const RdataFields fields(specs, sizeof(specs), &fields_wire[0],
+                             fields_wire.size());
     constructCommonTestsRRSIG(fields);
 }
 
@@ -254,17 +256,15 @@ TEST_F(RdataFieldsTest, convertRdatatoParams) {
     expected_wire.erase(expected_wire.begin(), expected_wire.begin() + 2);
 
     // Copy the data in separate storage
-    vector<uint8_t> spec_store(fields.getFieldCount() *
-                               sizeof(RdataFields::FieldSpec));
+    vector<uint8_t> spec_store(fields.getFieldDataSize());
     void* cp_spec = &spec_store[0];
     memcpy(cp_spec, fields.getFieldSpecData(), spec_store.size());
     vector<uint8_t> data_store(fields.getDataLength());
     memcpy(&data_store[0], fields.getData(), fields.getDataLength());
 
     // Restore the data in the form of RdataFields
-    const RdataFields fields_byparams(
-        static_cast<const RdataFields::FieldSpec*>(cp_spec),
-        fields.getFieldCount(), &data_store[0], fields.getDataLength());
+    const RdataFields fields_byparams(cp_spec, fields.getFieldDataSize(),
+                                      &data_store[0], fields.getDataLength());
 
     // Check it's valid
     constructCommonTestsRRSIG(fields_byparams);
@@ -275,7 +275,7 @@ TEST_F(RdataFieldsTest, convertRdatatoParams) {
 //
 void
 RdataFieldsTest::constructCommonTestsOPT(const RdataFields& fields) {
-    EXPECT_EQ(0, fields.getFieldCount());
+    EXPECT_EQ(0, fields.getFieldDataSize());
     EXPECT_EQ(0, fields.getDataLength());
     EXPECT_EQ((const uint8_t*) NULL, fields.getData());
     fields.toWire(obuffer);