Cleanup LogField Registration - #13645
Conversation
Prevent bugs like 7772518 ("Fix Proxy-Protocol log field symbols", apache#13345). Every field registration duplicated an add() to global_field_list and an emplace() into field_symbol_hash, with the symbol spelled out twice. Log::register_field() does both and keys the hash off the field's own symbol, so the list and the hash cannot drift apart.
No caller ever asked for the copying behaviour, and the default of true made the ownership of an added field read as ambiguous at each call site. LogFieldList now unconditionally takes ownership.
There was a problem hiding this comment.
🟡 Changes recommended
Log::register_field() can still desynchronize the global list and symbol hash if the hash insertion fails (e.g., duplicate symbol), so the helper should be made atomic/robust before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors log field registration to use a single helper (Log::register_field) so the global list and the symbol hash are populated consistently, and simplifies LogFieldList::add() by removing the unused “copy” flag so ownership is unambiguous.
Changes:
- Centralize log field registration via
Log::register_field(LogField*)and update callers. - Remove
LogFieldList::add(LogField*, bool copy)in favor ofadd(LogField*)(always takes ownership) and update call sites.
File summaries
| File | Description |
|---|---|
| src/proxy/logging/LogFormat.cc | Updates field list additions to the new LogFieldList::add(LogField*) ownership semantics. |
| src/proxy/logging/LogField.cc | Removes copy-path in LogFieldList::add() so the list always owns/enqueues the passed pointer. |
| src/proxy/logging/Log.cc | Introduces Log::register_field() and converts field initialization to use it. |
| src/api/InkAPI.cc | Switches TS log field registration to use Log::register_field(). |
| include/proxy/logging/LogField.h | Updates LogFieldList::add() declaration to remove the copy parameter/default. |
| include/proxy/logging/Log.h | Declares the new Log::register_field() helper. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fbef2ca to
8bd4f27
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new LogFieldList::add() ownership semantics aren’t consistently documented (including stale in-file comments), which can mislead future callers and lead to incorrect memory ownership usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
The comment claimed elements are copied on insert, which stopped being true when the unused copy flag was dropped (and the flag defaulted to false before that anyway).
There was a problem hiding this comment.
🔵 Needs a closer look
Log::register_field() can fail internally (duplicate symbol) but returns no status, allowing callers (notably TSLogFieldRegister) to report success even if registration was skipped.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/proxy/logging/Log.cc:316
- Log::register_field() has an internal failure path (duplicate symbol) but returns void, so callers cannot detect that registration was skipped. In particular, TSLogFieldRegister() will still return TS_SUCCESS even if register_field() rejects the new field (e.g., due to a race between the pre-check and insertion), which can mislead plugin code.
src/proxy/logging/Log.cc:331 - This comment still refers to "without copying", but LogFieldList::add() no longer has copy semantics. Consider rewording to describe ownership directly (the list takes ownership and deletes fields on destruction) to avoid implying a still-existing copy option.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Prevent bugs like 7772518 (#13345).
Every field registration duplicated an add() to global_field_list and
an emplace() into field_symbol_hash, with the symbol spelled out twice.
Log::register_field() does both and keys the hash off the field's own
symbol, so the list and the hash cannot drift apart.
No caller ever asked for the copying behaviour, and the default of true
made the ownership of an added field read as ambiguous at each call
site. LogFieldList now unconditionally takes ownership.