From 2a15a3ff1f98ab250c383c3cb9fb38628e3fdf41 Mon Sep 17 00:00:00 2001 From: Radoslaw Jablonski Date: Tue, 12 Mar 2019 09:02:06 +0000 Subject: [PATCH] virt: Fix parsing configuration options -lv_config function was re-written to use complex config callback -fixed invalid parsing that caused collectd termination even if values in config file for virt were correct(ExtraStats and RefreshInterval) -added return statement to properly finish successful parsing 'ExtraStats' option value - without that, parsing of config file will fail if 'ExtraStats' option is used in the collectd config file because return code is the one for unrecognized parameter key. -fixed parsing of RefreshInterval - previously error was thrown by mistake if value was not written between double quotes(i.e. "60" was fine but 60 was not) -previously validation for boolean option value(such as value of 'IgnoreSelected' parameter) in virt plugin was missing and bad input in config file could be unnoticed by user(in config file, boolean value is parsed from its string representation). Now validation is added and error is thrown whenever value can't be parsed directly as 'true'|'yes'|'on' or 'false'|'no'|'off'. -additionally error messages have been added in all places where parsing failed in order to notify the user about the reason of collectd initialization failure. Change-Id: I2069977395371e89f4a02eb544ffb7bda8c5c5ce Signed-off-by: Radoslaw Jablonski --- Makefile.am | 5 +- src/virt.c | 519 +++++++++++++++++++++++++++++++++--------------------------- 2 files changed, 289 insertions(+), 235 deletions(-) diff --git a/Makefile.am b/Makefile.am index 485869e8..a6a102f0 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1939,12 +1939,13 @@ virt_la_CFLAGS = $(AM_CFLAGS) \ virt_la_LDFLAGS = $(PLUGIN_LDFLAGS) virt_la_LIBADD = libignorelist.la $(BUILD_WITH_LIBVIRT_LIBS) $(BUILD_WITH_LIBXML2_LIBS) -test_plugin_virt_SOURCES = src/virt_test.c +test_plugin_virt_SOURCES = src/virt_test.c src/daemon/configfile.c \ + src/daemon/types_list.c test_plugin_virt_CPPFLAGS = $(AM_CPPFLAGS) \ $(BUILD_WITH_LIBVIRT_CPPFLAGS) $(BUILD_WITH_LIBXML2_CFLAGS) test_plugin_virt_LDFLAGS = $(PLUGIN_LDFLAGS) \ $(BUILD_WITH_LIBVIRT_LDFLAGS) $(BUILD_WITH_LIBXML2_LDFLAGS) -test_plugin_virt_LDADD = libplugin_mock.la \ +test_plugin_virt_LDADD = liboconfig.la libplugin_mock.la \ $(BUILD_WITH_LIBVIRT_LIBS) $(BUILD_WITH_LIBXML2_LIBS) check_PROGRAMS += test_plugin_virt TESTS += test_plugin_virt diff --git a/src/virt.c b/src/virt.c index 2f64b505..92c120ff 100644 --- a/src/virt.c +++ b/src/virt.c @@ -117,32 +117,6 @@ typedef struct virt_notif_thread_s { bool is_active; } virt_notif_thread_t; -static const char *config_keys[] = {"Connection", - - "RefreshInterval", - - "Domain", - "BlockDevice", - "BlockDeviceFormat", - "BlockDeviceFormatBasename", - "InterfaceDevice", - "IgnoreSelected", - - "HostnameFormat", - "HostnameMetadataNS", - "HostnameMetadataXPath", - "InterfaceFormat", - - "PluginInstanceFormat", - - "Instances", - "ExtraStats", - "PersistentNotification", - - "ReportBlockDevices", - "ReportNetworkInterfaces", - NULL}; - /* PersistentNotification is false by default */ static bool persistent_notification = false; @@ -465,7 +439,6 @@ const char *domain_reasons[][DOMAIN_STATE_REASON_MAX_SIZE] = { }; #endif /* HAVE_DOM_REASON */ -#define NR_CONFIG_KEYS ((sizeof config_keys / sizeof config_keys[0]) - 1) #define NANOSEC_IN_SEC 1e9 #define GET_STATS(_f, _name, ...) \ @@ -1076,8 +1049,18 @@ static void disk_block_stats_submit(struct lv_block_stats *bstats, sfree(dev_copy); } -static unsigned int parse_ex_stats_flags(char **exstats, int numexstats) { +/** + * Function for parsing ExtraStats configuration options. + * Result of parsing is stored under 'out_parsed_flags' pointer. + * + * Returns 0 in case of success and 1 in case of parsing error + */ +static int parse_ex_stats_flags(unsigned int *out_parsed_flags, char **exstats, + int numexstats) { unsigned int ex_stats_flags = ex_stats_none; + + assert(out_parsed_flags != NULL); + for (int i = 0; i < numexstats; i++) { for (int j = 0; ex_stats_table[j].name != NULL; j++) { if (strcasecmp(exstats[i], ex_stats_table[j].name) == 0) { @@ -1090,10 +1073,13 @@ static unsigned int parse_ex_stats_flags(char **exstats, int numexstats) { if (ex_stats_table[j + 1].name == NULL) { ERROR(PLUGIN_NAME " plugin: Unmatched ExtraStats option: %s", exstats[i]); + return 1; } } } - return ex_stats_flags; + + *out_parsed_flags = ex_stats_flags; + return 0; } static void domain_state_submit_notif(virDomainPtr dom, int state, int reason) { @@ -1166,224 +1152,276 @@ static int lv_init_ignorelists() { return 0; } -static int lv_config(const char *key, const char *value) { - if (virInitialize() != 0) - return 1; - - if (lv_init_ignorelists() != 0) - return 1; +/* Validates config option that may take multiple strings arguments. + * Returns 0 on success, -1 otherwise */ +static int check_config_multiple_string_entry(const oconfig_item_t *ci) { + if (ci->values_num < 1) { + ERROR(PLUGIN_NAME + " plugin: the '%s' option requires at least one string argument", + ci->key); + return -1; + } - if (strcasecmp(key, "Connection") == 0) { - char *tmp = strdup(value); - if (tmp == NULL) { - ERROR(PLUGIN_NAME " plugin: Connection strdup failed."); - return 1; + for (int i = 0; i < ci->values_num; ++i) { + if (ci->values[i].type != OCONFIG_TYPE_STRING) { + ERROR(PLUGIN_NAME + " plugin: one of the '%s' options is not a valid string", + ci->key); + return -1; } - sfree(conn_string); - conn_string = tmp; - return 0; } - if (strcasecmp(key, "RefreshInterval") == 0) { - char *eptr = NULL; - interval = strtol(value, &eptr, 10); - if (eptr == NULL || *eptr != '\0') - return 1; - return 0; - } + return 0; +} - if (strcasecmp(key, "Domain") == 0) { - if (ignorelist_add(il_domains, value)) - return 1; - return 0; - } - if (strcasecmp(key, "BlockDevice") == 0) { - if (ignorelist_add(il_block_devices, value)) - return 1; - return 0; +static int lv_config(oconfig_item_t *ci) { + if (lv_init_ignorelists() != 0) { + ERROR(PLUGIN_NAME " plugin: lv_init_ignorelist failed."); + return -1; } - if (strcasecmp(key, "BlockDeviceFormat") == 0) { - if (strcasecmp(value, "target") == 0) - blockdevice_format = target; - else if (strcasecmp(value, "source") == 0) - blockdevice_format = source; - else { - ERROR(PLUGIN_NAME " plugin: unknown BlockDeviceFormat: %s", value); - return -1; - } - return 0; - } - if (strcasecmp(key, "BlockDeviceFormatBasename") == 0) { - blockdevice_format_basename = IS_TRUE(value) ? true : false; - return 0; - } - if (strcasecmp(key, "InterfaceDevice") == 0) { - if (ignorelist_add(il_interface_devices, value)) - return 1; - return 0; - } + for (int i = 0; i < ci->children_num; ++i) { + oconfig_item_t *c = ci->children + i; - if (strcasecmp(key, "IgnoreSelected") == 0) { - if (IS_TRUE(value)) { - ignorelist_set_invert(il_domains, 0); - ignorelist_set_invert(il_block_devices, 0); - ignorelist_set_invert(il_interface_devices, 0); - } else { - ignorelist_set_invert(il_domains, 1); - ignorelist_set_invert(il_block_devices, 1); - ignorelist_set_invert(il_interface_devices, 1); - } - return 0; - } + if (strcasecmp(c->key, "Connection") == 0) { + if (cf_util_get_string(c, &conn_string) != 0 || conn_string == NULL) { + ERROR(PLUGIN_NAME " plugin: Could not get 'Connection' parameter"); + return -1; + } - if (strcasecmp(key, "HostnameMetadataNS") == 0) { - char *tmp = strdup(value); - if (tmp == NULL) { - ERROR(PLUGIN_NAME " plugin: HostnameMetadataNS strdup failed."); - return 1; - } - sfree(hm_ns); - hm_ns = tmp; - return 0; - } + continue; + } else if (strcasecmp(c->key, "RefreshInterval") == 0) { + if (cf_util_get_int(c, &interval) != 0) { + ERROR(PLUGIN_NAME " plugin: Could not get 'RefreshInterval' parameter"); + return -1; + } - if (strcasecmp(key, "HostnameMetadataXPath") == 0) { - char *tmp = strdup(value); - if (tmp == NULL) { - ERROR(PLUGIN_NAME " plugin: HostnameMetadataXPath strdup failed."); - return 1; - } - sfree(hm_xpath); - hm_xpath = tmp; - return 0; - } + continue; + } else if (strcasecmp(c->key, "Domain") == 0) { + char *domain_name = NULL; + if (cf_util_get_string(c, &domain_name) != 0 || domain_name == NULL) { + ERROR(PLUGIN_NAME " plugin: Could not get 'Domain' parameter"); + return -1; + } - if (strcasecmp(key, "HostnameFormat") == 0) { - char *value_copy = strdup(value); - if (value_copy == NULL) { - ERROR(PLUGIN_NAME " plugin: strdup failed."); - return -1; - } + if (ignorelist_add(il_domains, domain_name)) { + ERROR(PLUGIN_NAME " plugin: Adding '%s' to domain-ignorelist failed", + domain_name); + sfree(domain_name); + return -1; + } - char *fields[HF_MAX_FIELDS]; - int n = strsplit(value_copy, fields, HF_MAX_FIELDS); - if (n < 1) { - sfree(value_copy); - ERROR(PLUGIN_NAME " plugin: HostnameFormat: no fields"); - return -1; - } + sfree(domain_name); + continue; + } else if (strcasecmp(c->key, "BlockDevice") == 0) { + char *device_name = NULL; + if (cf_util_get_string(c, &device_name) != 0 || device_name == NULL) { + ERROR(PLUGIN_NAME " plugin: Could not get 'BlockDevice' parameter"); + return -1; + } - for (int i = 0; i < n; ++i) { - if (strcasecmp(fields[i], "hostname") == 0) - hostname_format[i] = hf_hostname; - else if (strcasecmp(fields[i], "name") == 0) - hostname_format[i] = hf_name; - else if (strcasecmp(fields[i], "uuid") == 0) - hostname_format[i] = hf_uuid; - else if (strcasecmp(fields[i], "metadata") == 0) - hostname_format[i] = hf_metadata; + if (ignorelist_add(il_block_devices, device_name) != 0) { + ERROR(PLUGIN_NAME + " plugin: Adding '%s' to block-device-ignorelist failed", + device_name); + sfree(device_name); + return -1; + } + + sfree(device_name); + continue; + } else if (strcasecmp(c->key, "BlockDeviceFormat") == 0) { + char *device_format = NULL; + if (cf_util_get_string(c, &device_format) != 0 || device_format == NULL) { + ERROR(PLUGIN_NAME + " plugin: Could not get 'BlockDeviceFormat' parameter"); + return -1; + } + + if (strcasecmp(device_format, "target") == 0) + blockdevice_format = target; + else if (strcasecmp(device_format, "source") == 0) + blockdevice_format = source; else { - ERROR(PLUGIN_NAME " plugin: unknown HostnameFormat field: %s", - fields[i]); - sfree(value_copy); + ERROR(PLUGIN_NAME " plugin: unknown BlockDeviceFormat: %s", + device_format); + sfree(device_format); return -1; } - } - sfree(value_copy); - for (int i = n; i < HF_MAX_FIELDS; ++i) - hostname_format[i] = hf_none; + sfree(device_format); + continue; + } else if (strcasecmp(c->key, "BlockDeviceFormatBasename") == 0) { + if (cf_util_get_boolean(c, &blockdevice_format_basename) != 0) { + ERROR(PLUGIN_NAME + " plugin: Could not get 'BlockDeviceFormatBasename' parameter"); + return -1; + } - return 0; - } + continue; + } else if (strcasecmp(c->key, "InterfaceDevice") == 0) { + char *interface_name = NULL; + if (cf_util_get_string(c, &interface_name) != 0 || + interface_name == NULL) { + ERROR(PLUGIN_NAME " plugin: Could not get 'InterfaceDevice' parameter"); + return -1; + } - if (strcasecmp(key, "PluginInstanceFormat") == 0) { - char *value_copy = strdup(value); - if (value_copy == NULL) { - ERROR(PLUGIN_NAME " plugin: strdup failed."); - return -1; - } + if (ignorelist_add(il_interface_devices, interface_name)) { + ERROR(PLUGIN_NAME " plugin: Adding '%s' to interface-ignorelist failed", + interface_name); + sfree(interface_name); + return -1; + } - char *fields[PLGINST_MAX_FIELDS]; - int n = strsplit(value_copy, fields, PLGINST_MAX_FIELDS); - if (n < 1) { - sfree(value_copy); - ERROR(PLUGIN_NAME " plugin: PluginInstanceFormat: no fields"); - return -1; - } + sfree(interface_name); + continue; + } else if (strcasecmp(c->key, "IgnoreSelected") == 0) { + bool ignore_selected = false; + if (cf_util_get_boolean(c, &ignore_selected) != 0) { + ERROR(PLUGIN_NAME " plugin: Could not get 'IgnoreSelected' parameter"); + return -1; + } - for (int i = 0; i < n; ++i) { - if (strcasecmp(fields[i], "none") == 0) { - plugin_instance_format[i] = plginst_none; - break; - } else if (strcasecmp(fields[i], "name") == 0) - plugin_instance_format[i] = plginst_name; - else if (strcasecmp(fields[i], "uuid") == 0) - plugin_instance_format[i] = plginst_uuid; - else if (strcasecmp(fields[i], "metadata") == 0) - plugin_instance_format[i] = plginst_metadata; - else { - ERROR(PLUGIN_NAME " plugin: unknown PluginInstanceFormat field: %s", - fields[i]); - sfree(value_copy); + if (ignore_selected) { + ignorelist_set_invert(il_domains, 0); + ignorelist_set_invert(il_block_devices, 0); + ignorelist_set_invert(il_interface_devices, 0); + } else { + ignorelist_set_invert(il_domains, 1); + ignorelist_set_invert(il_block_devices, 1); + ignorelist_set_invert(il_interface_devices, 1); + } + + continue; + } else if (strcasecmp(c->key, "HostnameMetadataNS") == 0) { + if (cf_util_get_string(c, &hm_ns) != 0) { + ERROR(PLUGIN_NAME + " plugin: Could not get 'HostnameMetadataNS' parameter"); return -1; } - } - sfree(value_copy); - for (int i = n; i < PLGINST_MAX_FIELDS; ++i) - plugin_instance_format[i] = plginst_none; + continue; + } else if (strcasecmp(c->key, "HostnameMetadataXPath") == 0) { + if (cf_util_get_string(c, &hm_xpath) != 0) { + ERROR(PLUGIN_NAME + " plugin: Could not get 'HostnameMetadataXPath' parameter"); + return -1; + } - return 0; - } + continue; + } else if (strcasecmp(c->key, "HostnameFormat") == 0) { + /* this option can take multiple strings arguments in one config line*/ + if (check_config_multiple_string_entry(c) != 0) + return -1; - if (strcasecmp(key, "InterfaceFormat") == 0) { - if (strcasecmp(value, "name") == 0) - interface_format = if_name; - else if (strcasecmp(value, "address") == 0) - interface_format = if_address; - else if (strcasecmp(value, "number") == 0) - interface_format = if_number; - else { - ERROR(PLUGIN_NAME " plugin: unknown InterfaceFormat: %s", value); - return -1; - } - return 0; - } + const int params_num = c->values_num; + for (int i = 0; i < params_num; ++i) { + const char *param_name = c->values[i].value.string; + if (strcasecmp(param_name, "hostname") == 0) + hostname_format[i] = hf_hostname; + else if (strcasecmp(param_name, "name") == 0) + hostname_format[i] = hf_name; + else if (strcasecmp(param_name, "uuid") == 0) + hostname_format[i] = hf_uuid; + else if (strcasecmp(param_name, "metadata") == 0) + hostname_format[i] = hf_metadata; + else { + ERROR(PLUGIN_NAME " plugin: unknown HostnameFormat field: %s", + param_name); + return -1; + } + } - if (strcasecmp(key, "Instances") == 0) { - char *eptr = NULL; - double val = strtod(value, &eptr); + for (int i = params_num; i < HF_MAX_FIELDS; ++i) + hostname_format[i] = hf_none; - if (*eptr != '\0') { - ERROR(PLUGIN_NAME " plugin: Invalid value for Instances = '%s'", value); - return 1; - } - if (val <= 0) { - ERROR(PLUGIN_NAME " plugin: Instances <= 0 makes no sense."); - return 1; - } - if (val > NR_INSTANCES_MAX) { - ERROR(PLUGIN_NAME " plugin: Instances=%f > NR_INSTANCES_MAX=%i" - " use a lower setting or recompile the plugin.", - val, NR_INSTANCES_MAX); - return 1; - } + continue; + } else if (strcasecmp(c->key, "PluginInstanceFormat") == 0) { + /* this option can handle list of string parameters in one line*/ + if (check_config_multiple_string_entry(c) != 0) + return -1; - nr_instances = (int)val; - DEBUG(PLUGIN_NAME " plugin: configured %i instances", nr_instances); - return 0; - } + const int params_num = c->values_num; + for (int i = 0; i < params_num; ++i) { + const char *param_name = c->values[i].value.string; + if (strcasecmp(param_name, "none") == 0) { + plugin_instance_format[i] = plginst_none; + break; + } else if (strcasecmp(param_name, "name") == 0) + plugin_instance_format[i] = plginst_name; + else if (strcasecmp(param_name, "uuid") == 0) + plugin_instance_format[i] = plginst_uuid; + else if (strcasecmp(param_name, "metadata") == 0) + plugin_instance_format[i] = plginst_metadata; + else { + ERROR(PLUGIN_NAME " plugin: unknown PluginInstanceFormat field: %s", + param_name); + + return -1; + } + } + + for (int i = params_num; i < PLGINST_MAX_FIELDS; ++i) + plugin_instance_format[i] = plginst_none; + + continue; + } else if (strcasecmp(c->key, "InterfaceFormat") == 0) { + char *format = NULL; + if (cf_util_get_string(c, &format) != 0 || format == NULL) { + ERROR(PLUGIN_NAME " plugin: could not get 'InterfaceFormat' parameter"); + return -1; + } + + if (strcasecmp(format, "name") == 0) + interface_format = if_name; + else if (strcasecmp(format, "address") == 0) + interface_format = if_address; + else if (strcasecmp(format, "number") == 0) + interface_format = if_number; + else { + ERROR(PLUGIN_NAME " plugin: unknown InterfaceFormat: %s", format); + sfree(format); + return -1; + } + + sfree(format); + continue; + } else if (strcasecmp(c->key, "Instances") == 0) { + if (cf_util_get_int(c, &nr_instances) != 0) { + ERROR(PLUGIN_NAME " plugin: could not get 'Instances' parameter"); + return -1; + } + + if (nr_instances <= 0) { + ERROR(PLUGIN_NAME " plugin: Instances <= 0 makes no sense."); + return -1; + } + if (nr_instances > NR_INSTANCES_MAX) { + ERROR(PLUGIN_NAME " plugin: Instances=%i > NR_INSTANCES_MAX=%i" + " use a lower setting or recompile the plugin.", + nr_instances, NR_INSTANCES_MAX); + return -1; + } + + DEBUG(PLUGIN_NAME " plugin: configured %i instances", nr_instances); + continue; + } else if (strcasecmp(c->key, "ExtraStats") == 0) { + char *ex_str = NULL; + + if (cf_util_get_string(c, &ex_str) != 0 || ex_str == NULL) { + ERROR(PLUGIN_NAME " plugin: could not get 'ExtraStats' parameter"); + return -1; + } - if (strcasecmp(key, "ExtraStats") == 0) { - char *localvalue = strdup(value); - if (localvalue != NULL) { char *exstats[EX_STATS_MAX_FIELDS]; - int numexstats = - strsplit(localvalue, exstats, STATIC_ARRAY_SIZE(exstats)); - extra_stats = parse_ex_stats_flags(exstats, numexstats); - sfree(localvalue); + int numexstats = strsplit(ex_str, exstats, STATIC_ARRAY_SIZE(exstats)); + int status = parse_ex_stats_flags(&extra_stats, exstats, numexstats); + sfree(ex_str); + if (status != 0) { + ERROR(PLUGIN_NAME " plugin: parsing 'ExtraStats' option failed"); + return status; + } #ifdef HAVE_JOB_STATS if ((extra_stats & ex_stats_job_stats_completed) && @@ -1391,29 +1429,44 @@ static int lv_config(const char *key, const char *value) { ERROR(PLUGIN_NAME " plugin: Invalid job stats configuration. Only one " "type of job statistics can be collected at the same " "time"); - return 1; + return -1; } #endif - } - } - if (strcasecmp(key, "PersistentNotification") == 0) { - persistent_notification = IS_TRUE(value); - return 0; - } + /* ExtraStats parsed successfully*/ + continue; + } else if (strcasecmp(c->key, "PersistentNotification") == 0) { + if (cf_util_get_boolean(c, &persistent_notification) != 0) { + ERROR(PLUGIN_NAME + " plugin: could not get 'PersistentNotification' parameter"); + return -1; + } - if (strcasecmp(key, "ReportBlockDevices") == 0) { - report_block_devices = IS_TRUE(value); - return 0; - } + continue; + } else if (strcasecmp(c->key, "ReportBlockDevices") == 0) { + if (cf_util_get_boolean(c, &report_block_devices) != 0) { + ERROR(PLUGIN_NAME + " plugin: could not get 'ReportBlockDevices' parameter"); + return -1; + } - if (strcasecmp(key, "ReportNetworkInterfaces") == 0) { - report_network_interfaces = IS_TRUE(value); - return 0; + continue; + } else if (strcasecmp(c->key, "ReportNetworkInterfaces") == 0) { + if (cf_util_get_boolean(c, &report_network_interfaces) != 0) { + ERROR(PLUGIN_NAME + " plugin: could not get 'ReportNetworkInterfaces' parameter"); + return -1; + } + + continue; + } else { + /* Unrecognised option. */ + ERROR(PLUGIN_NAME " plugin: Unrecognized option: '%s'", c->key); + return -1; + } } - /* Unrecognised option. */ - return -1; + return 0; } static int lv_connect(void) { @@ -2920,7 +2973,7 @@ static int lv_shutdown(void) { } void module_register(void) { - plugin_register_config(PLUGIN_NAME, lv_config, config_keys, NR_CONFIG_KEYS); + plugin_register_complex_config("virt", lv_config); plugin_register_init(PLUGIN_NAME, lv_init); plugin_register_shutdown(PLUGIN_NAME, lv_shutdown); } -- 2.11.0