From e7abc53b702279b9bdd2fa66db92623f0b6c1377 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Thu, 7 Mar 2019 12:03:28 +0200 Subject: [PATCH] MXS-2304 Clean up configuration serialization The parameters are now written in the order they appear in the module parameter definitions. Also enabled a previously disabled part in server unit test. --- server/core/config.cc | 26 +++++++++++++++----------- server/core/filter.cc | 15 +++++++-------- server/core/internal/config.hh | 19 +++++++++++++------ server/core/monitor.cc | 30 ++++++++---------------------- server/core/server.cc | 19 ++++++++++--------- server/core/service.cc | 27 ++++++++++++++------------- server/core/test/test_server.cc | 3 --- 7 files changed, 67 insertions(+), 72 deletions(-) diff --git a/server/core/config.cc b/server/core/config.cc index c5b587b1b..be1a268f4 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -4869,24 +4869,28 @@ bool config_parse_disk_space_threshold(SERVER::DiskSpaceLimits* pDisk_space_thre return success; } -void dump_param_list(int file, - const MXS_CONFIG_PARAMETER* list, - const std::unordered_set& ignored, - const MXS_MODULE_PARAM* common_params, - const MXS_MODULE_PARAM* module_params) +std::string generate_config_string(const std::string& instance_name, const MXS_CONFIG_PARAMETER& parameters, + const MXS_MODULE_PARAM* common_param_defs, + const MXS_MODULE_PARAM* module_param_defs) { - for (const auto& p : *list) + string output = "[" + instance_name + "]\n";; + // Common params and module params are null-terminated arrays. Loop over both and print parameter + // names and values. + for (auto param_set : {common_param_defs, module_param_defs}) { - const string& name = p.first; - const string& value = p.second; - if (ignored.count(name) == 0 && !value.empty()) + for (int i = 0; param_set[i].name; i++) { - if (dprintf(file, "%s=%s\n", name.c_str(), value.c_str()) == -1) + auto param_info = param_set + i; + string param_name = param_info->name; + if (parameters.contains(param_name)) { - MXS_ERROR("Failed to serialize service value: %d, %s", errno, mxs_strerror(errno)); + // Parameter value in the container can be an empty string and still be printed. + string param_value = parameters.get_string(param_name); + output += param_name + "=" + param_value + "\n"; } } } + return output; } /** diff --git a/server/core/filter.cc b/server/core/filter.cc index 4b60ca0a9..9fb5304fe 100644 --- a/server/core/filter.cc +++ b/server/core/filter.cc @@ -514,18 +514,17 @@ static bool create_filter_config(const SFilterDef& filter, const char* filename) } Guard guard(filter->lock); - - dprintf(file, "[%s]\n", filter->name.c_str()); - dprintf(file, "%s=%s\n", CN_TYPE, CN_FILTER); - dprintf(file, "%s=%s\n", CN_MODULE, filter->module.c_str()); - const MXS_MODULE* mod = get_module(filter->module.c_str(), NULL); mxb_assert(mod); - MXS_MODULE_PARAM no_common_params = {}; - dump_param_list(file, filter->parameters, {CN_TYPE, CN_MODULE}, &no_common_params, mod->parameters); + string config_str = generate_config_string(filter->name, *filter->parameters, + config_filter_params, mod->parameters); + if (dprintf(file, "%s", config_str.c_str()) == -1) + { + MXS_ERROR("Could not write serialized configuration to file '%s': %d, %s", + filename, errno, mxs_strerror(errno)); + } close(file); - return true; } diff --git a/server/core/internal/config.hh b/server/core/internal/config.hh index b5b4f7654..4e5aa568c 100644 --- a/server/core/internal/config.hh +++ b/server/core/internal/config.hh @@ -233,12 +233,19 @@ bool get_suffixed_duration(const char* zValue, std::chrono::milliseconds* pDuration, mxs::config::DurationUnit* pUnit = nullptr); -// Dump a parameter list into a file as `key=value` pairs -void dump_param_list(int file, - const MXS_CONFIG_PARAMETER* list, - const std::unordered_set& ignored, - const MXS_MODULE_PARAM* common_params, - const MXS_MODULE_PARAM* module_params); +/** + * Generate configuration file contents out of module configuration parameters. Only parameters defined + * in the parameter definition arrays are printed. Printing is in the order the parameters are given in + * the definitions. + * + * @param instance_name The module instance name + * @param parameters Configuration parameter values + * @param common_param_defs Common module parameter definitions. These are printed first. + * @param module_param_defs Module-specific parameter definitions. + */ +std::string generate_config_string(const std::string& instance_name, const MXS_CONFIG_PARAMETER& parameters, + const MXS_MODULE_PARAM* common_param_defs, + const MXS_MODULE_PARAM* module_param_defs); /** * Check whether a parameter can be modified at runtime diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 3dc122c47..19328dd6f 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -1325,7 +1325,6 @@ Monitor* monitor_server_in_use(const SERVER* server) static bool create_monitor_config(const Monitor* monitor, const char* filename) { int file = open(filename, O_EXCL | O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); - if (file == -1) { MXS_ERROR("Failed to open file '%s' when serializing monitor '%s': %d, %s", @@ -1338,31 +1337,18 @@ static bool create_monitor_config(const Monitor* monitor, const char* filename) { Guard guard(monitor->m_lock); - dprintf(file, "[%s]\n", monitor->m_name); - dprintf(file, "%s=monitor\n", CN_TYPE); - - if (!monitor->m_servers.empty()) - { - dprintf(file, "%s=", CN_SERVERS); - for (MXS_MONITORED_SERVER* db : monitor->m_servers) - { - if (db != monitor->m_servers[0]) - { - dprintf(file, ","); - } - dprintf(file, "%s", db->server->name()); - } - dprintf(file, "\n"); - } const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), NULL); mxb_assert(mod); - dump_param_list(file, - &monitor->parameters, - {CN_TYPE, CN_SERVERS}, - config_monitor_params, - mod->parameters); + string config = generate_config_string(monitor->m_name, monitor->parameters, + config_monitor_params, mod->parameters); + + if (dprintf(file, "%s", config.c_str()) == -1) + { + MXS_ERROR("Could not write serialized configuration to file '%s': %d, %s", + filename, errno, mxs_strerror(errno)); + } } close(file); diff --git a/server/core/server.cc b/server/core/server.cc index d7de48540..79b2d9442 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -880,22 +880,23 @@ bool Server::create_server_config(const Server* server, const char* filename) } // TODO: Check for return values on all of the dprintf calls - dprintf(file, "[%s]\n", server->name()); - dprintf(file, "%s=server\n", CN_TYPE); - const MXS_MODULE* mod = get_module(server->m_settings.protocol.c_str(), MODULE_PROTOCOL); - dump_param_list(file, - ParamAdaptor(server->m_settings.all_parameters), - {CN_TYPE}, - config_server_params, - mod->parameters); + string config = generate_config_string(server->name(), + *ParamAdaptor(server->m_settings.all_parameters), + config_server_params, + mod->parameters); // Print custom parameters for (const auto& elem : server->m_settings.custom_parameters) { - dprintf(file, "%s=%s\n", elem.first.c_str(), elem.second.c_str()); + config += elem.first + "=" + elem.second + "\n"; } + if (dprintf(file, "%s", config.c_str()) == -1) + { + MXS_ERROR("Could not write serialized configuration to file '%s': %d, %s", + filename, errno, mxs_strerror(errno)); + } close(file); return true; } diff --git a/server/core/service.cc b/server/core/service.cc index 1e0f0de5a..885ca561a 100644 --- a/server/core/service.cc +++ b/server/core/service.cc @@ -1417,12 +1417,23 @@ bool Service::dump_config(const char* filename) const return false; } + const MXS_MODULE* mod = get_module(router_name(), NULL); + mxb_assert(mod); + + MXS_CONFIG_PARAMETER params_to_print = svc_config_param; + // The next text-mode parameter may not be up-to-date, print them manually. TODO: Fix + params_to_print.remove(CN_FILTERS); + params_to_print.remove(CN_SERVERS); + + string config = generate_config_string(name(), params_to_print, config_service_params, mod->parameters); + if (dprintf(file, "%s", config.c_str()) == -1) + { + MXS_ERROR("Could not write serialized configuration to file '%s': %d, %s", + filename, errno, mxs_strerror(errno)); + } /** * TODO: Check for return values on all of the dprintf calls */ - dprintf(file, "[%s]\n", name()); - dprintf(file, "%s=service\n", CN_TYPE); - if (!m_filters.empty()) { dprintf(file, "%s=", CN_FILTERS); @@ -1452,16 +1463,6 @@ bool Service::dump_config(const char* filename) const } dprintf(file, "\n"); } - - const MXS_MODULE* mod = get_module(router_name(), NULL); - mxb_assert(mod); - - dump_param_list(file, - &svc_config_param, - {CN_TYPE, CN_FILTERS, CN_SERVERS}, - config_service_params, - mod->parameters); - close(file); return true; diff --git a/server/core/test/test_server.cc b/server/core/test/test_server.cc index 0d8fe4dbe..d003c6d41 100644 --- a/server/core/test/test_server.cc +++ b/server/core/test/test_server.cc @@ -149,12 +149,9 @@ bool test_serialize() TEST(created->serialize(), "Failed to synchronize the copied server"); /** Check that they serialize to identical files */ - // TODO: Disabled for now. Enable once config parameters are kept in a well-defined order. - /* char cmd[1024]; sprintf(cmd, "diff ./%s ./%s", config_name, old_config_name); TEST(system(cmd) == 0, "The files are not identical"); - */ return true; }