NameserverEntry provides NameserverAddress

And the NameserverAddress is made safe with regard to changing indices
inside the NameserverEntry.

git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac408@3804 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/nsas/Makefile.am

@@ -15,7 +15,7 @@ libnsas_la_SOURCES += hash_key.cc hash_key.h
 libnsas_la_SOURCES += hash_table.h
 libnsas_la_SOURCES += lru_list.h
 libnsas_la_SOURCES += nameserver_address_store.cc nameserver_address_store.h
-libnsas_la_SOURCES += nameserver_address.h
+libnsas_la_SOURCES += nameserver_address.h nameserver_address.cc
 libnsas_la_SOURCES += nameserver_entry.cc nameserver_entry.h
 libnsas_la_SOURCES += nsas_entry_compare.h
 libnsas_la_SOURCES += nsas_entry.h nsas_types.h

src/lib/nsas/nameserver_address.cc

@@ -0,0 +1,30 @@
+// Copyright (C) 2010  CZ NIC
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+// $id$
+
+#include "nameserver_address.h"
+#include "nameserver_entry.h"
+
+namespace isc {
+namespace nsas {
+
+void
+NameserverAddress::updateRTT(uint32_t rtt) const {
+    // We delegate it to the address entry inside the nameserver entry
+    ns_->updateAddressRTT(rtt, address_.getAddress(), family_);
+}
+
+} // namespace nsas
+} // namespace isc

src/lib/nsas/nameserver_address.h

@@ -19,12 +19,18 @@
 
 #include <boost/shared_ptr.hpp>
 
+#include <exceptions/exceptions.h>
+
 #include "asiolink.h"
-#include "nameserver_entry.h"
+#include "address_entry.h"
+#include "nsas_types.h"
 
 namespace isc {
 namespace nsas {
 
+class ZoneEntry;
+class NameserverEntry;
+
 /// \brief Empty \c NameserverEntry pointer exception
 ///
 /// Thrown if the the \c NameservrEntry pointer in the \c boost::shared_ptr that passed
@@ -40,17 +46,20 @@ public:
 /// \brief Nameserver Address
 ///
 /// This class implements the object that returned from NSAS when the resolver
-/// request an address for the name server. It contains one IOAddress object
+/// request an address for the name server. It contains one address
 /// that can be used by resolver. When the resolver get query back from the name
-/// server, it should update the name server's RTT(Round Trip Time) with this 
+/// server, it should update the name server's RTT(Round Trip Time) with this
 /// object.
+///
+/// It is not thread safe, only reentrant. It is expected to be kept inside
+/// the resolver and used only once for the address and once for the update.
 
 class NameserverAddress {
 public:
     /// \brief Constructor
     ///
     /// The NameserverAddress object will contain one shared_ptr object that
-    /// pointed to NameserverEntry which contains the address as well as it's 
+    /// pointed to NameserverEntry which contains the address as well as it's
     /// corresponding index. The user can update it's RTT with the index later.
     ///
     /// \param namerserver A shared_ptr that points to a NameserverEntry object
@@ -59,32 +68,21 @@ public:
     /// \param index The address's index in NameserverEntry's addresses vector
     /// \param family Address family, V4_ONLY or V6_ONLY
     NameserverAddress(const boost::shared_ptr<NameserverEntry>& nameserver,
-        size_t index, AddressFamily family):
-        ns_(nameserver), index_(index), family_(family)
+        const AddressEntry& address, AddressFamily family):
+        ns_(nameserver), address_(address), family_(family)
     {
-        if(!ns_.get()) {
+        if(!ns_) {
             isc_throw(NullNameserverEntryPointer, "NULL NameserverEntry pointer.");
         }
     }
 
     /// \brief Default Constructor
-    ///
-    /// \todo Is it needed? This one seems to make no sense.
-    NameserverAddress(): index_(0), family_(V4_ONLY)
-    {
-    }
-
-    /// \brief Destructor
-    ///
-    /// Empty destructor.
-    ~NameserverAddress()
-    {
-    }
+    NameserverAddress() : address_(asiolink::IOAddress("::1")) { }
 
     /// \brief Return address
     ///
     asiolink::IOAddress getAddress() const {
-        return ns_.get()->getAddressAtIndex(index_, family_);
+        return (address_.getAddress());
     }
 
     /// \brief Update Round-trip Time
@@ -92,13 +90,26 @@ public:
     /// When the user get one request back from the name server, it should
     /// update the address's RTT.
     /// \param rtt The new Round-Trip Time
-    void updateRTT(uint32_t rtt) {
-        ns_.get()->updateAddressRTTAtIndex(rtt, index_, family_);
+    void updateRTT(uint32_t rtt) const;
+
+    /// Short access to the AddressEntry inside.
+    //@{
+    const AddressEntry& getAddressEntry() const {
+        return (address_);
+    }
+    AddressEntry& getAddressEntry() {
+        return (address_);
     }
+    //@}
 private:
 
+    /*
+     * Note: Previous implementation used index into the entry. That is wrong,
+     * as the list of addresses may change. Thil would cause setting a
+     * different address or a crash.
+     */
     boost::shared_ptr<NameserverEntry> ns_;  ///< Shared-pointer to NameserverEntry object
-    size_t index_;                           ///< The address index in NameserverEntry
+    AddressEntry address_;            ///< The address
     AddressFamily family_;                   ///< The address family (V4_ONLY or V6_ONLY)
 };
 

src/lib/nsas/nameserver_entry.cc

@@ -49,7 +49,7 @@ namespace nsas {
 namespace {
 
 // Just shorter type alias
-typedef mutex::scoped_lock Lock;
+typedef recursive_mutex::scoped_lock Lock;
 
 }
 
@@ -100,15 +100,19 @@ NameserverEntry::getAddresses(AddressVector& addresses,
             return (getState());
     }
 
+    shared_ptr<NameserverEntry> self(shared_from_this());
     // If any address is OK, just pass everything we have
     if (family == ANY_OK) {
-        addresses.insert(addresses.end(), addresses_[V4_ONLY].begin(),
-            addresses_[V4_ONLY].end());
-        addresses.insert(addresses.end(), addresses_[V6_ONLY].begin(),
-            addresses_[V6_ONLY].end());
+        BOOST_FOREACH(const AddressEntry& entry, addresses_[V6_ONLY]) {
+            addresses.push_back(NameserverAddress(self, entry, V6_ONLY));
+        }
+        BOOST_FOREACH(const AddressEntry& entry, addresses_[V4_ONLY]) {
+            addresses.push_back(NameserverAddress(self, entry, V4_ONLY));
+        }
     } else {
-        addresses.insert(addresses.end(), addresses_[family].begin(),
-            addresses_[family].end());
+        BOOST_FOREACH(const AddressEntry& entry, addresses_[family]) {
+            addresses.push_back(NameserverAddress(self, entry, family));
+        }
     }
     if (getState() == EXPIRED && expired_ok) {
         return READY;
@@ -125,7 +129,7 @@ NameserverEntry::getAddress(NameserverAddress& address, AddressFamily family) {
         return (false);
     } else {
         address = NameserverAddress(shared_from_this(),
-            address_selectors_[family](), family);
+            addresses_[family][address_selectors_[family]()], family);
         return (true);
     }
 }
@@ -188,6 +192,19 @@ NameserverEntry::updateAddressRTTAtIndex(uint32_t rtt, size_t index,
     updateAddressSelector(addresses_[family], address_selectors_[family]);
 }
 
+void
+NameserverEntry::updateAddressRTT(uint32_t rtt,
+    const asiolink::IOAddress& address, AddressFamily family)
+{
+    Lock lock(mutex_);
+    for (size_t i(0); i < addresses_[family].size(); ++ i) {
+        if (addresses_[family][i].getAddress().equal(address)) {
+            updateAddressRTTAtIndex(rtt, i, family);
+            return;
+        }
+    }
+}
+
 // Sets the address to be unreachable
 void
 NameserverEntry::setAddressUnreachable(const IOAddress& address) {

src/lib/nsas/nameserver_entry.h

@@ -35,6 +35,7 @@
 #include "resolver_interface.h"
 #include "nsas_entry.h"
 #include "random_number_generator.h"
+#include "nameserver_address.h"
 
 namespace isc {
 namespace nsas {
@@ -82,13 +83,6 @@ class ResolverInterface;
 /// for several zones (hence is pointed to by more than one zone entry), and
 /// may have several addresses associated with it.
 ///
-/// When created, zero or more addresses may be given.  At any time, the list
-/// of addresses may be updated.  This may occur (a) after creation, either to
-/// to get the list of addresses when none have been supplied or to replace
-/// glue records, or (b) when the object has been accessed but found to be
-/// expired (the address records have reached their TTL).
-/// TODO: Add code for update of addresses
-///
 /// The addresses expire after their TTL has been reached.  For simplicity,
 /// (and because it is unlikely that A and AAAA records from the same zone have
 /// different TTLs) there is one expiration time for all address records.
@@ -103,7 +97,7 @@ class ResolverInterface;
 class NameserverEntry : public NsasEntry<NameserverEntry>, public Fetchable {
 public:
     /// List of addresses associated with this nameserver
-    typedef std::vector<AddressEntry>   AddressVector;
+    typedef std::vector<NameserverAddress>   AddressVector;
     typedef AddressVector::iterator     AddressVectorIterator;
 
     /// \brief Constructor where no A records are supplied.
@@ -139,8 +133,7 @@ public:
      *     arrived and is is returned, but the other is still on the way).
      * \todo Should we sort out unreachable addresses as well?
      */
-    Fetchable::State getAddresses(
-        NameserverEntry::AddressVector& addresses,
+    Fetchable::State getAddresses(AddressVector& addresses,
         AddressFamily family = ANY_OK, bool expired_ok = false);
 
     /// \brief Return one address
@@ -169,11 +162,26 @@ public:
 
     /// \brief Update RTT of the address that corresponding to the index
     ///
+    /// Shouldn't probably be used directly. Use corresponding
+    /// NameserverAddress.
     /// \param rtt Round-Trip Time
     /// \param index The address's index in address vector
     /// \param family The address family, V4_ONLY or V6_ONLY
     void updateAddressRTTAtIndex(uint32_t rtt, size_t index,
         AddressFamily family);
+    /**
+     * \short Update RTT of an address.
+     *
+     * This is similar to updateAddressRTTAtIndex, but you pass the address,
+     * not it's index. Passing the index might be unsafe, because the position
+     * of the address or the cound of addresses may change in time.
+     *
+     * \param rtt Round-Trip Time
+     * \param address The address whose RTT should be updated.
+     * \param family The address family, V4_ONLY or V6_ONLY
+     */
+    void updateAddressRTT(uint32_t rtt, const asiolink::IOAddress& address,
+        AddressFamily family);
 
     /// \brief Set Address Unreachable
     ///
@@ -244,8 +252,7 @@ public:
     //@}
 
 private:
-    // TODO Read-write lock?
-    mutable boost::mutex    mutex_;     ///< Mutex protecting this object
+    mutable boost::recursive_mutex    mutex_;     ///< Mutex protecting this object
     std::string     name_;              ///< Canonical name of the nameserver
     isc::dns::RRClass classCode_;       ///< Class of the nameserver
     /**

src/lib/nsas/random_number_generator.h

@@ -83,7 +83,7 @@ public:
         // Init with the current time
         rng_.seed(time(NULL));
     }
-    
+
     /// \brief Default constructor
     ///
     WeightedRandomIntegerGenerator():
@@ -122,11 +122,6 @@ public:
             - cumulative_.begin() + min_;
     }
 
-    /// \brief Destroctor
-    ~WeightedRandomIntegerGenerator()
-    {
-    }
-
 private:
     /// \brief Check the validation of probabilities vector
     ///

src/lib/nsas/tests/nameserver_address_unittest.cc

@@ -23,6 +23,7 @@
 #include <dns/rrttl.h>
 
 #include "../nameserver_address.h"
+#include "../nameserver_entry.h"
 #include "nsas_test.h"
 
 namespace isc {
@@ -68,7 +69,7 @@ public:
     uint32_t getAddressRTTAtIndex(uint32_t index) { 
         NameserverEntry::AddressVector addresses;
         ns_.get()->getAddresses(addresses);
-        return addresses[index].getRTT();
+        return (addresses[index].getAddressEntry().getRTT());
     }
 
 private:
@@ -87,31 +88,26 @@ class NameserverAddressTest : public ::testing::Test {
 protected:
     // Constructor
     NameserverAddressTest(): 
-        ns_address_(ns_sample_.getNameserverEntry(), TEST_ADDRESS_INDEX,
-            V4_ONLY),
-        invalid_ns_address_(ns_sample_.getNameserverEntry(),
-            ns_sample_.getAddressesCount(), V4_ONLY)
+        ns_address_(ns_sample_.getNameserverEntry(),
+            ns_sample_.getNameserverEntry()->getAddressAtIndex(
+            TEST_ADDRESS_INDEX, V4_ONLY), V4_ONLY)
     {
     }
 
     NameserverEntrySample ns_sample_;
     // Valid NameserverAddress object
     NameserverAddress ns_address_;
-
-    // NameserverAddress object that constructed with invalid index
-    NameserverAddress invalid_ns_address_;
 };
 
 // Test that the address is equal to the address in NameserverEntry
 TEST_F(NameserverAddressTest, Address) {
     EXPECT_TRUE(ns_address_.getAddress().equal( ns_sample_.getAddressAtIndex(TEST_ADDRESS_INDEX)));
 
-    // It will trigger an assert with the invalid index
-    ASSERT_DEATH(invalid_ns_address_.getAddress(), "");
-
     boost::shared_ptr<NameserverEntry> empty_ne((NameserverEntry*)NULL);
     // It will throw an NullNameserverEntryPointer exception with the empty NameserverEntry shared pointer
-    ASSERT_THROW({NameserverAddress empty_ns_address(empty_ne, 0, V4_ONLY);}, NullNameserverEntryPointer);
+    ASSERT_THROW({NameserverAddress empty_ns_address(empty_ne,
+        asiolink::IOAddress("127.0.0.1"), V4_ONLY);},
+        NullNameserverEntryPointer);
 }
 
 // Test that the RTT is updated

src/lib/nsas/tests/nameserver_entry_unittest.cc

@@ -122,10 +122,10 @@ TEST_F(NameserverEntryTest, InitialRTT) {
 
     // Check they are not 0 and they are all small, they should be some kind
     // of randomish numbers, so we can't expect much more here
-    BOOST_FOREACH(AddressEntry& entry, vec) {
-        EXPECT_GT(entry.getRTT(), 0);
+    BOOST_FOREACH(NameserverAddress& entry, vec) {
+        EXPECT_GT(entry.getAddressEntry().getRTT(), 0);
         // 20 is some arbitrary small value
-        EXPECT_LT(entry.getRTT(), 20);
+        EXPECT_LT(entry.getAddressEntry().getRTT(), 20);
     }
 }
 
@@ -143,7 +143,7 @@ TEST_F(NameserverEntryTest, SetRTT) {
 
     // Take the first address and change the RTT.
     IOAddress first_address = vec[0].getAddress();
-    uint32_t first_rtt = vec[0].getRTT();
+    uint32_t first_rtt = vec[0].getAddressEntry().getRTT();
     uint32_t new_rtt = first_rtt + 42;
     alpha->setAddressRTT(first_address, new_rtt);
 
@@ -156,7 +156,7 @@ TEST_F(NameserverEntryTest, SetRTT) {
         i != newvec.end(); ++i) {
         if (i->getAddress().equal(first_address)) {
             ++matchcount;
-            EXPECT_EQ(i->getRTT(), new_rtt);
+            EXPECT_EQ(i->getAddressEntry().getRTT(), new_rtt);
         }
     }
 
@@ -178,7 +178,7 @@ TEST_F(NameserverEntryTest, Unreachable) {
 
     // Take the first address and mark as unreachable.
     IOAddress first_address = vec[0].getAddress();
-    EXPECT_FALSE(vec[0].isUnreachable());
+    EXPECT_FALSE(vec[0].getAddressEntry().isUnreachable());
 
     alpha->setAddressUnreachable(first_address);
 
@@ -191,7 +191,7 @@ TEST_F(NameserverEntryTest, Unreachable) {
         i != newvec.end(); ++i) {
         if (i->getAddress().equal(first_address)) {
             ++matchcount;
-            EXPECT_TRUE(i->isUnreachable());
+            EXPECT_TRUE(i->getAddressEntry().isUnreachable());
         }
     }
 
@@ -450,7 +450,7 @@ TEST_F(NameserverEntryTest, KeepRTT) {
     EXPECT_EQ(Fetchable::READY, entry->getAddresses(addresses, V6_ONLY, true));
     ASSERT_EQ(2, addresses.size());
     EXPECT_EQ("2001:db8::1", addresses[1].getAddress().toText());
-    BOOST_FOREACH(const AddressEntry& address, addresses) {
+    BOOST_FOREACH(const NameserverAddress& address, addresses) {
         entry->setAddressRTT(address.getAddress(), 123);
     }
 
@@ -475,8 +475,8 @@ TEST_F(NameserverEntryTest, KeepRTT) {
     ASSERT_EQ(2, addresses.size());
     EXPECT_EQ("2001:db8::1", addresses[1].getAddress().toText());
     // They should have the RTT we set to them
-    BOOST_FOREACH(AddressEntry& address, addresses) {
-        EXPECT_EQ(123, address.getRTT());
+    BOOST_FOREACH(NameserverAddress& address, addresses) {
+        EXPECT_EQ(123, address.getAddressEntry().getRTT());
     }
 }
 
@@ -574,22 +574,22 @@ TEST_F(NameserverEntryTest, UpdateRTT) {
     uint32_t stable_rtt = 100;
 
     // Update the rtt
-    ns->updateAddressRTTAtIndex(stable_rtt, 0, V4_ONLY);
+    vec[0].updateRTT(stable_rtt);
 
     vec.clear();
     ns->getAddresses(vec);
-    uint32_t new_rtt = vec[0].getRTT();
+    uint32_t new_rtt = vec[0].getAddressEntry().getRTT();
 
     // The rtt should not close to new rtt immediately
     EXPECT_TRUE((stable_rtt - new_rtt) > (new_rtt - init_rtt));
 
     // Update the rtt for enough times
     for(int i = 0; i < 10000; ++i){
-        ns->updateAddressRTTAtIndex(stable_rtt, 0, V4_ONLY);
+        vec[0].updateRTT(stable_rtt);
     }
     vec.clear();
     ns->getAddresses(vec);
-    new_rtt = vec[0].getRTT();
+    new_rtt = vec[0].getAddressEntry().getRTT();
 
     // The rtt should be close to stable rtt value
     EXPECT_TRUE((stable_rtt - new_rtt) < (new_rtt - init_rtt));