Tidy up packet_deserialise(), move concept to its own header

src/gateway/ClientHandler.cpp

@@ -74,7 +74,7 @@ void ClientHandler::state_update(ClientState new_state) {
 	enter_states[context_.state](context_);
 }
 
-void ClientHandler::packet_skip(BinaryStream& stream) {
+void ClientHandler::skip(BinaryStream& stream) {
 	CLIENT_DEBUG_FILTER(logger_, LF_NETWORK, context_)
 		<< ClientState_to_string(context_.state) << " skipping "
 		<< protocol::to_string(opcode_)
@@ -88,7 +88,7 @@ void ClientHandler::handle_ping(BinaryStream& stream) {
 
 	protocol::CMSG_PING packet;
 
-	if(!packet_deserialise(packet, stream)) {
+	if(!deserialise(packet, stream)) {
 		return;
 	}
 
@@ -121,10 +121,8 @@ void ClientHandler::stop_timer() {
 const std::string& ClientHandler::client_identify() const {	
 	if(context_.client_id) {
 		if(client_id_ext_.empty()) {
-			client_id_ext_ = std::format("{} ({}, {}): ",
-										  client_id_, 
-			                              context_.client_id->username,
-			                              context_.client_id->id
+			client_id_ext_ = std::format(
+				"{} ({}, {}): ", client_id_,  context_.client_id->username, context_.client_id->id
 			);
 		}
 

src/gateway/ClientHandler.h

@@ -21,6 +21,7 @@
 #include <concepts>
 #include <chrono>
 #include <memory>
+#include <optional>
 
 namespace ember {
 
@@ -47,8 +48,11 @@ class ClientHandler final {
 	void close();
 	const std::string& client_identify() const;
 
-	bool packet_deserialise(auto& packet, BinaryStream& stream);
-	void packet_skip(BinaryStream& stream);
+	template<protocol::is_packet T>
+	std::optional<T> deserialise(BinaryStream& stream);
+
+	bool deserialise(protocol::is_packet auto& packet, BinaryStream& stream);
+	void skip(BinaryStream& stream);
 
 	void state_update(ClientState new_state);
 	void handle_message(BinaryStream& stream);

src/gateway/ClientHandler.inl

@@ -9,58 +9,73 @@
 #pragma once
 
 #include "ConnectionDefines.h"
+#include <protocol/Concepts.h>
 #include <logger/Logger.h>
+#include <optional>
 
-bool ClientHandler::packet_deserialise(auto& packet, BinaryStream& stream) {
-	if(packet->read_from_stream(stream) != protocol::State::DONE) {
-		const auto state = stream.state();
-
-		/*
-		* READ_LIMIT_ERR:
-		* Deserialisation failed due to an attempt to read beyond the
-		* message boundary. This could be caused by an error in the message
-		* definition or a malicious client spoofing the size in the
-		* header. We can recover from this.
-		* 
-		* BUFF_LIMIT_ERR:
-		* Deserialisation failed due to a buffer underrun - this should never
-		* happen and message framing has likely been lost if this ever
-		* occurs. Don't try to recover.
-		*/
-		if(state == BinaryStream::State::READ_LIMIT_ERR) {
+bool ClientHandler::deserialise(protocol::is_packet auto& packet, BinaryStream& stream) {
+	if(packet->read_from_stream(stream) == protocol::State::DONE) {
+
+		if(stream.read_limit() != stream.total_read()) {
 			LOG_DEBUG_FILTER(logger_, LF_NETWORK)
-				<< "Deserialisation of "
-				<< protocol::to_string(packet.opcode)
-				<< " failed, skipping any remaining data" << LOG_ASYNC;
-
-			stream.skip(stream.read_limit() - stream.total_read());
-		} else if(state == BinaryStream::State::BUFF_LIMIT_ERR) {
-			LOG_ERROR_FILTER(logger_, LF_NETWORK)
-				<< "Message framing lost at "
-				<< protocol::to_string(packet.opcode)
-				<< " from " << client_identify() << LOG_ASYNC;
-
-			close();
-		} else {
-			LOG_ERROR_FILTER(logger_, LF_NETWORK)
-				<< "Deserialisation failed but stream has not errored for "
+				<< "Skipping superfluous stream data in message "
 				<< protocol::to_string(packet.opcode)
 				<< " from " << client_identify() << LOG_ASYNC;
 
-			close();
+			stream.skip(stream.read_limit() - stream.total_read());
 		}
 
-		return false;
-	}
+		return true;
+	}	
+
+	/*
+	 * READ_LIMIT_ERR:
+	 * Deserialisation failed due to an attempt to read beyond the
+	 * message boundary. This could be caused by an error in the message
+	 * definition or a malicious client spoofing the size in the
+	 * header. We can recover from this.
+	 * 
+	 * BUFF_LIMIT_ERR:
+	 * Deserialisation failed due to a buffer underrun - this should never
+	 * happen and message framing has likely been lost if this ever
+	 * occurs. Don't try to recover.
+	 */
+	const auto state = stream.state();
 
-	if(stream.read_limit() != stream.total_read()) {
+	if(state == BinaryStream::State::READ_LIMIT_ERR) {
 		LOG_DEBUG_FILTER(logger_, LF_NETWORK)
-			<< "Skipping superfluous stream data in message "
+			<< "Deserialisation of "
 			<< protocol::to_string(packet.opcode)
-			<< " from " << client_identify() << LOG_ASYNC;
+			<< " failed, skipping any remaining data" << LOG_ASYNC;
 
 		stream.skip(stream.read_limit() - stream.total_read());
+	} else if(state == BinaryStream::State::BUFF_LIMIT_ERR) {
+		LOG_ERROR_FILTER(logger_, LF_NETWORK)
+			<< "Message framing lost at "
+			<< protocol::to_string(packet.opcode)
+			<< " from " << client_identify() << LOG_ASYNC;
+
+		close();
+	} else {
+		LOG_ERROR_FILTER(logger_, LF_NETWORK)
+			<< "Deserialisation failed but stream has not errored for "
+			<< protocol::to_string(packet.opcode)
+			<< " from " << client_identify() << LOG_ASYNC;
+
+		close();
 	}
 
-	return true;
-};
+	return false;
+}
+
+template<protocol::is_packet T>
+std::optional<T> ClientHandler::deserialise(BinaryStream& stream) {
+	T packet;
+	const auto result = deserialise(packet, stream);
+
+	if(result) {
+		return packet;
+	} else {
+		return std::nullopt;
+	}
+}

src/gateway/packetlog/PacketLogger.h

@@ -9,7 +9,7 @@
 #pragma once
 
 #include "PacketSink.h"
-#include <protocol/Packets.h>
+#include <protocol/Concepts.h>
 #include <spark/buffers/pmr/BufferAdaptor.h>
 #include <spark/buffers/BufferAdaptor.h>
 #include <spark/buffers/BinaryStream.h>

src/gateway/states/Authentication.cpp

@@ -71,12 +71,14 @@ void handle_authentication(ClientContext& ctx) {
 
 	auto& auth_ctx = std::get<Context>(ctx.state_ctx);
 
-	if(!ctx.handler->packet_deserialise(auth_ctx.packet, *ctx.stream)) {
+	if(!ctx.handler->deserialise(auth_ctx.packet, *ctx.stream)) {
 		return;
 	}
 
-	CLIENT_DEBUG_GLOB(ctx) << "Received session proof for "
-		<< auth_ctx.packet->username << LOG_ASYNC;
+	CLIENT_DEBUG_GLOB(ctx)
+		<< "Received session proof for "
+		<< auth_ctx.packet->username
+		<< LOG_ASYNC;
 
 	if(auth_ctx.packet->build == 0) {
 		// todo - check game build & server ID
@@ -306,7 +308,7 @@ void handle_packet(ClientContext& ctx, protocol::ClientOpcode opcode) {
 			handle_authentication(ctx);
 			break;
 		default:
-			ctx.handler->packet_skip(*ctx.stream);
+			ctx.handler->skip(*ctx.stream);
 	}
 }
 

src/gateway/states/CharacterList.cpp

@@ -73,7 +73,7 @@ void character_rename(ClientContext& ctx) {
 
 	protocol::CMSG_CHAR_RENAME packet;
 
-	if(!ctx.handler->packet_deserialise(packet, *ctx.stream)) {
+	if(!ctx.handler->deserialise(packet, *ctx.stream)) {
 		return;
 	}
 
@@ -146,7 +146,7 @@ void character_create(ClientContext& ctx) {
 
 	protocol::CMSG_CHAR_CREATE packet;
 
-	if(!ctx.handler->packet_deserialise(packet, *ctx.stream)) {
+	if(!ctx.handler->deserialise(packet, *ctx.stream)) {
 		return;
 	}
 
@@ -173,7 +173,7 @@ void character_delete(ClientContext& ctx) {
 
 	protocol::CMSG_CHAR_DELETE packet;
 
-	if(!ctx.handler->packet_deserialise(packet, *ctx.stream)) {
+	if(!ctx.handler->deserialise(packet, *ctx.stream)) {
 		return;
 	}
 
@@ -202,7 +202,7 @@ void player_login(ClientContext& ctx) {
 
 	protocol::CMSG_PLAYER_LOGIN packet;
 
-	if(!ctx.handler->packet_deserialise(packet, *ctx.stream)) {
+	if(!ctx.handler->deserialise(packet, *ctx.stream)) {
 		return;
 	}
 
@@ -242,7 +242,7 @@ void handle_packet(ClientContext& ctx, protocol::ClientOpcode opcode) {
 			player_login(ctx);
 			break;
 		default:
-			ctx.handler->packet_skip(*ctx.stream);
+			ctx.handler->skip(*ctx.stream);
 	}
 }
 

src/gateway/states/SessionClose.cpp

@@ -16,7 +16,7 @@ void enter(ClientContext& ctx) {
 }
 
 void handle_packet(ClientContext& ctx, protocol::ClientOpcode opcode) {
-	ctx.handler->packet_skip(*ctx.stream);
+	ctx.handler->skip(*ctx.stream);
 }
 
 void handle_event(ClientContext& ctx, const Event* event) {

src/libs/protocol/CMakeLists.txt

@@ -42,6 +42,7 @@ set(LIBRARY_SRC
     include/protocol/ResultCodes.h
     include/protocol/PacketHeaders.h
 	include/protocol/SmartMessage.h
+    include/protocol/Concepts.h
 )
 
 add_library(${LIBRARY_NAME} ${LIBRARY_SRC})

src/libs/protocol/include/protocol/Concepts.h

@@ -0,0 +1,16 @@
+/*
+ * Copyright (c) 2024 Ember
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#pragma once
+
+namespace ember::protocol {
+
+template<typename T>
+concept is_packet = requires { typename T::packet_tag; };
+
+} // protocol, ember

src/libs/protocol/include/protocol/Packet.h

@@ -9,6 +9,7 @@
 #pragma once
 
 #include <protocol/PacketHeaders.h>
+#include <protocol/Concepts.h>
 #include <concepts>
 #include <cstddef>
 
@@ -19,19 +20,18 @@ enum class State {
 	INITIAL, DONE, ERRORED
 };
 
-template <typename HeaderT, typename HeaderT::OpcodeType op_, typename Payload>
+template <typename HeaderType, typename HeaderType::OpcodeType op_, typename Payload>
 struct Packet final {
 	struct packet_tag_t{};
 	using packet_tag = packet_tag_t;
 
-	using OpcodeType = typename HeaderT::OpcodeType;
-	using SizeType = typename HeaderT::SizeType;
-	using PayloadType = Payload;
+	using OpcodeType = typename HeaderType::OpcodeType;
+	using SizeType = typename HeaderType::SizeType;
 
 	static constexpr OpcodeType opcode = op_;
-	static constexpr std::size_t HEADER_WIRE_SIZE = HeaderT::WIRE_SIZE;
+	static constexpr std::size_t HEADER_WIRE_SIZE = HeaderType::WIRE_SIZE;
 
-	PayloadType payload;
+	Payload payload;
 
 	State read_from_stream(auto& stream) {
 		return payload.read_from_stream(stream);
@@ -62,7 +62,4 @@ using ServerPacket = Packet<ServerHeader, opcode, Payload>;
 template<ClientOpcode opcode, typename Payload>
 using ClientPacket = Packet<ClientHeader, opcode, Payload>;
 
-template<typename T>
-concept is_packet = requires { typename T::packet_tag; };
-
 } // protocol, ember