From d0e2590eec07925abdc656cadca205738e3471ed Mon Sep 17 00:00:00 2001 From: Pavel Rochnyack Date: Sat, 13 Jul 2019 22:52:18 +0700 Subject: [PATCH] daemon: Check if plugin actually loaded before reporting configuration issues Issue: #3215 --- src/daemon/configfile.c | 161 +++++++++++++++++++++++------------------------ src/daemon/plugin.c | 6 +- src/daemon/plugin.h | 1 + src/daemon/plugin_mock.c | 2 + 4 files changed, 85 insertions(+), 85 deletions(-) diff --git a/src/daemon/configfile.c b/src/daemon/configfile.c index 1a3c4f45..3a7364ed 100644 --- a/src/daemon/configfile.c +++ b/src/daemon/configfile.c @@ -137,40 +137,58 @@ static cf_callback_t *cf_search(const char *type) { return cf_cb; } -static int cf_dispatch(const char *type, const char *orig_key, - const char *orig_value) { - cf_callback_t *cf_cb; - plugin_ctx_t old_ctx; - char *key; - char *value; - int ret; - int i = 0; +static int cf_dispatch_option(const cf_callback_t *cf_cb, oconfig_item_t *ci) { + const char *plugin = cf_cb->type; + const char *orig_key = ci->key; if (orig_key == NULL) return EINVAL; - DEBUG("type = %s, key = %s, value = %s", ESCAPE_NULL(type), orig_key, - ESCAPE_NULL(orig_value)); + /* (Re)construct string value for option */ + char buffer[4096]; + int buffer_free = sizeof(buffer); + char *buffer_ptr = buffer; - if ((cf_cb = cf_search(type)) == NULL) { - WARNING("Found a configuration for the `%s' plugin, but " - "the plugin isn't loaded or didn't register " - "a configuration callback.", - type); - return -1; + for (int i = 0; i < ci->values_num; i++) { + int status = -1; + + if (ci->values[i].type == OCONFIG_TYPE_STRING) + status = + ssnprintf(buffer_ptr, buffer_free, " %s", ci->values[i].value.string); + else if (ci->values[i].type == OCONFIG_TYPE_NUMBER) + status = ssnprintf(buffer_ptr, buffer_free, " %lf", + ci->values[i].value.number); + else if (ci->values[i].type == OCONFIG_TYPE_BOOLEAN) + status = ssnprintf(buffer_ptr, buffer_free, " %s", + ci->values[i].value.boolean ? "true" : "false"); + + if ((status < 0) || (status >= buffer_free)) + return -1; + buffer_free -= status; + buffer_ptr += status; } - if ((key = strdup(orig_key)) == NULL) + /* skip the initial space */ + const char *orig_value = buffer + 1; + + DEBUG("plugin = %s, key = %s, value = %s", ESCAPE_NULL(plugin), orig_key, + ESCAPE_NULL(orig_value)); + + char *key = strdup(orig_key); + if (key == NULL) return 1; - if ((value = strdup(orig_value)) == NULL) { + + char *value = strdup(orig_value); + if (value == NULL) { free(key); return 2; } - ret = -1; + int ret = -1; - old_ctx = plugin_set_ctx(cf_cb->ctx); + plugin_ctx_t old_ctx = plugin_set_ctx(cf_cb->ctx); + int i; for (i = 0; i < cf_cb->keys_num; i++) { if ((cf_cb->keys[i] != NULL) && (strcasecmp(cf_cb->keys[i], key) == 0)) { ret = (*cf_cb->callback)(key, value); @@ -181,13 +199,13 @@ static int cf_dispatch(const char *type, const char *orig_key, plugin_set_ctx(old_ctx); if (i >= cf_cb->keys_num) - WARNING("Plugin `%s' did not register for value `%s'.", type, key); + WARNING("Plugin `%s' did not register for value `%s'.", plugin, key); free(key); free(value); return ret; -} /* int cf_dispatch */ +} /* int cf_dispatch_option */ static int dispatch_global_option(const oconfig_item_t *ci) { if (ci->values_num != 1) { @@ -299,38 +317,6 @@ static int dispatch_loadplugin(oconfig_item_t *ci) { return ret_val; } /* int dispatch_value_loadplugin */ -static int dispatch_value_plugin(const char *plugin, oconfig_item_t *ci) { - char buffer[4096]; - char *buffer_ptr; - int buffer_free; - - buffer_ptr = buffer; - buffer_free = sizeof(buffer); - - for (int i = 0; i < ci->values_num; i++) { - int status = -1; - - if (ci->values[i].type == OCONFIG_TYPE_STRING) - status = - ssnprintf(buffer_ptr, buffer_free, " %s", ci->values[i].value.string); - else if (ci->values[i].type == OCONFIG_TYPE_NUMBER) - status = ssnprintf(buffer_ptr, buffer_free, " %lf", - ci->values[i].value.number); - else if (ci->values[i].type == OCONFIG_TYPE_BOOLEAN) - status = ssnprintf(buffer_ptr, buffer_free, " %s", - ci->values[i].value.boolean ? "true" : "false"); - - if ((status < 0) || (status >= buffer_free)) - return -1; - buffer_free -= status; - buffer_ptr += status; - } - /* skip the initial space */ - buffer_ptr = buffer + 1; - - return cf_dispatch(plugin, ci->key, buffer_ptr); -} /* int dispatch_value_plugin */ - static int dispatch_value(oconfig_item_t *ci) { int ret = 0; @@ -364,71 +350,84 @@ static int dispatch_block_plugin(oconfig_item_t *ci) { return -1; } - const char *name = ci->values[0].value.string; - if (strcmp("libvirt", name) == 0) { + const char *plugin_name = ci->values[0].value.string; + if (strcmp("libvirt", plugin_name) == 0) { /* TODO(octo): Remove this legacy. */ WARNING("The \"libvirt\" plugin has been renamed to \"virt\" to avoid " "problems with the build system. " "Your configuration is still using the old name. " "Please change it to use \"virt\" as soon as possible. " "This compatibility code will go away eventually."); - name = "virt"; + plugin_name = "virt"; } - if (IS_TRUE(global_option_get("AutoLoadPlugin"))) { + bool plugin_loaded = plugin_is_loaded(plugin_name); + + if (!plugin_loaded && IS_TRUE(global_option_get("AutoLoadPlugin"))) { plugin_ctx_t ctx = {0}; - plugin_ctx_t old_ctx; - int status; /* default to the global interval set before loading this plugin */ ctx.interval = cf_get_default_interval(); - ctx.name = strdup(name); + ctx.name = strdup(plugin_name); - old_ctx = plugin_set_ctx(ctx); - status = plugin_load(name, /* flags = */ false); + plugin_ctx_t old_ctx = plugin_set_ctx(ctx); + int status = plugin_load(plugin_name, /* flags = */ false); /* reset to the "global" context */ plugin_set_ctx(old_ctx); if (status != 0) { - ERROR("Automatically loading plugin \"%s\" failed " + ERROR("Automatically loading plugin `%s' failed " "with status %i.", - name, status); + plugin_name, status); return status; } + plugin_loaded = true; + } + + if (!plugin_loaded) { + WARNING("There is configuration for the `%s' plugin, but the plugin isn't " + "loaded. Please check your configuration.", + plugin_name); + + /* Try to be backward-compatible with previous versions */ + return 0; } /* Check for a complex callback first */ for (cf_complex_callback_t *cb = complex_callback_head; cb != NULL; cb = cb->next) { - if (strcasecmp(name, cb->type) == 0) { - plugin_ctx_t old_ctx; - int ret_val; - - old_ctx = plugin_set_ctx(cb->ctx); - ret_val = (cb->callback(ci)); + if (strcasecmp(plugin_name, cb->type) == 0) { + plugin_ctx_t old_ctx = plugin_set_ctx(cb->ctx); + int ret_val = (cb->callback(ci)); plugin_set_ctx(old_ctx); return ret_val; } } /* Hm, no complex plugin found. Dispatch the values one by one */ - for (int i = 0, ret = 0; i < ci->children_num; i++) { + cf_callback_t *cf_cb = cf_search(plugin_name); + if (cf_cb == NULL) { + WARNING("Found a configuration for the `%s' plugin, but " + "the plugin didn't register a configuration callback.", + plugin_name); + return -1; + } + + for (int i = 0; i < ci->children_num; i++) { if (ci->children[i].children == NULL) { oconfig_item_t *child = ci->children + i; - ret = dispatch_value_plugin(name, child); + int ret = cf_dispatch_option(cf_cb, child); if (ret != 0) { - ERROR("Plugin %s failed to handle option %s, return code: %i", name, - child->key, ret); + ERROR("Plugin `%s' failed to handle option `%s', return code: %i", + plugin_name, child->key, ret); return ret; } } else { WARNING("There is a `%s' block within the " - "configuration for the %s plugin. " - "The plugin either only expects " - "\"simple\" configuration statements " - "or wasn't loaded using `LoadPlugin'." - " Please check your configuration.", - ci->children[i].key, name); + "configuration for the `%s' plugin. " + "The plugin only expects \"simple\" configuration options. " + "Blocks are not supported. Please check your configuration.", + ci->children[i].key, plugin_name); } } diff --git a/src/daemon/plugin.c b/src/daemon/plugin.c index c18b2c41..1f8ab2db 100644 --- a/src/daemon/plugin.c +++ b/src/daemon/plugin.c @@ -906,15 +906,13 @@ void plugin_set_dir(const char *dir) { ERROR("plugin_set_dir: strdup(\"%s\") failed", dir); } -static bool plugin_is_loaded(char const *name) { - int status; - +bool plugin_is_loaded(char const *name) { if (plugins_loaded == NULL) plugins_loaded = c_avl_create((int (*)(const void *, const void *))strcasecmp); assert(plugins_loaded != NULL); - status = c_avl_get(plugins_loaded, name, /* ret_value = */ NULL); + int status = c_avl_get(plugins_loaded, name, /* ret_value = */ NULL); return status == 0; } diff --git a/src/daemon/plugin.h b/src/daemon/plugin.h index 6b3a030c..3c301586 100644 --- a/src/daemon/plugin.h +++ b/src/daemon/plugin.h @@ -233,6 +233,7 @@ void plugin_set_dir(const char *dir); * this case. */ int plugin_load(const char *name, bool global); +bool plugin_is_loaded(char const *name); int plugin_init_all(void); void plugin_read_all(void); diff --git a/src/daemon/plugin_mock.c b/src/daemon/plugin_mock.c index 8fe9d51e..69d5e28a 100644 --- a/src/daemon/plugin_mock.c +++ b/src/daemon/plugin_mock.c @@ -41,6 +41,8 @@ void plugin_set_dir(const char *dir) { /* nop */ int plugin_load(const char *name, bool global) { return ENOTSUP; } +bool plugin_is_loaded(const char *name) { return false; } + int plugin_register_config(const char *name, int (*callback)(const char *key, const char *val), const char **keys, int keys_num) { -- 2.11.0