refactor: RPC `port-test` improvements (#6274)

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,
+Set to `ipv4` to check IPv4, or set to `ipv6` to check IPv6.
-or set to `any` to check if the port is open on *any* of the IP protocol versions.
+For backwards compatibility, it is allowed to omit this argument to get the behaviour before Transmission `4.1.0`,
-Omitting `ipProtocol` is the same as setting it to `any`.
+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.

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<http_announce_data const*>(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<http_announce_data*>(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<scrape_data*>(vdata);
 
     auto& response = data->response();

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 const& [status, body, primary_ip, did_connect, did_timeout, user_data] = web_response;
-    auto* data = static_cast<struct tr_rpc_idle_data*>(user_data);
+    auto* data = static_cast<tr_rpc_idle_data*>(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_idle_function_done(data, "Unknown error, please file a bug report to us");
-        tr_variantDictAddBool(data->args_out, TR_KEY_port_is_open, is_open);
+        return;
-        tr_idle_function_done(data, SuccessResult);
     }
+
+    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<struct tr_rpc_idle_data*>(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<struct add_torrent_idle_data*>(user_data);
 
     tr_logAddTrace(fmt::format(

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<double>(total_time) >= task->timeoutSecs();
+                    task->response.primary_ip = primary_ip;
                     curl_multi_remove_handle(multi.get(), e);
                     remove_task(*task);
                 }

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;

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<tr_webseed_task*>(vtask);

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,
+            auto response = tr_web::FetchResponse{ http_code, std::string{ AddrStr[k_] }, std::string{}, true,
-                                                   std::string{ AddrStr[k_] },
+                                                   false,     options.done_func_user_data };
-                                                   true,
-                                                   false,
-                                                   options.done_func_user_data };
             options.done_func(response);
         }