Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3917563390
##########
src/tsutil/Metrics.cc:
##########
@@ -245,9 +252,57 @@ Metrics::Storage::rename(Metrics::IdType id,
std::string_view name)
return true;
}
+bool
+Metrics::Storage::set_listed(Metrics::IdType id, bool listed)
+{
+ if (!allocated(id)) {
+ return false;
+ }
+
+ auto [blob_ix, offset] = _splitID(id);
+ Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
+
+ // Only this bit, so a flag added later is not clobbered by unlisting or
relisting.
+ if (listed) {
+ std::get<2>(*blob)[offset].fetch_and(static_cast<uint8_t>(~UNLISTED),
MEMORY_ORDER);
+ } else {
+ std::get<2>(*blob)[offset].fetch_or(UNLISTED, MEMORY_ORDER);
+ }
+
+ return true;
+}
+
+bool
+Metrics::Storage::listed(Metrics::IdType id) const
+{
+ if (!allocated(id)) {
+ return false;
+ }
+
+ auto [blob_ix, offset] = _splitID(id);
+ Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
+
+ return (std::get<2>(*blob)[offset].load(MEMORY_ORDER) & UNLISTED) == 0;
+}
+
// Iterator implementation
+Metrics::iterator::iterator(const Metrics &m) : _metrics(m), _it(0),
_bound(m._storage->current_id())
+{
+ skip_unlisted();
+}
+
+Metrics::iterator::iterator(const Metrics &m, IdType pos) : _metrics(m),
_it(pos), _bound(m._storage->current_id())
+{
+ // Iteration never visits an unlisted slot, so an iterator must not rest on
one either: used as a
+ // range bound it would be stepped over and never reached. find() resolves
that case to end()
+ // before it gets here; this keeps the invariant true for any other
positional construction.
+ skip_unlisted();
+}
Review Comment:
The iterator bound check uses numeric comparison against `_bound` (built
with `MetricType::COUNTER` type bits). When `find()` constructs an iterator
from a GAUGE id, `_it` includes GAUGE type bits (bit 29), so `_it >= _bound` is
immediately true and the iterator behaves as exhausted. This makes
`Metrics::find()` incorrectly return `end()` for listed GAUGE metrics.
Normalize positional iterator state to always use a COUNTER-typed positional
id (as `advance()` already does) before calling `skip_unlisted()`.
--
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]
