Rewrite clone counting to use one map instead of two

7 files changed, 63 insertions, 76 deletions

diff --git a/include/usermanager.h b/include/usermanager.h
index 040f91852..eb1d75e8d 100644
--- a/include/usermanager.h
+++ b/include/usermanager.h
@@ -21,15 +21,28 @@
 
 #include <list>
 
-/** A list of ip addresses cross referenced against clone counts */
-typedef std::map<irc::sockets::cidr_mask, unsigned int> clonemap;
-
 class CoreExport UserManager
 {
+ public:
+	struct CloneCounts
+	{
+		unsigned int global;
+		unsigned int local;
+		CloneCounts() : global(0), local(0) { }
+	};
+
+	/** Container that maps IP addresses to clone counts
+	 */
+	typedef std::map<irc::sockets::cidr_mask, CloneCounts> CloneMap;
+
  private:
-	/** Map of local ip addresses for clone counting
+	/** Map of IP addresses for clone counting
+	 */
+	CloneMap clonemap;
+
+	/** A CloneCounts that contains zero for both local and global
 	 */
-	clonemap local_clones;
+	const CloneCounts zeroclonecounts;
 
  public:
 	/** Constructor, initializes variables
@@ -62,11 +75,6 @@ class CoreExport UserManager
 	 */
 	unsigned int unregistered_count;
 
-	/** Map of global ip addresses for clone counting
-	 * XXX - this should be private, but m_clones depends on it currently.
-	 */
-	clonemap global_clones;
-
 	/**
 	 * Reset the already_sent IDs so we don't wrap it around and drop a message
 	 * Also removes all expired invites
@@ -102,15 +110,10 @@ class CoreExport UserManager
 	 */
 	void QuitUser(User* user, const std::string& quitreason, const std::string* operreason = NULL);
 
-	/** Add a user to the local clone map
-	 * @param user The user to add
-	 */
-	void AddLocalClone(User *user);
-
-	/** Add a user to the global clone map
+	/** Add a user to the clone map
 	 * @param user The user to add
 	 */
-	void AddGlobalClone(User *user);
+	void AddClone(User* user);
 
 	/** Remove all clone counts from the user, you should
 	 * use this if you change the user's IP address
@@ -119,17 +122,18 @@ class CoreExport UserManager
 	 */
 	void RemoveCloneCounts(User *user);
 
-	/** Return the number of global clones of this user
-	 * @param user The user to get a count for
-	 * @return The global clone count of this user
+	/** Return the number of local and global clones of this user
+	 * @param user The user to get the clone counts for
+	 * @return The clone counts of this user. The returned reference is volatile - you
+	 * must assume that it becomes invalid as soon as you call any function other than
+	 * your own.
 	 */
-	unsigned long GlobalCloneCount(User *user);
+	const CloneCounts& GetCloneCounts(User* user) const;
 
-	/** Return the number of local clones of this user
-	 * @param user The user to get a count for
-	 * @return The local clone count of this user
+	/** Return a map containg IP addresses and their clone counts
+	 * @return The clone count map
 	 */
-	unsigned long LocalCloneCount(User *user);
+	const CloneMap& GetCloneMap() const { return clonemap; }
 
 	/** Return a count of all global users, unknown and known connections
 	 * @return The number of users on the network, including local unregistered users
diff --git a/src/modules/m_cgiirc.cpp b/src/modules/m_cgiirc.cpp
index e95b838a2..0f60995a3 100644
--- a/src/modules/m_cgiirc.cpp
+++ b/src/modules/m_cgiirc.cpp
@@ -192,8 +192,7 @@ class ModuleCgiIRC : public Module
 	{
 		ServerInstance->Users->RemoveCloneCounts(user);
 		user->SetClientIP(newip.c_str());
-		ServerInstance->Users->AddLocalClone(user);
-		ServerInstance->Users->AddGlobalClone(user);
+		ServerInstance->Users->AddClone(user);
 	}
 
 	void HandleIdentOrPass(LocalUser* user, const std::string& newip, bool was_pass)
diff --git a/src/modules/m_check.cpp b/src/modules/m_check.cpp
index d7cfe74bd..ad0e2394d 100644
--- a/src/modules/m_check.cpp
+++ b/src/modules/m_check.cpp
@@ -230,8 +230,9 @@ class CommandCheck : public Command
 				/*
 			 	 * Unlike Asuka, I define a clone as coming from the same host. --w00t
 			 	 */
-				user->SendText("%s member %-3lu %s%s (%s@%s) %s ",
-					checkstr.c_str(), ServerInstance->Users->GlobalCloneCount(i->first),
+				const UserManager::CloneCounts& clonecount = ServerInstance->Users->GetCloneCounts(i->first);
+				user->SendText("%s member %-3u %s%s (%s@%s) %s ",
+					checkstr.c_str(), clonecount.global,
 					i->second->GetAllPrefixChars(), i->first->nick.c_str(),
 					i->first->ident.c_str(), i->first->dhost.c_str(), i->first->fullname.c_str());
 			}
diff --git a/src/modules/m_clones.cpp b/src/modules/m_clones.cpp
index 1f505e9a2..c51c8d3b4 100644
--- a/src/modules/m_clones.cpp
+++ b/src/modules/m_clones.cpp
@@ -48,11 +48,12 @@ class CommandClones : public Command
 		user->WriteServ(clonesstr + " START");
 
 		/* hostname or other */
-		// XXX I really don't like marking global_clones public for this. at all. -- w00t
-		for (clonemap::iterator x = ServerInstance->Users->global_clones.begin(); x != ServerInstance->Users->global_clones.end(); x++)
+		const UserManager::CloneMap& clonemap = ServerInstance->Users->GetCloneMap();
+		for (UserManager::CloneMap::const_iterator i = clonemap.begin(); i != clonemap.end(); ++i)
 		{
-			if (x->second >= limit)
-				user->WriteServ(clonesstr + " "+ ConvToStr(x->second) + " " + x->first.str());
+			const UserManager::CloneCounts& counts = i->second;
+			if (counts.global >= limit)
+				user->WriteServ(clonesstr + " " + ConvToStr(counts.global) + " " + i->first.str());
 		}
 
 		user->WriteServ(clonesstr + " END");
diff --git a/src/modules/m_spanningtree/uid.cpp b/src/modules/m_spanningtree/uid.cpp
index f248b3860..79bd6936f 100644
--- a/src/modules/m_spanningtree/uid.cpp
+++ b/src/modules/m_spanningtree/uid.cpp
@@ -126,7 +126,7 @@ CmdResult CommandUID::HandleServer(TreeServer* remoteserver, std::vector<std::st
 
 	_new->SetClientIP(params[6].c_str());
 
-	ServerInstance->Users->AddGlobalClone(_new);
+	ServerInstance->Users->AddClone(_new);
 	remoteserver->UserCount++;
 
 	bool dosend = true;
diff --git a/src/usermanager.cpp b/src/usermanager.cpp
index ee6eafbb5..5dc410fff 100644
--- a/src/usermanager.cpp
+++ b/src/usermanager.cpp
@@ -73,8 +73,7 @@ void UserManager::AddUser(int socket, ListenSocket* via, irc::sockets::sockaddrs
 	New->signon = ServerInstance->Time() + ServerInstance->Config->dns_timeout;
 	New->lastping = 1;
 
-	ServerInstance->Users->AddLocalClone(New);
-	ServerInstance->Users->AddGlobalClone(New);
+	this->AddClone(New);
 
 	this->local_users.push_front(New);
 
@@ -203,58 +202,40 @@ void UserManager::QuitUser(User* user, const std::string& quitreason, const std:
 	user->PurgeEmptyChannels();
 }
 
-void UserManager::AddLocalClone(User *user)
+void UserManager::AddClone(User* user)
 {
-	local_clones[user->GetCIDRMask()]++;
-}
-
-void UserManager::AddGlobalClone(User *user)
-{
-	global_clones[user->GetCIDRMask()]++;
+	CloneCounts& counts = clonemap[user->GetCIDRMask()];
+	counts.global++;
+	if (IS_LOCAL(user))
+		counts.local++;
 }
 
 void UserManager::RemoveCloneCounts(User *user)
 {
-	if (IS_LOCAL(user))
+	CloneMap::iterator it = clonemap.find(user->GetCIDRMask());
+	if (it != clonemap.end())
 	{
-		clonemap::iterator x = local_clones.find(user->GetCIDRMask());
-		if (x != local_clones.end())
+		CloneCounts& counts = it->second;
+		counts.global--;
+		if (counts.global == 0)
 		{
-			x->second--;
-			if (!x->second)
-			{
-				local_clones.erase(x);
-			}
+			// No more users from this IP, remove entry from the map
+			clonemap.erase(it);
+			return;
 		}
-	}
 
-	clonemap::iterator y = global_clones.find(user->GetCIDRMask());
-	if (y != global_clones.end())
-	{
-		y->second--;
-		if (!y->second)
-		{
-			global_clones.erase(y);
-		}
+		if (IS_LOCAL(user))
+			counts.local--;
 	}
 }
 
-unsigned long UserManager::GlobalCloneCount(User *user)
-{
-	clonemap::iterator x = global_clones.find(user->GetCIDRMask());
-	if (x != global_clones.end())
-		return x->second;
-	else
-		return 0;
-}
-
-unsigned long UserManager::LocalCloneCount(User *user)
+const UserManager::CloneCounts& UserManager::GetCloneCounts(User* user) const
 {
-	clonemap::iterator x = local_clones.find(user->GetCIDRMask());
-	if (x != local_clones.end())
-		return x->second;
+	CloneMap::const_iterator it = clonemap.find(user->GetCIDRMask());
+	if (it != clonemap.end())
+		return it->second;
 	else
-		return 0;
+		return zeroclonecounts;
 }
 
 void UserManager::ServerNoticeAll(const char* text, ...)
diff --git a/src/users.cpp b/src/users.cpp
index 6a31f0a93..a78e9635b 100644
--- a/src/users.cpp
+++ b/src/users.cpp
@@ -497,14 +497,15 @@ void LocalUser::CheckClass(bool clone_count)
 	}
 	else if (clone_count)
 	{
-		if ((a->GetMaxLocal()) && (ServerInstance->Users->LocalCloneCount(this) > a->GetMaxLocal()))
+		const UserManager::CloneCounts& clonecounts = ServerInstance->Users->GetCloneCounts(this);
+		if ((a->GetMaxLocal()) && (clonecounts.local > a->GetMaxLocal()))
 		{
 			ServerInstance->Users->QuitUser(this, "No more connections allowed from your host via this connect class (local)");
 			if (a->maxconnwarn)
 				ServerInstance->SNO->WriteToSnoMask('a', "WARNING: maximum LOCAL connections (%ld) exceeded for IP %s", a->GetMaxLocal(), this->GetIPString().c_str());
 			return;
 		}
-		else if ((a->GetMaxGlobal()) && (ServerInstance->Users->GlobalCloneCount(this) > a->GetMaxGlobal()))
+		else if ((a->GetMaxGlobal()) && (clonecounts.global > a->GetMaxGlobal()))
 		{
 			ServerInstance->Users->QuitUser(this, "No more connections allowed from your host via this connect class (global)");
 			if (a->maxconnwarn)