daemon: Check if plugin actually loaded before reporting configuration issues

Issue: #3215

diff --git a/src/daemon/configfile.c b/src/daemon/configfile.c
index 1a3c4f4..3a7364e 100644 (file)
--- 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 c18b2c4..1f8ab2d 100644 (file)
--- 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 6b3a030..3c30158 100644 (file)
--- 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 8fe9d51..69d5e28 100644 (file)
--- 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) {