From 3321a591ef1ae58b66073db78701e99bef1d574b Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Fri, 18 Jan 2019 12:37:34 +0200 Subject: [PATCH] MXS-2271 Move some monitor management functions to an internal class The class MonitorManager contains monitor-related functions that should not be called from modules. MonitorManager can access private fields and methods of the monitor. --- include/maxscale/monitor.hh | 11 +++ server/core/config.cc | 2 +- server/core/config_runtime.cc | 2 +- server/core/gateway.cc | 2 +- server/core/internal/monitor.hh | 48 +++++++++--- server/core/monitor.cc | 114 ++++++++++++++--------------- server/core/test/test_modulecmd.cc | 2 +- 7 files changed, 108 insertions(+), 73 deletions(-) diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index 32c1878dc..c7e5aa81a 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -246,6 +246,7 @@ class MXS_MONITOR { public: MXS_MONITOR(const std::string& name, const std::string& module, MXS_MONITOR_API* api); + ~MXS_MONITOR(); char* name; /**< Monitor instance name */ const std::string module_name; /**< Name of the monitor module */ @@ -288,6 +289,16 @@ public: MxsDiskSpaceThreshold* disk_space_threshold = NULL; /**< Disk space thresholds */ int64_t disk_space_check_interval = -1; /**< How often should a disk space check be made * at most. */ +private: + friend class MonitorManager; + + /** + * Configure base class. Called by monitor creation code. + * + * @param params Config parameters + * @return True on success + */ + bool configure(const MXS_CONFIG_PARAMETER* params); }; /** diff --git a/server/core/config.cc b/server/core/config.cc index f871d8ea5..1f4c3fee2 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -3832,7 +3832,7 @@ int create_new_monitor(CONFIG_CONTEXT* obj, std::set& monitored_ser return 1; } - MXS_MONITOR* monitor = monitor_create(obj->object, module, obj->parameters); + MXS_MONITOR* monitor = MonitorManager::create_monitor(obj->object, module, obj->parameters); if (monitor == NULL) { MXS_ERROR("Failed to create monitor '%s'.", obj->object); diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 625d94eaa..668717291 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -1195,7 +1195,7 @@ bool runtime_create_monitor(const char* name, const char* module) if (ok) { - if ((monitor = monitor_create(name, module, params)) == NULL) + if ((monitor = MonitorManager::create_monitor(name, module, params)) == NULL) { config_runtime_error("Could not create monitor '%s' with module '%s'", name, module); } diff --git a/server/core/gateway.cc b/server/core/gateway.cc index 1418f69e4..2bd3c6d34 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -2262,7 +2262,7 @@ int main(int argc, char** argv) monitor_stop_all(); /*< Destroy all monitors */ - monitor_destroy_all(); + MonitorManager::destroy_all_monitors(); /*< * Wait for worker threads to exit. diff --git a/server/core/internal/monitor.hh b/server/core/internal/monitor.hh index e042a4925..c90b856c9 100644 --- a/server/core/internal/monitor.hh +++ b/server/core/internal/monitor.hh @@ -79,8 +79,42 @@ static const MXS_ENUM_VALUE mxs_monitor_event_enum_values[] = std::unique_ptr monitor_get_list(); -MXS_MONITOR* monitor_create(const std::string& name, const std::string& module, MXS_CONFIG_PARAMETER* params); -void monitor_destroy(MXS_MONITOR*); +/** + * This class contains internal monitor management functions that should not be exposed in the public + * monitor class. It's a friend of MXS_MONITOR. + */ +class MonitorManager +{ +public: + + /** + * Creates a new monitor. Loads the module, calls constructor and configure, and adds monitor to the + * global list. + * + * @param name The configuration name of the monitor + * @param module The module name to load + * @return The newly created monitor, or NULL on error + */ + static MXS_MONITOR* create_monitor(const std::string& name, const std::string& module, + MXS_CONFIG_PARAMETER* params); + + /** + * @brief Destroys all monitors. At this point all monitors should + * have been stopped. + * + * @attn Must only be called in single-thread context at system shutdown. + */ + static void destroy_all_monitors(); + + /** + * Free a monitor, first stop the monitor and then remove the monitor from + * the chain of monitors and free the memory. + * + * @param mon The monitor to free + */ + static void destroy_monitor(MXS_MONITOR*); +}; + void monitor_start(MXS_MONITOR*, const MXS_CONFIG_PARAMETER*); void monitor_stop(MXS_MONITOR*); @@ -98,14 +132,6 @@ void monitor_deactivate(MXS_MONITOR* monitor); void monitor_stop_all(); void monitor_start_all(); -/** - * @brief Destroys all monitors. At this point all monitors should - * have been stopped. - * - * @attn Must only be called in single-thread context at system shutdown. - */ -void monitor_destroy_all(); - MXS_MONITOR* monitor_find(const char*); MXS_MONITOR* monitor_repurpose_destroyed(const char* name, const char* module); @@ -117,7 +143,7 @@ void monitor_list(DCB*); bool monitor_add_server(MXS_MONITOR* mon, SERVER* server); void monitor_remove_server(MXS_MONITOR* mon, SERVER* server); void monitor_add_user(MXS_MONITOR*, const char*, const char*); -void monitor_add_parameters(MXS_MONITOR* monitor, MXS_CONFIG_PARAMETER* params); +void monitor_add_parameters(MXS_MONITOR* monitor, const MXS_CONFIG_PARAMETER* params); bool monitor_remove_parameter(MXS_MONITOR* monitor, const char* key); void monitor_set_parameter(MXS_MONITOR* monitor, const char* key, const char* value); diff --git a/server/core/monitor.cc b/server/core/monitor.cc index a03451765..43c1ffb0a 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -96,15 +96,8 @@ static uint64_t server_type_bits = SERVER_MASTER | SERVER_SLAVE | SERVER_JOINED static uint64_t all_server_bits = SERVER_RUNNING | SERVER_MAINT | SERVER_MASTER | SERVER_SLAVE | SERVER_JOINED | SERVER_NDB; -/** - * Create a new monitor, load the associated module for the monitor - * and start execution on the monitor. - * - * @param name The configuration name of the monitor - * @param module The module name to load - * @return The newly created monitor, or NULL on error - */ -MXS_MONITOR* monitor_create(const string& name, const string& module, MXS_CONFIG_PARAMETER* params) +MXS_MONITOR* MonitorManager::create_monitor(const string& name, const string& module, + MXS_CONFIG_PARAMETER* params) { MXS_MONITOR_API* api = (MXS_MONITOR_API*)load_module(module.c_str(), MODULE_MONITOR); if (!api) @@ -119,43 +112,9 @@ MXS_MONITOR* monitor_create(const string& name, const string& module, MXS_CONFIG return NULL; } - mon->api = api; - mon->read_timeout = config_get_integer(params, CN_BACKEND_READ_TIMEOUT); - mon->write_timeout = config_get_integer(params, CN_BACKEND_WRITE_TIMEOUT); - mon->connect_timeout = config_get_integer(params, CN_BACKEND_CONNECT_TIMEOUT); - mon->connect_attempts = config_get_integer(params, CN_BACKEND_CONNECT_ATTEMPTS); - mon->interval = config_get_integer(params, CN_MONITOR_INTERVAL); - mon->journal_max_age = config_get_integer(params, CN_JOURNAL_MAX_AGE); - mon->script_timeout = config_get_integer(params, CN_SCRIPT_TIMEOUT); - mon->script = config_get_string(params, CN_SCRIPT); - mon->events = config_get_enum(params, CN_EVENTS, mxs_monitor_event_enum_values); - mon->disk_space_check_interval = config_get_integer(params, CN_DISK_SPACE_CHECK_INTERVAL); - - for (auto& s : mxs::strtok(config_get_string(params, CN_SERVERS), ",")) - { - fix_object_name(s); - monitor_add_server(mon, Server::find_by_unique_name(s)); - } - - monitor_add_user(mon, config_get_string(params, CN_USER), config_get_string(params, CN_PASSWORD)); - - /* The previous config values were normal types and were checked before this function - * to be correct. The following is a complicated type and needs to be checked now. */ - bool error = false; - const char* threshold_string = config_get_string(params, CN_DISK_SPACE_THRESHOLD); - if (!monitor_set_disk_space_threshold(mon, threshold_string)) - { - MXS_ERROR("Invalid value for '%s' for monitor %s: %s", - CN_DISK_SPACE_THRESHOLD, mon->name, threshold_string); - error = true; - } - + bool error = !mon->configure(params); if (!error) { - // Store module, used when the monitor is serialized - monitor_set_parameter(mon, CN_MODULE, module.c_str()); - 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'.", @@ -187,13 +146,48 @@ MXS_MONITOR::MXS_MONITOR(const string& name, const string& module, MXS_MONITOR_A memset(journal_hash, 0, sizeof(journal_hash)); } -/** - * Free a monitor, first stop the monitor and then remove the monitor from - * the chain of monitors and free the memory. - * - * @param mon The monitor to free - */ -void monitor_destroy(MXS_MONITOR* mon) +bool MXS_MONITOR::configure(const MXS_CONFIG_PARAMETER* params) +{ + read_timeout = config_get_integer(params, CN_BACKEND_READ_TIMEOUT); + write_timeout = config_get_integer(params, CN_BACKEND_WRITE_TIMEOUT); + connect_timeout = config_get_integer(params, CN_BACKEND_CONNECT_TIMEOUT); + connect_attempts = config_get_integer(params, CN_BACKEND_CONNECT_ATTEMPTS); + interval = config_get_integer(params, CN_MONITOR_INTERVAL); + journal_max_age = config_get_integer(params, CN_JOURNAL_MAX_AGE); + script_timeout = config_get_integer(params, CN_SCRIPT_TIMEOUT); + script = config_get_string(params, CN_SCRIPT); + events = config_get_enum(params, CN_EVENTS, mxs_monitor_event_enum_values); + disk_space_check_interval = config_get_integer(params, CN_DISK_SPACE_CHECK_INTERVAL); + monitor_add_user(this, config_get_string(params, CN_USER), config_get_string(params, CN_PASSWORD)); + + for (auto& s : mxs::strtok(config_get_string(params, CN_SERVERS), ",")) + { + fix_object_name(s); + monitor_add_server(this, Server::find_by_unique_name(s)); + } + + /* The previous config values were normal types and were checked by the config manager + * to be correct. The following is a complicated type and needs to be checked separately. */ + bool error = false; + const char* threshold_string = config_get_string(params, CN_DISK_SPACE_THRESHOLD); + if (!monitor_set_disk_space_threshold(this, threshold_string)) + { + MXS_ERROR("Invalid value for '%s' for monitor %s: %s", + CN_DISK_SPACE_THRESHOLD, name, threshold_string); + error = true; + } + + if (!error) + { + // Store module name into parameter storage. + monitor_set_parameter(this, CN_MODULE, module_name.c_str()); + // Add all config settings to text-mode storage. Needed for serialization. + monitor_add_parameters(this, params); + } + return !error; +} + +void MonitorManager::destroy_monitor(MXS_MONITOR* mon) { MXS_MONITOR* ptr; @@ -219,14 +213,18 @@ void monitor_destroy(MXS_MONITOR* mon) guard.unlock(); mon->api->destroyInstance(mon->instance); - delete mon->disk_space_threshold; - config_parameter_free(mon->parameters); - monitor_server_free_all(mon->monitored_servers); - MXS_FREE(mon->name); delete mon; } -void monitor_destroy_all() +MXS_MONITOR::~MXS_MONITOR() +{ + delete disk_space_threshold; + config_parameter_free(parameters); + monitor_server_free_all(monitored_servers); + MXS_FREE(name); +} + +void MonitorManager::destroy_all_monitors() { // monitor_destroy() grabs 'monLock', so it cannot be grabbed here // without additional changes. But this function should only be @@ -235,7 +233,7 @@ void monitor_destroy_all() while (allMonitors) { MXS_MONITOR* monitor = allMonitors; - monitor_destroy(monitor); + destroy_monitor(monitor); } } @@ -815,7 +813,7 @@ bool check_monitor_permissions(MXS_MONITOR* monitor, const char* query) * @param monitor Monitor * @param params Config parameters */ -void monitor_add_parameters(MXS_MONITOR* monitor, MXS_CONFIG_PARAMETER* params) +void monitor_add_parameters(MXS_MONITOR* monitor, const MXS_CONFIG_PARAMETER* params) { Guard guard(monitor->lock); while (params) diff --git a/server/core/test/test_modulecmd.cc b/server/core/test/test_modulecmd.cc index 381eba82b..79f313ae6 100644 --- a/server/core/test/test_modulecmd.cc +++ b/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, NULL); + MonitorManager::create_monitor(name, actual_module, NULL); const MODULECMD* cmd;