Use module-style parameters with monitors

The common monitor parameters are now stored as module-style
parameters. This makes the error reporting as well as the type checks for
the parameters consistent with parameters declared by the modules.

server/core/config.cc

@@ -212,7 +212,7 @@ static int config_get_release_string(char* release);
 bool config_has_duplicate_sections(const char* config, DUPLICATE_CONTEXT* context);
 int create_new_service(CONFIG_CONTEXT *obj);
 int create_new_server(CONFIG_CONTEXT *obj);
-int create_new_monitor(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj, std::set<std::string>& monitored_servers);
+int create_new_monitor(CONFIG_CONTEXT *obj, std::set<std::string>& monitored_servers);
 int create_new_listener(CONFIG_CONTEXT *obj);
 int create_new_filter(CONFIG_CONTEXT *obj);
 int configure_new_service(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj);
@@ -285,7 +285,7 @@ const MXS_MODULE_PARAM config_monitor_params[] =
     {CN_BACKEND_WRITE_TIMEOUT, MXS_MODULE_PARAM_COUNT, "2"},
     {CN_BACKEND_CONNECT_ATTEMPTS, MXS_MODULE_PARAM_COUNT, "1"},
     {CN_DISK_SPACE_THRESHOLD, MXS_MODULE_PARAM_STRING},
-    {CN_DISK_SPACE_CHECK_INTERVAL, MXS_MODULE_PARAM_COUNT},
+    {CN_DISK_SPACE_CHECK_INTERVAL, MXS_MODULE_PARAM_COUNT, "0"},
     {NULL}
 };
 
@@ -1144,7 +1144,7 @@ process_config_context(CONFIG_CONTEXT *context)
                 }
                 else if (!strcmp(type, CN_MONITOR))
                 {
-                    error_count += create_new_monitor(context, obj, monitored_servers);
+                    error_count += create_new_monitor(obj, monitored_servers);
                 }
                 else if (strcmp(type, CN_SERVER) != 0 && strcmp(type, CN_FILTER) != 0)
                 {
@@ -3287,239 +3287,74 @@ int configure_new_service(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj)
 
 /**
  * Create a new monitor
- * @param context The complete configuration context
+ *
  * @param obj               Monitor configuration context
  * @param monitored_servers Set containing the servers that are already monitored
+ *
  * @return Number of errors
  */
-int create_new_monitor(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj, std::set<std::string>& monitored_servers)
+int create_new_monitor(CONFIG_CONTEXT *obj, std::set<std::string>& monitored_servers)
 {
-    int error_count = 0;
+    bool err = false;
 
-    char *module = config_get_value(obj->parameters, CN_MODULE);
+    // TODO: Use server list parameter type for this
-    if (module)
+    for (auto&& s: mxs::strtok(config_get_string(obj->parameters, CN_SERVERS), ", \t"))
     {
-        if ((obj->element = monitor_create(obj->object, module)) == NULL)
+        SERVER* server = server_find_by_unique_name(s.c_str());
-        {
-            MXS_ERROR("Failed to create monitor '%s'.", obj->object);
-            error_count++;
-        }
-    }
-    else
-    {
-        obj->element = NULL;
-        MXS_ERROR("Monitor '%s' is missing the required 'module' parameter.", obj->object);
-        error_count++;
-    }
 
-    char *servers = config_get_value(obj->parameters, CN_SERVERS);
+        if (!server)
-
-    if (error_count == 0)
         {
-        MXS_MONITOR* monitor = (MXS_MONITOR*)obj->element;
+            MXS_ERROR("Unable to find server '%s' that is configured in "
-        const MXS_MODULE *mod = get_module(module, MODULE_MONITOR);
+                      "the monitor '%s'.", s.c_str(), obj->object);
-
+            err = true;
-        if (mod)
-        {
-            config_add_defaults(obj, mod->parameters);
-            monitor_add_parameters(monitor, obj->parameters);
         }
-        else
+        else if (monitored_servers.insert(s.c_str()).second == false)
-        {
-            error_count++;
-        }
-
-        char *interval_str = config_get_value(obj->parameters, CN_MONITOR_INTERVAL);
-        if (interval_str)
-        {
-            char *endptr;
-            long interval = strtol(interval_str, &endptr, 0);
-            /* The interval must be >0 because it is used as a divisor.
-                Perhaps a greater minimum value should be added? */
-            if (*endptr == '\0' && interval > 0)
-            {
-                monitor_set_interval(monitor, (unsigned long)interval);
-            }
-            else
-            {
-                MXS_NOTICE("Invalid '%s' parameter for monitor '%s', "
-                           "using default value of %d milliseconds.",
-                           CN_MONITOR_INTERVAL, obj->object, DEFAULT_MONITOR_INTERVAL);
-            }
-        }
-        else
-        {
-            MXS_NOTICE("Monitor '%s' is missing the '%s' parameter, "
-                       "using default value of %d milliseconds.",
-                       obj->object, CN_MONITOR_INTERVAL, DEFAULT_MONITOR_INTERVAL);
-        }
-
-        char *journal_age = config_get_value(obj->parameters, CN_JOURNAL_MAX_AGE);
-        if (journal_age)
-        {
-            char *endptr;
-            long interval = strtol(journal_age, &endptr, 0);
-
-            if (*endptr == '\0' && interval > 0)
-            {
-                monitor_set_journal_max_age(monitor, (time_t)interval);
-            }
-            else
-            {
-                error_count++;
-                MXS_NOTICE("Invalid '%s' parameter for monitor '%s'",
-                           CN_JOURNAL_MAX_AGE, obj->object);
-            }
-        }
-        else
-        {
-            MXS_NOTICE("Monitor '%s' is missing the '%s' parameter, "
-                       "using default value of %d seconds.",
-                       obj->object, CN_JOURNAL_MAX_AGE, DEFAULT_JOURNAL_MAX_AGE);
-        }
-
-        char *script_timeout = config_get_value(obj->parameters, CN_SCRIPT_TIMEOUT);
-        if (script_timeout)
-        {
-            char *endptr;
-            long interval = strtol(script_timeout, &endptr, 0);
-
-            if (*endptr == '\0' && interval > 0)
-            {
-                monitor_set_script_timeout(monitor, (uint32_t)interval);
-            }
-            else
-            {
-                error_count++;
-                MXS_NOTICE("Invalid '%s' parameter for monitor '%s'",
-                           CN_SCRIPT_TIMEOUT, obj->object);
-            }
-        }
-        else
-        {
-            MXS_NOTICE("Monitor '%s' is missing the '%s' parameter, "
-                       "using default value of %d seconds.",
-                       obj->object, CN_SCRIPT_TIMEOUT, DEFAULT_SCRIPT_TIMEOUT);
-        }
-
-        char *connect_timeout = config_get_value(obj->parameters, CN_BACKEND_CONNECT_TIMEOUT);
-        if (connect_timeout)
-        {
-            if (!monitor_set_network_timeout(monitor, MONITOR_CONNECT_TIMEOUT,
-                                          atoi(connect_timeout), CN_BACKEND_CONNECT_TIMEOUT))
-            {
-                MXS_ERROR("Failed to set '%s'", CN_BACKEND_CONNECT_TIMEOUT);
-                error_count++;
-            }
-        }
-
-        char *read_timeout = config_get_value(obj->parameters, CN_BACKEND_READ_TIMEOUT);
-        if (read_timeout)
-        {
-            if (!monitor_set_network_timeout(monitor, MONITOR_READ_TIMEOUT,
-                                          atoi(read_timeout), CN_BACKEND_READ_TIMEOUT))
-            {
-                MXS_ERROR("Failed to set '%s'", CN_BACKEND_READ_TIMEOUT);
-                error_count++;
-            }
-        }
-
-        char *write_timeout = config_get_value(obj->parameters, CN_BACKEND_WRITE_TIMEOUT);
-        if (write_timeout)
-        {
-            if (!monitor_set_network_timeout(monitor, MONITOR_WRITE_TIMEOUT,
-                                          atoi(write_timeout), CN_BACKEND_WRITE_TIMEOUT))
-            {
-                MXS_ERROR("Failed to set '%s'", CN_BACKEND_WRITE_TIMEOUT);
-                error_count++;
-            }
-        }
-
-        char *connect_attempts = config_get_value(obj->parameters, CN_BACKEND_CONNECT_ATTEMPTS);
-        if (connect_attempts)
-        {
-            if (!monitor_set_network_timeout(monitor, MONITOR_CONNECT_ATTEMPTS,
-                                          atoi(connect_attempts), CN_BACKEND_CONNECT_ATTEMPTS))
-            {
-                MXS_ERROR("Failed to set '%s'", CN_BACKEND_CONNECT_ATTEMPTS);
-                error_count++;
-            }
-        }
-
-        const char* disk_space_threshold = config_get_value(obj->parameters, CN_DISK_SPACE_THRESHOLD);
-        if (disk_space_threshold)
-        {
-            if (!monitor_set_disk_space_threshold(monitor, disk_space_threshold))
-            {
-                MXS_ERROR("Invalid value for '%s' for monitor %s: %s",
-                          CN_DISK_SPACE_THRESHOLD, monitor->name, disk_space_threshold);
-                error_count++;
-            }
-        }
-
-        const char* disk_space_check_interval =
-            config_get_value(obj->parameters, CN_DISK_SPACE_CHECK_INTERVAL);
-        if (disk_space_check_interval)
-        {
-            char* endptr;
-            long int value = strtoll(disk_space_check_interval, &endptr, 0);
-            if (*endptr == 0 && value >= 0)
-            {
-                monitor->disk_space_check_interval = value;
-            }
-            else
-            {
-                MXS_ERROR("Invalid value for '%s': %s",
-                          CN_DISK_SPACE_CHECK_INTERVAL, disk_space_check_interval);
-                ++error_count;
-            }
-        }
-
-        if (servers)
-        {
-            /* get the servers to monitor */
-            char *s, *lasts;
-            s = strtok_r(servers, ",", &lasts);
-            while (s)
-            {
-                CONFIG_CONTEXT *obj1 = context;
-                int found = 0;
-                while (obj1)
-                {
-                    if (strcmp(trim(s), obj1->object) == 0 && obj->element && obj1->element)
-                    {
-                        found = 1;
-                        if (monitored_servers.insert(obj1->object).second == false)
         {
             MXS_WARNING("Multiple monitors are monitoring server [%s]. "
-                                        "This will cause undefined behavior.",
+                        "This will cause undefined behavior.", s.c_str());
-                                        obj1->object);
-                        }
-                        monitor_add_server(monitor, (SERVER*)obj1->element);
-                    }
-                    obj1 = obj1->next;
-                }
-                if (!found)
-                {
-                    MXS_ERROR("Unable to find server '%s' that is "
-                              "configured in the monitor '%s'.", s, obj->object);
-                    error_count++;
-                }
-
-                s = strtok_r(NULL, ",", &lasts);
         }
     }
 
-        char *user = config_get_value(obj->parameters, CN_USER);
+    if (err)
-        char *passwd = config_get_password(obj->parameters);
-        if (user && passwd)
     {
-            monitor_add_user(monitor, user, passwd);
+        return 1;
     }
-        else if (user)
+
+    const char* module = config_get_string(obj->parameters, CN_MODULE);
+    ss_dassert(module);
+
+    if (const MXS_MODULE* mod = get_module(module, MODULE_MONITOR))
     {
-            MXS_ERROR("Monitor '%s' defines a username but does not define a password.",
+        config_add_defaults(obj, config_monitor_params);
-                      obj->object);
+        config_add_defaults(obj, mod->parameters);
+    }
+    else
+    {
+        MXS_ERROR("Unable to load monitor module '%s'.", module);
+        return 1;
+    }
+
+    MXS_MONITOR* monitor = monitor_create(obj->object, module, obj->parameters);
+
+    if (monitor == NULL)
+    {
+        MXS_ERROR("Failed to create monitor '%s'.", obj->object);
+        return 1;
+    }
+
+    obj->element = monitor;
+
+    int error_count = 0;
+
+    // TODO: Parse this in the configuration
+    const char* dst = config_get_value(obj->parameters, CN_DISK_SPACE_THRESHOLD);
+
+    if (dst)
+    {
+        if (!monitor_set_disk_space_threshold(monitor, dst))
+        {
+            MXS_ERROR("Invalid value for '%s' for monitor %s: %s",
+                      CN_DISK_SPACE_THRESHOLD, monitor->name, dst);
             error_count++;
         }
     }

server/core/config_runtime.cc

@@ -418,30 +418,54 @@ bool runtime_alter_server(SERVER *server, const char *key, const char *value)
     return valid;
 }
 
+static const MXS_MODULE_PARAM* get_type_parameters(const char* type)
+{
+    if (strcmp(type, CN_SERVICE) == 0)
+    {
+        return config_service_params;
+    }
+    else if (strcmp(type, CN_LISTENER) == 0)
+    {
+        return config_listener_params;
+    }
+    else if (strcmp(type, CN_MONITOR) == 0)
+    {
+        return config_monitor_params;
+    }
+    else if (strcmp(type, CN_FILTER) == 0)
+    {
+        return config_filter_params;
+    }
+
+    MXS_NOTICE("Module type with no default parameters used: %s", type);
+    ss_info_dassert(!true, "Module type with no default parameters used");
+    return NULL;
+}
+
 /**
  * @brief Add default parameters to a monitor
  *
  * @param monitor Monitor to modify
  */
-static void add_monitor_defaults(MXS_MONITOR *monitor)
+static MXS_CONFIG_PARAMETER* load_defaults(const char* name, const char* module_type,
+                                           const char* object_type)
 {
-    /** Inject the default module parameters in case we only deleted
+    MXS_CONFIG_PARAMETER* rval = NULL;
-     * a parameter */
+    CONFIG_CONTEXT ctx = {(char*)""};
-    CONFIG_CONTEXT ctx = {};
-    ctx.object = (char*)"";
-    const MXS_MODULE *mod = get_module(monitor->module_name, MODULE_MONITOR);
 
-    if (mod)
+    if (const MXS_MODULE* mod = get_module(name, module_type))
     {
+        config_add_defaults(&ctx, get_type_parameters(object_type));
         config_add_defaults(&ctx, mod->parameters);
-        monitor_add_parameters(monitor, ctx.parameters);
+        rval = ctx.parameters;
-        config_parameter_free(ctx.parameters);
     }
     else
     {
-        MXS_ERROR("Failed to load module '%s'. See previous error messages for more details.",
+        MXS_ERROR("Failed to load module '%s'. See previous error messages for "
-                  monitor->module_name);
+                  "more details.", name);
     }
+
+    return rval;
 }
 
 bool runtime_alter_monitor(MXS_MONITOR *monitor, const char *key, const char *value)
@@ -975,15 +999,18 @@ bool runtime_create_monitor(const char *name, const char *module)
         {
             MXS_DEBUG("Repurposed monitor '%s'", name);
         }
-        else if ((monitor = monitor_create(name, module)) == NULL)
+        else if (MXS_CONFIG_PARAMETER* params = load_defaults(module, MODULE_MONITOR, CN_MONITOR))
+        {
+            if ((monitor = monitor_create(name, module, params)) == NULL)
             {
                 runtime_error("Could not create monitor '%s' with module '%s'", name, module);
             }
 
+            config_parameter_free(params);
+        }
+
         if (monitor)
         {
-            add_monitor_defaults(monitor);
-
             if (monitor_serialize(monitor))
             {
                 MXS_NOTICE("Created monitor '%s'", name);

server/core/internal/monitor.h

@@ -47,7 +47,7 @@ typedef enum
     MONITOR_CONNECT_ATTEMPTS = 3
 } monitor_timeouts_t;
 
-MXS_MONITOR *monitor_create(const char *, const char *);
+MXS_MONITOR *monitor_create(const char *, const char *, MXS_CONFIG_PARAMETER* params);
 void monitor_destroy(MXS_MONITOR *);
 
 void monitor_start(MXS_MONITOR *, const MXS_CONFIG_PARAMETER*);

server/core/monitor.cc

@@ -99,8 +99,16 @@ static uint64_t all_server_bits = SERVER_RUNNING |  SERVER_MAINT | SERVER_MASTER
  * @param module        The module to load
  * @return      The newly created monitor
  */
-MXS_MONITOR* monitor_create(const char *name, const char *module)
+MXS_MONITOR* monitor_create(const char *name, const char *module, MXS_CONFIG_PARAMETER* params)
 {
+    MXS_MONITOR_API* api = (MXS_MONITOR_API*)load_module(module, MODULE_MONITOR);
+
+    if (api == NULL)
+    {
+        MXS_ERROR("Unable to load monitor module '%s'.", name);
+        return NULL;
+    }
+
     char* my_name = MXS_STRDUP(name);
     char *my_module = MXS_STRDUP(module);
     MXS_MONITOR *mon = (MXS_MONITOR *)MXS_MALLOC(sizeof(MXS_MONITOR));
@@ -113,37 +121,37 @@ MXS_MONITOR* monitor_create(const char *name, const char *module)
         return NULL;
     }
 
-    if ((mon->api = (MXS_MONITOR_API*)load_module(module, MODULE_MONITOR)) == NULL)
+    mon->api = api;
-    {
-        MXS_ERROR("Unable to load monitor module '%s'.", my_name);
-        MXS_FREE(mon);
-        MXS_FREE(my_module);
-        MXS_FREE(my_name);
-        return NULL;
-    }
     mon->active = true;
     mon->state = MONITOR_STATE_ALLOC;
     mon->name = my_name;
     mon->module_name = my_module;
-    mon->instance = NULL;
     mon->monitored_servers = NULL;
-    *mon->password = '\0';
+    mon->read_timeout = config_get_integer(params, CN_BACKEND_READ_TIMEOUT);
-    *mon->user = '\0';
+    mon->write_timeout = config_get_integer(params, CN_BACKEND_WRITE_TIMEOUT);
-    mon->read_timeout = DEFAULT_READ_TIMEOUT;
+    mon->connect_timeout = config_get_integer(params, CN_BACKEND_CONNECT_TIMEOUT);
-    mon->write_timeout = DEFAULT_WRITE_TIMEOUT;
+    mon->connect_attempts = config_get_integer(params, CN_BACKEND_CONNECT_ATTEMPTS);
-    mon->connect_timeout = DEFAULT_CONNECT_TIMEOUT;
+    mon->interval = config_get_integer(params, CN_MONITOR_INTERVAL);
-    mon->connect_attempts = DEFAULT_CONNECTION_ATTEMPTS;
+    mon->journal_max_age = config_get_integer(params, CN_JOURNAL_MAX_AGE);
-    mon->interval = DEFAULT_MONITOR_INTERVAL;
+    mon->script_timeout = config_get_integer(params, CN_SCRIPT_TIMEOUT);
-    mon->journal_max_age = DEFAULT_JOURNAL_MAX_AGE;
-    mon->script_timeout = DEFAULT_SCRIPT_TIMEOUT;
-    mon->parameters = NULL;
     mon->check_maintenance_flag = MAINTENANCE_FLAG_NOCHECK;
     mon->ticks = 0;
+    mon->parameters = NULL;
     memset(mon->journal_hash, 0, sizeof(mon->journal_hash));
     mon->disk_space_threshold = NULL;
-    mon->disk_space_check_interval = 0;
+    mon->disk_space_check_interval = config_get_integer(params, CN_DISK_SPACE_CHECK_INTERVAL);
     spinlock_init(&mon->lock);
 
+    for (auto&& s: mxs::strtok(config_get_string(params, CN_SERVERS), ", \t"))
+    {
+        monitor_add_server(mon, server_find_by_unique_name(s.c_str()));
+    }
+
+    monitor_add_user(mon, config_get_string(params, CN_USER),
+                     config_get_string(params, CN_PASSWORD));
+
+    monitor_add_parameters(mon, params);
+
     if ((mon->instance = mon->api->createInstance(mon)) == NULL)
     {
         MXS_ERROR("Unable to create monitor instance for '%s', using module '%s'.",
@@ -151,6 +159,7 @@ MXS_MONITOR* monitor_create(const char *name, const char *module)
         MXS_FREE(mon);
         MXS_FREE(my_module);
         MXS_FREE(my_name);
+        return NULL;
     }
 
     spinlock_acquire(&monLock);
@@ -338,6 +347,7 @@ monitor_stop_all()
  */
 bool monitor_add_server(MXS_MONITOR *mon, SERVER *server)
 {
+    ss_dassert(mon && server);
     bool rval = false;
 
     if (monitor_server_in_use(server))

server/core/test/test_modulecmd.cc

@@ -407,7 +407,7 @@ int test_domain_matching(const char* actual_module,
     /** Create a monitor */
     char *libdir = MXS_STRDUP_A("../../modules/monitor/mariadbmon/");
     set_libdir(libdir);
-    monitor_create(name, actual_module);
+    monitor_create(name, actual_module, NULL);
 
     const MODULECMD *cmd;