refactor upnp code

CMakeLists.txt

@@ -1076,6 +1076,8 @@ set_src(ENGINE_SERVER GLOB src/engine/server
   sql_server.h
   sql_string_helpers.cpp
   sql_string_helpers.h
+  upnp.cpp
+  upnp.h
 )
 set_src(GAME_SERVER GLOB_RECURSE src/game/server
   antibot.cpp
@@ -1741,7 +1743,7 @@ foreach(target ${TARGETS_OWN})
   endif()
   if(UPNP)
     target_compile_definitions(${target} PRIVATE CONF_UPNP)
-	target_include_directories(${target} PRIVATE ${MINIUPNPC_INCLUDE_DIRS})
+    target_include_directories(${target} PRIVATE ${MINIUPNPC_INCLUDE_DIRS})
   endif()
   if(VIDEORECORDER)
     target_compile_definitions(${target} PRIVATE CONF_VIDEORECORDER)

README.md

@@ -80,7 +80,7 @@ Whether to optimize for development, speeding up the compilation process a littl
 
 * **-DUPNP=[ON|OFF]** <br>
 Whether to enable UPnP support for the server.
-You need to install `libminiupnpc-dev`.
+You need to install `libminiupnpc-dev` on Debian, `miniupnpc` on Arch Linux.
 
 * **-GNinja** <br>
 Use the Ninja build system instead of Make. This automatically parallizes the build and is generally faster. Compile with `ninja` instead of `make`. Install Ninja with `sudo apt install ninja-build` on Debian, `sudo pacman -S --needed ninja` on Arch Linux.

src/engine/server/server.cpp

@@ -34,17 +34,10 @@
 #include <vector>
 #include <engine/shared/linereader.h>
 #include <game/extrainfo.h>
-#include <game/version.h>
 
 #include "register.h"
 #include "server.h"
 
-#if defined(CONF_UPNP)
-	#include <miniupnpc/miniupnpc.h>
-	#include <miniupnpc/upnpcommands.h>
-	#include <miniupnpc/upnperrors.h>
-#endif
-
 #if defined(CONF_FAMILY_WINDOWS)
 	#define WIN32_LEAN_AND_MEAN
 	#include <windows.h>
@@ -1970,42 +1963,7 @@ int CServer::Run()
 	}
 
 #if defined(CONF_UPNP)
-	if(g_Config.m_SvUseUPnP)
+	m_UPnP.Open(BindAddr);
-	{
-		struct UPNPUrls upnp_urls;
-		struct IGDdatas upnp_data;
-		char aLanAddr[64];
-		char aPort[6];
-		int error = 0;
-		struct UPNPDev *upnp_dev = upnpDiscover(2000, NULL, NULL, 0, 0, 2, &error);
-		int status = UPNP_GetValidIGD(upnp_dev, &upnp_urls, &upnp_data, aLanAddr, sizeof(aLanAddr));
-		dbg_msg("upnp", "status=%d, lan_addr=%s", status, aLanAddr);
-
-		if(status == 1)
-		{
-			dbg_msg("upnp", "found valid IGD: %s", upnp_urls.controlURL);
-			str_format(aPort, sizeof(aPort), "%d", BindAddr.port);
-			error = UPNP_AddPortMapping(upnp_urls.controlURL, upnp_data.first.servicetype, 
-					aPort, aPort, aLanAddr,
-					"DDNet Server " GAME_RELEASE_VERSION,
-					"UDP", NULL, "0");
-
-			if(error)
-			{
-				dbg_msg("upnp", "failed to map port");
-				dbg_msg("upnp", "error: %s", strupnperror(error));
-			}
-			else
-				dbg_msg("upnp", "successfully mapped port");
-		}
-		else
-		{
-				dbg_msg("upnp", "no valid IGD found");
-				dbg_msg("upnp", "disabled");
-		}
-		FreeUPNPUrls(&upnp_urls);
-		freeUPNPDevlist(upnp_dev);
-	}
 #endif
 
 	if(!m_NetServer.Open(BindAddr, &m_ServerBan, g_Config.m_SvMaxClients, g_Config.m_SvMaxClientsPerIP, 0))
@@ -2276,6 +2234,10 @@ int CServer::Run()
 	}
 #endif
 
+#if defined (CONF_UPNP)
+	m_UPnP.Shutdown();
+#endif
+
 	return ErrorShutdown();
 }
 

src/engine/server/server.h

@@ -28,6 +28,10 @@
 #include "authmanager.h"
 #include "name_ban.h"
 
+#if defined (CONF_UPNP)
+	#include "upnp.h"
+#endif
+
 #if defined (CONF_SQL)
 	#include "sql_connector.h"
 	#include "sql_server.h"
@@ -92,6 +96,10 @@ class CServer : public IServer
 	class IConsole *m_pConsole;
 	class IStorage *m_pStorage;
 
+#if defined(CONF_UPNP)
+	CUPnP m_UPnP;
+#endif
+
 #if defined(CONF_SQL)
 	lock m_GlobalSqlLock;
 

src/engine/server/upnp.cpp

@@ -0,0 +1,74 @@
+#ifdef CONF_UPNP
+
+#include <miniupnpc/miniupnpc.h>
+#include <miniupnpc/upnpcommands.h>
+#include <miniupnpc/upnperrors.h>
+#include <engine/shared/config.h>
+#include <base/system.h>
+#include <game/version.h>
+#include "upnp.h"
+
+void CUPnP::Open(NETADDR Address)
+{
+	if(g_Config.m_SvUseUPnP)
+	{
+		m_Enabled = false;
+		m_Addr = Address;
+		m_UPnPUrls = (struct UPNPUrls *)malloc(sizeof(struct UPNPUrls));
+		m_UPnPData = (struct IGDdatas *)malloc(sizeof(struct IGDdatas));
+
+		char aLanAddr[64];
+		char aPort[6];
+		int Error;
+
+		m_UPnPDevice = upnpDiscover(2000, NULL, NULL, 0, 0, 2, &Error);
+
+		int Status = UPNP_GetValidIGD(m_UPnPDevice, m_UPnPUrls, m_UPnPData, aLanAddr, sizeof(aLanAddr));
+		dbg_msg("upnp", "status=%d, lan_addr=%s", Status, aLanAddr);
+
+		if(Status == 1)
+		{
+			m_Enabled = true;
+			dbg_msg("upnp", "found valid IGD: %s", m_UPnPUrls->controlURL);
+			str_format(aPort, sizeof(aPort), "%d", m_Addr.port);
+			Error = UPNP_AddPortMapping(m_UPnPUrls->controlURL, m_UPnPData->first.servicetype, 
+					aPort, aPort, aLanAddr,
+					"DDNet Server " GAME_RELEASE_VERSION,
+					"UDP", NULL, "0");
+
+			if(Error)
+				dbg_msg("upnp", "failed to map port, error: %s", strupnperror(Error));
+			else
+				dbg_msg("upnp", "successfully mapped port");
+		}
+		else
+			dbg_msg("upnp", "no valid IGD found, disabled");
+	}
+}
+
+void CUPnP::Shutdown()
+{
+	if(g_Config.m_SvUseUPnP)
+	{
+		if(m_Enabled)
+		{
+			char aPort[6];
+			str_format(aPort, sizeof(aPort), "%d", m_Addr.port);
+			int Error = UPNP_DeletePortMapping(m_UPnPUrls->controlURL, m_UPnPData->first.servicetype, aPort, "UDP", NULL);
+
+			if(Error != 0)
+			{
+				dbg_msg("upnp", "failed to delete port mapping on shutdown: %s", strupnperror(Error));
+			}
+			FreeUPNPUrls(m_UPnPUrls);
+			freeUPNPDevlist(m_UPnPDevice);
+		}
+		free(m_UPnPUrls);
+		free(m_UPnPData);
+		m_UPnPUrls = NULL;
+		m_UPnPData = NULL;
+	}
+
+}
+
+#endif

src/engine/server/upnp.h

@@ -0,0 +1,18 @@
+#ifndef ENGINE_SERVER_UPNP_H
+#define ENGINE_SERVER_UPNP_H
+
+#include <base/system.h>
+class CUPnP
+{
+	NETADDR m_Addr;
+	struct UPNPUrls *m_UPnPUrls;
+	struct IGDdatas *m_UPnPData;
+	struct UPNPDev *m_UPnPDevice;
+	bool m_Enabled;
+
+public:
+	void Open(NETADDR Address);
+	void Shutdown(); 
+};
+
+#endif

src/engine/shared/config_variables.h

@@ -222,7 +222,7 @@ MACRO_CONFIG_INT(SvSqlQueriesDelay, sv_sql_queries_delay, 1, 0, 20, CFGFLAG_SERV
 #endif
 
 #if defined(CONF_UPNP)
-MACRO_CONFIG_INT(SvUseUPnP, sv_use_upnp, 1, 0, 1, CFGFLAG_SERVER, "Enables UPnP support.")
+MACRO_CONFIG_INT(SvUseUPnP, sv_use_upnp, 0, 0, 1, CFGFLAG_SERVER, "Enables UPnP support.")
 #endif
 
 MACRO_CONFIG_INT(SvDDRaceRules, sv_ddrace_rules, 1, 0, 1, CFGFLAG_SERVER, "Whether the default mod rules are displayed or not")