dht code related refactor and cleanup (#1107)

dht code related refactor and cleanup

include/libtorrent/kademlia/dht_storage.hpp

@@ -39,7 +39,6 @@ POSSIBILITY OF SUCH DAMAGE.
 #include <libtorrent/kademlia/types.hpp>
 
 #include <libtorrent/socket.hpp>
-#include <libtorrent/sha1_hash.hpp>
 #include <libtorrent/address.hpp>
 #include <libtorrent/span.hpp>
 #include <libtorrent/string_view.hpp>
@@ -109,7 +108,7 @@ namespace dht
 		// If the torrent tracked contains a name, such a name
 		// must be stored as a string in peers["n"]
 		//
-		// If the scrape parameter is true, you should fill these keys::
+		// If the scrape parameter is true, you should fill these keys:
 		// 
 		//    peers["BFpe"] - with the standard bit representation of a
 		//                    256 bloom filter containing the downloaders

include/libtorrent/kademlia/msg.hpp

@@ -33,9 +33,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #ifndef TORRENT_KADEMLIA_MSG_HPP
 #define TORRENT_KADEMLIA_MSG_HPP
 
-#include <string>
 #include "libtorrent/socket.hpp"
-#include "libtorrent/kademlia/node_id.hpp"
 
 namespace libtorrent {
 
@@ -44,10 +42,6 @@ class entry;
 
 namespace dht {
 
-typedef std::vector<char> packet_t;
-typedef std::vector<node_entry> nodes_t;
-typedef std::vector<tcp::endpoint> peers_t;
-
 struct msg
 {
 	msg(bdecode_node const& m, udp::endpoint const& ep): message(m), addr(ep) {}

include/libtorrent/kademlia/node.hpp

@@ -72,7 +72,7 @@ namespace libtorrent { namespace dht
 struct traversal_algorithm;
 struct dht_observer;
 
-TORRENT_EXTRA_EXPORT void write_nodes_entry(entry& r, nodes_t const& nodes);
+TORRENT_EXTRA_EXPORT entry write_nodes_entry(std::vector<node_entry> const& nodes);
 
 struct null_type {};
 

include/libtorrent/kademlia/node_id.hpp

@@ -32,19 +32,16 @@ POSSIBILITY OF SUCH DAMAGE.
 #ifndef NODE_ID_HPP
 #define NODE_ID_HPP
 
-#include <algorithm>
 #include <vector>
 #include <cstdint>
 
 #include <libtorrent/config.hpp>
-#include <libtorrent/peer_id.hpp>
+#include <libtorrent/sha1_hash.hpp>
 #include <libtorrent/address.hpp>
 
 namespace libtorrent { namespace dht
 {
 
-struct node_entry;
-
 using node_id = libtorrent::sha1_hash;
 
 // returns the distance between the two nodes
@@ -70,7 +67,7 @@ TORRENT_EXTRA_EXPORT bool verify_secret_id(node_id const& nid);
 TORRENT_EXTRA_EXPORT node_id generate_id_impl(address const& ip_, std::uint32_t r);
 
 TORRENT_EXTRA_EXPORT bool verify_id(node_id const& nid, address const& source_ip);
-TORRENT_EXTRA_EXPORT bool matching_prefix(node_entry const& n, int mask, int prefix, int offset);
+TORRENT_EXTRA_EXPORT bool matching_prefix(node_id const& nid, int mask, int prefix, int offset);
 TORRENT_EXTRA_EXPORT node_id generate_prefix_mask(int bits);
 
 } } // namespace libtorrent::dht

include/libtorrent/sha1_hash.hpp

@@ -277,8 +277,6 @@ namespace libtorrent
 	// peer IDs, node IDs etc.
 	using sha1_hash = digest32<160>;
 
-	typedef sha1_hash peer_id;
-
 #if TORRENT_USE_IOSTREAM
 
 	// print a sha1_hash object to an ostream as 40 hexadecimal digits

src/kademlia/dht_storage.cpp

@@ -43,23 +43,14 @@ POSSIBILITY OF SUCH DAMAGE.
 #include <libtorrent/aux_/time.hpp>
 #include <libtorrent/config.hpp>
 #include <libtorrent/time.hpp>
-#include <libtorrent/socket.hpp>
-#include <libtorrent/sha1_hash.hpp>
 #include <libtorrent/bloom_filter.hpp>
-#include <libtorrent/address.hpp>
 #include <libtorrent/session_settings.hpp>
-#include <libtorrent/performance_counters.hpp>
 #include <libtorrent/random.hpp>
 
-#include <libtorrent/kademlia/item.hpp>
-#include <libtorrent/kademlia/node_id.hpp>
-
 namespace libtorrent {
 namespace dht {
 namespace
 {
-	using detail::write_endpoint;
-
 	// this is the entry for every peer
 	// the timestamp is there to make it possible
 	// to remove stale peers
@@ -112,16 +103,16 @@ namespace
 		std::string salt;
 	};
 
-	void touch_item(dht_immutable_item* f, address const& addr)
+	void touch_item(dht_immutable_item& f, address const& addr)
 	{
-		f->last_seen = aux::time_now();
+		f.last_seen = aux::time_now();
 
 		// maybe increase num_announcers if we haven't seen this IP before
 		sha1_hash const iphash = hash_address(addr);
-		if (!f->ips.find(iphash))
+		if (!f.ips.find(iphash))
 		{
-			f->ips.set(iphash);
+			f.ips.set(iphash);
-			++f->num_announcers;
+			++f.num_announcers;
 		}
 	}
 
@@ -169,10 +160,6 @@ namespace
 
 	class dht_default_storage final : public dht_storage_interface, boost::noncopyable
 	{
-		using table_t = std::map<node_id, torrent_entry>;
-		using dht_immutable_table_t = std::map<node_id, dht_immutable_item>;
-		using dht_mutable_table_t = std::map<node_id, dht_mutable_item>;
-
 	public:
 
 		explicit dht_default_storage(dht_settings const& settings)
@@ -198,11 +185,11 @@ namespace
 			m_node_ids = ids;
 		}
 
-		bool get_peers(sha1_hash const& info_hash, udp protocol
+		bool get_peers(sha1_hash const& info_hash, udp const protocol
 			, bool const noseed, bool const scrape
 			, entry& peers) const override
 		{
-			table_t::const_iterator i = m_map.lower_bound(info_hash);
+			auto const i = m_map.lower_bound(info_hash);
 			if (i == m_map.end()) return false;
 			if (i->first != info_hash) return false;
 
@@ -234,7 +221,7 @@ namespace
 				if (!v.peers.empty() && protocol == udp::v6())
 					max /= 4;
 				// we're picking "to_pick" from a list of "num" at random.
-				int const to_pick = (std::min)(int(v.peers.size()), max);
+				int const to_pick = std::min(int(v.peers.size()), max);
 				std::set<peer_entry>::const_iterator iter = v.peers.begin();
 				entry::list_type& pe = peers["values"].list();
 
@@ -258,13 +245,13 @@ namespace
 					else
 					{
 						// maybe replace an item we've already picked
-						if (random(t-1) >= to_pick) continue;
+						if (random(t - 1) >= to_pick) continue;
 						str = &pe[random(to_pick - 1)].string();
 					}
 
 					str->resize(18);
 					std::string::iterator out = str->begin();
-					write_endpoint(iter->addr, out);
+					detail::write_endpoint(iter->addr, out);
 					str->resize(out - str->begin());
 
 					++m;
@@ -277,7 +264,7 @@ namespace
 			, tcp::endpoint const& endp
 			, string_view name, bool const seed) override
 		{
-			table_t::iterator ti = m_map.find(info_hash);
+			auto const ti = m_map.find(info_hash);
 			torrent_entry* v;
 			if (ti == m_map.end())
 			{
@@ -288,8 +275,8 @@ namespace
 					// we need to remove some. Remove the ones with the
 					// fewest peers
 					int num_peers = int(m_map.begin()->second.peers.size());
-					table_t::iterator candidate = m_map.begin();
+					auto candidate = m_map.begin();
-					for (table_t::iterator i = m_map.begin()
+					for (auto i = m_map.begin()
 						, end(m_map.end()); i != end; ++i)
 					{
 						if (int(i->second.peers.size()) > num_peers) continue;
@@ -320,7 +307,7 @@ namespace
 			peer.addr = endp;
 			peer.added = aux::time_now();
 			peer.seed = seed;
-			std::set<peer_entry>::iterator i = v->peers.find(peer);
+			auto i = v->peers.find(peer);
 			if (i != v->peers.end())
 			{
 				v->peers.erase(i++);
@@ -343,7 +330,7 @@ namespace
 		bool get_immutable_item(sha1_hash const& target
 			, entry& item) const override
 		{
-			dht_immutable_table_t::const_iterator i = m_immutable_table.find(target);
+			auto const i = m_immutable_table.find(target);
 			if (i == m_immutable_table.end()) return false;
 
 			item["v"] = bdecode(i->second.value.get()
@@ -356,13 +343,13 @@ namespace
 			, address const& addr) override
 		{
 			TORRENT_ASSERT(!m_node_ids.empty());
-			dht_immutable_table_t::iterator i = m_immutable_table.find(target);
+			auto i = m_immutable_table.find(target);
 			if (i == m_immutable_table.end())
 			{
 				// make sure we don't add too many items
 				if (int(m_immutable_table.size()) >= m_settings.max_dht_items)
 				{
-					auto j = pick_least_important_item(m_node_ids
+					auto const j = pick_least_important_item(m_node_ids
 						, m_immutable_table);
 
 					TORRENT_ASSERT(j != m_immutable_table.end());
@@ -372,7 +359,7 @@ namespace
 				dht_immutable_item to_add;
 				to_add.value.reset(new char[buf.size()]);
 				to_add.size = int(buf.size());
-				memcpy(to_add.value.get(), buf.data(), buf.size());
+				std::memcpy(to_add.value.get(), buf.data(), buf.size());
 
 				std::tie(i, std::ignore) = m_immutable_table.insert(
 					std::make_pair(target, std::move(to_add)));
@@ -381,13 +368,13 @@ namespace
 
 //			std::fprintf(stderr, "added immutable item (%d)\n", int(m_immutable_table.size()));
 
-			touch_item(&i->second, addr);
+			touch_item(i->second, addr);
 		}
 
 		bool get_mutable_item_seq(sha1_hash const& target
 			, sequence_number& seq) const override
 		{
-			dht_mutable_table_t::const_iterator i = m_mutable_table.find(target);
+			auto const i = m_mutable_table.find(target);
 			if (i == m_mutable_table.end()) return false;
 
 			seq = i->second.seq;
@@ -395,10 +382,10 @@ namespace
 		}
 
 		bool get_mutable_item(sha1_hash const& target
-			, sequence_number seq, bool force_fill
+			, sequence_number const seq, bool const force_fill
 			, entry& item) const override
 		{
-			dht_mutable_table_t::const_iterator i = m_mutable_table.find(target);
+			auto const i = m_mutable_table.find(target);
 			if (i == m_mutable_table.end()) return false;
 
 			dht_mutable_item const& f = i->second;
@@ -415,20 +402,20 @@ namespace
 		void put_mutable_item(sha1_hash const& target
 			, span<char const> buf
 			, signature const& sig
-			, sequence_number seq
+			, sequence_number const seq
 			, public_key const& pk
 			, span<char const> salt
 			, address const& addr) override
 		{
 			TORRENT_ASSERT(!m_node_ids.empty());
-			dht_mutable_table_t::iterator i = m_mutable_table.find(target);
+			auto i = m_mutable_table.find(target);
 			if (i == m_mutable_table.end())
 			{
 				// this is the case where we don't have an item in this slot
 				// make sure we don't add too many items
 				if (int(m_mutable_table.size()) >= m_settings.max_dht_items)
 				{
-					auto j = pick_least_important_item(m_node_ids
+					auto const j = pick_least_important_item(m_node_ids
 						, m_mutable_table);
 
 					TORRENT_ASSERT(j != m_mutable_table.end());
@@ -442,7 +429,7 @@ namespace
 				to_add.salt.assign(salt.data(), salt.size());
 				to_add.sig = sig;
 				to_add.key = pk;
-				memcpy(to_add.value.get(), buf.data(), buf.size());
+				std::memcpy(to_add.value.get(), buf.data(), buf.size());
 
 				std::tie(i, std::ignore) = m_mutable_table.insert(
 					std::make_pair(target, std::move(to_add)));
@@ -462,19 +449,19 @@ namespace
 					}
 					item.seq = seq;
 					item.sig = sig;
-					memcpy(item.value.get(), buf.data(), buf.size());
+					std::memcpy(item.value.get(), buf.data(), buf.size());
 				}
 			}
 
-			touch_item(&i->second, addr);
+			touch_item(i->second, addr);
 		}
 
 		void tick() override
 		{
-			time_point now(aux::time_now());
+			time_point const now(aux::time_now());
 
 			// look through all peers and see if any have timed out
-			for (table_t::iterator i = m_map.begin(), end(m_map.end()); i != end;)
+			for (auto i = m_map.begin(), end(m_map.end()); i != end;)
 			{
 				torrent_entry& t = i->second;
 				purge_peers(t.peers);
@@ -496,8 +483,7 @@ namespace
 			// item lifetime must >= 120 minutes.
 			if (lifetime < minutes(120)) lifetime = minutes(120);
 
-			for (dht_immutable_table_t::iterator i = m_immutable_table.begin();
+			for (auto i = m_immutable_table.begin(); i != m_immutable_table.end();)
-				i != m_immutable_table.end();)
 			{
 				if (i->second.last_seen + lifetime > now)
 				{
@@ -508,8 +494,7 @@ namespace
 				m_counters.immutable_data -= 1;
 			}
 
-			for (dht_mutable_table_t::iterator i = m_mutable_table.begin();
+			for (auto i = m_mutable_table.begin(); i != m_mutable_table.end();)
-				i != m_mutable_table.end();)
 			{
 				if (i->second.last_seen + lifetime > now)
 				{
@@ -531,14 +516,13 @@ namespace
 		dht_storage_counters m_counters;
 
 		std::vector<node_id> m_node_ids;
-		table_t m_map;
+		std::map<node_id, torrent_entry> m_map;
-		dht_immutable_table_t m_immutable_table;
+		std::map<node_id, dht_immutable_item> m_immutable_table;
-		dht_mutable_table_t m_mutable_table;
+		std::map<node_id, dht_mutable_item> m_mutable_table;
 
 		void purge_peers(std::set<peer_entry>& peers)
 		{
-			for (std::set<peer_entry>::iterator i = peers.begin()
+			for (auto i = peers.begin(), end(peers.end()); i != end;)
-				, end(peers.end()); i != end;)
 			{
 				// the peer has timed out
 				if (i->added + minutes(int(announce_interval * 3 / 2)) < aux::time_now())

src/kademlia/node.cpp

@@ -759,14 +759,16 @@ void node::lookup_peers(sha1_hash const& info_hash, entry& reply
 	m_storage.get_peers(info_hash, protocol(), noseed, scrape, reply);
 }
 
-void write_nodes_entry(entry& r, nodes_t const& nodes)
+entry write_nodes_entry(std::vector<node_entry> const& nodes)
 {
+	entry r;
 	std::back_insert_iterator<std::string> out(r.string());
 	for (auto const& n : nodes)
 	{
 		std::copy(n.id.begin(), n.id.end(), out);
 		write_endpoint(udp::endpoint(n.addr(), n.port()), out);
 	}
+	return r;
 }
 
 // build response
@@ -1160,9 +1162,9 @@ void node::write_nodes_entries(sha1_hash const& info_hash
 	// entry based on the protocol the request came in with
 	if (want.type() != bdecode_node::list_t)
 	{
-		nodes_t n;
+		std::vector<node_entry> n;
 		m_table.find_node(info_hash, n, 0);
-		write_nodes_entry(r[protocol_nodes_key()], n);
+		r[protocol_nodes_key()] = write_nodes_entry(n);
 		return;
 	}
 
@@ -1178,9 +1180,9 @@ void node::write_nodes_entries(sha1_hash const& info_hash
 			continue;
 		auto wanted_node = m_nodes.find(wanted.string_value().to_string());
 		if (wanted_node == m_nodes.end()) continue;
-		nodes_t n;
+		std::vector<node_entry> n;
 		wanted_node->second->m_table.find_node(info_hash, n, 0);
-		write_nodes_entry(r[wanted_node->second->protocol_nodes_key()], n);
+		r[wanted_node->second->protocol_nodes_key()] = write_nodes_entry(n);
 	}
 }
 

src/kademlia/node_id.cpp

@@ -198,9 +198,9 @@ node_id generate_id(address const& ip)
 	return generate_id_impl(ip, random(0xffffffff));
 }
 
-bool matching_prefix(node_entry const& n, int mask, int prefix, int offset)
+bool matching_prefix(node_id const& nid, int mask, int prefix, int offset)
 {
-	node_id id = n.id;
+	node_id id = nid;
 	id <<= offset;
 	return (id[0] & mask) == prefix;
 }

src/kademlia/routing_table.cpp

@@ -740,7 +740,7 @@ ip_ok:
 
 			for (j = b.begin(); j != b.end(); ++j)
 			{
-				if (!matching_prefix(*j, mask, candidate_prefix, prefix_offset)) continue;
+				if (!matching_prefix(j->id, mask, candidate_prefix, prefix_offset)) continue;
 				nodes.push_back(j);
 			}
 		}

test/test_dht.cpp

@@ -140,9 +140,10 @@ void lazy_from_entry(entry const& e, bdecode_node& l)
 	TEST_CHECK(ret == 0);
 }
 
-void write_peers(entry::dictionary_type& r, std::set<tcp::endpoint> const& peers)
+entry write_peers(std::set<tcp::endpoint> const& peers)
 {
-	entry::list_type& pe = r["values"].list();
+	entry r;
+	entry::list_type& pe = r.list();
 	for (auto const& p : peers)
 	{
 		std::string endpoint(18, '\0');
@@ -151,6 +152,7 @@ void write_peers(entry::dictionary_type& r, std::set<tcp::endpoint> const& peers
 		endpoint.resize(out - endpoint.begin());
 		pe.push_back(entry(endpoint));
 	}
+	return r;
 }
 
 struct msg_args
@@ -200,14 +202,14 @@ struct msg_args
 	msg_args& want(std::string w)
 	{ a["want"].list().push_back(w); return *this; }
 
-	msg_args& nodes(nodes_t const& n)
+	msg_args& nodes(std::vector<node_entry> const& n)
-	{ if (!n.empty()) dht::write_nodes_entry(a["nodes"], n); return *this; }
+	{ if (!n.empty()) a["nodes"] = dht::write_nodes_entry(n); return *this; }
 
-	msg_args& nodes6(nodes_t const& n)
+	msg_args& nodes6(std::vector<node_entry> const& n)
-	{ if (!n.empty()) dht::write_nodes_entry(a["nodes6"], n); return *this; }
+	{ if (!n.empty()) a["nodes6"] = dht::write_nodes_entry(n); return *this; }
 
 	msg_args& peers(std::set<tcp::endpoint> const& p)
-	{ if (!p.empty()) write_peers(a.dict(), p); return *this; }
+	{ if (!p.empty()) a.dict()["values"] = write_peers(p); return *this; }
 
 	entry a;
 };
@@ -1754,7 +1756,7 @@ void test_bootstrap(address(&rand_addr)())
 	}
 
 	udp::endpoint found_node(rand_addr(), 2235);
-	nodes_t nodes;
+	std::vector<node_entry> nodes;
 	nodes.push_back(found_node);
 	g_sent_packets.clear();
 	if (initial_node.address().is_v4())
@@ -1858,7 +1860,7 @@ void test_short_nodes(address(&rand_addr)())
 	}
 
 	udp::endpoint found_node(rand_addr(), 2235);
-	nodes_t nodes;
+	std::vector<node_entry> nodes;
 	nodes.push_back(found_node);
 	g_sent_packets.clear();
 	msg_args args;
@@ -1949,7 +1951,7 @@ void test_get_peers(address(&rand_addr)())
 	peers[0].insert(tcp::endpoint(rand_addr(), 4113));
 
 	udp::endpoint next_node(rand_addr(), 2235);
-	nodes_t nodes;
+	std::vector<node_entry> nodes;
 	nodes.push_back(next_node);
 
 	g_sent_packets.clear();
@@ -2250,7 +2252,7 @@ TORRENT_TEST(immutable_put)
 			std::snprintf(tok, sizeof(tok), "%02d", i);
 
 			msg_args args;
-			args.token(tok).port(1234).nid(nodes[i].id).nodes(nodes_t(1, nodes[i]));
+			args.token(tok).port(1234).nid(nodes[i].id).nodes({nodes[i]});
 			send_dht_response(t.dht_node, response, nodes[i].ep(), args);
 			g_sent_packets.erase(packet);
 		}
@@ -2352,7 +2354,7 @@ TORRENT_TEST(mutable_put)
 			std::snprintf(tok, sizeof(tok), "%02d", i);
 
 			msg_args args;
-			args.token(tok).port(1234).nid(nodes[i].id).nodes(nodes_t(1, nodes[i]));
+			args.token(tok).port(1234).nid(nodes[i].id).nodes({nodes[i]});
 			send_dht_response(t.dht_node, response, nodes[i].ep(), args);
 			g_sent_packets.erase(packet);
 		}
@@ -2472,7 +2474,7 @@ TORRENT_TEST(traversal_done)
 
 		// add the address of the closest node to the first response
 		if (i == 1)
-			args.nodes(nodes_t(1, nodes[8]));
+			args.nodes({nodes[8]});
 
 		send_dht_response(t.dht_node, response, nodes[i].ep(), args);
 		g_sent_packets.erase(packet);