Copilot commented on code in PR #13146:
URL: https://github.com/apache/trafficserver/pull/13146#discussion_r3302724393
##########
src/api/InkAPI.cc:
##########
@@ -3299,6 +3302,347 @@ TSMgmtUpdateRegister(TSCont contp, const char
*plugin_name, const char *plugin_f
global_config_cbs->insert(reinterpret_cast<INKContInternal *>(contp),
plugin_name, plugin_file_name);
}
+////////////////////////////////////////////////////////////////////
+//
+// Config Registry - plugin config reload registration
+//
+////////////////////////////////////////////////////////////////////
+
+namespace
+{
+// Handle behind the opaque TSCfgLoadCtx. The wrapper lambda new's one per
+// handler invocation; TSCfgLoadCtx{Complete,Fail} delete it. Caches the
+// strings/nodes the getters need to return as stable pointers.
+struct PluginConfigContext {
+ ConfigContext ctx;
+ std::string filename;
+ std::string reload_token;
+ YAML::Node supplied_yaml;
+ YAML::Node reload_directives;
+ std::atomic<bool> consumed{false};
+};
+
+DbgCtl dbg_ctl_plugin_config{"config.reload"};
+
+/// Verify we're inside TSPluginInit() after a successful TSPluginRegister().
+/// Returns the active plugin name on success, nullptr on failure (after
logging).
+const char *
+validate_plugin_init(const char *api_fn)
+{
+ if (!plugin_reg_current) {
+ Error("[unknown-plugin] %s must be called from TSPluginInit()", api_fn);
+ return nullptr;
+ }
+ if (!plugin_reg_current->plugin_registered ||
plugin_reg_current->plugin_name == nullptr) {
+ Error("[%s] %s must be called after TSPluginRegister() with a non-null
plugin_name",
+ plugin_reg_current->plugin_path ? plugin_reg_current->plugin_path :
"?", api_fn);
+ return nullptr;
+ }
+ return plugin_reg_current->plugin_name;
+}
+} // anonymous namespace
+
+TSReturnCode
+TSCfgRegister(const TSCfgRegistrationInfo *info)
+{
+ const char *plugin_name = validate_plugin_init("TSCfgRegister");
+ if (!plugin_name) {
+ return TS_ERROR;
+ }
+
+ if (info == nullptr) {
+ Error("[%s] TSCfgRegister: info is null", plugin_name);
+ return TS_ERROR;
+ }
+ if (info->key.empty() || info->config_path.empty() || info->handler ==
nullptr) {
+ Error("[%s] TSCfgRegister: missing required fields
(key/config_path/handler)", plugin_name);
+ return TS_ERROR;
+ }
+
+ std::string key_str{info->key};
+ std::string config_path_str{info->config_path};
+ std::string filename_record_str{info->filename_record};
+
+ config::ConfigSource cfg_source =
+ (info->source == TS_CFG_SOURCE_FILE_AND_RPC) ?
config::ConfigSource::FileAndRpc : config::ConfigSource::FileOnly;
+
+ auto cb = info->handler;
+ auto udata = info->data;
+
+ // Lambda captures only the function pointer + opaque user data. Plugin name
+ // and key live on the Entry; the wrapper looks them up via the ctx that the
+ // framework attaches them to (set_plugin_name in ConfigRegistry).
+ config::ConfigReloadHandler wrapper = [cb, udata](ConfigContext ctx) {
+ auto *handle = new PluginConfigContext{};
+ handle->reload_token = ctx.get_reload_token();
+ handle->supplied_yaml = ctx.supplied_yaml();
+ handle->reload_directives = ctx.reload_directives();
+ auto const &key = ctx.get_description();
+ if (auto const *entry = config::ConfigRegistry::Get_Instance().find(key);
entry != nullptr) {
+ handle->filename = entry->resolve_filename();
+ }
+ handle->ctx = std::move(ctx);
+
+ Dbg(dbg_ctl_plugin_config, "Invoking plugin config handler for '%s'",
key.c_str());
+ cb(reinterpret_cast<TSCfgLoadCtx>(handle), udata);
+ // Past this point @p handle may be deleted (synchronous completion).
Deferred
+ // completion is detected upstream via ctx.is_terminal(). If the plugin
never
+ // completes, the handle leaks but core's progress timeout still unblocks.
+ };
+
+ // Duplicate-key handling lives in ConfigRegistry::do_register and emits a
+ // Warning identifying both owners; callers can probe with TSCfgIsRegistered.
+ config::ConfigRegistry::Get_Instance().register_plugin_config(key_str,
plugin_name, config_path_str, filename_record_str,
+
std::move(wrapper), cfg_source, {}, info->is_required);
+
+ Dbg(dbg_ctl_plugin_config, "[%s] TSCfgRegister: registered '%s' (file: %s)",
plugin_name, key_str.c_str(),
+ config_path_str.c_str());
+ return TS_SUCCESS;
+}
+
+bool
+TSCfgIsRegistered(std::string_view key)
+{
+ if (key.empty()) {
+ return false;
+ }
+ return config::ConfigRegistry::Get_Instance().contains(std::string{key});
+}
+
+TSReturnCode
+TSCfgAttachReloadTrigger(std::string_view key, std::string_view record_name)
+{
+ const char *plugin_name = validate_plugin_init("TSCfgAttachReloadTrigger");
+ if (!plugin_name) {
+ return TS_ERROR;
+ }
+ if (key.empty() || record_name.empty()) {
+ Error("[%s] TSCfgAttachReloadTrigger: key and record_name required",
plugin_name);
+ return TS_ERROR;
+ }
+
+ std::string key_str{key};
+ std::string record_str{record_name};
+ if (config::ConfigRegistry::Get_Instance().attach(key_str,
record_str.c_str()) != 0) {
+ Dbg(dbg_ctl_plugin_config, "[%s] TSCfgAttachReloadTrigger: key '%s' not
found", plugin_name, key_str.c_str());
+ return TS_ERROR;
+ }
+
+ Dbg(dbg_ctl_plugin_config, "[%s] TSCfgAttachReloadTrigger: attached record
'%s' to config '%s'", plugin_name, record_str.c_str(),
+ key_str.c_str());
+ return TS_SUCCESS;
+}
+
+TSReturnCode
+TSCfgAddFileDependency(const TSCfgFileDependencyInfo *info)
+{
+ const char *plugin_name = validate_plugin_init("TSCfgAddFileDependency");
+ if (!plugin_name) {
+ return TS_ERROR;
+ }
+ if (info == nullptr) {
+ Error("[%s] TSCfgAddFileDependency: info is null", plugin_name);
+ return TS_ERROR;
+ }
+ if (info->key.empty() || info->config_path.empty()) {
+ Error("[%s] TSCfgAddFileDependency: missing required fields
(key/config_path)", plugin_name);
+ return TS_ERROR;
+ }
+
+ auto ®istry = config::ConfigRegistry::Get_Instance();
+ std::string key_str = std::string{info->key};
+ std::string path_str = std::string{info->config_path};
+ std::string filename_record_str = std::string{info->filename_record};
+ std::string dep_key_str = std::string{info->dep_key};
+ bool const has_dep_key = !dep_key_str.empty();
+
+ // filename_record_str may be empty; add_file_dependency /
resolve_config_filename treat empty as "no record".
+ const char *filename_record_arg = filename_record_str.empty() ? nullptr :
filename_record_str.c_str();
+
+ int rc = has_dep_key ?
+ registry.add_file_and_node_dependency(key_str, dep_key_str,
filename_record_arg, path_str.c_str(), info->is_required) :
+ registry.add_file_dependency(key_str, filename_record_arg,
path_str.c_str(), info->is_required);
+
+ if (rc != 0) {
+ Dbg(dbg_ctl_plugin_config, "[%s] TSCfgAddFileDependency: key '%s' not
found", plugin_name, key_str.c_str());
Review Comment:
TSCfgAddFileDependency logs "key '%s' not found" whenever the registry call
returns non-zero, but add_file_dependency/add_file_and_node_dependency can also
fail for reasons other than an unknown key (e.g., dep_key collisions,
record-callback wiring failures). Please adjust the diagnostic so it doesn’t
misreport the failure cause (include key/dep_key/filename_record and avoid
claiming the key is missing unless verified).
##########
src/mgmt/config/ConfigRegistry.cc:
##########
@@ -333,40 +377,54 @@ ConfigRegistry::add_file_dependency(const std::string
&key, const char *filename
config_key = it->second.key;
}
- auto resolved = resolve_config_filename(filename_record, default_filename);
+ bool has_record = (filename_record != nullptr && filename_record[0] != '\0');
+ auto resolved = resolve_config_filename(has_record ? filename_record :
nullptr, default_filename);
- Dbg(dbg_ctl, "Adding file dependency '%s' (resolved: %s) to config '%s'",
filename_record, resolved.c_str(), key.c_str());
+ Dbg(dbg_ctl, "Adding file dependency '%s' (resolved: %s) to config '%s'",
has_record ? filename_record : "<none>",
+ resolved.c_str(), key.c_str());
// Register with FileManager for mtime-based change detection.
- // When rereadConfig() detects the file changed, it calls
RecSetSyncRequired()
- // on the filename_record, which triggers on_record_change below.
- FileManager::instance().addFile(resolved.c_str(), filename_record, false,
is_required);
+ // When filename_record is empty (e.g. plugin configs), pass the config key
+ // as the configName so process_config_update routes mtime changes back to
+ // ConfigRegistry::schedule_reload().
+ const char *config_name = has_record ? filename_record : config_key.c_str();
+ FileManager::instance().addFile(resolved.c_str(), config_name, false,
is_required);
+
+ if (has_record) {
+ return wire_record_callback(filename_record, config_key);
+ }
Review Comment:
ConfigRegistry::add_file_dependency registers the file with FileManager
before attempting wire_record_callback() when filename_record is set. If
callback wiring fails, FileManager will still watch the file even though
changes won’t trigger the registry handler. Consider wiring the record callback
first (or rolling back the FileManager registration on failure) to avoid
leaving partial state after an error.
##########
src/api/InkAPI.cc:
##########
@@ -3299,6 +3302,347 @@ TSMgmtUpdateRegister(TSCont contp, const char
*plugin_name, const char *plugin_f
global_config_cbs->insert(reinterpret_cast<INKContInternal *>(contp),
plugin_name, plugin_file_name);
}
+////////////////////////////////////////////////////////////////////
+//
+// Config Registry - plugin config reload registration
+//
+////////////////////////////////////////////////////////////////////
+
+namespace
+{
+// Handle behind the opaque TSCfgLoadCtx. The wrapper lambda new's one per
+// handler invocation; TSCfgLoadCtx{Complete,Fail} delete it. Caches the
+// strings/nodes the getters need to return as stable pointers.
+struct PluginConfigContext {
+ ConfigContext ctx;
+ std::string filename;
+ std::string reload_token;
+ YAML::Node supplied_yaml;
+ YAML::Node reload_directives;
+ std::atomic<bool> consumed{false};
+};
+
+DbgCtl dbg_ctl_plugin_config{"config.reload"};
+
+/// Verify we're inside TSPluginInit() after a successful TSPluginRegister().
+/// Returns the active plugin name on success, nullptr on failure (after
logging).
+const char *
+validate_plugin_init(const char *api_fn)
+{
+ if (!plugin_reg_current) {
+ Error("[unknown-plugin] %s must be called from TSPluginInit()", api_fn);
+ return nullptr;
+ }
+ if (!plugin_reg_current->plugin_registered ||
plugin_reg_current->plugin_name == nullptr) {
+ Error("[%s] %s must be called after TSPluginRegister() with a non-null
plugin_name",
+ plugin_reg_current->plugin_path ? plugin_reg_current->plugin_path :
"?", api_fn);
+ return nullptr;
+ }
+ return plugin_reg_current->plugin_name;
+}
+} // anonymous namespace
+
+TSReturnCode
+TSCfgRegister(const TSCfgRegistrationInfo *info)
+{
+ const char *plugin_name = validate_plugin_init("TSCfgRegister");
+ if (!plugin_name) {
+ return TS_ERROR;
+ }
+
+ if (info == nullptr) {
+ Error("[%s] TSCfgRegister: info is null", plugin_name);
+ return TS_ERROR;
+ }
+ if (info->key.empty() || info->config_path.empty() || info->handler ==
nullptr) {
+ Error("[%s] TSCfgRegister: missing required fields
(key/config_path/handler)", plugin_name);
+ return TS_ERROR;
+ }
+
+ std::string key_str{info->key};
+ std::string config_path_str{info->config_path};
+ std::string filename_record_str{info->filename_record};
+
+ config::ConfigSource cfg_source =
+ (info->source == TS_CFG_SOURCE_FILE_AND_RPC) ?
config::ConfigSource::FileAndRpc : config::ConfigSource::FileOnly;
+
+ auto cb = info->handler;
+ auto udata = info->data;
+
+ // Lambda captures only the function pointer + opaque user data. Plugin name
+ // and key live on the Entry; the wrapper looks them up via the ctx that the
+ // framework attaches them to (set_plugin_name in ConfigRegistry).
+ config::ConfigReloadHandler wrapper = [cb, udata](ConfigContext ctx) {
+ auto *handle = new PluginConfigContext{};
+ handle->reload_token = ctx.get_reload_token();
+ handle->supplied_yaml = ctx.supplied_yaml();
+ handle->reload_directives = ctx.reload_directives();
+ auto const &key = ctx.get_description();
+ if (auto const *entry = config::ConfigRegistry::Get_Instance().find(key);
entry != nullptr) {
+ handle->filename = entry->resolve_filename();
+ }
+ handle->ctx = std::move(ctx);
+
+ Dbg(dbg_ctl_plugin_config, "Invoking plugin config handler for '%s'",
key.c_str());
+ cb(reinterpret_cast<TSCfgLoadCtx>(handle), udata);
+ // Past this point @p handle may be deleted (synchronous completion).
Deferred
+ // completion is detected upstream via ctx.is_terminal(). If the plugin
never
+ // completes, the handle leaks but core's progress timeout still unblocks.
+ };
+
+ // Duplicate-key handling lives in ConfigRegistry::do_register and emits a
+ // Warning identifying both owners; callers can probe with TSCfgIsRegistered.
+ config::ConfigRegistry::Get_Instance().register_plugin_config(key_str,
plugin_name, config_path_str, filename_record_str,
+
std::move(wrapper), cfg_source, {}, info->is_required);
+
+ Dbg(dbg_ctl_plugin_config, "[%s] TSCfgRegister: registered '%s' (file: %s)",
plugin_name, key_str.c_str(),
+ config_path_str.c_str());
+ return TS_SUCCESS;
+}
+
+bool
+TSCfgIsRegistered(std::string_view key)
+{
+ if (key.empty()) {
+ return false;
+ }
+ return config::ConfigRegistry::Get_Instance().contains(std::string{key});
+}
+
+TSReturnCode
+TSCfgAttachReloadTrigger(std::string_view key, std::string_view record_name)
+{
+ const char *plugin_name = validate_plugin_init("TSCfgAttachReloadTrigger");
+ if (!plugin_name) {
+ return TS_ERROR;
+ }
+ if (key.empty() || record_name.empty()) {
+ Error("[%s] TSCfgAttachReloadTrigger: key and record_name required",
plugin_name);
+ return TS_ERROR;
+ }
+
+ std::string key_str{key};
+ std::string record_str{record_name};
+ if (config::ConfigRegistry::Get_Instance().attach(key_str,
record_str.c_str()) != 0) {
+ Dbg(dbg_ctl_plugin_config, "[%s] TSCfgAttachReloadTrigger: key '%s' not
found", plugin_name, key_str.c_str());
Review Comment:
TSCfgAttachReloadTrigger logs "key '%s' not found" for any non-zero return
from ConfigRegistry::attach(), but attach() can also fail when
RecRegisterConfigUpdateCb wiring fails for the record. Consider logging a more
accurate message (e.g., include both key and record_name, and avoid implying
the key is missing unless that was the cause).
--
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]
