Copilot commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3715597646
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread
%s", id, ink_get_hrtime() - start, thread_name);
Review Comment:
If `ink_get_thread_name()` fails or does not guarantee null-termination in
all cases, the debug log may print uninitialized/garbage data. Consider
zero-initializing `thread_name` (e.g., `thread_name[0] = '\\0';`) and/or
checking the return value and falling back to a known string like
`\"<unknown>\"`.
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread
%s", id, ink_get_hrtime() - start, thread_name);
Review Comment:
The format string uses `%d` but `id` is an `unsigned int`. This is a varargs
type mismatch and can cause undefined behavior. Use `%u` (or an appropriate
`PRI*` macro) to match the unsigned type.
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread
%s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) :
Continuation(new_ProxyMutex()), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when
the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // Until the task threads are registered and spawned, ET_TASK is ET_CALL and
there is nowhere else
+ // to send this.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK) ||
eventProcessor.thread_group[ET_TASK]._count == 0) {
+ return false;
+ }
+
+ ConfigInfoDestroyer *destroyer = new ConfigInfoDestroyer(id, info);
+
+ if (eventProcessor.schedule_imm(destroyer, ET_TASK) == nullptr) {
+ // The event system is shutting down and will never run the destroyer.
+ delete destroyer;
+ return false;
+ }
+
+ return true;
}
+} // namespace
+
class ConfigInfoReleaser : public Continuation
{
Review Comment:
Closing the anonymous namespace here changes the linkage/visibility of
`ConfigInfoReleaser` (and anything that follows) from internal to external,
which can unintentionally export symbols and diverge from the prior
encapsulation. If `ConfigInfoReleaser` is intended to remain TU-local, keep it
inside the unnamed namespace (move this closing brace below it or remove the
early close).
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread
%s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) :
Continuation(new_ProxyMutex()), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when
the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // Until the task threads are registered and spawned, ET_TASK is ET_CALL and
there is nowhere else
+ // to send this.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK) ||
eventProcessor.thread_group[ET_TASK]._count == 0) {
+ return false;
+ }
Review Comment:
This reaches into `eventProcessor.thread_group[ET_TASK]._count` (a member
named like an internal field). If there’s an existing public API/helper to
query whether an event type has threads (or whether `ET_TASK` is available),
prefer that over inspecting `_count` directly to reduce coupling to
`eventProcessor` internals.
--
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]
