From fa79697f4548f5f55c3703c995afa47862d7f949 Mon Sep 17 00:00:00 2001 From: arvidn Date: Tue, 14 Jan 2020 23:46:05 +0100 Subject: [PATCH] deprecate (and remove) upnp_ignore_nonrouters setting --- ChangeLog | 1 + include/libtorrent/settings_pack.hpp | 6 +- include/libtorrent/upnp.hpp | 19 ++---- src/session.cpp | 4 -- src/session_impl.cpp | 3 +- src/settings_pack.cpp | 2 +- src/upnp.cpp | 97 +++------------------------- test/test_magnet.cpp | 1 - test/test_upnp.cpp | 4 +- 9 files changed, 25 insertions(+), 112 deletions(-) diff --git a/ChangeLog b/ChangeLog index edfa10255..a93b92542 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ + * deprecate upnp_ignore_nonrouters setting * don't attempt sending event=stopped if event=start never succeeded * make sure &key= stays consistent between different source IPs (as mandated by BEP7) * fix binding sockets to outgoing interface diff --git a/include/libtorrent/settings_pack.hpp b/include/libtorrent/settings_pack.hpp index cc3452c6f..73c7689a4 100644 --- a/include/libtorrent/settings_pack.hpp +++ b/include/libtorrent/settings_pack.hpp @@ -309,11 +309,15 @@ namespace aux { // trackers fail or not. use_dht_as_fallback, +#if TORRENT_ABI_VERSION == 1 // ``upnp_ignore_nonrouters`` indicates whether or not the UPnP // implementation should ignore any broadcast response from a device // whose address is not the configured router for this machine. i.e. // it's a way to not talk to other people's routers by mistake. - upnp_ignore_nonrouters, + upnp_ignore_nonrouters TORRENT_DEPRECATED_ENUM, +#else + deprecated_upnp_ignore_nonrouters, +#endif // ``use_parole_mode`` specifies if parole mode should be used. Parole // mode means that peers that participate in pieces that fail the hash diff --git a/include/libtorrent/upnp.hpp b/include/libtorrent/upnp.hpp index e110e93cd..55cec0f59 100644 --- a/include/libtorrent/upnp.hpp +++ b/include/libtorrent/upnp.hpp @@ -149,8 +149,7 @@ struct TORRENT_EXTRA_EXPORT upnp final { upnp(io_service& ios , std::string const& user_agent - , aux::portmap_callback& cb - , bool ignore_nonrouters); + , aux::portmap_callback& cb); ~upnp(); void set_user_agent(std::string const& v) { m_user_agent = v; } @@ -199,7 +198,7 @@ private: std::shared_ptr self() { return shared_from_this(); } void map_timer(error_code const& ec); - void try_map_upnp(bool timer = false); + void try_map_upnp(); void discover_device_impl(); void resend_request(error_code const& e); @@ -297,13 +296,6 @@ private: bool disabled = false; - // this is true if the IP of this device is not - // one of our default routes. i.e. it may be someone - // else's router, we just happen to have multicast - // enabled across networks - // this is only relevant if ignore_non_routers is set. - bool non_router = false; - mutable std::shared_ptr upnp_connection; #if TORRENT_USE_ASSERTS @@ -330,7 +322,7 @@ private: aux::portmap_callback& m_callback; // current retry count - int m_retry_count; + int m_retry_count = 0; io_service& m_io_service; @@ -354,9 +346,8 @@ private: // map them anyway. deadline_timer m_map_timer; - bool m_disabled; - bool m_closing; - bool m_ignore_non_routers; + bool m_disabled = false; + bool m_closing = false; std::string m_model; diff --git a/src/session.cpp b/src/session.cpp index fb84b251c..c6d1c4a08 100644 --- a/src/session.cpp +++ b/src/session.cpp @@ -138,10 +138,6 @@ namespace { // socket until the disk write is complete set.set_int(settings_pack::max_queued_disk_bytes, 1); - // don't keep track of all upnp devices, keep - // the device list small - set.set_bool(settings_pack::upnp_ignore_nonrouters, true); - // never keep more than one 16kB block in // the send buffer set.set_int(settings_pack::send_buffer_watermark, 9); diff --git a/src/session_impl.cpp b/src/session_impl.cpp index 5c6807dc5..e96bb0198 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -6709,8 +6709,7 @@ namespace aux { m_upnp = std::make_shared(m_io_service , m_settings.get_bool(settings_pack::anonymous_mode) ? "" : m_settings.get_str(settings_pack::user_agent) - , *this - , m_settings.get_bool(settings_pack::upnp_ignore_nonrouters)); + , *this); m_upnp->start(); m_upnp->discover_device(); diff --git a/src/settings_pack.cpp b/src/settings_pack.cpp index abe36e8c9..52bbf3aa7 100644 --- a/src/settings_pack.cpp +++ b/src/settings_pack.cpp @@ -135,7 +135,7 @@ constexpr int CLOSE_FILE_INTERVAL = 0; SET(send_redundant_have, true, nullptr), DEPRECATED_SET(lazy_bitfields, false, nullptr), SET(use_dht_as_fallback, false, nullptr), - SET(upnp_ignore_nonrouters, false, nullptr), + DEPRECATED_SET(upnp_ignore_nonrouters, false, nullptr), SET(use_parole_mode, true, nullptr), SET(use_read_cache, true, nullptr), DEPRECATED_SET(use_write_cache, true, nullptr), diff --git a/src/upnp.cpp b/src/upnp.cpp index 350178511..f7e950f13 100644 --- a/src/upnp.cpp +++ b/src/upnp.cpp @@ -96,11 +96,9 @@ upnp::rootdevice& upnp::rootdevice::operator=(rootdevice&&) = default; // interface by default upnp::upnp(io_service& ios , std::string const& user_agent - , aux::portmap_callback& cb - , bool ignore_nonrouters) + , aux::portmap_callback& cb) : m_user_agent(user_agent) , m_callback(cb) - , m_retry_count(0) , m_io_service(ios) , m_resolver(ios) , m_socket(udp::endpoint(make_address_v4("239.255.255.250" @@ -108,9 +106,6 @@ upnp::upnp(io_service& ios , m_broadcast_timer(ios) , m_refresh_timer(ios) , m_map_timer(ios) - , m_disabled(false) - , m_closing(false) - , m_ignore_non_routers(ignore_nonrouters) , m_last_if_update(min_time()) { } @@ -152,7 +147,7 @@ void upnp::log(char const* fmt, ...) const if (!should_log()) return; va_list v; va_start(v, fmt); - char msg[500]; + char msg[1024]; std::vsnprintf(msg, sizeof(msg), fmt, v); va_end(v); m_callback.log_portmap(portmap_transport::upnp, msg); @@ -441,50 +436,6 @@ void upnp::on_reply(udp::endpoint const& from, span buffer) return; } - bool non_router = false; - if (m_ignore_non_routers) - { - auto const routes = enum_routes(m_io_service, ec); - if (std::none_of(routes.begin(), routes.end() - , [from] (ip_route const& rt) { return rt.gateway == from.address(); })) - { - // this upnp device is filtered because it's not in the - // list of configured routers - 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. " - , print_endpoint(from).c_str()); - for (auto const& route : routes) - { - num_chars += std::snprintf(msg + num_chars, sizeof(msg) - std::size_t(num_chars), "(%s,%s) " - , print_address(route.gateway).c_str() - , print_address(route.netmask).c_str()); - if (num_chars >= int(sizeof(msg))) break; - } - log("%s", msg); - } -#endif - non_router = true; - } - } - } - http_parser p; bool error = false; p.incoming(buffer, error); @@ -616,7 +567,6 @@ void upnp::on_reply(udp::endpoint const& from, span buffer) #endif return; } - d.non_router = non_router; TORRENT_ASSERT(d.mapping.empty()); for (auto const& j : m_mappings) @@ -635,15 +585,12 @@ void upnp::on_reply(udp::endpoint const& from, span buffer) // iterate over the devices we know and connect and issue the mappings try_map_upnp(); - if (m_ignore_non_routers) - { - ADD_OUTSTANDING_ASYNC("upnp::map_timer"); - // check back in a little bit to see if we have seen any - // devices at one of our default routes. If not, we want to override - // ignoring them and use them instead (better than not working). - m_map_timer.expires_from_now(seconds(1), ec); - m_map_timer.async_wait(std::bind(&upnp::map_timer, self(), _1)); - } + // check back in a little bit to see if we have seen any + // devices at one of our default routes. If not, we want to override + // ignoring them and use them instead (better than not working). + m_map_timer.expires_from_now(seconds(1), ec); + ADD_OUTSTANDING_ASYNC("upnp::map_timer"); + m_map_timer.async_wait(std::bind(&upnp::map_timer, self(), _1)); } void upnp::map_timer(error_code const& ec) @@ -653,40 +600,16 @@ void upnp::map_timer(error_code const& ec) if (ec) return; if (m_closing) return; - try_map_upnp(true); + try_map_upnp(); } -void upnp::try_map_upnp(bool const timer) +void upnp::try_map_upnp() { TORRENT_ASSERT(is_single_thread()); if (m_devices.empty()) return; - bool override_ignore_non_routers = false; - if (m_ignore_non_routers && timer) - { - // if we don't have any devices that match our default route, we - // should try to map with the ones we did hear from anyway, - // regardless of if they are not running at our gateway. - override_ignore_non_routers = std::none_of(m_devices.begin() - , m_devices.end(), [](rootdevice const& d) { return !d.non_router; }); -#ifndef TORRENT_DISABLE_LOGGING - if (override_ignore_non_routers) - { - log("overriding ignore non-routers"); - } -#endif - } - for (auto i = m_devices.begin(), end(m_devices.end()); i != end; ++i) { - // if we're ignoring non-routers, skip them. If on_timer is - // set, we expect to have received all responses and if we don't - // have any devices at our default route, then issue requests - // to any device we found. - if (m_ignore_non_routers && i->non_router - && !override_ignore_non_routers) - continue; - if (i->control_url.empty() && !i->upnp_connection && !i->disabled) { // we don't have a WANIP or WANPPP url for this device, diff --git a/test/test_magnet.cpp b/test/test_magnet.cpp index 441baa9d0..ed686de42 100644 --- a/test/test_magnet.cpp +++ b/test/test_magnet.cpp @@ -87,7 +87,6 @@ TORRENT_TEST(magnet) pack.set_int(settings_pack::file_pool_size, 543); pack.set_int(settings_pack::urlseed_wait_retry, 74); pack.set_int(settings_pack::initial_picker_threshold, 351); - pack.set_bool(settings_pack::upnp_ignore_nonrouters, true); pack.set_bool(settings_pack::coalesce_writes, true); pack.set_bool(settings_pack::close_redundant_connections, false); pack.set_int(settings_pack::auto_scrape_interval, 235); diff --git a/test/test_upnp.cpp b/test/test_upnp.cpp index 206a7e1e2..bdcf7b00d 100644 --- a/test/test_upnp.cpp +++ b/test/test_upnp.cpp @@ -196,7 +196,7 @@ void run_upnp_test(char const* root_filename, char const* control_name, int igd_ std::string user_agent = "test agent"; upnp_callback cb; - auto upnp_handler = std::make_shared(ios, user_agent, cb, false); + auto upnp_handler = std::make_shared(ios, user_agent, cb); upnp_handler->start(); upnp_handler->discover_device(); @@ -283,7 +283,7 @@ TORRENT_TEST(upnp_max_mappings) { lt::io_service ios; upnp_callback cb; - auto upnp_handler = std::make_shared(ios, "test agent", cb, false); + auto upnp_handler = std::make_shared(ios, "test agent", cb); for (int i = 0; i < 50; ++i) {