[2433] Merge remote-tracking branch 'origin/trac2432' into trac2433

src/lib/dns/rrset_collection.cc

@@ -15,6 +15,7 @@
 #include <dns/rrset_collection.h>
 #include <dns/master_loader_callbacks.h>
 #include <dns/master_loader.h>
+#include <dns/rrcollator.h>
 
 #include <exceptions/exceptions.h>
 
@@ -32,16 +33,6 @@ RRsetCollection::loaderCallback(const std::string&, size_t, const std::string&)
 }
 
 void
-RRsetCollection::addRRset(const Name& name, const RRClass& rrclass,
-                          const RRType& rrtype, const RRTTL& rrttl,
-                          const rdata::RdataPtr& data)
-{
-    RRsetPtr rrset(new BasicRRset(name, rrclass, rrtype, rrttl));
-    rrset->addRdata(data);
-    addRRset(rrset);
-}
-
-void
 RRsetCollection::addRRset(RRsetPtr rrset) {
     const CollectionKey key(rrset->getClass(), rrset->getType(),
                             rrset->getName());
@@ -55,41 +46,32 @@ RRsetCollection::addRRset(RRsetPtr rrset) {
     rrsets_.insert(std::pair<CollectionKey, RRsetPtr>(key, rrset));
 }
 
-RRsetCollection::RRsetCollection(const char* filename, const Name& origin,
-                                 const RRClass& rrclass)
+template<typename T>
+void
+RRsetCollection::constructHelper(T source, const isc::dns::Name& origin,
+                                 const isc::dns::RRClass& rrclass)
 {
+    RRCollator collator(boost::bind(&RRsetCollection::addRRset, this, _1));
     MasterLoaderCallbacks callbacks
         (boost::bind(&RRsetCollection::loaderCallback, this, _1, _2, _3),
          boost::bind(&RRsetCollection::loaderCallback, this, _1, _2, _3));
-    MasterLoader loader(filename, origin, rrclass, callbacks,
-                        boost::bind(&RRsetCollection::addRRset,
-                                    this, _1, _2, _3, _4, _5),
+    MasterLoader loader(source, origin, rrclass, callbacks,
+                        collator.getCallback(),
                         MasterLoader::DEFAULT);
     loader.load();
+    collator.flush();
 }
 
-RRsetCollection::RRsetCollection(std::istream& input_stream, const Name& origin,
+RRsetCollection::RRsetCollection(const char* filename, const Name& origin,
                                  const RRClass& rrclass)
 {
-    MasterLoaderCallbacks callbacks
-        (boost::bind(&RRsetCollection::loaderCallback, this, _1, _2, _3),
-         boost::bind(&RRsetCollection::loaderCallback, this, _1, _2, _3));
-    MasterLoader loader(input_stream, origin, rrclass, callbacks,
-                        boost::bind(&RRsetCollection::addRRset,
-                                    this, _1, _2, _3, _4, _5),
-                        MasterLoader::DEFAULT);
-    loader.load();
+    constructHelper<const char*>(filename, origin, rrclass);
 }
 
-const AbstractRRset*
-RRsetCollection::find(const Name& name, const RRType& rrtype,
-                      const RRClass& rrclass) const {
-    const CollectionKey key(rrclass, rrtype, name);
-    CollectionMap::const_iterator it = rrsets_.find(key);
-    if (it != rrsets_.end()) {
-        return (&(*it->second));
-    }
-    return (NULL);
+RRsetCollection::RRsetCollection(std::istream& input_stream, const Name& origin,
+                                 const RRClass& rrclass)
+{
+    constructHelper<std::istream&>(input_stream, origin, rrclass);
 }
 
 RRsetPtr
@@ -115,12 +97,19 @@ RRsetCollection::find(const Name& name, const RRClass& rrclass,
     return (ConstRRsetPtr());
 }
 
-void
+bool
 RRsetCollection::removeRRset(const Name& name, const RRClass& rrclass,
                              const RRType& rrtype)
 {
     const CollectionKey key(rrclass, rrtype, name);
-    rrsets_.erase(key);
+
+    CollectionMap::iterator it = rrsets_.find(key);
+    if (it == rrsets_.end()) {
+        return (false);
+    }
+
+    rrsets_.erase(it);
+    return (true);
 }
 
 RRsetCollectionBase::IterPtr

src/lib/dns/rrset_collection.h

@@ -31,6 +31,10 @@ namespace dns {
 class RRsetCollection : public RRsetCollectionBase {
 public:
     /// \brief Constructor.
+    ///
+    /// This constructor creates an empty collection without any data in
+    /// it. RRsets can be added to the collection with the \c addRRset()
+    /// method.
     RRsetCollection() {}
 
     /// \brief Constructor.
@@ -73,43 +77,49 @@ public:
     /// and managed by the \c RRsetCollection. It throws an
     /// \c isc::InvalidParameter exception if an rrset with the same
     /// class, type and name already exists.
+    ///
+    /// Callers must not modify the RRset after adding it to the
+    /// collection, as the rrset is indexed internally by the
+    /// collection.
     void addRRset(isc::dns::RRsetPtr rrset);
 
     /// \brief Remove an RRset from the collection.
     ///
     /// RRset(s) matching the \c name, \c rrclass and \c rrtype are
     /// removed from the collection.
-    void removeRRset(const isc::dns::Name& name,
+    ///
+    /// \returns \c true if a matching RRset was deleted, \c false if no
+    /// such RRset exists.
+    bool removeRRset(const isc::dns::Name& name,
                      const isc::dns::RRClass& rrclass,
                      const isc::dns::RRType& rrtype);
 
     /// \brief Find a matching RRset in the collection.
     ///
     /// Returns the RRset in the collection that exactly matches the
-    /// given \c name and \c rrtype.  If no matching RRset is found,
-    /// \c NULL is returned.
+    /// given \c name, \c rrclass and \c rrtype.  If no matching RRset
+    /// is found, \c NULL is returned.
     ///
     /// \param name The name of the RRset to search for.
-    /// \param rrtype The type of the RRset to search for.
     /// \param rrclass The class of the RRset to search for.
-    /// \returns A pointer to the RRset if found, \c NULL otherwise.
-    virtual const isc::dns::AbstractRRset* find
-        (const isc::dns::Name& name, const isc::dns::RRType& rrtype,
-         const isc::dns::RRClass& rrclass)
-        const;
+    /// \param rrtype The type of the RRset to search for.
+    /// \returns The RRset if found, \c NULL otherwise.
+    virtual isc::dns::ConstRRsetPtr find(const isc::dns::Name& name,
+                                         const isc::dns::RRClass& rrclass,
+                                         const isc::dns::RRType& rrtype) const;
 
+    /// \brief Find a matching RRset in the collection (non-const
+    /// variant).
+    ///
+    /// See above for a description of the method and arguments.
     isc::dns::RRsetPtr find(const isc::dns::Name& name,
                             const isc::dns::RRClass& rrclass,
                             const isc::dns::RRType& rrtype);
 
-    isc::dns::ConstRRsetPtr find(const isc::dns::Name& name,
-                                 const isc::dns::RRClass& rrclass,
-                                 const isc::dns::RRType& rrtype) const;
-
 private:
-    void addRRset(const isc::dns::Name& name, const isc::dns::RRClass& rrclass,
-                  const isc::dns::RRType& rrtype, const isc::dns::RRTTL& rrttl,
-                  const isc::dns::rdata::RdataPtr& data);
+    template<typename T>
+    void constructHelper(T source, const isc::dns::Name& origin,
+                         const isc::dns::RRClass& rrclass);
     void loaderCallback(const std::string&, size_t, const std::string&);
 
     typedef boost::tuple<isc::dns::RRClass, isc::dns::RRType, isc::dns::Name>

src/lib/dns/rrset_collection_base.h

@@ -41,16 +41,16 @@ public:
     /// \brief Find a matching RRset in the collection.
     ///
     /// Returns the RRset in the collection that exactly matches the
-    /// given \c name and \c rrtype.  If no matching RRset is found,
-    /// \c NULL is returned.
+    /// given \c name, \c rrclass and \c rrtype.  If no matching RRset
+    /// is found, \c NULL is returned.
     ///
     /// \param name The name of the RRset to search for.
     /// \param rrtype The type of the RRset to search for.
     /// \param rrclass The class of the RRset to search for.
-    /// \returns A pointer to the RRset if found, \c NULL otherwise.
-    virtual const isc::dns::AbstractRRset* find
-        (const isc::dns::Name& name, const isc::dns::RRType& rrtype,
-         const isc::dns::RRClass& rrclass)
+    /// \returns The RRset if found, \c NULL otherwise.
+    virtual isc::dns::ConstRRsetPtr find
+        (const isc::dns::Name& name, const isc::dns::RRClass& rrclass,
+         const isc::dns::RRType& rrtype)
         const = 0;
 
     /// \brief Destructor
@@ -105,11 +105,11 @@ public:
     ///
     /// It behaves like a \c std::iterator forward iterator, so please
     /// see its documentation for usage.
-    class iterator : std::iterator<std::forward_iterator_tag,
+    class Iterator : std::iterator<std::forward_iterator_tag,
                                    const isc::dns::AbstractRRset>
     {
     public:
-        explicit iterator(IterPtr iter) :
+        explicit Iterator(IterPtr iter) :
             iter_(iter)
         {}
 
@@ -117,22 +117,22 @@ public:
             return (iter_->getValue());
         }
 
-        iterator& operator++() {
+        Iterator& operator++() {
             iter_ = iter_->getNext();
             return (*this);
         }
 
-        iterator operator++(int) {
-            iterator tmp(iter_);
+        Iterator operator++(int) {
+            Iterator tmp(iter_);
             ++*this;
             return (tmp);
         }
 
-        bool operator==(const iterator& other) const {
+        bool operator==(const Iterator& other) const {
             return (iter_->equals(*other.iter_));
         }
 
-        bool operator!=(const iterator& other) const {
+        bool operator!=(const Iterator& other) const {
             return (!iter_->equals(*other.iter_));
         }
 
@@ -142,14 +142,14 @@ public:
 
     /// \brief Returns an iterator pointing to the beginning of the
     /// collection.
-    iterator begin() {
-      return iterator(getBeginning());
+    Iterator begin() {
+      return Iterator(getBeginning());
     }
 
     /// \brief Returns an iterator pointing past the end of the
     /// collection.
-    iterator end() {
-      return iterator(getEnd());
+    Iterator end() {
+      return Iterator(getEnd());
     }
 };
 

src/lib/dns/tests/rrset_collection_unittest.cc

@@ -44,8 +44,8 @@ TEST_F(RRsetCollectionTest, istreamConstructor) {
     std::ifstream fs(TEST_DATA_SRCDIR "/example.org");
     RRsetCollection collection2(fs, origin, rrclass);
 
-    RRsetCollectionBase::iterator iter = collection.begin();
-    RRsetCollectionBase::iterator iter2 = collection2.begin();
+    RRsetCollectionBase::Iterator iter = collection.begin();
+    RRsetCollectionBase::Iterator iter2 = collection2.begin();
     while (iter != collection.end()) {
          EXPECT_TRUE(iter2 != collection2.end());
          EXPECT_EQ((*iter).toText(), (*iter2).toText());
@@ -55,34 +55,6 @@ TEST_F(RRsetCollectionTest, istreamConstructor) {
     EXPECT_TRUE(iter2 == collection2.end());
 }
 
-TEST_F(RRsetCollectionTest, findBase) {
-    // Test the find() that returns isc::dns::AbstractRRset*
-    const AbstractRRset* rrset = collection.find(Name("www.example.org"),
-                                                 RRType::A(), rrclass);
-    EXPECT_NE(static_cast<AbstractRRset*>(NULL), rrset);
-    EXPECT_EQ(RRType::A(), rrset->getType());
-    EXPECT_EQ(RRTTL(3600), rrset->getTTL());
-    EXPECT_EQ(RRClass("IN"), rrset->getClass());
-    EXPECT_EQ(Name("www.example.org"), rrset->getName());
-
-    // foo.example.org doesn't exist
-    rrset = collection.find(Name("foo.example.org"), RRType::A(), rrclass);
-    EXPECT_EQ(static_cast<AbstractRRset*>(NULL), rrset);
-
-    // www.example.org exists, but not with MX
-    rrset = collection.find(Name("www.example.org"), RRType::MX(), rrclass);
-    EXPECT_EQ(static_cast<AbstractRRset*>(NULL), rrset);
-
-    // www.example.org exists, with AAAA
-    rrset = collection.find(Name("www.example.org"), RRType::AAAA(), rrclass);
-    EXPECT_NE(static_cast<AbstractRRset*>(NULL), rrset);
-
-    // www.example.org with AAAA does not exist in RRClass::CH()
-    rrset = collection.find(Name("www.example.org"), RRType::AAAA(),
-                            RRClass::CH());
-    EXPECT_EQ(static_cast<AbstractRRset*>(NULL), rrset);
-}
-
 template <typename T, typename TP>
 void doFind(T& collection, const RRClass& rrclass) {
     // Test the find() that returns ConstRRsetPtr
@@ -152,13 +124,20 @@ doAddAndRemove(RRsetCollection& collection, const RRClass& rrclass) {
         collection.addRRset(rrset);
     }, isc::InvalidParameter);
 
-    // Remove foo.example.org/A
-    collection.removeRRset(Name("foo.example.org"), rrclass, RRType::A());
+    // Remove foo.example.org/A, which should pass
+    bool exists = collection.removeRRset(Name("foo.example.org"),
+                                         rrclass, RRType::A());
+    EXPECT_TRUE(exists);
 
     // foo.example.org/A should not exist now
     rrset_found = collection.find(Name("foo.example.org"), rrclass,
                                   RRType::A());
     EXPECT_FALSE(rrset_found);
+
+    // Removing foo.example.org/A should fail now
+    exists = collection.removeRRset(Name("foo.example.org"),
+                                    rrclass, RRType::A());
+    EXPECT_FALSE(exists);
 }
 
 TEST_F(RRsetCollectionTest, addAndRemove) {
@@ -184,7 +163,7 @@ TEST_F(RRsetCollectionTest, iteratorTest) {
 
     // Here, we just count the records and do some basic tests on them.
     size_t count = 0;
-    for (RRsetCollection::iterator it = collection.begin();
+    for (RRsetCollection::Iterator it = collection.begin();
          it != collection.end(); ++it) {
          ++count;
          const AbstractRRset& rrset = *it;
@@ -204,12 +183,10 @@ public:
     MyRRsetCollection()
     {}
 
-    virtual const isc::dns::AbstractRRset* find
-        (const isc::dns::Name&, const isc::dns::RRType&,
-         const isc::dns::RRClass&)
-        const
-    {
-        return (NULL);
+    virtual isc::dns::ConstRRsetPtr find(const isc::dns::Name&,
+                                         const isc::dns::RRClass&,
+                                         const isc::dns::RRType&) const {
+        return (ConstRRsetPtr());
     }
 
     typedef std::list<isc::dns::RRset> MyCollection;