[2096] Remove the manual iteration interface from RdataReader

The return value of next() tells only what boundary it is now (between
Rdatas, in the middle of one rdata, or completely at the end).

As a side effect, it now can detect end of single rdata.

Part of the tests were dropped, as they make no sense. Others were
adapted. Some more will come.

Also, documentation was not yet updated.

src/lib/datasrc/memory/rdata_reader.cc

@@ -30,25 +30,6 @@ RdataReader::emptyDataAction(const uint8_t*, size_t) {
     // Do nothing here. On purpose, it's not unfinished.
 }
 
-RdataReader::Result::Result(const LabelSequence& label,
-                            unsigned attributes) :
-    label_(label),
-    data_(NULL),
-    size_(0),
-    type_(NAME),
-    compressible_((attributes & NAMEATTR_COMPRESSIBLE) != 0),
-    additional_((attributes & NAMEATTR_ADDITIONAL) != 0)
-{}
-
-RdataReader::Result::Result(const uint8_t* data, size_t size) :
-    label_(Name::ROOT_NAME()),
-    data_(data),
-    size_(size),
-    type_(DATA),
-    compressible_(false),
-    additional_(false)
-{}
-
 RdataReader::RdataReader(const RRClass& rrclass, const RRType& rrtype,
                          const uint8_t* data,
                          size_t rdata_count, size_t sig_count,
@@ -81,10 +62,11 @@ RdataReader::rewind() {
     sig_pos_ = 0;
 }
 
-RdataReader::Result
+RdataReader::Boundary
 RdataReader::nextInternal(const NameAction& name_action,
                           const DataAction& data_action)
 {
+
     if (spec_pos_ < spec_count_) {
         const RdataFieldSpec& spec(spec_.fields[(spec_pos_ ++) %
                                                 spec_.field_count]);
@@ -92,38 +74,39 @@ RdataReader::nextInternal(const NameAction& name_action,
             const LabelSequence sequence(data_ + data_pos_);
             data_pos_ += sequence.getSerializedLength();
             name_action(sequence, spec.name_attributes);
-            return (Result(sequence, spec.name_attributes));
         } else {
             const size_t length(spec.type == RdataFieldSpec::FIXEDLEN_DATA ?
                                 spec.fixeddata_len : lengths_[length_pos_ ++]);
-            Result result(data_ + data_pos_, length);
+            const uint8_t* const pos = data_ + data_pos_;
             data_pos_ += length;
-            data_action(result.data(), result.size());
-            return (result);
+            data_action(pos, length);
         }
+        return (spec_pos_ % spec_.field_count == 0 ?
+                RDATA_BOUNDARY : NO_BOUNDARY);
     } else {
         sigs_ = data_ + data_pos_;
-        return (Result());
+        return (RRSET_BOUNDARY);
     }
 }
 
-RdataReader::Result
+RdataReader::Boundary
 RdataReader::next() {
     return (nextInternal(name_action_, data_action_));
 }
 
-RdataReader::Result
+RdataReader::Boundary
 RdataReader::nextSig() {
     if (sig_pos_ < sig_count_) {
         if (sigs_ == NULL) {
             // We didn't find where the signatures start yet. We do it
             // by iterating the whole data and then returning the state
             // back.
-            size_t data_pos = data_pos_;
-            size_t spec_pos = spec_pos_;
-            size_t length_pos = length_pos_;
+            const size_t data_pos = data_pos_;
+            const size_t spec_pos = spec_pos_;
+            const size_t length_pos = length_pos_;
             // When the next() gets to the last item, it sets the sigs_
-            while (nextInternal(emptyNameAction, emptyDataAction)) {}
+            while (nextInternal(emptyNameAction, emptyDataAction) !=
+                   RRSET_BOUNDARY) {}
             assert(sigs_ != NULL);
             // Return the state
             data_pos_ = data_pos;
@@ -131,16 +114,16 @@ RdataReader::nextSig() {
             length_pos_ = length_pos;
         }
         // Extract the result
-        Result result(sigs_ + sig_data_pos_, lengths_[var_count_total_ +
-                      sig_pos_]);
+        const size_t length = lengths_[var_count_total_ + sig_pos_];
+        const uint8_t* const pos = sigs_ + sig_data_pos_;
         // Move the position of iterator.
         sig_data_pos_ += lengths_[var_count_total_ + sig_pos_];
         sig_pos_ ++;
         // Call the callback
-        data_action_(result.data(), result.size());
-        return (result);
+        data_action_(pos, length);
+        return (RDATA_BOUNDARY);
     } else {
-        return (Result());
+        return (RRSET_BOUNDARY);
     }
 }
 

src/lib/datasrc/memory/rdata_reader.h

@@ -120,125 +120,81 @@ public:
                 const NameAction& name_action = &emptyNameAction,
                 const DataAction& data_action = &emptyDataAction);
 
-    /// \brief The type of data returned from this iteration.
-    enum DataType {
-        NAME, ///< This iteration returns domain label
-        DATA, ///< This iteration returns unstructuder data
-        END   ///< No more data to return
-    };
-
-    /// \brief Data from one iteration
-    ///
-    /// Each time you call next() or nextSig(), it returns some data.
-    /// This holds the data.
-    ///
-    /// It is valid only for as long as the RdataReader that returned it.
-    ///
-    /// All the methods can be called under any circumstances. However,
-    /// if the required property is not valid for the given type (eg.
-    /// when calling size() on type() == NAME), it returns some "empty"
-    /// value (0, NULL, or the like).
-    class Result {
-    public:
-        /// \brief Default constructor
-        ///
-        /// It creates an empty result (with no data) of type END.
-        Result() :
-            // TODO: Do we maybe want to have a static one to copy
-            // instead of constructing new one from the root Name?
-            label_(dns::Name::ROOT_NAME()),
-            data_(NULL),
-            size_(0),
-            type_(END),
-            compressible_(false),
-            additional_(false)
-        {}
-        /// \brief Constructor from a domain label
-        ///
-        /// Creates the NAME type result. Used internally from RdataReader.
-        ///
-        /// \param label The label to hold
-        /// \param attributes The attributes, as stored by the serialized
-        ///     data.
-        Result(const dns::LabelSequence& label, unsigned attributes);
-        /// \brief Constructor from data
-        ///
-        /// Creates the DATA type result. Used internally from RdataReader.
-        ///
-        /// \param data The data pointer to hold.
-        /// \param size The size to hold.
-        Result(const uint8_t* data, size_t size);
-        /// \brief The type of data returned.
-        DataType type() const { return (type_); }
-        /// \brief The raw data.
-        ///
-        /// This is only valid if type() == DATA.
-        const uint8_t* data() const { return (data_); }
-        /// \brief The size of the raw data.
-        ///
-        /// This is the number of bytes the data takes. It is valid only
-        /// if type() == DATA.
-        size_t size() const { return (size_); }
-        /// \brief The domain label.
-        ///
-        /// This holds the domain label. It is only valid if type() == NAME.
-        const dns::LabelSequence& label() const { return (label_); }
-        /// \brief Is the name in label() compressible?
-        ///
-        /// This is valid only if type() == NAME.
-        bool compressible() const { return (compressible_); }
-        /// \brief Does the name expect additional processing?
-        ///
-        /// This is valid only if type() == NAME.
-        bool additional() const { return (additional_); }
-        /// \brief If there are data returned.
-        ///
-        /// This returns if there are any data at all returned. This is
-        /// equivalent to action != END, but it allows for more convenient
-        /// code of a loop through the data.
-        operator bool() const {
-            return (type() != END);
-        }
-    private:
-        dns::LabelSequence label_;
-        const uint8_t* data_;
-        size_t size_;
-        DataType type_;
-        bool compressible_;
-        bool additional_;
+    /// \brief Result of next() and nextSig()
+    ///
+    /// This specifies if there's any boundary in the data at the
+    /// place where the corresponding call to next() or nextSig()
+    /// finished.
+    enum Boundary {
+        NO_BOUNDARY,    ///< It is in the middle of Rdata
+        RDATA_BOUNDARY, ///< At the end of single Rdata
+        RRSET_BOUNDARY  ///< At the end of the RRset (past the end)
     };
 
     /// \brief Step to next piece of data.
     ///
-    /// This returns the next available data. Also, the apropriate hook
-    /// (name_action or data_action, depending on the data type) as passed
-    /// to the constructor is called.
-    ///
-    /// If there are no more data, a Result with type END is returned and
-    /// no callback is called.
-    Result next();
+    /// Iterate over the next field and call appropriate hook (name_action
+    /// or data_action, depending on the type) as passed to the constructor.
+    ///
+    /// \return It returns NO_BOUNDARY if the next call to next() will process
+    ///     data of the same rdata as this one. RDATA_BOUNDARY is returned when
+    ///     this field is the last of the current rdata. If there are no more
+    ///     data to process, no hook is called and RRSET_BOUNDARY is returned.
+    ///     Therefore, at the end of the whole data, once it processes the last
+    ///     field and returns RDATA_BOUNDARY and then it returns RRSET_BOUNDARY
+    ///     on the next call.
+    Boundary next();
     /// \brief Call next() until the end.
     ///
     /// This is just convenience method to iterate through all the data.
     /// It calls next until it reaches the end (it does not revind before,
     /// therefore if you already called next() yourself, it does not start
     /// at the beginning).
-    ///
-    /// The method only makes sense if you set the callbacks in constructor.
     void iterate() {
-        while (next()) { }
+        while (next() != RRSET_BOUNDARY) { }
+    }
+    /// \brief Call next() until the end of current rdata.
+    ///
+    /// This is a convenience method to iterate until the end of current
+    /// rdata. Notice this may cause more than one field being processed,
+    /// as some rrtypes are more complex.
+    ///
+    /// \return If there was Rdata to iterate through.
+    bool iterateRdata() {
+        while (true) {
+            switch(next()) {
+                case NO_BOUNDARY: break;
+                case RDATA_BOUNDARY: return (true);
+                case RRSET_BOUNDARY: return (false);
+            }
+        }
     }
     /// \brief Step to next piece 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.
-    Result nextSig();
+    Boundary nextSig();
     /// \brief Iterate through all RRSig data.
     ///
     /// This is almost the same as iterate(), but it iterates through the
     /// RRSig data instead.
-    void iterateSig() {
-        while (nextSig()) { }
+    void iterateAllSigs() {
+        while (nextSig() != RRSET_BOUNDARY) { }
+    }
+    /// \brief Iterate through the current RRSig Rdata.
+    ///
+    /// This is almote the same as iterateRdata, except it is for single
+    /// signature Rdata.
+    ///
+    /// In practice, this should process one DATA field.
+    bool iterateSingleSig() {
+        while (true) {
+            switch(next()) {
+                case NO_BOUNDARY: break;
+                case RDATA_BOUNDARY: return (true);
+                case RRSET_BOUNDARY: return (false);
+            }
+        }
     }
     /// \brief Rewind the iterator to the beginnig of data.
     ///
@@ -276,7 +232,7 @@ private:
     // The positions in data.
     size_t data_pos_, spec_pos_, length_pos_;
     size_t sig_pos_, sig_data_pos_;
-    Result nextInternal(const NameAction& name_action,
+    Boundary nextInternal(const NameAction& name_action,
                         const DataAction& data_action);
 };
 

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

@@ -313,89 +313,6 @@ public:
     }
 };
 
-// Decode using reader with the return value of next
-class NextDecoder {
-public:
-    static void decode(const isc::dns::RRClass& rrclass,
-                       const isc::dns::RRType& rrtype,
-                       size_t rdata_count, size_t sig_count, size_t,
-                       const vector<uint8_t>& encoded_data, size_t,
-                       MessageRenderer& renderer)
-    {
-        RdataReader reader(rrclass, rrtype, &encoded_data[0], rdata_count,
-                           sig_count);
-        RdataReader::Result field;
-        while ((field = reader.next())) {
-            switch (field.type()) {
-                case RdataReader::DATA:
-                    renderer.writeData(field.data(), field.size());
-                    break;
-                case RdataReader::NAME:
-                    renderer.writeName(field.label(), field.compressible());
-                    break;
-                default:
-                    FAIL();
-            }
-        }
-
-        renderer.writeName(dummy_name2);
-
-        while ((field = reader.nextSig())) {
-            switch (field.type()) {
-                case RdataReader::DATA:
-                    renderer.writeData(field.data(), field.size());
-                    break;
-                default: // There are also no NAME fields in RRSigs
-                    FAIL();
-            }
-        }
-    }
-};
-
-// Decode using reader with the return value of next, but after the reader
-// was used and rewund.
-class RewundDecoder {
-public:
-    static void decode(const isc::dns::RRClass& rrclass,
-                       const isc::dns::RRType& rrtype,
-                       size_t rdata_count, size_t sig_count, size_t,
-                       const vector<uint8_t>& encoded_data, size_t,
-                       MessageRenderer& renderer)
-    {
-        RdataReader reader(rrclass, rrtype, &encoded_data[0], rdata_count,
-                           sig_count);
-        // Use the reader first and rewind it
-        reader.iterateSig();
-        reader.iterate();
-        reader.rewind();
-        RdataReader::Result field;
-        while ((field = reader.next())) {
-            switch (field.type()) {
-                case RdataReader::DATA:
-                    renderer.writeData(field.data(), field.size());
-                    break;
-                case RdataReader::NAME:
-                    renderer.writeName(field.label(), field.compressible());
-                    break;
-                default:
-                    FAIL();
-            }
-        }
-
-        renderer.writeName(dummy_name2);
-
-        while ((field = reader.nextSig())) {
-            switch (field.type()) {
-                case RdataReader::DATA:
-                    renderer.writeData(field.data(), field.size());
-                    break;
-                default: // There are also no NAME fields in RRSigs
-                    FAIL();
-            }
-        }
-    }
-};
-
 // Check using callbacks and calling next until the end.
 class CallbackDecoder {
 public:
@@ -410,9 +327,9 @@ public:
                            boost::bind(renderNameField, &renderer,
                                        additionalRequired(rrtype), _1, _2),
                            boost::bind(renderDataField, &renderer, _1, _2));
-        while (reader.next()) { }
+        while (reader.next() != RdataReader::RRSET_BOUNDARY) { }
         renderer.writeName(dummy_name2);
-        while (reader.nextSig()) { }
+        while (reader.nextSig() != RdataReader::RRSET_BOUNDARY) { }
     }
 };
 
@@ -432,7 +349,7 @@ public:
                            boost::bind(renderDataField, &renderer, _1, _2));
         reader.iterate();
         renderer.writeName(dummy_name2);
-        reader.iterateSig();
+        reader.iterateAllSigs();
     }
 };
 
@@ -485,14 +402,14 @@ public:
         // Now, we let all sigs to be copied to data. We disable the
         // renderer for this.
         current = NULL;
-        reader.iterateSig();
+        reader.iterateAllSigs();
         // Now return the renderer and render the rest of the data
         current = &renderer;
         reader.iterate();
         // Now, this should not break anything and should be valid, but should
         // return ends.
-        EXPECT_FALSE(reader.next());
-        EXPECT_FALSE(reader.nextSig());
+        EXPECT_EQ(RdataReader::RRSET_BOUNDARY, reader.next());
+        EXPECT_EQ(RdataReader::RRSET_BOUNDARY, reader.nextSig());
         // Render the name and the sigs
         renderer.writeName(dummy_name2);
         renderer.writeData(&data[0], data.size());
@@ -501,7 +418,7 @@ public:
     }
 };
 
-typedef ::testing::Types<ManualDecoderStyle, NextDecoder, RewundDecoder,
+typedef ::testing::Types<ManualDecoderStyle,
                          CallbackDecoder, IterateDecoder,
                          HybridDecoder<true, true>, HybridDecoder<true, false>,
                          HybridDecoder<false, true>,
@@ -795,45 +712,4 @@ TEST_F(RdataSerializationTest, badAddSIGRdata) {
     EXPECT_THROW(encoder_.addSIGRdata(big_sigrdata), RdataEncodingError);
 }
 
-// Test the result returns what it was constructed with.
-TEST_F(RdataSerializationTest, readerResult) {
-    // Default constructor
-    RdataReader::Result empty;
-    // Everything should be at the "empty" values, type END
-    EXPECT_EQ(RdataReader::END, empty.type());
-    EXPECT_EQ(NULL, empty.data());
-    EXPECT_EQ(0, empty.size());
-    EXPECT_TRUE(empty.label().equals(LabelSequence(Name::ROOT_NAME())));
-    EXPECT_FALSE(empty);
-    EXPECT_FALSE(empty.compressible());
-    EXPECT_FALSE(empty.additional());
-    // Constructor from label sequence
-    LabelSequence seq(Name("example.org"));
-    RdataReader::Result compressible(seq, NAMEATTR_COMPRESSIBLE);
-    EXPECT_EQ(RdataReader::NAME, compressible.type());
-    EXPECT_EQ(NULL, compressible.data());
-    EXPECT_EQ(0, compressible.size());
-    EXPECT_TRUE(compressible.label().equals(seq));
-    EXPECT_TRUE(compressible);
-    EXPECT_TRUE(compressible.compressible());
-    EXPECT_FALSE(compressible.additional());
-    RdataReader::Result incompressible(seq, NAMEATTR_ADDITIONAL);
-    EXPECT_EQ(RdataReader::NAME, incompressible.type());
-    EXPECT_EQ(NULL, incompressible.data());
-    EXPECT_EQ(0, incompressible.size());
-    EXPECT_TRUE(incompressible.label().equals(seq));
-    EXPECT_TRUE(incompressible);
-    EXPECT_FALSE(incompressible.compressible());
-    EXPECT_TRUE(incompressible.additional());
-    // Constructor from data
-    uint8_t byte;
-    RdataReader::Result data(&byte, 1);
-    EXPECT_EQ(RdataReader::DATA, data.type());
-    EXPECT_EQ(&byte, data.data());
-    EXPECT_EQ(1, data.size());
-    EXPECT_TRUE(data.label().equals(LabelSequence(Name::ROOT_NAME())));
-    EXPECT_TRUE(data);
-    EXPECT_FALSE(data.compressible());
-    EXPECT_FALSE(data.additional());
-}
 }