Clean up client and server command sending / tables

src/client/client.cpp

@@ -83,8 +83,10 @@ u32 PacketCounter::sum() const
 void PacketCounter::print(std::ostream &o) const
 {
 	for (const auto &it : m_packets) {
-		auto name = it.first >= TOCLIENT_NUM_MSG_TYPES ? "?"
+		auto name = it.first >= TOCLIENT_NUM_MSG_TYPES ? nullptr
 			: toClientCommandTable[it.first].name;
+		if (!name)
+			name = "?";
 		o << "cmd " << it.first << " (" << name << ") count "
 			<< it.second << std::endl;
 	}
@@ -991,27 +993,25 @@ inline void Client::handleCommand(NetworkPacket* pkt)
 void Client::ProcessData(NetworkPacket *pkt)
 {
 	ToClientCommand command = (ToClientCommand) pkt->getCommand();
-	u32 sender_peer_id = pkt->getPeerId();
 
-	//infostream<<"Client: received command="<<command<<std::endl;
+	m_packetcounter.add(static_cast<u16>(command));
-	m_packetcounter.add((u16)command);
 	g_profiler->graphAdd("client_received_packets", 1);
 
 	/*
 		If this check is removed, be sure to change the queue
 		system to know the ids
 	*/
-	if(sender_peer_id != PEER_ID_SERVER) {
+	if (pkt->getPeerId() != PEER_ID_SERVER) {
 		infostream << "Client::ProcessData(): Discarding data not "
-			"coming from server: peer_id=" << sender_peer_id << " command=" << pkt->getCommand()
+			"coming from server: peer_id=" << static_cast<int>(pkt->getPeerId())
-			<< std::endl;
+			<< " command=" << static_cast<unsigned>(command) << std::endl;
 		return;
 	}
 
 	// Command must be handled into ToClientCommandHandler
 	if (command >= TOCLIENT_NUM_MSG_TYPES) {
 		infostream << "Client: Ignoring unknown command "
-			<< command << std::endl;
+			<< static_cast<unsigned>(command) << std::endl;
 		return;
 	}
 
@@ -1027,24 +1027,19 @@ void Client::ProcessData(NetworkPacket *pkt)
 
 	if (m_server_ser_ver == SER_FMT_VER_INVALID) {
 		infostream << "Client: Server serialization"
-				" format invalid or not initialized."
+				" format invalid. Skipping incoming command "
-				" Skipping incoming command=" << command << std::endl;
+				<< static_cast<unsigned>(command) << std::endl;
 		return;
 	}
 
-	/*
-	  Handle runtime commands
-	*/
-
 	handleCommand(pkt);
 }
 
 void Client::Send(NetworkPacket* pkt)
 {
-	m_con->Send(PEER_ID_SERVER,
+	auto &scf = serverCommandFactoryTable[pkt->getCommand()];
-		serverCommandFactoryTable[pkt->getCommand()].channel,
+	FATAL_ERROR_IF(!scf.name, "packet type missing in table");
-		pkt,
+	m_con->Send(PEER_ID_SERVER, scf.channel, pkt, scf.reliable);
-		serverCommandFactoryTable[pkt->getCommand()].reliable);
 }
 
 // Will fill up 12 + 12 + 4 + 4 + 4 + 1 + 1 + 1 bytes

src/clientiface.cpp

@@ -784,10 +784,21 @@ void ClientInterface::UpdatePlayerList()
 	}
 }
 
-void ClientInterface::send(session_t peer_id, u8 channelnum,
+void ClientInterface::send(session_t peer_id, NetworkPacket *pkt)
-		NetworkPacket *pkt, bool reliable)
 {
-	m_con->Send(peer_id, channelnum, pkt, reliable);
+	auto &ccf = clientCommandFactoryTable[pkt->getCommand()];
+	FATAL_ERROR_IF(!ccf.name, "packet type missing in table");
+
+	m_con->Send(peer_id, ccf.channel, pkt, ccf.reliable);
+}
+
+void ClientInterface::sendCustom(session_t peer_id, u8 channel, NetworkPacket *pkt, bool reliable)
+{
+	// check table anyway to prevent mistakes
+	FATAL_ERROR_IF(!clientCommandFactoryTable[pkt->getCommand()].name,
+		"packet type missing in table");
+
+	m_con->Send(peer_id, channel, pkt, reliable);
 }
 
 void ClientInterface::sendToAll(NetworkPacket *pkt)
@@ -797,9 +808,9 @@ void ClientInterface::sendToAll(NetworkPacket *pkt)
 		RemoteClient *client = client_it.second;
 
 		if (client->net_proto_version != 0) {
-			m_con->Send(client->peer_id,
+			auto &ccf = clientCommandFactoryTable[pkt->getCommand()];
-					clientCommandFactoryTable[pkt->getCommand()].channel, pkt,
+			FATAL_ERROR_IF(!ccf.name, "packet type missing in table");
-					clientCommandFactoryTable[pkt->getCommand()].reliable);
+			m_con->Send(client->peer_id, ccf.channel, pkt, ccf.reliable);
 		}
 	}
 }

src/clientiface.h

@@ -482,8 +482,11 @@ public:
 	/* get list of client player names */
 	const std::vector<std::string> &getPlayerNames() const { return m_clients_names; }
 
-	/* send message to client */
+	/* send to one client */
-	void send(session_t peer_id, u8 channelnum, NetworkPacket *pkt, bool reliable);
+	void send(session_t peer_id, NetworkPacket *pkt);
+
+	/* send to one client, deviating from the standard params */
+	void sendCustom(session_t peer_id, u8 channel, NetworkPacket *pkt, bool reliable);
 
 	/* send to all clients */
 	void sendToAll(NetworkPacket *pkt);

src/network/clientopcodes.cpp

@@ -19,8 +19,10 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 */
 
 #include "clientopcodes.h"
+#include "client/client.h"
 
-const static ToClientCommandHandler null_command_handler = {"TOCLIENT_NULL", TOCLIENT_STATE_ALL, &Client::handleCommand_Null};
+const static ToClientCommandHandler null_command_handler =
+	{"TOCLIENT_NULL", TOCLIENT_STATE_ALL, &Client::handleCommand_Null};
 
 const ToClientCommandHandler toClientCommandTable[TOCLIENT_NUM_MSG_TYPES] =
 {
@@ -126,7 +128,7 @@ const ToClientCommandHandler toClientCommandTable[TOCLIENT_NUM_MSG_TYPES] =
 	{ "TOCLIENT_SET_LIGHTING",             TOCLIENT_STATE_CONNECTED, &Client::handleCommand_SetLighting }, // 0x63,
 };
 
-const static ServerCommandFactory null_command_factory = { "TOSERVER_NULL", 0, false };
+const static ServerCommandFactory null_command_factory = { nullptr, 0, false };
 
 /*
 	Channels used for Client -> Server communication
@@ -223,5 +225,5 @@ const ServerCommandFactory serverCommandFactoryTable[TOSERVER_NUM_MSG_TYPES] =
 	{ "TOSERVER_FIRST_SRP",          1, true }, // 0x50
 	{ "TOSERVER_SRP_BYTES_A",        1, true }, // 0x51
 	{ "TOSERVER_SRP_BYTES_M",        1, true }, // 0x52
-	{ "TOSERVER_UPDATE_CLIENT_INFO", 1, true }, // 0x53
+	{ "TOSERVER_UPDATE_CLIENT_INFO", 2, true }, // 0x53
 };

src/network/clientopcodes.h

@@ -20,10 +20,10 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #pragma once
 
-#include "client/client.h"
 #include "networkprotocol.h"
 
 class NetworkPacket;
+class Client;
 
 enum ToClientConnectionState {
 	TOCLIENT_STATE_NOT_CONNECTED,

src/network/networkprotocol.h

@@ -251,7 +251,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 typedef u16 session_t;
 
-enum ToClientCommand
+enum ToClientCommand : u16
 {
 	TOCLIENT_HELLO = 0x02,
 	/*
@@ -288,9 +288,7 @@ enum ToClientCommand
 		u8 (bool) reconnect
 	*/
 
-	TOCLIENT_INIT_LEGACY = 0x10, // Obsolete
+	TOCLIENT_BLOCKDATA = 0x20,
-
-	TOCLIENT_BLOCKDATA = 0x20, //TODO: Multiple blocks
 	TOCLIENT_ADDNODE = 0x21,
 	/*
 		v3s16 position
@@ -299,23 +297,15 @@ enum ToClientCommand
 	*/
 	TOCLIENT_REMOVENODE = 0x22,
 
-	TOCLIENT_PLAYERPOS = 0x23, // Obsolete
-	TOCLIENT_PLAYERINFO = 0x24, // Obsolete
-	TOCLIENT_OPT_BLOCK_NOT_FOUND = 0x25, // Obsolete
-	TOCLIENT_SECTORMETA = 0x26, // Obsolete
-
 	TOCLIENT_INVENTORY = 0x27,
 	/*
 		[0] u16 command
 		[2] serialized inventory
 	*/
 
-	TOCLIENT_OBJECTDATA = 0x28, // Obsolete
-
 	TOCLIENT_TIME_OF_DAY = 0x29,
 	/*
 		u16 time (0-23999)
-		Added in a later version:
 		f1000 time_speed
 	*/
 
@@ -337,8 +327,6 @@ enum ToClientCommand
 		bool should_be_cached
 	*/
 
-	// (oops, there is some gap here)
-
 	TOCLIENT_CHAT_MESSAGE = 0x2F,
 	/*
 		u8 version
@@ -349,8 +337,6 @@ enum ToClientCommand
 		wstring message
 	*/
 
-	TOCLIENT_CHAT_MESSAGE_OLD = 0x30, // Obsolete
-
 	TOCLIENT_ACTIVE_OBJECT_REMOVE_ADD = 0x31,
 	/*
 		u16 count of removed objects
@@ -424,26 +410,13 @@ enum ToClientCommand
 		string url
 	*/
 
-	TOCLIENT_TOOLDEF = 0x39,
-	/*
-		u32 length of the next item
-		serialized ToolDefManager
-	*/
-
 	TOCLIENT_NODEDEF = 0x3a,
 	/*
 		u32 length of the next item
 		serialized NodeDefManager
 	*/
 
-	TOCLIENT_CRAFTITEMDEF = 0x3b,
-	/*
-		u32 length of the next item
-		serialized CraftiItemDefManager
-	*/
-
 	TOCLIENT_ANNOUNCE_MEDIA = 0x3c,
-
 	/*
 		u32 number of files
 		for each texture {
@@ -647,8 +620,6 @@ enum ToClientCommand
 
 	*/
 
-	TOCLIENT_DELETE_PARTICLESPAWNER_LEGACY = 0x48, // Obsolete
-
 	TOCLIENT_HUDADD = 0x49,
 	/*
 		u32 id
@@ -882,7 +853,7 @@ enum ToClientCommand
 	TOCLIENT_NUM_MSG_TYPES = 0x64,
 };
 
-enum ToServerCommand
+enum ToServerCommand : u16
 {
 	TOSERVER_INIT = 0x02,
 	/*
@@ -895,14 +866,10 @@ enum ToServerCommand
 		std::string player name
 	*/
 
-	TOSERVER_INIT_LEGACY = 0x10, // Obsolete
-
 	TOSERVER_INIT2 = 0x11,
 	/*
-		Sent as an ACK for TOCLIENT_INIT.
+		Sent as an ACK for TOCLIENT_AUTH_ACCEPT.
 		After this, the server can send data.
-
-		[0] u16 TOSERVER_INIT2
 	*/
 
 	TOSERVER_MODCHANNEL_JOIN = 0x17,
@@ -925,10 +892,6 @@ enum ToServerCommand
 	 	std::string message
 	 */
 
-	TOSERVER_GETBLOCK = 0x20, // Obsolete
-	TOSERVER_ADDNODE = 0x21, // Obsolete
-	TOSERVER_REMOVENODE = 0x22, // Obsolete
-
 	TOSERVER_PLAYERPOS = 0x23,
 	/*
 		[0] u16 command
@@ -961,12 +924,6 @@ enum ToServerCommand
 		...
 	*/
 
-	TOSERVER_ADDNODE_FROM_INVENTORY = 0x26, // Obsolete
-	TOSERVER_CLICK_OBJECT = 0x27, // Obsolete
-	TOSERVER_GROUND_ACTION = 0x28, // Obsolete
-	TOSERVER_RELEASE = 0x29, // Obsolete
-	TOSERVER_SIGNTEXT = 0x30, // Obsolete
-
 	TOSERVER_INVENTORY_ACTION = 0x31,
 	/*
 		See InventoryAction in inventorymanager.h
@@ -978,16 +935,11 @@ enum ToServerCommand
 		wstring message
 	*/
 
-	TOSERVER_SIGNNODETEXT = 0x33, // Obsolete
-	TOSERVER_CLICK_ACTIVEOBJECT = 0x34, // Obsolete
-
 	TOSERVER_DAMAGE = 0x35,
 	/*
 		u8 amount
 	*/
 
-	TOSERVER_PASSWORD_LEGACY = 0x36, // Obsolete
-
 	TOSERVER_PLAYERITEM = 0x37,
 	/*
 		Sent to change selected item.
@@ -1063,8 +1015,6 @@ enum ToServerCommand
 			u32 token
 	*/
 
-	TOSERVER_BREATH = 0x42, // Obsolete
-
 	TOSERVER_CLIENT_READY = 0x43,
 	/*
 		u8 major

src/network/serveropcodes.cpp

@@ -19,8 +19,10 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 */
 
 #include "serveropcodes.h"
+#include "server.h"
 
-const static ToServerCommandHandler null_command_handler = { "TOSERVER_NULL", TOSERVER_STATE_ALL, &Server::handleCommand_Null };
+const static ToServerCommandHandler null_command_handler =
+	{ "TOSERVER_NULL", TOSERVER_STATE_ALL, &Server::handleCommand_Null };
 
 const ToServerCommandHandler toServerCommandTable[TOSERVER_NUM_MSG_TYPES] =
 {
@@ -110,7 +112,7 @@ const ToServerCommandHandler toServerCommandTable[TOSERVER_NUM_MSG_TYPES] =
 	{ "TOSERVER_UPDATE_CLIENT_INFO",       TOSERVER_STATE_INGAME, &Server::handleCommand_UpdateClientInfo }, // 0x53
 };
 
-const static ClientCommandFactory null_command_factory = { "TOCLIENT_NULL", 0, false };
+const static ClientCommandFactory null_command_factory = { nullptr, 0, false };
 
 /*
 	Channels used for Server -> Client communication
@@ -140,7 +142,7 @@ const ClientCommandFactory clientCommandFactoryTable[TOCLIENT_NUM_MSG_TYPES] =
 	null_command_factory, // 0x0D
 	null_command_factory, // 0x0E
 	null_command_factory, // 0x0F
-	{ "TOCLIENT_INIT",                     0, true }, // 0x10
+	null_command_factory, // 0x10
 	null_command_factory, // 0x11
 	null_command_factory, // 0x12
 	null_command_factory, // 0x13
@@ -217,7 +219,7 @@ const ClientCommandFactory clientCommandFactoryTable[TOCLIENT_NUM_MSG_TYPES] =
 	{ "TOCLIENT_SET_SUN",                  0, true }, // 0x5a
 	{ "TOCLIENT_SET_MOON",                 0, true }, // 0x5b
 	{ "TOCLIENT_SET_STARS",                0, true }, // 0x5c
-	null_command_factory, // 0x5d
+	{ "TOCLIENT_MOVE_PLAYER_REL",          0, true }, // 0x5d
 	null_command_factory, // 0x5e
 	null_command_factory, // 0x5f
 	{ "TOCLIENT_SRP_BYTES_S_B",            0, true }, // 0x60

src/network/serveropcodes.h

@@ -20,10 +20,10 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #pragma once
 
-#include "server.h"
 #include "networkprotocol.h"
 
 class NetworkPacket;
+class Server;
 
 enum ToServerConnectionState {
 	TOSERVER_STATE_NOT_CONNECTED,
@@ -33,7 +33,7 @@ enum ToServerConnectionState {
 };
 struct ToServerCommandHandler
 {
-	const std::string name;
+	const char *name;
 	ToServerConnectionState state;
 	void (Server::*handler)(NetworkPacket* pkt);
 };

src/server.cpp

@@ -1062,11 +1062,13 @@ void Server::Receive(float timeout)
 			infostream << "Server::Receive(): SerializationError: what()="
 					<< e.what() << std::endl;
 		} catch (const ClientStateError &e) {
-			errorstream << "ProcessData: peer=" << peer_id << " what()="
+			errorstream << "ClientStateError: peer=" << peer_id << " what()="
 					 << e.what() << std::endl;
 			DenyAccess(peer_id, SERVER_ACCESSDENIED_UNEXPECTED_DATA);
-		} catch (const con::PeerNotFoundException &e) {
+		} catch (con::PeerNotFoundException &e) {
-			// Do nothing
+			infostream << "Server: PeerNotFoundException" << std::endl;
+		} catch (ClientNotFoundException &e) {
+			infostream << "Server: ClientNotFoundException" << std::endl;
 		}
 	}
 }
@@ -1189,7 +1191,7 @@ void Server::ProcessData(NetworkPacket *pkt)
 		// Command must be handled into ToServerCommandHandler
 		if (command >= TOSERVER_NUM_MSG_TYPES) {
 			infostream << "Server: Ignoring unknown command "
-					 << command << std::endl;
+					 << static_cast<unsigned>(command) << std::endl;
 			return;
 		}
 
@@ -1201,9 +1203,9 @@ void Server::ProcessData(NetworkPacket *pkt)
 		u8 peer_ser_ver = getClient(peer_id, CS_InitDone)->serialization_version;
 
 		if(peer_ser_ver == SER_FMT_VER_INVALID) {
-			errorstream << "Server::ProcessData(): Cancelling: Peer"
+			errorstream << "Server: Peer serialization format invalid. "
-					" serialization format invalid or not initialized."
+					"Skipping incoming command "
-					" Skipping incoming command=" << command << std::endl;
+					<< static_cast<unsigned>(command) << std::endl;
 			return;
 		}
 
@@ -1216,9 +1218,10 @@ void Server::ProcessData(NetworkPacket *pkt)
 		if (m_clients.getClientState(peer_id) < CS_Active) {
 			if (command == TOSERVER_PLAYERPOS) return;
 
-			errorstream << "Got packet command: " << command << " for peer id "
+			errorstream << "Server: Got packet command "
-					<< peer_id << " but client isn't active yet. Dropping packet "
+					<< static_cast<unsigned>(command)
-					<< std::endl;
+					<< " for peer id " << peer_id
+					<< " but client isn't active yet. Dropping packet." << std::endl;
 			return;
 		}
 
@@ -1346,15 +1349,13 @@ void Server::printToConsoleOnly(const std::string &text)
 
 void Server::Send(NetworkPacket *pkt)
 {
+	FATAL_ERROR_IF(pkt->getPeerId() == 0, "Server::Send() missing peer ID");
 	Send(pkt->getPeerId(), pkt);
 }
 
 void Server::Send(session_t peer_id, NetworkPacket *pkt)
 {
-	m_clients.send(peer_id,
+	m_clients.send(peer_id, pkt);
-		clientCommandFactoryTable[pkt->getCommand()].channel,
-		pkt,
-		clientCommandFactoryTable[pkt->getCommand()].reliable);
 }
 
 void Server::SendMovement(session_t peer_id)
@@ -2137,9 +2138,8 @@ void Server::SendActiveObjectMessages(session_t peer_id, const std::string &data
 
 	pkt.putRawString(datas.c_str(), datas.size());
 
-	m_clients.send(pkt.getPeerId(),
+	auto &ccf = clientCommandFactoryTable[pkt.getCommand()];
-			reliable ? clientCommandFactoryTable[pkt.getCommand()].channel : 1,
+	m_clients.sendCustom(pkt.getPeerId(), reliable ? ccf.channel : 1, &pkt, reliable);
-			&pkt, reliable);
 }
 
 void Server::SendCSMRestrictionFlags(session_t peer_id)
@@ -2237,12 +2237,12 @@ s32 Server::playSound(ServerPlayingSound &params, bool ephemeral)
 			<< params.spec.loop << params.spec.fade << params.spec.pitch
 			<< ephemeral << params.spec.start_time;
 
-	bool as_reliable = !ephemeral;
+	const bool as_reliable = !ephemeral;
 
 	for (const session_t peer_id : dst_clients) {
 		if (!ephemeral)
 			params.clients.insert(peer_id);
-		m_clients.send(peer_id, 0, &pkt, as_reliable);
+		m_clients.sendCustom(peer_id, 0, &pkt, as_reliable);
 	}
 
 	if (!ephemeral)
@@ -2261,8 +2261,7 @@ void Server::stopSound(s32 handle)
 	pkt << handle;
 
 	for (session_t peer_id : psound.clients) {
-		// Send as reliable
+		Send(peer_id, &pkt);
-		m_clients.send(peer_id, 0, &pkt, true);
 	}
 
 	// Remove sound reference
@@ -2282,8 +2281,7 @@ void Server::fadeSound(s32 handle, float step, float gain)
 	pkt << handle << step << gain;
 
 	for (session_t peer_id : psound.clients) {
-		// Send as reliable
+		Send(peer_id, &pkt);
-		m_clients.send(peer_id, 0, &pkt, true);
 	}
 
 	// Remove sound reference
@@ -2340,8 +2338,7 @@ void Server::sendNodeChangePkt(NetworkPacket &pkt, v3s16 block_pos,
 			continue;
 		}
 
-		// Send as reliable
+		Send(client_id, &pkt);
-		m_clients.send(client_id, 0, &pkt, true);
 	}
 }
 
@@ -3670,8 +3667,8 @@ bool Server::dynamicAddMedia(std::string filepath,
 					to push the media over ALL channels to ensure it is processed before
 					it is used. In practice this means channels 1 and 0.
 				*/
-				m_clients.send(peer_id, 1, &legacy_pkt, true);
+				m_clients.sendCustom(peer_id, 1, &legacy_pkt, true);
-				m_clients.send(peer_id, 0, &legacy_pkt, true);
+				m_clients.sendCustom(peer_id, 0, &legacy_pkt, true);
 			} else {
 				waiting.emplace(peer_id);
 				Send(peer_id, &pkt);