bryancall commented on PR #13552: URL: https://github.com/apache/trafficserver/pull/13552#issuecomment-5328004665
Copilot posted three suppressed notes in its last review rather than inline threads, so there is nothing to reply to in place. Recording what I did with them here. Took two of them in e860bb7: - The redirect URL comment was describing a hazard the final code never has. It explained why passing `std::string` storage would be wrong, which is not visible in this diff at all, because the net change is from an unowned `ats_strdup` to a scoped one. Reworded to say what is true of the code as written: the parser needs a mutable buffer, it keeps no pointer into that buffer, and the duplicate previously leaked. - `stale_response` was managing the log filename with `strdup`/`free` while every other allocation in that file uses the ATS wrappers. Those three sites were the only bare ones, so they are now `TSstrdup`/`TSfree`. Partly took the third. The suggestion was to encode ownership in the types for `Cache`, holding `URLset` as owning smart pointers and wrapping `stripes_hash_table` with an `ats_free` deleter. I agree that is the better design, but it changes the type of a public member that two unrelated code paths iterate, which is more than a leak fix should carry, so I would rather do it separately. What I did take is the concrete risk behind it: `build_stripe_hash_table()` replaced the table without releasing the previous one, and it is called from two places. That is harmless while each `Cache` builds the table once, but the destructor added here makes that pointer owning, so the function now frees any table already installed. Verified on Fedora with GCC 16.1.1: clean build, no new warnings, 140/140 unit tests, with `xdebug`, `stale_response`, `jax_fingerprint` and `uri_signing` all compiled. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
