Type update, unification of arrays

short -> AddressFamily
uint32_t for indices to size_t

The address_, v4_addresses_ and v6_addresses_ arrays were unified

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

src/lib/nsas/nameserver_address.h

@@ -31,7 +31,8 @@ namespace nsas {
 /// into \c NameserverAddress' constructor is NULL
 class NullNameserverEntryPointer : public isc::Exception {
 public:
-    NullNameserverEntryPointer(const char* file, size_t line, const char* what) :
+    NullNameserverEntryPointer(const char* file, size_t line,
+        const char* what) :
         isc::Exception(file, line, what)
     {}
 };
@@ -56,8 +57,9 @@ public:
     /// the shared_ptr can avoid the NameserverEntry object being dropped while the
     /// request is processing.
     /// \param index The address's index in NameserverEntry's addresses vector
-    /// \param family Address family, AF_INET or AF_INET6
-    NameserverAddress(boost::shared_ptr<NameserverEntry>& nameserver, uint32_t index, short family):
+    /// \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)
     {
         if(!ns_.get()) {
@@ -67,7 +69,8 @@ public:
 
     /// \brief Default Constructor
     ///
-    NameserverAddress(): index_(0), family_(AF_INET)
+    /// \todo Is it needed? This one seems to make no sense.
+    NameserverAddress(): index_(0), family_(V4_ONLY)
     {
     }
 
@@ -80,8 +83,8 @@ public:
 
     /// \brief Return address
     ///
-    asiolink::IOAddress getAddress() const { 
-        return ns_.get()->getAddressAtIndex(index_, family_); 
+    asiolink::IOAddress getAddress() const {
+        return ns_.get()->getAddressAtIndex(index_, family_);
     }
 
     /// \brief Update Round-trip Time
@@ -89,14 +92,14 @@ 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) {
+        ns_.get()->updateAddressRTTAtIndex(rtt, index_, family_);
     }
 private:
 
     boost::shared_ptr<NameserverEntry> ns_;  ///< Shared-pointer to NameserverEntry object
-    uint32_t index_;                         ///< The address index in NameserverEntry
-    short family_;                           ///< Address family AF_INET or AF_INET6
+    size_t index_;                           ///< The address index in NameserverEntry
+    AddressFamily family_;                   ///< The address family (V4_ONLY or V6_ONLY)
 };
 
 } // namespace nsas

src/lib/nsas/nameserver_entry.cc

@@ -79,9 +79,6 @@ NameserverEntry::getAddresses(AddressVector& addresses,
 
     if (getState() == EXPIRED && !expired_ok) {
         return EXPIRED;
-
-        // Update the address selector
-        updateAddressSelector(v4_addresses_, v4_address_selector_);
     }
 
     switch (getState()) {
@@ -105,24 +102,13 @@ NameserverEntry::getAddresses(AddressVector& addresses,
 
     // If any address is OK, just pass everything we have
     if (family == ANY_OK) {
-        addresses.insert(addresses.end(), address_.begin(), address_.end());
+        addresses.insert(addresses.end(), addresses_[V4_ONLY].begin(),
+            addresses_[V4_ONLY].end());
+        addresses.insert(addresses.end(), addresses_[V6_ONLY].begin(),
+            addresses_[V6_ONLY].end());
     } else {
-        // Filter the addresses
-        short s_family(0);
-        switch (family) {
-            case V4_ONLY: s_family = AF_INET; break;
-            case V6_ONLY: s_family = AF_INET6; break;
-            default: assert(0); // This should never happen
-        }
-        // Now copy all entries that meet the criteria.  Since remove_copy_if
-        // does the inverse (copies all entries that do not meet the criteria),
-        // the predicate for address selection is negated.
-        remove_copy_if(address_.begin(), address_.end(),
-            back_inserter(addresses), boost::bind(addressSelection, s_family,
-            _1));
-
-        // Update the address selector
-        updateAddressSelector(v6_addresses_, v6_address_selector_);
+        addresses.insert(addresses.end(), addresses_[family].begin(),
+            addresses_[family].end());
     }
     if (getState() == EXPIRED && expired_ok) {
         return READY;
@@ -131,40 +117,27 @@ NameserverEntry::getAddresses(AddressVector& addresses,
 }
 
 // Return one address matching the given family
-bool NameserverEntry::getAddress(NameserverAddress& address, short family)
-{
+bool
+NameserverEntry::getAddress(NameserverAddress& address, AddressFamily family) {
     Lock lock(mutex_);
 
-// TODO Change to AddressFamily
-    // Get the shared_ptr object that point to "this" object
-    shared_ptr<NameserverEntry> shared_ptr_to_this = shared_from_this();
-
-    if(family == AF_INET){
-        if(v4_addresses_.size() == 0) return false;
-
-        address = NameserverAddress(shared_ptr_to_this, v4_address_selector_(), AF_INET);
-        return true;
-    } else if(family == AF_INET6){
-        if(v6_addresses_.size() == 0) return false;
-
-        //address = NameserverAddress(shared_from_this(), v6_address_selector_(), AF_INET6);
-        return true;
+    if (addresses_[family].empty()) {
+        return (false);
+    } else {
+        address = NameserverAddress(shared_from_this(),
+            address_selectors_[family](), family);
+        return (true);
     }
-    return false;
 }
 
 // Return the address corresponding to the family
-asiolink::IOAddress NameserverEntry::getAddressAtIndex(uint32_t index, short family) const
-{
+asiolink::IOAddress
+NameserverEntry::getAddressAtIndex(size_t index, AddressFamily family) const {
     Lock lock(mutex_);
 
-    const vector<AddressEntry> *addresses = &v4_addresses_;
-    if(family == AF_INET6){
-        addresses = &v6_addresses_;
-    }
-    assert(index < addresses->size());
+    assert(index < addresses_[family].size());
 
-    return (*addresses)[index].getAddress();
+    return (addresses_[family][index].getAddress());
 }
 
 // Set the address RTT to a specific value
@@ -173,55 +146,46 @@ NameserverEntry::setAddressRTT(const IOAddress& address, uint32_t rtt) {
     Lock lock(mutex_);
 
     // Search through the list of addresses for a match
-    for (AddressVectorIterator i = v4_addresses_.begin(); i != v4_addresses_.end(); ++i) {
-        if (i->getAddress().equal(address)) {
-            i->setRTT(rtt);
-
-            // Update the selector
-            updateAddressSelector(v4_addresses_, v4_address_selector_);
-            return;
+    AddressFamily family(V4_ONLY);
+    for (;;) {
+        BOOST_FOREACH(AddressEntry& entry, addresses_[family]) {
+            if (entry.getAddress().equal(address)) {
+                entry.setRTT(rtt);
+                updateAddressSelector(addresses_[family],
+                    address_selectors_[family]);
+                return;
+            }
         }
-    }
 
-    // Search the v6 list
-    for (AddressVectorIterator i = v6_addresses_.begin(); i != v6_addresses_.end(); ++i) {
-        if (i->getAddress().equal(address)) {
-            i->setRTT(rtt);
-
-            // Update the selector
-            updateAddressSelector(v6_addresses_, v6_address_selector_);
-            return;
+        // Hack. C++ does not allow ++ on enums, enumerating trough them is pain
+        switch (family) {
+            case V4_ONLY: family = V6_ONLY; break;
+            default: return;
         }
     }
 }
 
-// Update the address's rtt 
+// Update the address's rtt
 #define UPDATE_RTT_ALPHA 0.7
-void NameserverEntry::updateAddressRTTAtIndex(uint32_t rtt, uint32_t index, short family) {
+void
+NameserverEntry::updateAddressRTTAtIndex(uint32_t rtt, size_t index,
+    AddressFamily family)
+{
     Lock lock(mutex_);
 
-    vector<AddressEntry>* addresses = &v4_addresses_;
-    if(family == AF_INET6){
-        addresses = &v6_addresses_;
-    }
-
     //make sure it is a valid index
-    if(index >= addresses->size()) return;
+    if(index >= addresses_[family].size()) return;
 
     // Smoothly update the rtt
     // The algorithm is as the same as bind8/bind9:
     //    new_rtt = old_rtt * alpha + new_rtt * (1 - alpha), where alpha is a float number in [0, 1.0]
     // The default value for alpha is 0.7
-    uint32_t old_rtt = (*addresses)[index].getRTT();
-    uint32_t new_rtt = (int)(old_rtt * UPDATE_RTT_ALPHA + rtt * (1 - UPDATE_RTT_ALPHA));
-    (*addresses)[index].setRTT(new_rtt);
-
-    // Update the selector
-    if(family == AF_INET) { 
-        updateAddressSelector(v4_addresses_, v4_address_selector_);
-    } else if(family == AF_INET6) {
-        updateAddressSelector(v6_addresses_, v6_address_selector_);
-    }
+    uint32_t old_rtt = addresses_[family][index].getRTT();
+    uint32_t new_rtt = (uint32_t)(old_rtt * UPDATE_RTT_ALPHA + rtt *
+        (1 - UPDATE_RTT_ALPHA));
+    addresses_[family][index].setRTT(new_rtt);
+
+    updateAddressSelector(addresses_[family], address_selectors_[family]);
 }
 
 // Sets the address to be unreachable
@@ -236,7 +200,6 @@ NameserverEntry::setAddressUnreachable(const IOAddress& address) {
  * pointer to the entry so it is not destroyed too soon.
  */
 class NameserverEntry::ResolverCallback : public ResolverInterface::Callback {
-    // TODO This needs to bring in some changes from the constructor
     public:
         ResolverCallback(shared_ptr<NameserverEntry> entry,
             AddressFamily family, const RRType& type) :
@@ -266,20 +229,23 @@ class NameserverEntry::ResolverCallback : public ResolverInterface::Callback {
             while (! i->isLast()) {
                 // Try to find the original value and reuse its rtt
                 string address(i->getCurrent().toText());
-                int curr_rtt(-1);
-                BOOST_FOREACH(AddressEntry& entry, entry_->previous_addresses_)
+                AddressEntry *found(NULL);
+                BOOST_FOREACH(AddressEntry& entry,
+                    entry_->previous_addresses_[family_])
                 {
                     if (entry.getAddress().toText() == address) {
-                        curr_rtt = entry.getRTT();
+                        found = &entry;
                         break;
                     }
                 }
-                entries.push_back(AddressEntry(IOAddress(
-                    i->getCurrent().toText()),
-                    curr_rtt == -1 ? ++ rtt_ : curr_rtt));
+                entries.push_back(found ? *found : AddressEntry(IOAddress(
+                    i->getCurrent().toText()), ++ rtt_));
                 i->next();
             }
 
+            // We no longer expect this one here. We can drop it.
+            entry_->previous_addresses_[family_].clear();
+
             if (entries.empty()) {
                 // No data there, count it as a failure
                 failureInternal(lock);
@@ -291,15 +257,12 @@ class NameserverEntry::ResolverCallback : public ResolverInterface::Callback {
                 // Everything is here
                 if (!entry_->expect_address_[ANY_OK]) {
                     entry_->setState(READY);
-                    // We can drop the original addresses, we took everything
-                    entry_->previous_addresses_.clear();
                 }
                 // We have some address
                 entry_->has_address_[ANY_OK] =
                     entry_->has_address_[family_] = true;
                 // Put the addresses there
-                entry_->address_.insert(entry_->address_.end(),
-                    entries.begin(), entries.end());
+                entry_->addresses_[family_].swap(entries);
                 // Update the expiration time. If it is 0, it means we
                 // did not set it yet, so reset
                 time_t expiration(now + response->getTTL().getValue());
@@ -325,6 +288,10 @@ class NameserverEntry::ResolverCallback : public ResolverInterface::Callback {
         // Dispatches all relevant callbacks. Keeps lock unlocked afterwards.
         void dispatchCallbacks(Lock& lock)
         {
+            // There was a change (we wouldn't want to notify about it
+            // otherwise), so we update it
+            entry_->updateAddressSelector(entry_->addresses_[family_],
+                entry_->address_selectors_[family_]);
             // We dispatch ANY addresses if there is at last one address or
             // there's no chance we'll get some in future
             bool dispatch_any = entry_->has_address_[ANY_OK] ||
@@ -378,6 +345,8 @@ class NameserverEntry::ResolverCallback : public ResolverInterface::Callback {
                     entry_->setState(UNREACHABLE);
                 }
             }
+            // Drop the previous addresses, no use of them now
+            entry_->previous_addresses_[family_].clear();
             // Dispatch any relevant callbacks
             dispatchCallbacks(lock);
         }
@@ -406,8 +375,10 @@ NameserverEntry::askIP(shared_ptr<ResolverInterface> resolver,
         // Set internal state first
         // We store the old addresses so we can pick their RTT when
         // we get the same addresses again (most probably)
-        previous_addresses_.clear();
-        address_.swap(previous_addresses_);
+        previous_addresses_[V4_ONLY].clear();
+        previous_addresses_[V6_ONLY].clear();
+        addresses_[V4_ONLY].swap(previous_addresses_[V4_ONLY]);
+        addresses_[V6_ONLY].swap(previous_addresses_[V6_ONLY]);
         setState(IN_PROGRESS);
         has_address_[V4_ONLY] = has_address_[V6_ONLY] = has_address_[ANY_OK] =
             false;
@@ -444,13 +415,12 @@ NameserverEntry::askIP(shared_ptr<ResolverInterface> resolver,
 // Each address has a probability to be selected if multiple addresses are available
 // The weight factor is equal to 1/(rtt*rtt), then all the weight factors are normalized
 // to make the sum equal to 1.0
-void NameserverEntry::updateAddressSelector(std::vector<AddressEntry>& addresses, 
-        WeightedRandomIntegerGenerator& selector)
+void NameserverEntry::updateAddressSelector(
+    std::vector<AddressEntry>& addresses,
+    WeightedRandomIntegerGenerator& selector)
 {
-    Lock lock(mutex_);
-
     vector<double> probabilities;
-    for(vector<AddressEntry>::iterator it = addresses.begin(); 
+    for(vector<AddressEntry>::iterator it = addresses.begin();
             it != addresses.end(); ++it){
         uint32_t rtt = (*it).getRTT();
         if(rtt == 0) {
@@ -468,14 +438,14 @@ void NameserverEntry::updateAddressSelector(std::vector<AddressEntry>& addresses
 
     if(sum != 0) {
         // Normalize the probabilities to make the sum equal to 1.0
-        for(vector<double>::iterator it = probabilities.begin(); 
+        for(vector<double>::iterator it = probabilities.begin();
                 it != probabilities.end(); ++it){
             (*it) /= sum;
         }
     } else if(probabilities.size() > 0){
         // If all the nameservers are unreachable, the sum will be 0
         // So give each server equal opportunity to be selected.
-        for(vector<double>::iterator it = probabilities.begin(); 
+        for(vector<double>::iterator it = probabilities.begin();
                 it != probabilities.end(); ++it){
             (*it) = 1.0/probabilities.size();
         }

src/lib/nsas/nameserver_entry.h

@@ -145,15 +145,17 @@ public:
     ///
     /// Return one address corresponding to this nameserver
     /// \param address NameserverAddress object used to receive the address
-    /// \param family The family of user request, AF_INET or AF_INET6
+    /// \param family The family of user request, V4_ONLY or V6_ONLY
     /// \return true if one address is found, false otherwise
-    virtual bool getAddress(NameserverAddress& address, short family);
+    /// \todo What use is this function?
+    virtual bool getAddress(NameserverAddress& address, AddressFamily family);
 
     /// \brief Return Address that corresponding to the index
     ///
     /// \param index The address index in the address vector
-    /// \param family The address family, AF_INET or AF_INET6
-    asiolink::IOAddress getAddressAtIndex(uint32_t index, short family) const;
+    /// \param family The address family, V4_ONLY or V6_ONLY
+    asiolink::IOAddress getAddressAtIndex(size_t index,
+        AddressFamily family) const;
 
     /// \brief Update RTT
     ///
@@ -167,8 +169,9 @@ public:
     ///
     /// \param rtt Round-Trip Time
     /// \param index The address's index in address vector
-    /// \param family The address family, AF_INET or AF_INET6
-    void updateAddressRTTAtIndex(uint32_t rtt, uint32_t index, short family);
+    /// \param family The address family, V4_ONLY or V6_ONLY
+    void updateAddressRTTAtIndex(uint32_t rtt, size_t index,
+        AddressFamily family);
 
     /// \brief Set Address Unreachable
     ///
@@ -243,8 +246,16 @@ private:
     mutable boost::mutex    mutex_;     ///< Mutex protecting this object
     std::string     name_;              ///< Canonical name of the nameserver
     isc::dns::RRClass classCode_;       ///< Class of the nameserver
-    /// The current addresses and addresses from previous request to keep RTT
-    std::vector<AddressEntry> address_, previous_addresses_;
+    /**
+     * \short Address lists.
+     *
+     * Only V4_ONLY and V6_ONLY is used, therefore we use the nearest larger
+     * value as the size of the array.
+     *
+     * previous_addresses is kept until the data arrive again on re-fetch and
+     * is used to pick up the RTTs from there.
+     */
+    std::vector<AddressEntry> addresses_[ANY_OK], previous_addresses_[ANY_OK];
     time_t          expiration_;        ///< Summary expiration time. 0 = unset
     // Do we have some addresses already? Do we expect some to come?
     // These are set after asking for IP, if NOT_ASKED, they are uninitialized
@@ -265,14 +276,17 @@ private:
     ///
     /// \param addresses The address list
     /// \param selector Weighted random generator
-    void updateAddressSelector(std::vector<AddressEntry>& addresses, 
+    void updateAddressSelector(std::vector<AddressEntry>& addresses,
             WeightedRandomIntegerGenerator& selector);
 
-    // TODO Unite address_ and v?_addresses_
-    std::vector<AddressEntry> v4_addresses_;             ///< Set of V4 addresses
-    std::vector<AddressEntry> v6_addresses_;             ///< Set of V6 addresses
-    WeightedRandomIntegerGenerator v4_address_selector_; ///< Generate one integer according to different probability
-    WeightedRandomIntegerGenerator v6_address_selector_; ///< Generate one integer according to different probability
+    /**
+     * \short Generate one integer according to different probability.
+     *
+     * \todo Should this be more global? And it definitely should be
+     *     inside ZoneEntry, the selection should be done from all
+     *     that addresses.
+     */
+    WeightedRandomIntegerGenerator address_selectors_[ANY_OK];
 };
 
 }   // namespace dns

src/lib/nsas/nsas_types.h

@@ -25,7 +25,12 @@
 namespace isc {
 namespace nsas {
 
-/// \brief Address requested
+/**
+ * \brief Address requested
+ *
+ * The order is significant, it is used as array indices and sometime only
+ * the first two are used.
+ */
 enum AddressFamily {
     /// \short Interested only in IPv4 address
     V4_ONLY,

src/lib/nsas/random_number_generator.h

@@ -60,7 +60,7 @@ private:
 /// \brief Weighted random integer generator
 ///
 /// Generate random integers according different probabilities
-class WeightedRandomIntegerGenerator{
+class WeightedRandomIntegerGenerator {
 public:
     /// \brief Constructor
     ///
@@ -71,7 +71,8 @@ public:
     /// other integers and the probability is proportional to its value.
     /// \param min The minimum integer that generated, other integers will be 
     /// min, min + 1, ..., min + probabilities.size() - 1
-    WeightedRandomIntegerGenerator(const std::vector<double>& probabilities, int min = 0):
+    WeightedRandomIntegerGenerator(const std::vector<double>& probabilities,
+        size_t min = 0):
         dist_(0, 1.0), uniform_real_gen_(rng_, dist_), min_(min)
     {
         // The probabilities must be valid
@@ -95,7 +96,7 @@ public:
     /// Change the weights of each integers
     /// \param probabilities The probabies for all the integers
     /// \param min The minimum integer that generated
-    void reset(const std::vector<double>& probabilities, int min = 0)
+    void reset(const std::vector<double>& probabilities, size_t min = 0)
     {
         // The probabilities must be valid
         assert(isProbabilitiesValid(probabilities));
@@ -115,7 +116,7 @@ public:
     }
 
     /// \brief Generate weighted random integer
-    int operator()()
+    size_t operator()()
     {
         return std::lower_bound(cumulative_.begin(), cumulative_.end(), uniform_real_gen_()) 
             - cumulative_.begin() + min_;
@@ -156,7 +157,7 @@ private:
     boost::mt19937 rng_;                        ///< Mersenne Twister: A 623-dimensionally equidistributed uniform pseudo-random number generator 
     boost::uniform_real<> dist_;                ///< Uniformly distributed real numbers
     UniformRealGenerator uniform_real_gen_;     ///< Uniformly distributed random real numbers generator
-    int min_;                                   ///< The minimum integer that will be generated
+    size_t min_;                                   ///< The minimum integer that will be generated
 };
 
 }   // namespace dns

src/lib/nsas/tests/nameserver_address_unittest.cc

@@ -58,7 +58,7 @@ public:
 
     // Return the IOAddress corresponding to the index in rrv4_
     asiolink::IOAddress getAddressAtIndex(uint32_t index) {
-        return ns_.get()->getAddressAtIndex(index, AF_INET);
+        return ns_.get()->getAddressAtIndex(index, V4_ONLY);
     }
 
     // Return the addresses count stored in RRset
@@ -87,8 +87,10 @@ class NameserverAddressTest : public ::testing::Test {
 protected:
     // Constructor
     NameserverAddressTest(): 
-        ns_address_(ns_sample_.getNameserverEntry(), TEST_ADDRESS_INDEX, AF_INET),
-        invalid_ns_address_(ns_sample_.getNameserverEntry(), ns_sample_.getAddressesCount(), AF_INET)
+        ns_address_(ns_sample_.getNameserverEntry(), TEST_ADDRESS_INDEX,
+            V4_ONLY),
+        invalid_ns_address_(ns_sample_.getNameserverEntry(),
+            ns_sample_.getAddressesCount(), V4_ONLY)
     {
     }
 
@@ -109,7 +111,7 @@ TEST_F(NameserverAddressTest, Address) {
 
     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, AF_INET);}, NullNameserverEntryPointer);
+    ASSERT_THROW({NameserverAddress empty_ns_address(empty_ne, 0, V4_ONLY);}, NullNameserverEntryPointer);
 }
 
 // Test that the RTT is updated