[2096] Comment cleanup

src/lib/datasrc/memory/rdata_encoder.h

@@ -29,69 +29,6 @@
 
 #include <vector>
 
-/// \file rdata_encoder.h
-/// \brief Set of utility classes for encoding RDATA in memory efficient way.
-///
-/// This file defines a set of interfaces (classes, types, constants) to
-/// manipulate a given set of RDATA of the same type (normally associated with
-/// an RRset) that may be accompanied with RRSIGs in a memory efficient way.
-///
-/// The entire set of RDATA is stored in a packed form in a contiguous
-/// memory region.  It's opaque data, without containing non trivial
-/// data structures, so it can be located anywhere in the memory or even
-/// dumped to a file.
-///
-/// Two main classes are provided: one is
-/// \c isc::datasrc::memory::RdataEncoder, which allows
-/// the application to create encoded data for a set of RDATA;
-/// the other (TBD) provides an interface to iterate over encoded set of
-/// RDATA for purposes such as data lookups or rendering the data into the
-/// wire format to create a DNS message.
-///
-/// The actual encoding detail is private information to the implementation,
-/// and the application shouldn't assume anything about that except that
-/// each RDATA is considered to consist of one or more generic fields,
-/// and each field is typed as either opaque data or a domain name.
-/// A domain name field has additional attributes
-/// (see \c isc::datasrc::memory::RdataNameAttributes)
-/// so the application can change how the name should be handled in terms
-/// of the DNS protocol (e.g., whether it's subject to name compression).
-///
-/// The following are the current implementation of internal encoding, shown
-/// only for reference.  Applications must not assume this particular form
-/// for the encoded data; in fact, it can change in a future version of the
-/// implementation.
-/// \verbatim
-// The encoded data begin with a series of 16-bit length fields (values are
-// stored in the host byte order).  The sequence may be empty.
-// uint16_t n1_1: size of 1st variable len field (if any) of 1st RDATA
-// uint16_t n1_2: size of 2nd variable len field of 1st RDATA
-// ...
-// uint16_t nN_M: size of last (Mth) variable len field of last (Nth) RDATA
-// uint16_t ns1: size of 1st RRSIG (if any) data
-// ...
-// uint16_t nsL: size of last (Lth) RRSIG data
-// A sequence of packed data fields follows:
-// uint8_t[]: data field value, length specified by nI_J (in case it's
-//            variable-length) or by the per type field spec (in case it's
-//            fixed-length).
-// or
-// opaque data, LabelSequence::getSerializedLength() bytes: data for a name
-// uint8_t[ns1]: 1st RRSIG data
-// ...
-// uint8_t[nsL]: last RRSIG data
-// \endverbatim
-///
-/// As described above, this implementation treats RRSIGs as opaque data
-/// that don't contain any domain names.  Technically, it has a "signer"
-/// domain name field in the sense of RFC4034.  In practice, however, this
-/// field is essentially mere data; it's not subject to name compression,
-/// and since it's very likely to be a subdomain of (or equal to) the
-/// owner name of the corresponding RR (or, if used in a DNS message,
-/// some domain name that already appears before this field), so it won't
-/// be a target of name compression either.  By treating the entire RRSIG
-/// as single-field data we can make the implementation simpler, and probably
-/// make it faster in rendering it into a DNS message.
 
 namespace isc {
 namespace datasrc {

src/lib/datasrc/memory/rdata_field.h

@@ -20,9 +20,71 @@
 /// \file rdata_field.h
 ///
 /// This header should be considered private to the implementation and should
-/// not be used from outside.
+/// not be used included directly.
 ///
 /// It is used to share the definition of encoding for RRtypes.
+///
+/// These are types shared by classes in rdata_encoder.h and rdata_reader.h.
+/// The defyne a set of interfaces (classes, types, constants) to
+/// manipulate a given set of RDATA of the same type (normally associated with
+/// an RRset) that may be accompanied with RRSIGs in a memory efficient way.
+///
+/// The entire set of RDATA is stored in a packed form in a contiguous
+/// memory region.  It's opaque data, without containing non trivial
+/// data structures, so it can be located anywhere in the memory or even
+/// dumped to a file.
+///
+/// Two main classes are provided: one is
+/// \c isc::datasrc::memory::RdataEncoder, which allows
+/// the application to create encoded data for a set of RDATA;
+/// the isc::datasrc::memory::RdataReader provides an interface to iterate
+/// over encoded set of RDATA for purposes such as data lookups or rendering
+/// the data into the wire format to create a DNS message.
+///
+/// The actual encoding detail is private information to the implementation,
+/// and the application shouldn't assume anything about that except that
+/// each RDATA is considered to consist of one or more generic fields,
+/// and each field is typed as either opaque data or a domain name.
+/// A domain name field has additional attributes
+/// (see \c isc::datasrc::memory::RdataNameAttributes)
+/// so the application can change how the name should be handled in terms
+/// of the DNS protocol (e.g., whether it's subject to name compression).
+///
+/// The following are the current implementation of internal encoding, shown
+/// only for reference.  Applications must not assume this particular form
+/// for the encoded data; in fact, it can change in a future version of the
+/// implementation.
+/// \verbatim
+// The encoded data begin with a series of 16-bit length fields (values are
+// stored in the host byte order).  The sequence may be empty.
+// uint16_t n1_1: size of 1st variable len field (if any) of 1st RDATA
+// uint16_t n1_2: size of 2nd variable len field of 1st RDATA
+// ...
+// uint16_t nN_M: size of last (Mth) variable len field of last (Nth) RDATA
+// uint16_t ns1: size of 1st RRSIG (if any) data
+// ...
+// uint16_t nsL: size of last (Lth) RRSIG data
+// A sequence of packed data fields follows:
+// uint8_t[]: data field value, length specified by nI_J (in case it's
+//            variable-length) or by the per type field spec (in case it's
+//            fixed-length).
+// or
+// opaque data, LabelSequence::getSerializedLength() bytes: data for a name
+// uint8_t[ns1]: 1st RRSIG data
+// ...
+// uint8_t[nsL]: last RRSIG data
+// \endverbatim
+///
+/// As described above, this implementation treats RRSIGs as opaque data
+/// that don't contain any domain names.  Technically, it has a "signer"
+/// domain name field in the sense of RFC4034.  In practice, however, this
+/// field is essentially mere data; it's not subject to name compression,
+/// and since it's very likely to be a subdomain of (or equal to) the
+/// owner name of the corresponding RR (or, if used in a DNS message,
+/// some domain name that already appears before this field), so it won't
+/// be a target of name compression either.  By treating the entire RRSIG
+/// as single-field data we can make the implementation simpler, and probably
+/// make it faster in rendering it into a DNS message.
 
 namespace isc {
 namespace dns {

src/lib/datasrc/memory/rdata_reader.h

@@ -35,8 +35,30 @@ namespace memory {
 /// \brief Class to read serialized rdata
 ///
 /// This class allows you to read the data encoded by RDataEncoder.
-/// It is rather low-level -- it provides sequence of data fields
-/// and names. It does not give you convenient Rdata or RRset class.
+/// It is rather low-level -- it provides sequence of data fields.
+/// Each field is either opaque data, passed as a pointer and length,
+/// or a name, in the form of dns::LabelSequence (which is always
+/// absolute) and attributes.
+///
+/// Conceptually, these fields correspond to consecutive regions in
+/// wire-format representation of the RDATA, varying the type of above
+/// two cases depending on whether the region corresponds to a domain
+/// name or other data.  For example, for an MX RDATA the field
+/// sequence will be
+/// - 2 bytes of opaque data (which corresponds to the MX preference)
+/// - a domain name (which corresponds to the MX name)
+/// If the encoded data contain multiple MX RDATAs, the same type of
+/// sequence continues for the number of RDATAs.  Note that the opaque
+/// data field does not always correspond to a specific RDATA field
+/// as is the 2-byte preference field of MX.  For example, the field
+/// sequence for an SOA RDATA in terms of `RdataEncoder` will be:
+/// - a domain name (which corresponds to the SOA MNAME)
+/// - a domain name (which corresponds to the SOA RNAME)
+/// - 20 bytes of opaque data (for the rest of fields)
+///
+/// So, if you want to construct a general purpose dns::Rdata object
+/// from the field sequence, you'll need to build the complete
+/// wire-format data, and then construct a dns::Rdata object from it.
 ///
 /// To use it, contstruct it with the data you got from RDataEncoder,
 /// provide it with callbacks and then iterate through the data.
@@ -62,8 +84,8 @@ namespace memory {
 class RdataReader {
 public:
     /// \brief Function called on each name encountered in the data.
-    typedef boost::function<void(const dns::LabelSequence&, unsigned)>
-        NameAction;
+    typedef boost::function<void(const dns::LabelSequence&,
+                                 RdataNameAttributes)> NameAction;
     /// \brief Function called on each data field in the data.
     typedef boost::function<void(const uint8_t*, size_t)> DataAction;
 
@@ -96,7 +118,7 @@ public:
         RRSET_BOUNDARY  ///< At the end of the RRset (past the end)
     };
 
-    /// \brief Step to next piece of data.
+    /// \brief Step to next data field.
     ///
     /// Iterate over the next field and call appropriate hook (name_action
     /// or data_action, depending on the type) as passed to the constructor.
@@ -134,7 +156,7 @@ public:
             }
         }
     }
-    /// \brief Step to next piece of RRSig data.
+    /// \brief Step to next field of RRSig data.
     ///
     /// This is almost the same as next(), but it iterates through the
     /// associated RRSig data, not the data for the given RRType.
@@ -198,7 +220,7 @@ private:
     size_t data_pos_, spec_pos_, length_pos_;
     size_t sig_pos_, sig_data_pos_;
     Boundary nextInternal(const NameAction& name_action,
-                        const DataAction& data_action);
+                          const DataAction& data_action);
 };
 
 }

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

@@ -173,8 +173,8 @@ const Name dummy_name2("example.com");
 bool
 additionalRequired(const RRType& type) {
     // The set of RR types that require additional section processing.
-    // We'll pass it to renderNameField to check the stored attribute matches
-    // our expectation.
+    // We'll use it to determine what value should the renderNameField get
+    // and, if the stored attributes are as expected.
     static std::set<RRType> need_additionals;
     if (need_additionals.empty()) {
         need_additionals.insert(RRType::NS());
@@ -496,6 +496,13 @@ typedef ::testing::Types<ManualDecoderStyle,
                          HybridDecoder<false, true>,
                          HybridDecoder<false, false> >
     DecoderStyles;
+// Each decoder style must contain a decode() method. Such method is expected
+// to decode the passed data, first render the Rdata into the passed renderer,
+// then write the dummy_name2 there and write the RRSig data after that. It may
+// do other checks too.
+//
+// There are some slight differences to how to do the decoding, that's why we
+// have the typed test.
 TYPED_TEST_CASE(RdataEncodeDecodeTest, DecoderStyles);
 
 void