From adc209d7e8a6a1a5561b3d38036abd2f39d774e5 Mon Sep 17 00:00:00 2001 From: Yat Ho Date: Thu, 23 Nov 2023 13:02:21 +0800 Subject: [PATCH] refactor: RPC `port-test` improvements (#6274) --- docs/rpc-spec.md | 9 ++--- libtransmission/announcer-http.cc | 6 ++-- libtransmission/rpcimpl.cc | 33 ++++++++++++------- libtransmission/web.cc | 3 ++ libtransmission/web.h | 1 + libtransmission/webseed.cc | 2 +- tests/libtransmission/global-ip-cache-test.cc | 7 ++-- 7 files changed, 37 insertions(+), 24 deletions(-) diff --git a/docs/rpc-spec.md b/docs/rpc-spec.md index d30e4c190..4e083ef8c 100644 --- a/docs/rpc-spec.md +++ b/docs/rpc-spec.md @@ -664,16 +664,17 @@ Method name: `port-test` Request arguments: an optional argument `ipProtocol`. `ipProtocol` is a string specifying the IP protocol version to be used for the port test. -Set to `ipv4` to *only* check IPv4, set to `ipv6` to *only* check IPv6, -or set to `any` to check if the port is open on *any* of the IP protocol versions. -Omitting `ipProtocol` is the same as setting it to `any`. +Set to `ipv4` to check IPv4, or set to `ipv6` to check IPv6. +For backwards compatibility, it is allowed to omit this argument to get the behaviour before Transmission `4.1.0`, +which is to check whichever IP protocol the OS happened to use to connect to our port test service, +frankly not very useful. Response arguments: | Key | Value Type | Description | :-- | :-- | :-- | `port-is-open` | boolean | true if port is open, false if port is closed -| `ipProtocol` | string | copied from request argument `ipProtocol` if it was specified +| `ipProtocol` | string | `ipv4` if the test was carried out on IPv4, `ipv6` if the test was carried out on IPv6, unset if an error occured ### 4.5 Session shutdown This method tells the transmission session to shut down. diff --git a/libtransmission/announcer-http.cc b/libtransmission/announcer-http.cc index b5cfa68c4..292cbeb2f 100644 --- a/libtransmission/announcer-http.cc +++ b/libtransmission/announcer-http.cc @@ -106,7 +106,7 @@ struct http_announce_data bool handleAnnounceResponse(tr_web::FetchResponse const& web_response, tr_announce_response& response) { - auto const& [status, body, did_connect, did_timeout, vdata] = web_response; + auto const& [status, body, primary_ip, did_connect, did_timeout, vdata] = web_response; auto const& log_name = static_cast(vdata)->log_name; response.did_connect = did_connect; @@ -138,7 +138,7 @@ bool handleAnnounceResponse(tr_web::FetchResponse const& web_response, tr_announ void onAnnounceDone(tr_web::FetchResponse const& web_response) { - auto const& [status, body, did_connect, did_timeout, vdata] = web_response; + auto const& [status, body, primary_ip, did_connect, did_timeout, vdata] = web_response; auto* data = static_cast(vdata); auto const got_all_responses = ++data->requests_answered_count == data->requests_sent_count; @@ -486,7 +486,7 @@ private: void onScrapeDone(tr_web::FetchResponse const& web_response) { - auto const& [status, body, did_connect, did_timeout, vdata] = web_response; + auto const& [status, body, primary_ip, did_connect, did_timeout, vdata] = web_response; auto* const data = static_cast(vdata); auto& response = data->response(); diff --git a/libtransmission/rpcimpl.cc b/libtransmission/rpcimpl.cc index b8595acc4..3ceba77ac 100644 --- a/libtransmission/rpcimpl.cc +++ b/libtransmission/rpcimpl.cc @@ -28,6 +28,7 @@ #include "libtransmission/error.h" #include "libtransmission/file.h" #include "libtransmission/log.h" +#include "libtransmission/net.h" #include "libtransmission/peer-mgr.h" #include "libtransmission/quark.h" #include "libtransmission/rpcimpl.h" @@ -1214,8 +1215,8 @@ char const* torrentRenamePath( void onPortTested(tr_web::FetchResponse const& web_response) { - auto const& [status, body, did_connect, did_timeout, user_data] = web_response; - auto* data = static_cast(user_data); + auto const& [status, body, primary_ip, did_connect, did_timeout, user_data] = web_response; + auto* data = static_cast(user_data); if (status != 200) { @@ -1225,16 +1226,26 @@ void onPortTested(tr_web::FetchResponse const& web_response) _("Couldn't test port: {error} ({error_code})"), fmt::arg("error", tr_webGetResponseStr(status)), fmt::arg("error_code", status))); + return; } - else /* success */ + + auto const addr = tr_address::from_string(primary_ip); + if (!addr || !addr->is_valid()) { - bool const is_open = tr_strv_starts_with(body, '1'); - tr_variantDictAddBool(data->args_out, TR_KEY_port_is_open, is_open); - tr_idle_function_done(data, SuccessResult); + tr_idle_function_done(data, "Unknown error, please file a bug report to us"); + return; } + + bool const is_open = tr_strv_starts_with(body, '1'); + tr_variantDictAddBool(data->args_out, TR_KEY_port_is_open, is_open); + if (tr_variantDictFind(data->args_out, TR_KEY_ipProtocol) == nullptr) // `ipProtocol` was not specified in the request + { + tr_variantDictAddStrView(data->args_out, TR_KEY_ipProtocol, addr->is_ipv4() ? "ipv4"sv : "ipv6"sv); + } + tr_idle_function_done(data, SuccessResult); } -char const* portTest(tr_session* session, tr_variant* args_in, tr_variant* args_out, struct tr_rpc_idle_data* idle_data) +char const* portTest(tr_session* session, tr_variant* args_in, tr_variant* args_out, tr_rpc_idle_data* idle_data) { auto const port = session->advertisedPeerPort(); auto const url = fmt::format("https://portcheck.transmissionbt.com/{:d}", port.host()); @@ -1242,7 +1253,7 @@ char const* portTest(tr_session* session, tr_variant* args_in, tr_variant* args_ auto options = tr_web::FetchOptions{ url, onPortTested, idle_data }; if (std::string_view arg; tr_variantDictFindStrView(args_in, TR_KEY_ipProtocol, &arg)) { - tr_variantDictAddStrView(args_out, TR_KEY_ipProtocol, arg); + tr_variantDictAddStr(args_out, TR_KEY_ipProtocol, arg); if (arg == "ipv4"sv) { options.ip_proto = tr_web::FetchOptions::IPProtocol::V4; @@ -1251,7 +1262,7 @@ char const* portTest(tr_session* session, tr_variant* args_in, tr_variant* args_ { options.ip_proto = tr_web::FetchOptions::IPProtocol::V6; } - else if (arg != "any"sv) + else { return "invalid ip protocol string"; } @@ -1264,7 +1275,7 @@ char const* portTest(tr_session* session, tr_variant* args_in, tr_variant* args_ void onBlocklistFetched(tr_web::FetchResponse const& web_response) { - auto const& [status, body, did_connect, did_timeout, user_data] = web_response; + auto const& [status, body, primary_ip, did_connect, did_timeout, user_data] = web_response; auto* data = static_cast(user_data); auto* const session = data->session; @@ -1388,7 +1399,7 @@ struct add_torrent_idle_data void onMetadataFetched(tr_web::FetchResponse const& web_response) { - auto const& [status, body, did_connect, did_timeout, user_data] = web_response; + auto const& [status, body, primary_ip, did_connect, did_timeout, user_data] = web_response; auto* data = static_cast(user_data); tr_logAddTrace(fmt::format( diff --git a/libtransmission/web.cc b/libtransmission/web.cc index d4134761c..e600af3a2 100644 --- a/libtransmission/web.cc +++ b/libtransmission/web.cc @@ -740,12 +740,15 @@ public: auto req_bytes_sent = long{}; auto total_time = double{}; + char* primary_ip = nullptr; curl_easy_getinfo(e, CURLINFO_REQUEST_SIZE, &req_bytes_sent); curl_easy_getinfo(e, CURLINFO_TOTAL_TIME, &total_time); curl_easy_getinfo(e, CURLINFO_RESPONSE_CODE, &task->response.status); + curl_easy_getinfo(e, CURLINFO_PRIMARY_IP, &primary_ip); task->response.did_connect = task->response.status > 0 || req_bytes_sent > 0; task->response.did_timeout = task->response.status == 0 && std::chrono::duration(total_time) >= task->timeoutSecs(); + task->response.primary_ip = primary_ip; curl_multi_remove_handle(multi.get(), e); remove_task(*task); } diff --git a/libtransmission/web.h b/libtransmission/web.h index 6460e9465..c034abd1e 100644 --- a/libtransmission/web.h +++ b/libtransmission/web.h @@ -26,6 +26,7 @@ public: { long status = 0; // http server response, e.g. 200 std::string body; + std::string primary_ip; bool did_connect = false; bool did_timeout = false; void* user_data = nullptr; diff --git a/libtransmission/webseed.cc b/libtransmission/webseed.cc index aa974c9a0..bbe73426d 100644 --- a/libtransmission/webseed.cc +++ b/libtransmission/webseed.cc @@ -432,7 +432,7 @@ void on_idle(tr_webseed* webseed) void onPartialDataFetched(tr_web::FetchResponse const& web_response) { - auto const& [status, body, did_connect, did_timeout, vtask] = web_response; + auto const& [status, body, primary_ip, did_connect, did_timeout, vtask] = web_response; bool const success = status == 206; auto* const task = static_cast(vtask); diff --git a/tests/libtransmission/global-ip-cache-test.cc b/tests/libtransmission/global-ip-cache-test.cc index 3f93d51e6..deb8d2488 100644 --- a/tests/libtransmission/global-ip-cache-test.cc +++ b/tests/libtransmission/global-ip-cache-test.cc @@ -232,11 +232,8 @@ TEST_F(GlobalIPCacheTest, onResponseIPQuery) { void fetch(tr_web::FetchOptions&& options) override { - auto response = tr_web::FetchResponse{ http_code, - std::string{ AddrStr[k_] }, - true, - false, - options.done_func_user_data }; + auto response = tr_web::FetchResponse{ http_code, std::string{ AddrStr[k_] }, std::string{}, true, + false, options.done_func_user_data }; options.done_func(response); }