fix, ci: general code improvements, removed some ci runners. (#15)

* fix, ci: general code improvements, packet_size no longer gets converted to size_t, removed some warnings on windows, added more unittests, removed some ci runners

* fix: added/removed missing/unneeded spacing

* feat: added missing socket header

* ci: removed CI that isn't needed

* ci: readded ctest to a runner

* fix: Unit tests no longer complete if exception was found

* ci: simplified ci for linux

Author: Jaskowicz1

.github/workflows/CI.yml

@@ -26,17 +26,10 @@ jobs:
       fail-fast: false # Don't fail everything if one fails. We want to test each OS/Compiler individually
       matrix:
         cfg:
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-20.04, package: clang-10, cpp-version: clang++-10, ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: clang-11, cpp-version: clang++-11, ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: clang-12, cpp-version: clang++-12, ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: clang-13, cpp-version: clang++-13, ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: clang-14, cpp-version: clang++-14, ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: clang-15, cpp-version: clang++-15, ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-20.04, package: g++-9,    cpp-version: g++-9,      ctest: 'yes', cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: g++-10,   cpp-version: g++-10,     ctest: 'no',  cpack: 'yes', downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: g++-11,   cpp-version: g++-11,     ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: g++-12,   cpp-version: g++-12,     ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
-          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: g++-13,   cpp-version: g++-13,     ctest: 'no',  cpack: 'no',  downloadcpp: 'yes'}
+          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: clang-11, cpp-version: clang++-11, ctest: false, cpack: false }
+          - { arch: 'amd64', concurrency: 2, os: ubuntu-24.04, package: clang-16, cpp-version: clang++-16, ctest: true,  cpack: true  }
+          - { arch: 'amd64', concurrency: 2, os: ubuntu-22.04, package: g++-10,   cpp-version: g++-10,     ctest: false, cpack: false }
+          - { arch: 'amd64', concurrency: 2, os: ubuntu-24.04, package: g++-13,   cpp-version: g++-13,     ctest: false, cpack: false }
     steps:
       - name: Harden Runner
         uses: step-security/harden-runner@20cf305ff2072d973412fa9b1e3a4f227bda3c76 # v2.14.0
@@ -47,11 +40,7 @@ jobs:
         uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1
 
       - name: Install apt packages
-        run: sudo sed -i 's/azure\.//' /etc/apt/sources.list && sudo apt update && sudo apt-get install -y pkg-config
-
-      - name: Install C++ apt package
-        if: ${{ matrix.cfg.downloadcpp == 'yes' }}
-        run: sudo apt-get install -y ${{ matrix.cfg.package }}
+        run: sudo sed -i 's/azure\.//' /etc/apt/sources.list && sudo apt update && sudo apt-get install -y pkg-config ${{ matrix.cfg.package }}
 
       - name: Generate CMake
         run: cmake -B build
@@ -62,19 +51,19 @@ jobs:
         run: cmake --build build -j${{ matrix.cfg.concurrency }}
 
       - name: Run unit tests
-        if: ${{ matrix.cfg.ctest == 'yes' }}
+        if: ${{ matrix.cfg.ctest }}
         run: cd build && ctest -VV
         env:
           RCON_TESTING_IP: ${{secrets.RCON_TESTING_IP}}
           RCON_TESTING_PORT: ${{secrets.RCON_TESTING_PORT}}
           RCON_TESTING_PASSWORD: ${{secrets.RCON_TESTING_PASSWORD}}
 
       - name: Package distributable
-        if: ${{ matrix.cfg.cpack == 'yes' }}
+        if: ${{ matrix.cfg.cpack }}
         run: cd build && cpack --verbose
 
       - name: Upload Binary (DEB)
-        if: ${{ matrix.cfg.cpack == 'yes' }}
+        if: ${{ matrix.cfg.cpack }}
         uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6.0.0
         with:
           name: "librconpp - Debian Package ${{matrix.cfg.arch}}"
@@ -87,10 +76,6 @@ jobs:
       fail-fast: false # Don't cancel other matrix jobs if one fails
       matrix:
         cfg:
-          - { name: 'x64',            arch: x64, config: Release, vs: '2019', os: 'windows-2019', vsv: '16', upload: true,  options: '' }
-          - { name: 'x64',            arch: x64, config: Debug,   vs: '2019', os: 'windows-2019', vsv: '16', upload: true,  options: '' }
-          - { name: 'x86',            arch: x86, config: Release, vs: '2019', os: 'windows-2019', vsv: '16', upload: true,  options: '-T host=x86 ' }
-          - { name: 'x86',            arch: x86, config: Debug,   vs: '2019', os: 'windows-2019', vsv: '16', upload: true,  options: '-T host=x86 ' }
           - { name: 'x64',            arch: x64, config: Release, vs: '2022', os: 'windows-2022', vsv: '17', upload: true,  options: '' }
           - { name: 'x64',            arch: x64, config: Debug,   vs: '2022', os: 'windows-2022', vsv: '17', upload: true,  options: '' }
           - { name: 'x86',            arch: x86, config: Release, vs: '2022', os: 'windows-2022', vsv: '17', upload: true,  options: '-T host=x86' }

CMakeLists.txt

@@ -6,6 +6,11 @@ option(BUILD_SHARED_LIBS "Build shared libraries" ON)
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 add_compile_definitions(RCONPP_BUILD)
 
+if(WIN32)
+	add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
+	add_compile_definitions(_WINSOCK_DEPRECATED_NO_WARNINGS)
+endif ()
+
 set(RCONPP_ROOT_PATH ${CMAKE_CURRENT_SOURCE_DIR})
 
 project(rconpp

include/rconpp/client.h

@@ -77,7 +77,7 @@ class RCONPP_EXPORT rcon_client {
 	 *
 	 * @warning If you are expecting no response from the server, do NOT use the callback. You will halt the RCON process until the next received message (which will chain).
 	 */
-	void send_data(const std::string_view data, const int32_t id, data_type type, std::function<void(const response& retrieved_data)> callback = {}) {
+	void send_data(const std::string_view data, const int32_t id, const data_type type, std::function<void(const response& retrieved_data)> callback = {}) {
 		requests_queued.emplace_back(queued_request{ std::string{data}, id, type, std::move(callback) });
 	}
 
@@ -122,13 +122,6 @@ class RCONPP_EXPORT rcon_client {
 	 * @return A packet structure containing the length, size, data, and if server responded.
 	 */
 	packet read_packet();
-
-	/**
-	 * @brief Reads the first 4 bytes of a packet to get the packet size (not to be mistaken with length).
-	 *
-	 * @return The size (not length) of the packet.
-	 */
-	int read_packet_size();
 };
 
-} // namespace rconpp
+} // namespace rconpp

include/rconpp/export.h

@@ -12,4 +12,4 @@
 	#else
 		#define RCONPP_EXPORT
 	#endif
-#endif
+#endif

include/rconpp/server.h

@@ -112,13 +112,6 @@ class RCONPP_EXPORT rcon_server {
 	 * @brief Gathers all the packet's content (based on the length returned by `read_packet_length`)
 	 */
 	void read_packet(rconpp::connected_client client);
-
-	/**
-	 * @brief Reads the first 4 bytes of a packet to get the packet size (not to be mistaken with length).
-	 *
-	 * @return The size (not length) of the packet.
-	 */
-	int read_packet_size(const rconpp::connected_client client);
 };
 
-} // namespace rconpp
+} // namespace rconpp

include/rconpp/utilities.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <functional>
+
 #include "export.h"
 #include <string>
 #include <string_view>
@@ -8,9 +10,10 @@
 
 namespace rconpp {
 
-constexpr int DEFAULT_TIMEOUT = 4;
+constexpr int DEFAULT_TIMEOUT = 4; // In Seconds.
 constexpr int MIN_PACKET_SIZE = 10;
 constexpr int MIN_PACKET_LENGTH = 14;
+constexpr int MAX_RETRIES_TO_RECEIVE_INFO = 500;
 
 enum data_type {
 	/**
@@ -91,4 +94,11 @@ RCONPP_EXPORT int type_to_int(const std::vector<char>& buffer);
  */
 RCONPP_EXPORT void report_error();
 
-} // namespace rconpp
+/**
+ * @brief Reads the first 4 bytes of a packet to get the packet size (not to be mistaken with length).
+ *
+ * @return The size (not length) of the packet.
+ */
+RCONPP_EXPORT int read_packet_size(int socket, const std::function<void(const std::string_view log)>& on_log);
+
+} // namespace rconpp

src/rconpp/client.cpp

@@ -49,12 +49,12 @@ rconpp::response rconpp::rcon_client::send_data_sync(const std::string_view data
 bool rconpp::rcon_client::connect_to_server() {
 #ifdef _WIN32
 	// Initialize Winsock
-		WSADATA wsa_data;
-		int result = WSAStartup(MAKEWORD(2, 2), &wsa_data);
-		if (result != 0) {
-			on_log("WSAStartup failed. Error: " + std::to_string(result));
-			return false;
-		}
+	WSADATA wsa_data;
+	int result = WSAStartup(MAKEWORD(2, 2), &wsa_data);
+	if (result != 0) {
+		on_log("WSAStartup failed. Error: " + std::to_string(result));
+		return false;
+	}
 #endif
 
 	// Create new TCP socket.
@@ -71,7 +71,7 @@ bool rconpp::rcon_client::connect_to_server() {
 	}
 
 	// Setup port, address, and family.
-	struct sockaddr_in server {};
+	sockaddr_in server{};
 	server.sin_family = AF_INET;
 #ifdef _WIN32
 	#ifdef UNICODE
@@ -85,7 +85,8 @@ bool rconpp::rcon_client::connect_to_server() {
 	server.sin_port = htons(port);
 
 #ifdef _WIN32
-	int corrected_timeout = DEFAULT_TIMEOUT * 1000;
+	// DEFAULT_TIMEOUT is in seconds (library was originally built for Linux and Linux/Unix uses seconds, we just need to convert to milliseconds for Windows.
+	const int corrected_timeout = DEFAULT_TIMEOUT * 1000;
 	setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (const char*)&corrected_timeout, sizeof(corrected_timeout));
 #else
 	// Set a timeout of 4 seconds.
@@ -109,32 +110,27 @@ bool rconpp::rcon_client::connect_to_server() {
 rconpp::response rconpp::rcon_client::receive_information(int32_t id, rconpp::data_type type) {
 	// Whilst this loop is better than a while loop,
 	// it should really just keep going for a certain amount of seconds.
-	for (int i = 0; i < 500; i++) {
+	for (int i = 0; i < MAX_RETRIES_TO_RECEIVE_INFO; i++) {
 		packet packet_response = read_packet();
 
-		int packet_type = bit32_to_int(packet_response.data);
+		const auto packet_type = static_cast<data_type>(bit32_to_int(packet_response.data));
 
 		if (packet_response.length == 0) {
-			if (packet_type != SERVERDATA_AUTH)
+			if (packet_type != SERVERDATA_AUTH) {
 				return { "", packet_response.server_responded };
-			else
-				continue;
+			}
+
+			continue;
 		}
 
 		if (type == SERVERDATA_AUTH) {
-			if (packet_type == -1) {
-				return { "", false };
-			} else {
-				if (packet_type == id) {
-					return { "", true };
-				}
-			}
+			return { "", packet_type == id };
 		}
 
 		if (packet_type == id) {
 			std::string part{};
 
-			if(packet_response.size > 10) {
+			if (packet_response.size > MIN_PACKET_SIZE) {
 				part = std::string(&packet_response.data[8], &packet_response.data[packet_response.data.size()-1]);
 			}
 
@@ -146,29 +142,27 @@ rconpp::response rconpp::rcon_client::receive_information(int32_t id, rconpp::da
 }
 
 rconpp::packet rconpp::rcon_client::read_packet() {
-	size_t packet_size = read_packet_size();
+	const int packet_size = read_packet_size(static_cast<int>(sock), on_log);
 
 	packet temp_packet{};
 	temp_packet.length = packet_size + 4;
 
-	if(packet_size > 0) {
+	if (packet_size > 0) {
 		temp_packet.size = packet_size;
 	}
 
-	/*
-	 * If the packet size is -1, the server didn't respond.
-	 * If the packet size is 0, the server did respond but said nothing.
-	 */
+	// If the packet size is -1, the server didn't respond.
 	if (packet_size == -1) {
 		return temp_packet;
 	}
-	else if (packet_size == 0) {
-		temp_packet.server_responded = true;
-		return temp_packet;
-	}
 
 	temp_packet.server_responded = true;
 
+	// If the packet size is 0, the server did respond but said nothing.
+	if (packet_size == 0) {
+		return temp_packet;
+	}
+
 	std::vector<char> buffer{};
 	buffer.resize(temp_packet.length);
 
@@ -183,50 +177,38 @@ rconpp::packet rconpp::rcon_client::read_packet() {
 	return temp_packet;
 }
 
-int rconpp::rcon_client::read_packet_size() {
-	std::vector<char> buffer{};
-	buffer.resize(4);
-
-	/*
-	 * RCON gives the packet SIZE in the first four (4) bytes of each packet.
-	 * We simply just want to read that and then return it.
-	 */
-	if (recv(sock, buffer.data(), 4, 0) == -1) {
-		on_log("Did not receive a packet in time. Did the server send a response?");
-		report_error();
-		return -1;
-	}
-
-	return bit32_to_int(buffer);
-}
-
-void rconpp::rcon_client::start(bool return_after) {
-
+void rconpp::rcon_client::start(const bool return_after) {
 	auto block_calling_thread = [this]() {
 		std::mutex thread_mutex;
 		std::unique_lock thread_lock(thread_mutex);
 		this->terminating.wait(thread_lock);
 	};
 
-	if(port > 65535) {
+	if (address.empty()) {
+		on_log("Address is empty! You need to pass a valid address!");
+		return;
+	}
+
+	if (port > 65535) {
 		on_log("Invalid port! The port can't exceed 65535!");
 		return;
 	}
 
 	on_log("Attempting connection to RCON server...");
 
 	if (!connect_to_server()) {
-		on_log("RCON is aborting as it failed to initiate client.");
+		on_log("RCON++ is aborting as it failed to initiate client.");
 		return;
 	}
 
 	on_log("Connected successfully! Sending login data...");
 
 	// The server will send SERVERDATA_AUTH_RESPONSE once it's happy. If it's not -1, the server will have accepted us!
-	response response = send_data_sync(password, 1, data_type::SERVERDATA_AUTH, true);
+	// We use the _sync method here to do a blocking call.
+	const response response = send_data_sync(password, 1, data_type::SERVERDATA_AUTH, true);
 
 	if (!response.server_responded) {
-		on_log("Login data was incorrect. RCON will now abort.");
+		on_log("Login data was incorrect. RCON++ will now abort.");
 		return;
 	}
 
@@ -252,7 +234,7 @@ void rconpp::rcon_client::start(bool return_after) {
 		}
 	});
 
-	if(!return_after) {
+	if (!return_after) {
 		block_calling_thread();
 	}
 };