refactor: in tr_announce_list, replace port_and_str with aggregated parsed url (#6323)

2025-12-24 20:35:36 +00:00 · 2023-12-01 15:48:04 -06:00
parent c853ec3358
commit 25d67dd00d
8 changed files with 113 additions and 94 deletions
--- a/libtransmission/announce-list.cc
+++ b/libtransmission/announce-list.cc
@@ -78,23 +78,31 @@ bool tr_announce_list::replace(tr_tracker_id_t id, std::string_view announce_url
    return add(announce_url_sv, tier);
 }

-bool tr_announce_list::add(std::string_view announce_url_sv, tr_tracker_tier_t tier)
+bool tr_announce_list::add(std::string_view announce_url, tr_tracker_tier_t tier)
 {
-    auto const announce = tr_urlParseTracker(announce_url_sv);
-    if (!announce || !can_add(*announce))
+    // Make sure the announce URL is usable before we intern it.
+    if (auto const announce = tr_urlParseTracker(announce_url); !announce || !can_add(*announce))
+    {
+        return false;
+    }
+
+    // Parse again with the interned string so that `parsed` fields all
+    // point to the interned addresses. This second call should never
+    // fail, but check anyway to make the linter happy.
+    auto const announce_interned = tr_interned_string{ announce_url };
+    auto const parsed = tr_urlParseTracker(announce_interned.sv());
+    if (!parsed)
    {
        return false;
    }

    auto tracker = tracker_info{};
-    tracker.announce = announce_url_sv;
-    tracker.tier = get_tier(tier, *announce);
+    tracker.announce = announce_interned;
+    tracker.announce_parsed = *parsed;
+    tracker.tier = get_tier(tier, *parsed);
    tracker.id = next_unique_id();
-    tracker.host_and_port = fmt::format("{:s}:{:d}", announce->host, announce->port);
-    tracker.sitename = announce->sitename;
-    tracker.query = announce->query;

-    if (auto const scrape_str = announce_to_scrape(announce_url_sv); scrape_str)
+    if (auto const scrape_str = announce_to_scrape(tracker.announce.sv()); scrape_str)
    {
        tracker.scrape = *scrape_str;
    }
--- a/libtransmission/announce-list.h
+++ b/libtransmission/announce-list.h
@@ -16,6 +16,7 @@
 #include "libtransmission/interned-string.h"
 #include "libtransmission/tr-macros.h"
 #include "libtransmission/variant.h"
+#include "libtransmission/web-utils.h"

 struct tr_error;
 struct tr_url_parsed_t;
@@ -27,9 +28,7 @@ public:
    {
        tr_interned_string announce;
        tr_interned_string scrape;
-        tr_interned_string host_and_port; // 'example.org:80'
-        tr_interned_string sitename; // 'example'
-        tr_interned_string query; // 'name=ferret'
+        tr_url_parsed_t announce_parsed;
        tr_tracker_tier_t tier = 0;
        tr_tracker_id_t id = 0;

@@ -102,12 +101,12 @@ public:

    void add_to_map(tr_variant::Map& setme) const;

-    bool add(std::string_view announce_url_sv)
+    bool add(std::string_view announce_url)
    {
-        return add(announce_url_sv, this->nextTier());
+        return add(announce_url, this->nextTier());
    }

-    bool add(std::string_view announce_url_sv, tr_tracker_tier_t tier);
+    bool add(std::string_view announce_url, tr_tracker_tier_t tier);
    void add(tr_announce_list const& src);
    bool remove(std::string_view announce_url);
    bool remove(tr_tracker_id_t id);
--- a/libtransmission/announcer-common.h
+++ b/libtransmission/announcer-common.h
@@ -34,8 +34,6 @@ void tr_announcerParseHttpAnnounceResponse(tr_announce_response& response, std::

 void tr_announcerParseHttpScrapeResponse(tr_scrape_response& response, std::string_view benc, std::string_view log_name);

-tr_interned_string tr_announcerGetKey(tr_url_parsed_t const& parsed);
-
 [[nodiscard]] constexpr std::string_view tr_announce_event_get_string(tr_announce_event e)
 {
    switch (e)
--- a/libtransmission/announcer-udp.cc
+++ b/libtransmission/announcer-udp.cc
@@ -43,12 +43,13 @@
 #include "libtransmission/peer-mgr.h" // for tr_pex::fromCompact4()
 #include "libtransmission/tr-assert.h"
 #include "libtransmission/tr-buffer.h"
+#include "libtransmission/tr-strbuf.h"
 #include "libtransmission/utils.h"
 #include "libtransmission/web-utils.h"

-#define logwarn(interned, msg) tr_logAddWarn(msg, (interned).sv())
-#define logdbg(interned, msg) tr_logAddDebug(msg, (interned).sv())
-#define logtrace(interned, msg) tr_logAddTrace(msg, (interned).sv())
+#define logwarn(name, msg) tr_logAddWarn(msg, name)
+#define logdbg(name, msg) tr_logAddDebug(msg, name)
+#define logtrace(name, msg) tr_logAddTrace(msg, name)

 namespace
 {
@@ -300,9 +301,13 @@ struct tau_tracker
 {
    using Mediator = tr_announcer_udp::Mediator;

-    tau_tracker(Mediator& mediator, tr_interned_string key_in, tr_interned_string host_in, tr_port port_in)
-        : key{ key_in }
-        , host{ host_in }
+    tau_tracker(
+        Mediator& mediator,
+        std::string_view const interned_authority,
+        std::string_view const interned_host,
+        tr_port const port_in)
+        : authority{ interned_authority }
+        , host{ interned_host }
        , port{ port_in }
        , mediator_{ mediator }
    {
@@ -329,13 +334,13 @@ struct tau_tracker
        {
            this->connection_id = buf.to_uint64();
            this->connection_expiration_time = tr_time() + TauConnectionTtlSecs;
-            logdbg(this->key, fmt::format("Got a new connection ID from tracker: {}", this->connection_id));
+            logdbg(log_name(), fmt::format("Got a new connection ID from tracker: {}", this->connection_id));
        }
        else if (action == TAU_ACTION_ERROR)
        {
            std::string errmsg = !std::empty(buf) ? buf.to_string() : _("Connection failed");
            this->failAll(true, false, errmsg);
-            logdbg(this->key, std::move(errmsg));
+            logdbg(log_name(), std::move(errmsg));
        }

        this->upkeep();
@@ -363,12 +368,12 @@ struct tau_tracker
        if (!addr_pending_dns_ && addr_expires_at_ <= now)
        {
            addr_.reset();
-            addr_pending_dns_ = std::async(std::launch::async, lookup, this->host, this->port, this->key);
+            addr_pending_dns_ = std::async(std::launch::async, lookup, this->log_name(), this->host, this->port);
            return;
        }

        logtrace(
-            this->key,
+            log_name(),
            fmt::format(
                "connected {} ({} {}) -- connecting_at {}",
                is_connected(now),
@@ -381,7 +386,7 @@ struct tau_tracker
        {
            this->connecting_at = now;
            this->connection_transaction_id = tau_transaction_new();
-            logtrace(this->key, fmt::format("Trying to connect. Transaction ID is {}", this->connection_transaction_id));
+            logtrace(log_name(), fmt::format("Trying to connect. Transaction ID is {}", this->connection_transaction_id));

            auto buf = PayloadBuffer{};
            buf.add_uint64(0x41727101980LL);
@@ -416,7 +421,10 @@ private:
        return connection_id != tau_connection_t{} && now < connection_expiration_time;
    }

-    [[nodiscard]] static MaybeSockaddr lookup(tr_interned_string host, tr_port port, tr_interned_string logname)
+    [[nodiscard]] static MaybeSockaddr lookup(
+        std::string_view const interned_log_name,
+        std::string_view const interned_host,
+        tr_port const port)
    {
        auto szport = std::array<char, 16>{};
        *fmt::format_to(std::data(szport), "{:d}", port.host()) = '\0';
@@ -427,13 +435,14 @@ private:
        hints.ai_socktype = SOCK_DGRAM;

        addrinfo* info = nullptr;
-        if (int const rc = getaddrinfo(host.c_str(), std::data(szport), &hints, &info); rc != 0)
+        auto const szhost = tr_pathbuf{ interned_host };
+        if (int const rc = getaddrinfo(szhost.c_str(), std::data(szport), &hints, &info); rc != 0)
        {
            logwarn(
-                logname,
+                interned_log_name,
                fmt::format(
                    _("Couldn't look up '{address}:{port}': {error} ({error_code})"),
-                    fmt::arg("address", host.sv()),
+                    fmt::arg("address", interned_host),
                    fmt::arg("port", port.host()),
                    fmt::arg("error", gai_strerror(rc)),
                    fmt::arg("error_code", static_cast<int>(rc))));
@@ -445,7 +454,7 @@ private:
        memcpy(&ss, info->ai_addr, len);
        freeaddrinfo(info);

-        logdbg(logname, "DNS lookup succeeded");
+        logdbg(interned_log_name, "DNS lookup succeeded");
        return std::make_pair(ss, len);
    }

@@ -486,7 +495,7 @@ private:
        {
            if (auto& req = *it; req.expiresAt() <= now)
            {
-                logtrace(this->key, fmt::format("timeout {} req {}", name, fmt::ptr(&req)));
+                logtrace(log_name(), fmt::format("timeout {} req {}", name, fmt::ptr(&req)));
                req.fail(false, true, "");
                it = requests.erase(it);
            }
@@ -525,7 +534,7 @@ private:
                continue;
            }

-            logdbg(this->key, fmt::format("sending req {}", fmt::ptr(&req)));
+            logdbg(log_name(), fmt::format("sending req {}", fmt::ptr(&req)));
            req.sent_at = now;
            send_request(std::data(req.payload), std::size(req.payload));

@@ -542,7 +551,7 @@ private:

    void send_request(std::byte const* payload, size_t payload_len)
    {
-        logdbg(this->key, fmt::format("sending request w/connection id {}", this->connection_id));
+        logdbg(log_name(), fmt::format("sending request w/connection id {}", this->connection_id));

        auto buf = PayloadBuffer{};
        buf.add_uint64(this->connection_id);
@@ -552,8 +561,13 @@ private:
    }

 public:
-    tr_interned_string const key;
-    tr_interned_string const host;
+    [[nodiscard]] constexpr std::string_view log_name() const noexcept
+    {
+        return authority;
+    }
+
+    std::string_view const authority; // interned
+    std::string_view const host; // interned
    tr_port const port;

    time_t connecting_at = 0;
@@ -646,7 +660,7 @@ public:
            // is it a connection response?
            if (tracker.connecting_at != 0 && transaction_id == tracker.connection_transaction_id)
            {
-                logtrace(tracker.key, fmt::format("{} is my connection request!", transaction_id));
+                logtrace(tracker.log_name(), fmt::format("{} is my connection request!", transaction_id));
                tracker.on_connection_response(action_id, buf);
                return true;
            }
@@ -660,7 +674,7 @@ public:
                        [&transaction_id](auto const& req) { return req.transaction_id == transaction_id; });
                    it != std::end(reqs))
                {
-                    logtrace(tracker.key, fmt::format("{} is an announce request!", transaction_id));
+                    logtrace(tracker.log_name(), fmt::format("{} is an announce request!", transaction_id));
                    auto req = *it;
                    it = reqs.erase(it);
                    req.onResponse(action_id, buf);
@@ -677,7 +691,7 @@ public:
                        [&transaction_id](auto const& req) { return req.transaction_id == transaction_id; });
                    it != std::end(reqs))
                {
-                    logtrace(tracker.key, fmt::format("{} is a scrape request!", transaction_id));
+                    logtrace(tracker.log_name(), fmt::format("{} is a scrape request!", transaction_id));
                    auto req = *it;
                    it = reqs.erase(it);
                    req.onResponse(action_id, buf);
@@ -698,7 +712,7 @@ public:
 private:
    // Finds the tau_tracker struct that corresponds to this url.
    // If it doesn't exist yet, create one.
-    tau_tracker* getTrackerFromUrl(tr_interned_string announce_url)
+    tau_tracker* getTrackerFromUrl(tr_interned_string const announce_url)
    {
        // build a lookup key for this tracker
        auto const parsed = tr_urlParseTracker(announce_url);
@@ -709,20 +723,19 @@ private:
        }

        // see if we already have it
-        auto const key = tr_announcerGetKey(*parsed);
+        auto const authority = parsed->authority;
        for (auto& tracker : trackers_)
        {
-            if (tracker.key == key)
+            if (tracker.authority == authority)
            {
                return &tracker;
            }
        }

        // we don't have it -- build a new one
-        trackers_.emplace_back(mediator_, key, tr_interned_string(parsed->host), tr_port::from_host(parsed->port));
-        auto* const tracker = &trackers_.back();
-        logtrace(tracker->key, "New tau_tracker created");
-        return tracker;
+        auto& tracker = trackers_.emplace_back(mediator_, authority, parsed->host, tr_port::from_host(parsed->port));
+        logtrace(tracker.log_name(), "New tau_tracker created");
+        return &tracker;
    }

    [[nodiscard]] static constexpr bool isResponseMessage(tau_action_t action, size_t msglen) noexcept
--- a/libtransmission/announcer.cc
+++ b/libtransmission/announcer.cc
@@ -240,9 +240,8 @@ std::unique_ptr<tr_announcer> tr_announcer::create(tr_session* session, tr_annou
 struct tr_tracker
 {
    explicit tr_tracker(tr_announcer_impl* announcer, tr_announce_list::tracker_info const& info)
-        : host_and_port{ info.host_and_port }
-        , announce_url{ info.announce }
-        , sitename{ info.sitename }
+        : announce_url{ info.announce }
+        , announce_parsed{ info.announce_parsed }
        , scrape_info{ std::empty(info.scrape) ? nullptr : announcer->scrape_info(info.scrape) }
        , id{ info.id }
    {
@@ -335,9 +334,8 @@ struct tr_tracker
        return false;
    }

-    tr_interned_string const host_and_port;
    tr_interned_string const announce_url;
-    std::string_view const sitename;
+    tr_url_parsed_t const announce_parsed;
    tr_scrape_info* const scrape_info;

    std::string tracker_id;
@@ -353,17 +351,6 @@ private:
    std::optional<int64_t> downloader_count_;
 };

-// format: `${host}:${port}`
-tr_interned_string tr_announcerGetKey(tr_url_parsed_t const& parsed)
-{
-    auto buf = std::array<char, 1024>{};
-    auto* const begin = std::data(buf);
-    auto const* const end = fmt::format_to_n(begin, std::size(buf), "{:s}:{:d}", parsed.host, parsed.port).out;
-    auto const sv = std::string_view{ begin, static_cast<size_t>(end - begin) };
-
-    return tr_interned_string{ sv };
-}
-
 // ---

 /** @brief A group of trackers in a single tier, as per the multitracker spec */
@@ -463,11 +450,12 @@ struct tr_tier
        return std::nullopt;
    }

-    [[nodiscard]] std::string buildLogName() const
+    [[nodiscard]] auto buildLogName() const
    {
-        auto const* const current_tracker = currentTracker();
-        auto const host_and_port_sv = current_tracker == nullptr ? "?"sv : current_tracker->host_and_port.sv();
-        return fmt::format("{:s} at {:s}", tor->name(), host_and_port_sv);
+        auto const* const tracker = currentTracker();
+        return tracker != nullptr ?
+            fmt::format("{:s} at {:s}:{:d}", tor->name(), tracker->announce_parsed.host, tracker->announce_parsed.port) :
+            fmt::format("{:s} at ?", tor->name());
    }

    [[nodiscard]] bool canManualAnnounce() const
@@ -1243,27 +1231,29 @@ namespace on_scrape_done_helpers

 void on_scrape_error(tr_session const* /*session*/, tr_tier* tier, char const* errmsg)
 {
-    // increment the error count
-    auto* current_tracker = tier->currentTracker();
-    if (current_tracker != nullptr)
+    if (auto* const current_tracker = tier->currentTracker(); current_tracker != nullptr)
    {
        ++current_tracker->consecutive_failures;
+
+        tr_logAddDebugTier(
+            tier,
+            fmt::format(
+                "Tracker '{}' scrape error: {} (Can retry in {} seconds)",
+                current_tracker->announce_parsed.authority,
+                errmsg,
+                current_tracker->getRetryInterval()));
    }

    // set the error message
    tier->last_scrape_str = errmsg != nullptr ? errmsg : "";
+    tier->lastScrapeSucceeded = false;

    // switch to the next tracker
-    current_tracker = tier->useNextTracker();
-
-    // schedule a rescrape
-    auto const interval = current_tracker->getRetryInterval();
-    auto const* const host_and_port_cstr = current_tracker->host_and_port.c_str();
-    tr_logAddDebugTier(
-        tier,
-        fmt::format("Tracker '{}' scrape error: {} (Retrying in {} seconds)", host_and_port_cstr, errmsg, interval));
-    tier->lastScrapeSucceeded = false;
-    tier->scheduleNextScrape(interval);
+    if (auto* const current_tracker = tier->useNextTracker(); current_tracker != nullptr)
+    {
+        // schedule a rescrape
+        tier->scheduleNextScrape(current_tracker->getRetryInterval());
+    }
 }

 void checkMultiscrapeMax(tr_announcer_impl* announcer, tr_scrape_response const& response)
@@ -1617,17 +1607,20 @@ namespace tracker_view_helpers
 {
 [[nodiscard]] auto trackerView(tr_torrent const& tor, size_t tier_index, tr_tier const& tier, tr_tracker const& tracker)
 {
+    auto const& announce = tracker.announce_parsed;
    auto const now = tr_time();
    auto view = tr_tracker_view{};

-    view.host_and_port = tracker.host_and_port.c_str();
+    *fmt::format_to_n(
+         std::data(view.host_and_port),
+         std::size(view.host_and_port) - 1U,
+         "{:s}:{:d}",
+         announce.host,
+         announce.port)
+         .out = '\0';
+    *fmt::format_to_n(std::data(view.sitename), std::size(view.sitename) - 1U, "{:s}", announce.sitename).out = '\0';
    view.announce = tracker.announce_url.c_str();
    view.scrape = tracker.scrape_info == nullptr ? "" : tracker.scrape_info->scrape_url.c_str();
-    *std::copy_n(
-        std::begin(tracker.sitename),
-        std::min(std::size(tracker.sitename), sizeof(view.sitename) - 1),
-        view.sitename) = '\0';
-
    view.id = tracker.id;
    view.tier = tier_index;
    view.isBackup = &tracker != tier.currentTracker();
--- a/libtransmission/rpcimpl.cc
+++ b/libtransmission/rpcimpl.cc
@@ -386,7 +386,7 @@ namespace make_torrent_field_helpers
        tracker_map.try_emplace(TR_KEY_announce, tracker.announce.sv());
        tracker_map.try_emplace(TR_KEY_id, tracker.id);
        tracker_map.try_emplace(TR_KEY_scrape, tracker.scrape.sv());
-        tracker_map.try_emplace(TR_KEY_sitename, tracker.sitename.sv());
+        tracker_map.try_emplace(TR_KEY_sitename, tracker.announce_parsed.sitename);
        tracker_map.try_emplace(TR_KEY_tier, tracker.tier);
        vec.emplace_back(std::move(tracker_map));
    }
--- a/libtransmission/transmission.h
+++ b/libtransmission/transmission.h
@@ -1245,13 +1245,13 @@ enum tr_tracker_state

 /*
 * Unlike other _view structs, it is safe to keep a tr_tracker_view copy.
- * The announce, scrape, and host strings are interned & never go out-of-scope.
+ * The announce and scrape strings are interned & never go out-of-scope.
 */
 struct tr_tracker_view
 {
    char const* announce; // full announce URL
    char const* scrape; // full scrape URL
-    char const* host_and_port; // uniquely-identifying tracker name (`${host}:${port}`)
+    char host_and_port[72]; // uniquely-identifying tracker name (`${host}:${port}`)

    // The tracker site's name. Uses the first label before the public suffix
    // (https://publicsuffix.org/) in the announce URL's host.
--- a/tests/libtransmission/announce-list-test.cc
+++ b/tests/libtransmission/announce-list-test.cc
@@ -38,7 +38,9 @@ TEST_F(AnnounceListTest, canAdd)
    EXPECT_EQ(Announce, tracker.announce.sv());
    EXPECT_EQ("https://example.org/scrape"sv, tracker.scrape.sv());
    EXPECT_EQ(Tier, tracker.tier);
-    EXPECT_EQ("example.org:443"sv, tracker.host_and_port.sv());
+    EXPECT_EQ("example.org", tracker.announce_parsed.host);
+    EXPECT_EQ("example.org"sv, tracker.announce_parsed.authority);
+    EXPECT_EQ(443, tracker.announce_parsed.port);
 }

 TEST_F(AnnounceListTest, groupsSiblingsIntoSameTier)
@@ -62,9 +64,15 @@ TEST_F(AnnounceListTest, groupsSiblingsIntoSameTier)
    EXPECT_EQ(Announce1, announce_list.at(0).announce.sv());
    EXPECT_EQ(Announce2, announce_list.at(1).announce.sv());
    EXPECT_EQ(Announce3, announce_list.at(2).announce.sv());
-    EXPECT_EQ("example.org:443"sv, announce_list.at(0).host_and_port.sv());
-    EXPECT_EQ("example.org:80"sv, announce_list.at(1).host_and_port.sv());
-    EXPECT_EQ("example.org:999"sv, announce_list.at(2).host_and_port.sv());
+    EXPECT_EQ("example.org"sv, announce_list.at(0).announce_parsed.host);
+    EXPECT_EQ("example.org"sv, announce_list.at(1).announce_parsed.host);
+    EXPECT_EQ("example.org"sv, announce_list.at(2).announce_parsed.host);
+    EXPECT_EQ(443, announce_list.at(0).announce_parsed.port);
+    EXPECT_EQ(80, announce_list.at(1).announce_parsed.port);
+    EXPECT_EQ(999, announce_list.at(2).announce_parsed.port);
+    EXPECT_EQ("example.org"sv, announce_list.at(0).announce_parsed.authority);
+    EXPECT_EQ("example.org"sv, announce_list.at(1).announce_parsed.authority);
+    EXPECT_EQ("example.org:999"sv, announce_list.at(2).announce_parsed.authority);
 }

 TEST_F(AnnounceListTest, canAddWithoutScrape)