created portmap_callback and refactor for natpmp and upnp log optimization (#1096)

created portmap_callback and refactor for natpmp and upnp log optimization

include/libtorrent/Makefile.am

@@ -175,6 +175,7 @@ nobase_include_HEADERS = \
   aux_/byteswap.hpp                 \
   aux_/cppint_import_export.hpp     \
   aux_/ffs.hpp                      \
+  aux_/portmap.hpp                  \
   \
   extensions/smart_ban.hpp          \
   extensions/ut_metadata.hpp        \

include/libtorrent/aux_/portmap.hpp

@@ -0,0 +1,69 @@
+/*
+
+Copyright (c) 2016, Arvid Norberg, Alden Torres
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions
+are met:
+
+    * Redistributions of source code must retain the above copyright
+      notice, this list of conditions and the following disclaimer.
+    * Redistributions in binary form must reproduce the above copyright
+      notice, this list of conditions and the following disclaimer in
+      the documentation and/or other materials provided with the distribution.
+    * Neither the name of the author nor the names of its
+      contributors may be used to endorse or promote products derived
+      from this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+POSSIBILITY OF SUCH DAMAGE.
+
+*/
+
+#ifndef LIBTORRENT_PORTMAP_HPP
+#define LIBTORRENT_PORTMAP_HPP
+
+#include "libtorrent/config.hpp"
+
+namespace libtorrent {
+namespace aux
+{
+	// TODO: move this for a better place and integrate it with
+	// portmap error alerts
+	enum class portmap_transport
+	{
+		natpmp = 0,
+		upnp = 1
+	};
+
+	struct TORRENT_EXTRA_EXPORT portmap_callback
+	{
+		// int: port-mapping index
+		// address: external address as queried from router
+		// int: external port
+		// int: protocol (UDP, TCP)
+		// error_code: error, an empty error means success
+		// int: transport is 0 for NAT-PMP and 1 for UPnP
+		virtual void on_port_mapping(int mapping, address const& ip, int port
+			, int protocol, error_code const& ec, portmap_transport transport) = 0;
+#ifndef TORRENT_DISABLE_LOGGING
+		virtual bool should_log_portmap(portmap_transport transport) const = 0;
+		virtual void log_portmap(portmap_transport transport, char const* msg) const = 0;
+#endif
+
+	protected:
+		~portmap_callback() {}
+	};
+}}
+
+#endif // LIBTORRENT_PORTMAP_HPP

include/libtorrent/aux_/session_impl.hpp

@@ -78,6 +78,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/resolver.hpp"
 #include "libtorrent/invariant_check.hpp"
 #include "libtorrent/extensions.hpp"
+#include "libtorrent/aux_/portmap.hpp"
 
 #if TORRENT_COMPLETE_TYPES_REQUIRED
 #include "libtorrent/peer_connection.hpp"
@@ -185,6 +186,7 @@ namespace libtorrent
 		struct TORRENT_EXTRA_EXPORT session_impl final
 			: session_interface
 			, dht::dht_observer
+			, aux::portmap_callback
 			, boost::noncopyable
 			, uncork_interface
 			, single_threaded
@@ -377,8 +379,6 @@ namespace libtorrent
 			void add_obfuscated_hash(sha1_hash const& obfuscated, std::weak_ptr<torrent> const& t) override;
 #endif
 
-			void on_port_map_log(char const* msg, int map_transport);
-
 			void on_lsd_announce(error_code const& e);
 #ifndef TORRENT_DISABLE_LOGGING
 			void on_lsd_log(char const* log);
@@ -387,7 +387,8 @@ namespace libtorrent
 			// called when a port mapping is successful, or a router returns
 			// a failure to map a port
 			void on_port_mapping(int mapping, address const& ip, int port
-				, int protocol, error_code const& ec, int nat_transport);
+				, int protocol, error_code const& ec
+				, aux::portmap_transport transport) override;
 
 			bool is_aborted() const override { return m_abort; }
 			bool is_paused() const { return m_paused; }
@@ -624,6 +625,10 @@ namespace libtorrent
 				override TORRENT_FORMAT(3,4);
 			virtual void log_packet(message_direction_t dir, char const* pkt, int len
 				, udp::endpoint const& node) override;
+
+			virtual bool should_log_portmap(aux::portmap_transport transport) const override;
+			virtual void log_portmap(aux::portmap_transport transport, char const* msg)
+				const override;
 #endif
 
 			virtual bool on_dht_request(string_view query

include/libtorrent/natpmp.hpp

@@ -40,22 +40,16 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/deadline_timer.hpp"
 #include "libtorrent/time.hpp"
 #include "libtorrent/debug.hpp"
+#include "libtorrent/aux_/portmap.hpp"
 
 namespace libtorrent
 {
 
-// int: port mapping index
-// int: external port
-// std::string: error message
-typedef std::function<void(int, address, int, int, error_code const&)> portmap_callback_t;
-typedef std::function<void(char const*)> log_callback_t;
-
 struct TORRENT_EXTRA_EXPORT natpmp
 	: std::enable_shared_from_this<natpmp>
 	, single_threaded
 {
-	natpmp(io_service& ios, portmap_callback_t const& cb
+	natpmp(io_service& ios, aux::portmap_callback& cb);
-		, log_callback_t const& lcb);
 
 	void start();
 
@@ -84,6 +78,7 @@ private:
 	void close_impl();
 
 #ifndef TORRENT_DISABLE_LOGGING
+	bool should_log() const;
 	void log(char const* fmt, ...) const TORRENT_FORMAT(2, 3);
 #endif
 
@@ -126,10 +121,7 @@ private:
 		bool outstanding_request;
 	};
 
-	portmap_callback_t m_callback;
+	aux::portmap_callback& m_callback;
-#ifndef TORRENT_DISABLE_LOGGING
-	log_callback_t m_log_callback;
-#endif
 
 	std::vector<mapping_t> m_mappings;
 

include/libtorrent/upnp.hpp

@@ -41,6 +41,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/enum_net.hpp"
 #include "libtorrent/resolver.hpp"
 #include "libtorrent/debug.hpp"
+#include "libtorrent/aux_/portmap.hpp"
 
 #include <memory>
 #include <functional>
@@ -92,15 +93,6 @@ namespace libtorrent
 	// the boost.system error category for UPnP errors
 	TORRENT_EXPORT boost::system::error_category& get_upnp_category();
 
-// int: port-mapping index
-// address: external address as queried from router
-// int: external port
-// int: protocol (UDP, TCP)
-// std::string: error message
-// an empty string as error means success
-typedef std::function<void(int, address, int, int, error_code const&)> portmap_callback_t;
-typedef std::function<void(char const*)> log_callback_t;
-
 struct parse_state
 {
 	parse_state(): in_service(false) {}
@@ -132,7 +124,7 @@ struct TORRENT_EXTRA_EXPORT upnp final
 {
 	upnp(io_service& ios
 		, std::string const& user_agent
-		, portmap_callback_t const& cb, log_callback_t const& lcb
+		, aux::portmap_callback& cb
 		, bool ignore_nonrouters);
 	~upnp();
 
@@ -218,6 +210,7 @@ private:
 	void disable(error_code const& ec);
 	void return_error(int mapping, int code);
 #ifndef TORRENT_DISABLE_LOGGING
+	bool should_log() const;
 	void log(char const* msg, ...) const TORRENT_FORMAT(2,3);
 #endif
 
@@ -360,10 +353,7 @@ private:
 	// the set of devices we've found
 	std::set<rootdevice> m_devices;
 
-	portmap_callback_t m_callback;
+	aux::portmap_callback& m_callback;
-#ifndef TORRENT_DISABLE_LOGGING
-	log_callback_t m_log_callback;
-#endif
 
 	// current retry count
 	int m_retry_count;

src/natpmp.cpp

@@ -60,11 +60,8 @@ using namespace libtorrent;
 using namespace std::placeholders;
 
 natpmp::natpmp(io_service& ios
-	, portmap_callback_t const& cb, log_callback_t const& lcb)
+	, aux::portmap_callback& cb)
 	: m_callback(cb)
-#ifndef TORRENT_DISABLE_LOGGING
-	, m_log_callback(lcb)
-#endif
 	, m_currently_mapping(-1)
 	, m_retry_count(0)
 	, m_socket(ios)
@@ -74,7 +71,6 @@ natpmp::natpmp(io_service& ios
 	, m_disabled(false)
 	, m_abort(false)
 {
-	TORRENT_UNUSED(lcb);
 	// unfortunately async operations rely on the storage
 	// for this array not to be reallocated, by passing
 	// around pointers to its elements. so reserve size for now
@@ -90,8 +86,11 @@ void natpmp::start()
 	if (ec)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("failed to find default route: %s"
 				, convert_from_native(ec.message()).c_str());
+		}
 #endif
 		disable(ec);
 		return;
@@ -104,8 +103,11 @@ void natpmp::start()
 	m_nat_endpoint = nat_endpoint;
 
 #ifndef TORRENT_DISABLE_LOGGING
+	if (should_log())
+	{
 		log("found router at: %s"
 			, print_address(m_nat_endpoint.address()).c_str());
+	}
 #endif
 
 	m_socket.open(udp::v4(), ec);
@@ -170,18 +172,22 @@ bool natpmp::get_mapping(int index, int& local_port, int& external_port, int& pr
 }
 
 #ifndef TORRENT_DISABLE_LOGGING
+bool natpmp::should_log() const
+{
+	return m_callback.should_log_portmap(aux::portmap_transport::natpmp);
+}
+
 TORRENT_FORMAT(2, 3)
 void natpmp::log(char const* fmt, ...) const
 {
 	TORRENT_ASSERT(is_single_thread());
+	if (!should_log()) return;
 	char msg[200];
-
 	va_list v;
 	va_start(v, fmt);
 	std::vsnprintf(msg, sizeof(msg), fmt, v);
 	va_end(v);
-
+	m_callback.log_portmap(aux::portmap_transport::natpmp, msg);
-	m_log_callback(msg);
 }
 #endif
 
@@ -197,7 +203,8 @@ void natpmp::disable(error_code const& ec)
 		int const proto = i->protocol;
 		i->protocol = none;
 		int index = i - m_mappings.begin();
-		m_callback(index, address(), 0, proto, ec);
+		m_callback.on_port_mapping(index, address(), 0, proto, ec
+			, aux::portmap_transport::natpmp);
 	}
 	close_impl();
 }
@@ -243,6 +250,8 @@ int natpmp::add_mapping(protocol_type p, int const external_port
 
 	int const mapping_index = i - m_mappings.begin();
 #ifndef TORRENT_DISABLE_LOGGING
+	if (should_log())
+	{
 		natpmp::mapping_t const& m = *i;
 		log("add-mapping: proto: %s port: %d local-port: %d action: %s ttl: %" PRId64
 			, (m.protocol == none
@@ -252,6 +261,7 @@ int natpmp::add_mapping(protocol_type p, int const external_port
 			, (m.action == mapping_t::action_none
 				? "none" : m.action == mapping_t::action_add ? "add" : "delete")
 			, total_seconds(m.expires - aux::time_now()));
+	}
 #endif
 
 	update_mapping(mapping_index);
@@ -302,6 +312,8 @@ void natpmp::update_mapping(int const i)
 	natpmp::mapping_t const& m = m_mappings[i];
 
 #ifndef TORRENT_DISABLE_LOGGING
+	if (should_log())
+	{
 		log("update-mapping: proto: %s port: %d local-port: %d action: %s ttl: %" PRId64
 			, (m.protocol == none
 				? "none" : m.protocol == tcp ? "tcp" : "udp")
@@ -310,6 +322,7 @@ void natpmp::update_mapping(int const i)
 			, (m.action == mapping_t::action_none
 				? "none" : m.action == mapping_t::action_add ? "add" : "delete")
 			, total_seconds(m.expires - aux::time_now()));
+	}
 #endif
 
 	if (m.action == mapping_t::action_none
@@ -349,11 +362,14 @@ void natpmp::send_map_request(int const i)
 	write_uint32(ttl, out); // port mapping lifetime
 
 #ifndef TORRENT_DISABLE_LOGGING
+	if (should_log())
+	{
 		log("==> port map [ mapping: %d action: %s"
 			" proto: %s local: %u external: %u ttl: %u ]"
 			, i, m.action == mapping_t::action_add ? "add" : "delete"
 			, m.protocol == udp ? "udp" : "tcp"
 			, m.local_port, m.external_port, ttl);
+	}
 #endif
 
 	error_code ec;
@@ -411,8 +427,11 @@ void natpmp::on_reply(error_code const& e
 	if (e)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error on receiving reply: %s"
 				, convert_from_native(e.message()).c_str());
+		}
 #endif
 		return;
 	}
@@ -430,8 +449,11 @@ void natpmp::on_reply(error_code const& e
 	if (m_remote != m_nat_endpoint)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("received packet from wrong IP: %s"
 				, print_endpoint(m_remote).c_str());
+		}
 #endif
 		return;
 	}
@@ -461,7 +483,10 @@ void natpmp::on_reply(error_code const& e
 		m_external_ip = read_v4_address(in);
 
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("<== public IP address [ %s ]", print_address(m_external_ip).c_str());
+		}
 #endif
 		return;
 
@@ -551,14 +576,16 @@ void natpmp::on_reply(error_code const& e
 
 		m->expires = aux::time_now() + hours(2);
 		int const proto = m->protocol;
-		m_callback(index, address(), 0, proto
+		m_callback.on_port_mapping(index, address(), 0, proto
-			, error_code(ev, get_libtorrent_category()));
+			, error_code(ev, get_libtorrent_category())
+			, aux::portmap_transport::natpmp);
 	}
 	else if (m->action == mapping_t::action_add)
 	{
 		int const proto = m->protocol;
-		m_callback(index, m_external_ip, m->external_port, proto
+		m_callback.on_port_mapping(index, m_external_ip, m->external_port, proto
-			, error_code(errors::no_error, get_libtorrent_category()));
+			, error_code(errors::no_error, get_libtorrent_category())
+			, aux::portmap_transport::natpmp);
 	}
 
 	if (m_abort) return;

src/session_impl.cpp

@@ -39,6 +39,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include <cstdio> // for snprintf
 #include <cinttypes> // for PRId64 et.al.
 #include <functional>
+#include <type_traits>
 
 #if TORRENT_USE_INVARIANT_CHECKS
 #include <unordered_set>
@@ -5371,21 +5372,6 @@ namespace aux {
 			m_alerts.emplace_alert<lsd_peer_alert>(t->get_handle(), peer);
 	}
 
-	// TODO: perhaps this function should not exist when logging is disabled
-	void session_impl::on_port_map_log(
-		char const* msg, int map_transport)
-	{
-#ifndef TORRENT_DISABLE_LOGGING
-		TORRENT_ASSERT(map_transport >= 0 && map_transport <= 1);
-		// log message
-		if (m_alerts.should_post<portmap_log_alert>())
-			m_alerts.emplace_alert<portmap_log_alert>(map_transport, msg);
-#else
-		TORRENT_UNUSED(msg);
-		TORRENT_UNUSED(map_transport);
-#endif
-	}
-
 	namespace {
 		bool find_tcp_port_mapping(int transport, int mapping, listen_socket_t const& ls)
 		{
@@ -5400,10 +5386,13 @@ namespace aux {
 
 	// transport is 0 for NAT-PMP and 1 for UPnP
 	void session_impl::on_port_mapping(int mapping, address const& ip, int port
-		, int const protocol, error_code const& ec, int map_transport)
+		, int const protocol, error_code const& ec
+		, aux::portmap_transport transport)
 	{
 		TORRENT_ASSERT(is_single_thread());
 
+		int map_transport =
+			static_cast<std::underlying_type<aux::portmap_transport>::type>(transport);
 		TORRENT_ASSERT(map_transport >= 0 && map_transport <= 1);
 
 		if (ec && m_alerts.should_post<portmap_error_alert>())
@@ -6495,11 +6484,7 @@ namespace aux {
 
 		// the natpmp constructor may fail and call the callbacks
 		// into the session_impl.
-		m_natpmp = std::make_shared<natpmp>(std::ref(m_io_service)
+		m_natpmp = std::make_shared<natpmp>(std::ref(m_io_service), *this);
-			, std::bind(&session_impl::on_port_mapping
-				, this, _1, _2, _3, _4, _5, 0)
-			, std::bind(&session_impl::on_port_map_log
-				, this, _1, 0));
 		m_natpmp->start();
 
 		for (auto& s : m_listen_sockets)
@@ -6518,10 +6503,7 @@ namespace aux {
 		// the upnp constructor may fail and call the callbacks
 		m_upnp = std::make_shared<upnp>(std::ref(m_io_service)
 			, m_settings.get_str(settings_pack::user_agent)
-			, std::bind(&session_impl::on_port_mapping
+			, *this
-				, this, _1, _2, _3, _4, _5, 1)
-			, std::bind(&session_impl::on_port_map_log
-				, this, _1, 1)
 			, m_settings.get_bool(settings_pack::upnp_ignore_nonrouters));
 		m_upnp->start();
 
@@ -6661,6 +6643,21 @@ namespace aux {
 
 		m_alerts.emplace_alert<dht_pkt_alert>(pkt, len, d, node);
 	}
+
+	bool session_impl::should_log_portmap(aux::portmap_transport) const
+	{
+		return m_alerts.should_post<portmap_log_alert>();
+	}
+
+	void session_impl::log_portmap(aux::portmap_transport transport, char const* msg) const
+	{
+		int map_transport =
+			static_cast<std::underlying_type<aux::portmap_transport>::type>(transport);
+		TORRENT_ASSERT(map_transport >= 0 && map_transport <= 1);
+
+		if (m_alerts.should_post<portmap_log_alert>())
+			m_alerts.emplace_alert<portmap_log_alert>(map_transport, msg);
+	}
 #endif
 
 	bool session_impl::on_dht_request(string_view query

src/upnp.cpp

@@ -72,13 +72,10 @@ static error_code ignore_error;
 // interface by default
 upnp::upnp(io_service& ios
 	, std::string const& user_agent
-	, portmap_callback_t const& cb, log_callback_t const& lcb
+	, aux::portmap_callback& cb
 	, bool ignore_nonrouters)
 	: m_user_agent(user_agent)
 	, m_callback(cb)
-#ifndef TORRENT_DISABLE_LOGGING
-	, m_log_callback(lcb)
-#endif
 	, m_retry_count(0)
 	, m_io_service(ios)
 	, m_resolver(ios)
@@ -92,8 +89,6 @@ upnp::upnp(io_service& ios
 	, m_ignore_non_routers(ignore_nonrouters)
 	, m_last_if_update(min_time())
 {
-	TORRENT_UNUSED(lcb);
-	TORRENT_ASSERT(cb);
 }
 
 void upnp::start()
@@ -121,16 +116,22 @@ void upnp::discover_device()
 }
 
 #ifndef TORRENT_DISABLE_LOGGING
+bool upnp::should_log() const
+{
+	return m_callback.should_log_portmap(aux::portmap_transport::upnp);
+}
+
 TORRENT_FORMAT(2,3)
 void upnp::log(char const* fmt, ...) const
 {
 	TORRENT_ASSERT(is_single_thread());
+	if (!should_log()) return;
 	va_list v;
 	va_start(v, fmt);
 	char msg[500];
 	std::vsnprintf(msg, sizeof(msg), fmt, v);
 	va_end(v);
-	m_log_callback(msg);
+	m_callback.log_portmap(aux::portmap_transport::upnp, msg);
 }
 #endif
 
@@ -155,8 +156,11 @@ void upnp::discover_device_impl()
 	if (ec)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("broadcast failed: %s. Aborting."
 				, convert_from_native(ec.message()).c_str());
+		}
 #endif
 		disable(ec);
 		return;
@@ -366,7 +370,7 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 	{
 		m_interfaces = enum_net_interfaces(m_io_service, ec);
 #ifndef TORRENT_DISABLE_LOGGING
-		if (ec)
+		if (ec && should_log())
 		{
 			log("when receiving response from: %s: %s"
 				, print_endpoint(from).c_str(), convert_from_native(ec.message()).c_str());
@@ -378,6 +382,8 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 	if (!ec && !in_local_network(m_interfaces, from.address()))
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			char msg[400];
 			int num_chars = std::snprintf(msg, sizeof(msg)
 				, "ignoring response from: %s. IP is not on local network. "
@@ -390,6 +396,7 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 				if (num_chars >= int(sizeof(msg))) break;
 			}
 			log("%s", msg);
+		}
 #endif
 		return;
 	}
@@ -406,14 +413,19 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 			if (ec)
 			{
 #ifndef TORRENT_DISABLE_LOGGING
+				if (should_log())
+				{
 					log("failed to enumerate routes when "
 						"receiving response from: %s: %s"
 						, print_endpoint(from).c_str(), convert_from_native(ec.message()).c_str());
+				}
 #endif
 			}
 			else
 			{
 #ifndef TORRENT_DISABLE_LOGGING
+				if (should_log())
+				{
 					char msg[400];
 					int num_chars = std::snprintf(msg, sizeof(msg), "SSDP response from: "
 						"%s: IP is not a router. "
@@ -426,6 +438,7 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 						if (num_chars >= int(sizeof(msg))) break;
 					}
 					log("%s", msg);
+				}
 #endif
 				non_router = true;
 			}
@@ -438,8 +451,11 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 	if (error)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("received malformed HTTP from: %s"
 				, print_endpoint(from).c_str());
+		}
 #endif
 		return;
 	}
@@ -447,6 +463,8 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 	if (p.status_code() != 200 && p.method() != "notify")
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			if (p.method().empty())
 			{
 				log("HTTP status %u from %s"
@@ -457,6 +475,7 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 				log("HTTP method %s from %s"
 					, p.method().c_str(), print_endpoint(from).c_str());
 			}
+		}
 #endif
 		return;
 	}
@@ -464,8 +483,11 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 	if (!p.header_finished())
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("incomplete HTTP packet from %s"
 				, print_endpoint(from).c_str());
+		}
 #endif
 		return;
 	}
@@ -474,8 +496,11 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 	if (url.empty())
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("missing location header from %s"
 				, print_endpoint(from).c_str());
+		}
 #endif
 		return;
 	}
@@ -497,8 +522,11 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 		if (ec)
 		{
 #ifndef TORRENT_DISABLE_LOGGING
+			if (should_log())
+			{
 				log("invalid URL %s from %s: %s"
 					, d.url.c_str(), print_endpoint(from).c_str(), convert_from_native(ec.message()).c_str());
+			}
 #endif
 			return;
 		}
@@ -509,8 +537,11 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 		if (protocol != "http")
 		{
 #ifndef TORRENT_DISABLE_LOGGING
+			if (should_log())
+			{
 				log("unsupported protocol %s from %s"
 					, protocol.c_str(), print_endpoint(from).c_str());
+			}
 #endif
 			return;
 		}
@@ -518,12 +549,16 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 		if (d.port == 0)
 		{
 #ifndef TORRENT_DISABLE_LOGGING
+			if (should_log())
+			{
 				log("URL with port 0 from %s", print_endpoint(from).c_str());
+			}
 #endif
 			return;
 		}
 
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
 		{
 			log("found rootdevice: %s (%d)"
 				, d.url.c_str(), int(m_devices.size()));
@@ -533,8 +568,11 @@ void upnp::on_reply(udp::endpoint const& from, char* buffer
 		if (m_devices.size() >= 50)
 		{
 #ifndef TORRENT_DISABLE_LOGGING
+			if (should_log())
+			{
 				log("too many rootdevices: (%d). Ignoring %s"
 					, int(m_devices.size()), d.url.c_str());
+			}
 #endif
 			return;
 		}
@@ -910,8 +948,11 @@ void upnp::on_upnp_xml(error_code const& e
 	if (e && e != boost::asio::error::eof)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error while fetching control url from: %s: %s"
 				, d.url.c_str(), convert_from_native(e.message()).c_str());
+		}
 #endif
 		d.disabled = true;
 		return;
@@ -930,8 +971,11 @@ void upnp::on_upnp_xml(error_code const& e
 	if (p.status_code() != 200)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error while fetching control url from: %s: %s"
 				, d.url.c_str(), convert_from_native(p.message()).c_str());
+		}
 #endif
 		d.disabled = true;
 		return;
@@ -977,6 +1021,7 @@ void upnp::on_upnp_xml(error_code const& e
 	}
 
 #ifndef TORRENT_DISABLE_LOGGING
+	if (should_log())
 	{
 		log("found control URL: %s namespace %s "
 			"urlbase: %s in response from %s"
@@ -992,8 +1037,11 @@ void upnp::on_upnp_xml(error_code const& e
 	if (ec)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("failed to parse URL '%s': %s"
 				, d.control_url.c_str(), convert_from_native(ec.message()).c_str());
+		}
 #endif
 		d.disabled = true;
 		return;
@@ -1050,7 +1098,8 @@ void upnp::disable(error_code const& ec)
 		if (i->protocol == none) continue;
 		int const proto = i->protocol;
 		i->protocol = none;
-		m_callback(i - m_mappings.begin(), address(), 0, proto, ec);
+		m_callback.on_port_mapping(i - m_mappings.begin(), address(), 0, proto, ec
+			, aux::portmap_transport::upnp);
 	}
 
 	// we cannot clear the devices since there
@@ -1193,8 +1242,11 @@ void upnp::on_upnp_get_ip_address_response(error_code const& e
 	if (e && e != boost::asio::error::eof)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error while getting external IP address: %s"
 				, convert_from_native(e.message()).c_str());
+		}
 #endif
 		if (num_mappings() > 0) update_map(d, 0);
 		return;
@@ -1212,8 +1264,11 @@ void upnp::on_upnp_get_ip_address_response(error_code const& e
 	if (p.status_code() != 200)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error while getting external IP address: %s"
 				, convert_from_native(p.message()).c_str());
+		}
 #endif
 		if (num_mappings() > 0) update_map(d, 0);
 		return;
@@ -1230,8 +1285,11 @@ void upnp::on_upnp_get_ip_address_response(error_code const& e
 
 	span<char const> body = p.get_body();
 #ifndef TORRENT_DISABLE_LOGGING
+	if (should_log())
+	{
 		log("get external IP address response: %s"
 			, std::string(body.data(), body.size()).c_str());
+	}
 #endif
 
 	ip_address_parse_state s;
@@ -1277,8 +1335,11 @@ void upnp::on_upnp_map_response(error_code const& e
 	if (e && e != boost::asio::error::eof)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error while adding port map: %s"
 				, convert_from_native(e.message()).c_str());
+		}
 #endif
 		d.disabled = true;
 		return;
@@ -1375,13 +1436,17 @@ void upnp::on_upnp_map_response(error_code const& e
 	}
 
 #ifndef TORRENT_DISABLE_LOGGING
+	if (should_log())
+	{
 		log("map response: %s"
 			, std::string(body.data(), body.size()).c_str());
+	}
 #endif
 
 	if (s.error_code == -1)
 	{
-		m_callback(mapping, d.external_ip, m.external_port, m.protocol, error_code());
+		m_callback.on_port_mapping(mapping, d.external_ip, m.external_port, m.protocol, error_code()
+			, aux::portmap_transport::upnp);
 		if (d.lease_duration > 0)
 		{
 			m.expires = aux::time_now()
@@ -1424,7 +1489,8 @@ void upnp::return_error(int mapping, int code)
 		error_string += e->msg;
 	}
 	const int proto = m_mappings[mapping].protocol;
-	m_callback(mapping, address(), 0, proto, error_code(code, get_upnp_category()));
+	m_callback.on_port_mapping(mapping, address(), 0, proto, error_code(code, get_upnp_category())
+		, aux::portmap_transport::upnp);
 }
 
 void upnp::on_upnp_unmap_response(error_code const& e
@@ -1444,8 +1510,11 @@ void upnp::on_upnp_unmap_response(error_code const& e
 	if (e && e != boost::asio::error::eof)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error while deleting portmap: %s"
 				, convert_from_native(e.message()).c_str());
+		}
 #endif
 	}
 	else if (!p.header_finished())
@@ -1457,16 +1526,22 @@ void upnp::on_upnp_unmap_response(error_code const& e
 	else if (p.status_code() != 200)
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			log("error while deleting portmap: %s"
 				, convert_from_native(p.message()).c_str());
+		}
 #endif
 	}
 	else
 	{
 #ifndef TORRENT_DISABLE_LOGGING
+		if (should_log())
+		{
 			span<char const> body = p.get_body();
 			log("unmap response: %s"
 				, std::string(body.data(), body.size()).c_str());
+		}
 #endif
 	}
 
@@ -1478,9 +1553,10 @@ void upnp::on_upnp_unmap_response(error_code const& e
 
 	int const proto = m_mappings[mapping].protocol;
 
-	m_callback(mapping, address(), 0, proto, p.status_code() != 200
+	m_callback.on_port_mapping(mapping, address(), 0, proto, p.status_code() != 200
 		? error_code(p.status_code(), get_http_category())
-		: error_code(s.error_code, get_upnp_category()));
+		: error_code(s.error_code, get_upnp_category())
+		, aux::portmap_transport::upnp);
 
 	d.mapping[mapping].protocol = none;
 

test/test_natpmp.cpp

@@ -39,19 +39,33 @@ POSSIBILITY OF SUCH DAMAGE.
 
 using namespace libtorrent;
 
-void callback(int mapping, address extip, int port, int protocol, error_code const& err)
+namespace
 {
+	struct natpmp_callback : aux::portmap_callback
+	{
+		void on_port_mapping(int mapping, address const& ip, int port
+			, int protocol, error_code const& err
+			, aux::portmap_transport transport) override
+		{
 			std::cerr
 				<< "mapping: " << mapping
 				<< ", port: " << port
 				<< ", protocol: " << protocol
-		<< ", external-IP: " << print_address(extip)
+				<< ", external-IP: " << print_address(ip)
 				<< ", error: \"" << err.message() << "\"\n";
-}
+		}
+#ifndef TORRENT_DISABLE_LOGGING
+		virtual bool should_log_portmap(aux::portmap_transport transport) const override
+		{
+			return true;
+		}
 
-void log_callback(char const* line)
+		virtual void log_portmap(aux::portmap_transport transport, char const* msg) const override
-{
+		{
-	std::cerr << line << std::endl;
+			std::cerr << msg << std::endl;
+		}
+#endif
+	};
 }
 
 int main(int argc, char* argv[])
@@ -65,8 +79,8 @@ int main(int argc, char* argv[])
 		return 1;
 	}
 
-	std::shared_ptr<natpmp> natpmp_handler(new natpmp(
+	natpmp_callback cb;
-		ios, &callback, &log_callback));
+	auto natpmp_handler = std::make_shared<natpmp>(ios, cb);
 
 	deadline_timer timer(ios);
 
@@ -95,5 +109,3 @@ int main(int argc, char* argv[])
 	ios.run(ec);
 	std::cerr << "closing" << std::endl;
 }
-
-

test/test_upnp.cpp

@@ -102,12 +102,6 @@ void incoming_msearch(udp::endpoint const& from, char* buffer
 	if (ec) std::cerr << "*** error sending " << ec.message() << std::endl;
 }
 
-void log_callback(char const* err)
-{
-	std::cerr << "UPnP: " << err << std::endl;
-	//TODO: store the log and verify that some key messages are there
-}
-
 struct callback_info
 {
 	int mapping;
@@ -119,12 +113,32 @@ struct callback_info
 
 std::list<callback_info> callbacks;
 
-void callback(int mapping, address const& ip, int port, int protocol, error_code const& err)
+namespace
 {
+	struct upnp_callback : aux::portmap_callback
+	{
+		void on_port_mapping(int mapping, address const& ip, int port
+			, int protocol, error_code const& err
+			, aux::portmap_transport transport) override
+		{
 			callback_info info = {mapping, port, err};
 			callbacks.push_back(info);
 			std::cerr << "mapping: " << mapping << ", port: " << port << ", IP: " << ip
 				<< ", proto: " << protocol << ", error: \"" << err.message() << "\"\n";
+		}
+	#ifndef TORRENT_DISABLE_LOGGING
+		virtual bool should_log_portmap(aux::portmap_transport transport) const override
+		{
+			return true;
+		}
+
+		virtual void log_portmap(aux::portmap_transport transport, char const* msg) const override
+		{
+			std::cerr << "UPnP: " << msg << std::endl;
+			//TODO: store the log and verify that some key messages are there
+		}
+	#endif
+	};
 }
 
 void run_upnp_test(char const* root_filename, char const* router_model, char const* control_name, int igd_version)
@@ -159,8 +173,8 @@ void run_upnp_test(char const* root_filename, char const* router_model, char con
 
 	std::string user_agent = "test agent";
 
-	std::shared_ptr<upnp> upnp_handler = std::make_shared<upnp>(std::ref(ios)
+	upnp_callback cb;
-		, user_agent, &callback, &log_callback, false);
+	auto upnp_handler = std::make_shared<upnp>(ios, user_agent, cb, false);
 	upnp_handler->start();
 	upnp_handler->discover_device();