Make error reporting work properly, it seemed to loose errors.

First change: Add a culling list of inspsockets waiting to be closed, so the close() is less likely to be called before the buffer is entirely empty. This seems to work well on my LAN.
Second change: Add a SendError() method, rather than WriteLine("ERROR : ..."). This can be used to effect by also echoing out "Sent ERROR to %s: ..." onto the +l snomask meaning at least one end
will always see the error even if the ERROR command is lost due to latency or design of the transport (e.g. ssl which may be writing during a read event etc)


git-svn-id: http://svn.inspircd.org/repository/trunk/inspircd@6844 e03df62e-2008-0410-955e-edbf42e46eb7

6 files changed, 65 insertions, 42 deletions

diff --git a/include/inspircd.h b/include/inspircd.h
index bfd2ea421..d9918aa73 100644
--- a/include/inspircd.h
+++ b/include/inspircd.h
@@ -432,6 +432,8 @@ class InspIRCd : public classbase
 
  public:
 
+	std::map<InspSocket*,InspSocket*> SocketCull;
+
 	/** Build the ISUPPORT string by triggering all modules On005Numeric events
 	 */
 	void BuildISupport();
diff --git a/src/inspircd.cpp b/src/inspircd.cpp
index bacd92961..670b24441 100644
--- a/src/inspircd.cpp
+++ b/src/inspircd.cpp
@@ -911,6 +911,14 @@ void InspIRCd::DoOneIteration(bool process_module_sockets)
 	/* if any users was quit, take them out */
 	GlobalCulls.Apply();
 
+	/* If any inspsockets closed, remove them */
+	for (std::map<InspSocket*,InspSocket*>::iterator x = SocketCull.begin(); x != SocketCull.end(); ++x)
+	{
+		SE->DelFd(x->second);
+		x->second->Close();
+		delete x->second;
+	}
+	SocketCull.clear();
 }
 
 bool InspIRCd::IsIdent(const char* n)
@@ -933,7 +941,6 @@ bool InspIRCd::IsIdent(const char* n)
 	return true;
 }
 
-
 int InspIRCd::Run()
 {
 	while (true)
diff --git a/src/inspsocket.cpp b/src/inspsocket.cpp
index 5641ba6ef..3c30fd6e8 100644
--- a/src/inspsocket.cpp
+++ b/src/inspsocket.cpp
@@ -573,14 +573,16 @@ void SocketTimeout::Tick(time_t now)
 		this->sock->OnTimeout();
 		this->sock->OnError(I_ERR_TIMEOUT);
 		this->sock->timeout = true;
-		ServerInstance->SE->DelFd(this->sock);
+
 		/* NOTE: We must set this AFTER DelFd, as we added
 		 * this socket whilst writeable. This means that we
 		 * must DELETE the socket whilst writeable too!
 		 */
 		this->sock->state = I_ERROR;
-		this->sock->Close();
-		delete this->sock;
+
+		if (ServerInstance->SocketCull.find(this->sock) == ServerInstance->SocketCull.end())
+			ServerInstance->SocketCull[this->sock] = this->sock;
+
 		return;
 	}
 	else
@@ -717,17 +719,15 @@ void InspSocket::HandleEvent(EventType et, int errornum)
 	switch (et)
 	{
 		case EVENT_ERROR:
-			this->Instance->SE->DelFd(this);
-			this->Close();
-			delete this;
+			if (this->Instance->SocketCull.find(this) == this->Instance->SocketCull.end())
+				this->Instance->SocketCull[this] = this;
 			return;
 		break;
 		case EVENT_READ:
 			if (!this->Poll())
 			{
-				this->Instance->SE->DelFd(this);
-				this->Close();
-				delete this;
+				if (this->Instance->SocketCull.find(this) == this->Instance->SocketCull.end())
+					this->Instance->SocketCull[this] = this;
 				return;
 			}
 		break;
@@ -737,9 +737,8 @@ void InspSocket::HandleEvent(EventType et, int errornum)
 				this->WaitingForWriteEvent = false;
 				if (!this->OnWriteReady())
 				{
-					this->Instance->SE->DelFd(this);
-					this->Close();
-					delete this;
+					if (this->Instance->SocketCull.find(this) == this->Instance->SocketCull.end())
+						this->Instance->SocketCull[this] = this;
 					return;
 				}
 			}
@@ -758,9 +757,8 @@ void InspSocket::HandleEvent(EventType et, int errornum)
 			{
 				if (this->FlushWriteBuffer())
 				{
-					this->Instance->SE->DelFd(this);
-					this->Close();
-					delete this;
+					if (this->Instance->SocketCull.find(this) == this->Instance->SocketCull.end())
+						this->Instance->SocketCull[this] = this;
 					return;
 				}
 			}
diff --git a/src/modules/m_spanningtree/treesocket.h b/src/modules/m_spanningtree/treesocket.h
index ff06f0926..15ca8f4cb 100644
--- a/src/modules/m_spanningtree/treesocket.h
+++ b/src/modules/m_spanningtree/treesocket.h
@@ -171,6 +171,11 @@ class TreeSocket : public InspSocket
 	 */
 	virtual void OnError(InspSocketError e);
 
+	/** Sends an error to the remote server, and displays it locally to show
+	 * that it was sent.
+	 */
+	void SendError(const std::string &errormessage);
+
 	/** Handle socket disconnect event
 	 */
 	virtual int OnDisconnect();
diff --git a/src/modules/m_spanningtree/treesocket1.cpp b/src/modules/m_spanningtree/treesocket1.cpp
index 21d6b912f..a05ed6e4c 100644
--- a/src/modules/m_spanningtree/treesocket1.cpp
+++ b/src/modules/m_spanningtree/treesocket1.cpp
@@ -373,11 +373,17 @@ std::string TreeSocket::ListDifference(const std::string &one, const std::string
 	return result;
 }
 
+void TreeSocket::SendError(const std::string &errormessage)
+{
+	this->WriteLine("ERROR :"+errormessage);
+	this->Instance->SNO->WriteToSnoMask('l',"Sent \2ERROR\2 to "+this->InboundServerName+": "+errormessage);
+}
+
 bool TreeSocket::Capab(const std::deque<std::string> &params)
 {
 	if (params.size() < 1)
 	{
-		this->WriteLine("ERROR :Invalid number of parameters for CAPAB - Mismatched version");
+		this->SendError("Invalid number of parameters for CAPAB - Mismatched version");
 		return false;
 	}
 	if (params[0] == "START")
@@ -467,7 +473,7 @@ bool TreeSocket::Capab(const std::deque<std::string> &params)
 
 		if (reason.length())
 		{
-			this->WriteLine("ERROR :CAPAB negotiation failed: "+reason);
+			this->SendError("CAPAB negotiation failed: "+reason);
 			return false;
 		}
 	}
@@ -843,7 +849,7 @@ bool TreeSocket::ForceJoin(const std::string &source, std::deque<std::string> &p
 					 * danger bill bobbertson! (that's will robinsons older brother ;-) ...)
 					 */
 					this->Instance->WriteOpers("ERROR: We received a user with an unknown prefix '%c'. Closed connection to avoid a desync.",*permissions);
-					this->WriteLine(std::string("ERROR :Invalid prefix '")+(*permissions)+"' in FJOIN");
+					this->SendError(std::string("Invalid prefix '")+(*permissions)+"' in FJOIN");
 					return false;
 				}
 				usr++;
diff --git a/src/modules/m_spanningtree/treesocket2.cpp b/src/modules/m_spanningtree/treesocket2.cpp
index 588b12961..397a7c0b0 100644
--- a/src/modules/m_spanningtree/treesocket2.cpp
+++ b/src/modules/m_spanningtree/treesocket2.cpp
@@ -798,13 +798,13 @@ bool TreeSocket::RemoteServer(const std::string &prefix, std::deque<std::string>
 	TreeServer* ParentOfThis = Utils->FindServer(prefix);
 	if (!ParentOfThis)
 	{
-		this->WriteLine("ERROR :Protocol error - Introduced remote server from unknown server "+prefix);
+		this->SendError("Protocol error - Introduced remote server from unknown server "+prefix);
 		return false;
 	}
 	TreeServer* CheckDupe = Utils->FindServer(servername);
 	if (CheckDupe)
 	{
-		this->WriteLine("ERROR :Server "+servername+" already exists!");
+		this->SendError("Server "+servername+" already exists!");
 		this->Instance->SNO->WriteToSnoMask('l',"Server \2"+servername+"\2 being introduced from \2" + prefix + "\2 denied, already exists. Closing link with " + prefix);
 		return false;
 	}
@@ -843,15 +843,19 @@ bool TreeSocket::Outbound_Reply_Server(std::deque<std::string> &params)
 	irc::string servername = params[0].c_str();
 	std::string sname = params[0];
 	std::string password = params[1];
+	std::string description = params[3];
 	int hops = atoi(params[2].c_str());
 
+	this->InboundServerName = sname;
+	this->InboundDescription = description;
+
 	if (hops)
 	{
-		this->WriteLine("ERROR :Server too far away for authentication");
+		this->SendError("Server too far away for authentication");
 		this->Instance->SNO->WriteToSnoMask('l',"Server connection from \2"+sname+"\2 denied, server is too far away for authentication");
 		return false;
 	}
-	std::string description = params[3];
+
 	for (std::vector<Link>::iterator x = Utils->LinkBlocks.begin(); x < Utils->LinkBlocks.end(); x++)
 	{
 		if ((x->Name == servername) && ((ComparePass(this->MakePass(x->RecvPass,this->GetOurChallenge()),password)) || (x->RecvPass == password && (this->GetTheirChallenge().empty()))))
@@ -859,7 +863,7 @@ bool TreeSocket::Outbound_Reply_Server(std::deque<std::string> &params)
 			TreeServer* CheckDupe = Utils->FindServer(sname);
 			if (CheckDupe)
 			{
-				this->WriteLine("ERROR :Server "+sname+" already exists on server "+CheckDupe->GetParent()->GetName()+"!");
+				this->SendError("Server "+sname+" already exists on server "+CheckDupe->GetParent()->GetName()+"!");
 				this->Instance->SNO->WriteToSnoMask('l',"Server connection from \2"+sname+"\2 denied, already exists on server "+CheckDupe->GetParent()->GetName());
 				return false;
 			}
@@ -880,7 +884,7 @@ bool TreeSocket::Outbound_Reply_Server(std::deque<std::string> &params)
 			return true;
 		}
 	}
-	this->WriteLine("ERROR :Invalid credentials");
+	this->SendError("Invalid credentials");
 	this->Instance->SNO->WriteToSnoMask('l',"Server connection from \2"+sname+"\2 denied, invalid link credentials");
 	return false;
 }
@@ -892,15 +896,19 @@ bool TreeSocket::Inbound_Server(std::deque<std::string> &params)
 	irc::string servername = params[0].c_str();
 	std::string sname = params[0];
 	std::string password = params[1];
+	std::string description = params[3];
 	int hops = atoi(params[2].c_str());
 
+	this->InboundServerName = sname;
+	this->InboundDescription = description;
+
 	if (hops)
 	{
-		this->WriteLine("ERROR :Server too far away for authentication");
+		this->SendError("Server too far away for authentication");
 		this->Instance->SNO->WriteToSnoMask('l',"Server connection from \2"+sname+"\2 denied, server is too far away for authentication");
 		return false;
 	}
-	std::string description = params[3];
+
 	for (std::vector<Link>::iterator x = Utils->LinkBlocks.begin(); x < Utils->LinkBlocks.end(); x++)
 	{
 		if ((x->Name == servername) && ((ComparePass(this->MakePass(x->RecvPass,this->GetOurChallenge()),password) || x->RecvPass == password && (this->GetTheirChallenge().empty()))))
@@ -910,9 +918,9 @@ bool TreeSocket::Inbound_Server(std::deque<std::string> &params)
 			if (CheckDupeSocket)
 			{
 				/* If we find one, we abort the link to prevent a race condition */
-				this->WriteLine("ERROR :Negotiation collision");
+				this->SendError("Negotiation collision");
 				this->Instance->SNO->WriteToSnoMask('l',"Server connection from \2"+sname+"\2 denied, already exists in a negotiating state.");
-				CheckDupeSocket->WriteLine("ERROR :Negotiation collision");
+				CheckDupeSocket->SendError("Negotiation collision");
 				Instance->SE->DelFd(CheckDupeSocket);
 				CheckDupeSocket->Close();
 				delete CheckDupeSocket;
@@ -922,7 +930,7 @@ bool TreeSocket::Inbound_Server(std::deque<std::string> &params)
 			TreeServer* CheckDupe = Utils->FindServer(sname);
 			if (CheckDupe)
 			{
-				this->WriteLine("ERROR :Server "+sname+" already exists on server "+CheckDupe->GetParent()->GetName()+"!");
+				this->SendError("Server "+sname+" already exists on server "+CheckDupe->GetParent()->GetName()+"!");
 				this->Instance->SNO->WriteToSnoMask('l',"Server connection from \2"+sname+"\2 denied, already exists on server "+CheckDupe->GetParent()->GetName());
 				return false;
 			}
@@ -935,8 +943,6 @@ bool TreeSocket::Inbound_Server(std::deque<std::string> &params)
 
 			Utils->AddBurstingServer(sname,this);
 
-			this->InboundServerName = sname;
-			this->InboundDescription = description;
 			// this is good. Send our details: Our server name and description and hopcount of 0,
 			// along with the sendpass from this block.
 			this->WriteLine(std::string("SERVER ")+this->Instance->Config->ServerName+" "+this->MakePass(x->SendPass, this->GetTheirChallenge())+" 0 :"+this->Instance->Config->ServerDesc);
@@ -945,7 +951,7 @@ bool TreeSocket::Inbound_Server(std::deque<std::string> &params)
 			return true;
 		}
 	}
-	this->WriteLine("ERROR :Invalid credentials");
+	this->SendError("Invalid credentials");
 	this->Instance->SNO->WriteToSnoMask('l',"Server connection from \2"+sname+"\2 denied, invalid link credentials");
 	return false;
 }
@@ -1006,7 +1012,7 @@ bool TreeSocket::ProcessLine(std::string &line)
 			}
 			else if (command == "USER")
 			{
-				this->WriteLine("ERROR :Client connections to this port are prohibited.");
+				this->SendError("Client connections to this port are prohibited.");
 				return false;
 			}
 			else if (command == "CAPAB")
@@ -1015,14 +1021,13 @@ bool TreeSocket::ProcessLine(std::string &line)
 			}
 			else if ((command == "U") || (command == "S"))
 			{
-				this->WriteLine("ERROR :Cannot use the old-style mesh linking protocol with m_spanningtree.so!");
+				this->SendError("Cannot use the old-style mesh linking protocol with m_spanningtree.so!");
 				return false;
 			}
 			else
 			{
-				std::string error("ERROR :Invalid command in negotiation phase: ");
-				error.append(command.c_str());
-				this->WriteLine(error);
+				irc::string error = "Invalid command in negotiation phase: " + command;
+				this->SendError(assign(error));
 				return false;
 			}
 		break;
@@ -1037,7 +1042,7 @@ bool TreeSocket::ProcessLine(std::string &line)
 			}
 			else if ((command == "U") || (command == "S"))
 			{
-				this->WriteLine("ERROR :Cannot use the old-style mesh linking protocol with m_spanningtree.so!");
+				this->SendError("Cannot use the old-style mesh linking protocol with m_spanningtree.so!");
 				return false;
 			}
 			else if (command == "BURST")
@@ -1050,7 +1055,7 @@ bool TreeSocket::ProcessLine(std::string &line)
 					if ((delta < -300) || (delta > 300))
 					{
 						Instance->SNO->WriteToSnoMask('l',"\2ERROR\2: Your clocks are out by %d seconds (this is more than five minutes). Link aborted, \2PLEASE SYNC YOUR CLOCKS!\2",abs(delta));
-						WriteLine("ERROR :Your clocks are out by "+ConvToStr(abs(delta))+" seconds (this is more than ten minutes). Link aborted, PLEASE SYNC YOUR CLOCKS!");
+						SendError("Your clocks are out by "+ConvToStr(abs(delta))+" seconds (this is more than ten minutes). Link aborted, PLEASE SYNC YOUR CLOCKS!");
 						return false;
 					}
 					else if ((delta < -30) || (delta > 30))
@@ -1090,7 +1095,7 @@ bool TreeSocket::ProcessLine(std::string &line)
 
 		break;
 		case LISTENER:
-			this->WriteLine("ERROR :Internal error -- listening socket accepted its own descriptor!!!");
+			this->SendError("Internal error -- listening socket accepted its own descriptor!!!");
 			return false;
 		break;
 		case CONNECTING:
@@ -1447,7 +1452,7 @@ bool TreeSocket::ProcessLine(std::string &line)
 					switch (this->Instance->CallCommandHandler(command.c_str(), strparams, params.size(), who))
 					{
 						case CMD_INVALID:
-							this->WriteLine("ERROR :Unrecognised command '"+std::string(command.c_str())+"' -- possibly loaded mismatched modules");
+							this->SendError("Unrecognised command '"+std::string(command.c_str())+"' -- possibly loaded mismatched modules");
 							return false;
 						break;
 						case CMD_FAILURE: