Core/Misc: Refactor OpcodeTable reducing its size and modernize array/pointer storage

src/server/game/Server/Protocol/Opcodes.cpp

@@ -20,20 +20,17 @@
 #include "WorldSession.h"
 #include "Packets/AllPackets.h"
 
-template<class PacketClass, void(WorldSession::*HandlerFunction)(PacketClass&)>
-class PacketHandler : public ClientOpcodeHandler
+namespace
 {
-public:
-    PacketHandler(char const* name, SessionStatus status, PacketProcessing processing) : ClientOpcodeHandler(name, status, processing) { }
-
-    void Call(WorldSession* session, WorldPacket& packet) const override
-    {
-        std::remove_cv_t<PacketClass> nicePacket(std::move(packet));
-        nicePacket.Read();
-        (session->*HandlerFunction)(nicePacket);
-        session->LogUnprocessedTail(nicePacket.GetRawPacket());
-    }
-};
+template<class PacketClass, void(WorldSession::* HandlerFunction)(PacketClass&)>
+void CallHandlerWrapper(WorldSession* session, WorldPacket& packet)
+{
+    std::remove_cv_t<PacketClass> nicePacket(std::move(packet));
+    nicePacket.Read();
+    (session->*HandlerFunction)(nicePacket);
+    session->LogUnprocessedTail(nicePacket.GetRawPacket());
+}
+}
 
 OpcodeTable opcodeTable;
 
@@ -48,93 +45,100 @@ struct get_packet_class<void(WorldSession::*)(PacketClass&)>
     using type = PacketClass;
 };
 
-OpcodeTable::OpcodeTable()
-{
-    memset(_internalTableClient, 0, sizeof(_internalTableClient));
-    memset(_internalTableServer, 0, sizeof(_internalTableServer));
-}
+OpcodeTable::OpcodeTable() = default;
 
-OpcodeTable::~OpcodeTable()
-{
-    for (uint16 i = 0; i < NUM_OPCODE_HANDLERS; ++i)
-    {
-        delete _internalTableClient[i];
-        delete _internalTableServer[i];
-    }
-}
+OpcodeTable::~OpcodeTable() = default;
 
 bool OpcodeTable::ValidateClientOpcode(OpcodeClient opcode, char const* name) const
 {
-    if (uint32(opcode) == NULL_OPCODE)
+    if (opcode == UNKNOWN_OPCODE)
     {
         TC_LOG_ERROR("network", "Opcode {} does not have a value", name);
         return false;
     }
 
-    if (uint32(opcode) >= NUM_OPCODE_HANDLERS)
+    if (opcode < MIN_CMSG_OPCODE_NUMBER || opcode > MAX_CMSG_OPCODE_NUMBER)
     {
         TC_LOG_ERROR("network", "Tried to set handler for an invalid opcode {}", opcode);
         return false;
     }
 
-    if (_internalTableClient[opcode] != nullptr)
+    if ((*this)[opcode] != nullptr)
     {
-        TC_LOG_ERROR("network", "Tried to override client handler of {} with {} (opcode {})", _internalTableClient[opcode]->Name, name, opcode);
+        TC_LOG_ERROR("network", "Tried to override client handler of {} with {} (opcode {})", (*this)[opcode]->Name, name, opcode);
         return false;
     }
 
     return true;
 }
 
-template<typename Handler, Handler HandlerFunction>
-void OpcodeTable::ValidateAndSetClientOpcode(OpcodeClient opcode, char const* name, SessionStatus status, PacketProcessing processing)
+void OpcodeTable::ValidateAndSetClientOpcode(OpcodeClient opcode, char const* name, SessionStatus status, ClientOpcodeHandler::HandlerFunction call, PacketProcessing processing)
 {
     if (!ValidateClientOpcode(opcode, name))
         return;
 
-    _internalTableClient[opcode] = new PacketHandler<typename get_packet_class<Handler>::type, HandlerFunction>(name, status, processing);
+    _internalTableClient[opcode - MIN_CMSG_OPCODE_NUMBER].reset(new ClientOpcodeHandler{
+        .Name = name,
+        .Status = status,
+        .Call = call,
+        .ProcessingPlace = processing
+    });
 }
 
-void OpcodeTable::ValidateAndSetServerOpcode(OpcodeServer opcode, char const* name, SessionStatus status, ConnectionType conIdx)
+bool OpcodeTable::ValidateServerOpcode(OpcodeServer opcode, char const* name, ConnectionType conIdx) const
 {
-    if (uint32(opcode) == NULL_OPCODE)
+    if (opcode == UNKNOWN_OPCODE)
     {
         TC_LOG_ERROR("network", "Opcode {} does not have a value", name);
-        return;
+        return false;
     }
 
-    if (uint32(opcode) >= NUM_OPCODE_HANDLERS)
+    if (opcode < MIN_SMSG_OPCODE_NUMBER || opcode > MAX_SMSG_OPCODE_NUMBER)
     {
         TC_LOG_ERROR("network", "Tried to set handler for an invalid opcode {}", opcode);
-        return;
+        return false;
     }
 
     if (conIdx >= MAX_CONNECTION_TYPES)
     {
         TC_LOG_ERROR("network", "Tried to set invalid connection type {} for opcode {}", conIdx, name);
-        return;
+        return false;
     }
 
     if (IsInstanceOnlyOpcode(opcode) && conIdx != CONNECTION_TYPE_INSTANCE)
     {
         TC_LOG_ERROR("network", "Tried to set invalid connection type {} for instance only opcode {}", conIdx, name);
-        return;
+        return false;
     }
 
-    if (_internalTableServer[opcode] != nullptr)
+    if ((*this)[opcode] != nullptr)
     {
-        TC_LOG_ERROR("network", "Tried to override server handler of {} with {} (opcode {})", opcodeTable[opcode]->Name, name, opcode);
-        return;
+        TC_LOG_ERROR("network", "Tried to override server handler of {} with {} (opcode {})", (*this)[opcode]->Name, name, opcode);
+        return false;
     }
 
-    _internalTableServer[opcode] = new ServerOpcodeHandler(name, status, conIdx);
+    return true;
+}
+
+void OpcodeTable::ValidateAndSetServerOpcode(OpcodeServer opcode, char const* name, SessionStatus status, ConnectionType conIdx)
+{
+    if (!ValidateServerOpcode(opcode, name, conIdx))
+        return;
+
+    _internalTableServer[opcode - MIN_SMSG_OPCODE_NUMBER].reset(new ServerOpcodeHandler{ .Name = name, .Status = status, .ConnectionIndex = conIdx });
 }
 
 /// Correspondence between opcodes and their names
 void OpcodeTable::Initialize()
+{
+    InitializeClientOpcodes();
+    InitializeServerOpcodes();
+}
+
+void OpcodeTable::InitializeClientOpcodes()
 {
 #define DEFINE_HANDLER(opcode, status, processing, handler) \
-    ValidateAndSetClientOpcode<decltype(handler), handler>(opcode, #opcode, status, processing)
+    ValidateAndSetClientOpcode(opcode, #opcode, status, &CallHandlerWrapper<typename get_packet_class<decltype(handler)>::type, handler>, processing)
 
     DEFINE_HANDLER(CMSG_ABANDON_NPE_RESPONSE,                               STATUS_UNHANDLED, PROCESS_THREADUNSAFE, &WorldSession::Handle_NULL);
     DEFINE_HANDLER(CMSG_ACCEPT_GUILD_INVITE,                                STATUS_LOGGEDIN,  PROCESS_THREADUNSAFE, &WorldSession::HandleGuildAcceptInvite);
@@ -1002,7 +1006,10 @@ void OpcodeTable::Initialize()
     DEFINE_HANDLER(CMSG_WRAP_ITEM,                                          STATUS_LOGGEDIN,  PROCESS_THREADUNSAFE, &WorldSession::HandleWrapItem);
 
 #undef DEFINE_HANDLER
+}
 
+void OpcodeTable::InitializeServerOpcodes()
+{
 #define DEFINE_SERVER_OPCODE_HANDLER(opcode, status, con) \
     static_assert((status) == STATUS_NEVER || (status) == STATUS_UNHANDLED, "Invalid status for server opcode"); \
     ValidateAndSetServerOpcode(opcode, #opcode, status, con)
@@ -2203,15 +2210,15 @@ void OpcodeTable::Initialize()
 #undef DEFINE_SERVER_OPCODE_HANDLER
 }
 
-template<typename T>
+template<std::size_t MIN_OPCODE, std::size_t MAX_OPCODE, typename T>
 inline std::string GetOpcodeNameForLoggingImpl(T id)
 {
     uint32 opcode = uint32(id);
     char const* name = nullptr;
 
-    if (opcode < NUM_OPCODE_HANDLERS)
+    if (opcode >= MIN_OPCODE && opcode <= MAX_OPCODE)
     {
-        if (OpcodeHandler const* handler = opcodeTable[id])
+        if (auto const* handler = opcodeTable[id])
             name = handler->Name;
         else
             name = "UNKNOWN OPCODE";
@@ -2224,10 +2231,10 @@ inline std::string GetOpcodeNameForLoggingImpl(T id)
 
 std::string GetOpcodeNameForLogging(OpcodeClient opcode)
 {
-    return GetOpcodeNameForLoggingImpl(opcode);
+    return GetOpcodeNameForLoggingImpl<MIN_CMSG_OPCODE_NUMBER, MAX_CMSG_OPCODE_NUMBER>(opcode);
 }
 
 std::string GetOpcodeNameForLogging(OpcodeServer opcode)
 {
-    return GetOpcodeNameForLoggingImpl(opcode);
+    return GetOpcodeNameForLoggingImpl<MIN_SMSG_OPCODE_NUMBER, MAX_SMSG_OPCODE_NUMBER>(opcode);
 }

src/server/game/Server/Protocol/Opcodes.h

@@ -23,6 +23,8 @@
 #define _OPCODES_H
 
 #include "Define.h"
+#include <array>
+#include <memory>
 #include <string>
 
 enum ConnectionType : int8
@@ -34,13 +36,15 @@ enum ConnectionType : int8
     CONNECTION_TYPE_DEFAULT     = -1
 };
 
-enum OpcodeMisc : uint16
-{
-    MAX_OPCODE                                        = 0x3FFF,
-    NUM_OPCODE_HANDLERS                               = (MAX_OPCODE + 1),
-    UNKNOWN_OPCODE                                    = 0xFFFF,
-    NULL_OPCODE                                       = 0xBADD
-};
+constexpr std::size_t MIN_CMSG_OPCODE_NUMBER = 0x305C;
+constexpr std::size_t MAX_CMSG_OPCODE_NUMBER = 0x3A63;
+constexpr std::size_t NUM_CMSG_OPCODES = MAX_CMSG_OPCODE_NUMBER - MIN_CMSG_OPCODE_NUMBER + 1;
+
+constexpr std::size_t MIN_SMSG_OPCODE_NUMBER = 0x256C;
+constexpr std::size_t MAX_SMSG_OPCODE_NUMBER = 0x3052;
+constexpr std::size_t NUM_SMSG_OPCODES = MAX_SMSG_OPCODE_NUMBER - MIN_SMSG_OPCODE_NUMBER + 1;
+
+constexpr uint16 UNKNOWN_OPCODE = 0xBADD;   // special marker value for uninitialized WorldPackets
 
 enum OpcodeClient : uint16
 {
@@ -2107,10 +2111,10 @@ enum OpcodeServer : uint16
     SMSG_MULTIPLE_PACKETS                             = 0x3051,
 
     // Deleted opcodes, here only to allow compile
-    SMSG_ARENA_TEAM_STATS                             = 0xBADD,
+    SMSG_ARENA_TEAM_STATS                             = UNKNOWN_OPCODE,
 };
 
-inline bool IsInstanceOnlyOpcode(uint32 opcode)
+constexpr bool IsInstanceOnlyOpcode(uint32 opcode)
 {
     switch (opcode)
     {
@@ -2151,67 +2155,53 @@ enum PacketProcessing
 class WorldPacket;
 class WorldSession;
 
-class OpcodeHandler
+struct ClientOpcodeHandler final
 {
-public:
-    OpcodeHandler(char const* name, SessionStatus status) : Name(name), Status(status) { }
-    virtual ~OpcodeHandler() { }
+    using HandlerFunction = void (*)(WorldSession* session, WorldPacket& packet);
 
     char const* Name;
     SessionStatus Status;
-};
-
-class ClientOpcodeHandler : public OpcodeHandler
-{
-public:
-    ClientOpcodeHandler(char const* name, SessionStatus status, PacketProcessing processing)
-        : OpcodeHandler(name, status), ProcessingPlace(processing) { }
-
-    virtual void Call(WorldSession* session, WorldPacket& packet) const = 0;
-
+    HandlerFunction Call;
     PacketProcessing ProcessingPlace;
 };
 
-class ServerOpcodeHandler : public OpcodeHandler
+struct ServerOpcodeHandler final
 {
-public:
-    ServerOpcodeHandler(char const* name, SessionStatus status, ConnectionType conIdx)
-        : OpcodeHandler(name, status), ConnectionIndex(conIdx) { }
-
+    char const* Name;
+    SessionStatus Status;
     ConnectionType ConnectionIndex;
 };
 
 class OpcodeTable
 {
-    public:
-        OpcodeTable();
+public:
+    OpcodeTable();
+    ~OpcodeTable();
 
-        OpcodeTable(OpcodeTable const&) = delete;
-        OpcodeTable& operator=(OpcodeTable const&) = delete;
+    void Initialize();
 
-        ~OpcodeTable();
+    ClientOpcodeHandler const* operator[](OpcodeClient index) const
+    {
+        return _internalTableClient[index - MIN_CMSG_OPCODE_NUMBER].get();
+    }
 
-        void Initialize();
+    ServerOpcodeHandler const* operator[](OpcodeServer index) const
+    {
+        return _internalTableServer[index - MIN_SMSG_OPCODE_NUMBER].get();
+    }
 
-        ClientOpcodeHandler const* operator[](OpcodeClient index) const
-        {
-            return _internalTableClient[index];
-        }
+private:
+    bool ValidateClientOpcode(OpcodeClient opcode, char const* name) const;
+    void ValidateAndSetClientOpcode(OpcodeClient opcode, char const* name, SessionStatus status, ClientOpcodeHandler::HandlerFunction call, PacketProcessing processing);
 
-        ServerOpcodeHandler const* operator[](OpcodeServer index) const
-        {
-            return _internalTableServer[index];
-        }
+    bool ValidateServerOpcode(OpcodeServer opcode, char const* name, ConnectionType conIdx) const;
+    void ValidateAndSetServerOpcode(OpcodeServer opcode, char const* name, SessionStatus status, ConnectionType conIdx);
 
-    private:
-        bool ValidateClientOpcode(OpcodeClient opcode, char const* name) const;
-        template<typename Handler, Handler HandlerFunction>
-        void ValidateAndSetClientOpcode(OpcodeClient opcode, char const* name, SessionStatus status, PacketProcessing processing);
+    void InitializeClientOpcodes();
+    void InitializeServerOpcodes();
 
-        void ValidateAndSetServerOpcode(OpcodeServer opcode, char const* name, SessionStatus status, ConnectionType conIdx);
-
-        ClientOpcodeHandler* _internalTableClient[NUM_OPCODE_HANDLERS];
-        ServerOpcodeHandler* _internalTableServer[NUM_OPCODE_HANDLERS];
+    std::array<std::unique_ptr<ClientOpcodeHandler>, NUM_CMSG_OPCODES> _internalTableClient;
+    std::array<std::unique_ptr<ServerOpcodeHandler>, NUM_SMSG_OPCODES> _internalTableServer;
 };
 
 extern OpcodeTable opcodeTable;

src/server/game/Server/WorldSession.cpp

@@ -210,19 +210,14 @@ std::string WorldSession::GetPlayerInfo() const
 /// Send a packet to the client
 void WorldSession::SendPacket(WorldPacket const* packet, bool forced /*= false*/)
 {
-    if (packet->GetOpcode() == NULL_OPCODE)
+    if (packet->GetOpcode() < MIN_SMSG_OPCODE_NUMBER || packet->GetOpcode() > MAX_SMSG_OPCODE_NUMBER)
     {
-        TC_LOG_ERROR("network.opcode", "Prevented sending of NULL_OPCODE to {}", GetPlayerInfo());
-        return;
-    }
-    else if (packet->GetOpcode() == UNKNOWN_OPCODE)
-    {
-        TC_LOG_ERROR("network.opcode", "Prevented sending of UNKNOWN_OPCODE to {}", GetPlayerInfo());
+        char const* specialName = packet->GetOpcode() == UNKNOWN_OPCODE ? "UNKNOWN_OPCODE" : "INVALID_OPCODE";
+        TC_LOG_ERROR("network.opcode", "Prevented sending of {} (0x{:04X}) to {}", specialName, packet->GetOpcode(), GetPlayerInfo());
         return;
     }
 
     ServerOpcodeHandler const* handler = opcodeTable[static_cast<OpcodeServer>(packet->GetOpcode())];
-
     if (!handler)
     {
         TC_LOG_ERROR("network.opcode", "Prevented sending of opcode {} with non existing handler to {}", packet->GetOpcode(), GetPlayerInfo());

src/server/game/Server/WorldSocket.cpp

@@ -362,7 +362,7 @@ WorldSocket::ReadDataHandlerResult WorldSocket::ReadDataHandler()
 
     WorldPacket packet(std::move(_packetBuffer), GetConnectionType());
     OpcodeClient opcode = packet.read<OpcodeClient>();
-    if (uint32(opcode) >= uint32(NUM_OPCODE_HANDLERS))
+    if (opcode < MIN_CMSG_OPCODE_NUMBER || opcode > MAX_CMSG_OPCODE_NUMBER)
     {
         TC_LOG_ERROR("network", "WorldSocket::ReadHeaderHandler(): client {} sent wrong opcode (opcode: {})",
             GetRemoteIpAddress().to_string(), uint32(opcode));
@@ -485,7 +485,7 @@ WorldSocket::ReadDataHandlerResult WorldSocket::ReadDataHandler()
                 return ReadDataHandlerResult::Error;
             }
 
-            OpcodeHandler const* handler = opcodeTable[opcode];
+            ClientOpcodeHandler const* handler = opcodeTable[opcode];
             if (!handler)
             {
                 TC_LOG_ERROR("network.opcode", "No defined handler for opcode {} sent by {}", GetOpcodeNameForLogging(static_cast<OpcodeClient>(packet.GetOpcode())), _worldSession->GetPlayerInfo());