From: Kamil Wiatrowski Date: Sat, 9 Jun 2018 00:25:24 +0000 (+0100) Subject: pcie_errors: address review comments X-Git-Url: https://git.octo.it/?p=collectd.git;a=commitdiff_plain;h=2eb416081f0086a0e745902259aa07f919689ee8 pcie_errors: address review comments Fix some typos. Check for errors and truncation in snprintf. Return error from pcie_plugin_config. Change _Bool to bool. Other small fixes. Change-Id: I654e9ee3bbf58232c460a09e1a25862a593d0248 Signed-off-by: Kamil Wiatrowski --- diff --git a/src/collectd.conf.pod b/src/collectd.conf.pod index 20605f56..87b92e3a 100644 --- a/src/collectd.conf.pod +++ b/src/collectd.conf.pod @@ -6274,7 +6274,7 @@ notification for every error that is set. It checks for new errors at every read The device is indicated in plugin_instance according to format "domain:bus:dev.fn". Errors are divided into categories indicated by type_instance: "correctable", and for uncorrectable errors "non_fatal" or "fatal". -Fatal errros are reported as I and all others as I. +Fatal errors are reported as I and all others as I. B @@ -6301,13 +6301,12 @@ Directory used to access device config space. It is optional and defaults to =item B B|B -If true plugin will notify errors that are set to masked in Error Mask register. -Such errors are not reported to the PCI Express Root Complex. Defaults to -B. +If true plugin will notify about errors that are set to masked in Error Mask register. +Such errors are not reported to the PCI Express Root Complex. Defaults to B. =item B B|B -If false plugin will dispatch notfication only on set/clear of error. +If false plugin will dispatch notification only on set/clear of error. The ones already reported will be ignored. Defaults to B. =back diff --git a/src/pcie_errors.c b/src/pcie_errors.c index 1f7837d2..b239a8c5 100644 --- a/src/pcie_errors.c +++ b/src/pcie_errors.c @@ -49,11 +49,10 @@ #define PCIE_ECAP_OFFSET 0x100 /* ECAP always begin at offset 0x100 */ typedef struct pcie_config_s { - _Bool use_sysfs; - _Bool notif_masked; - _Bool persistent; + bool use_sysfs; + bool notif_masked; + bool persistent; char access_dir[PATH_MAX]; - _Bool config_error; } pcie_config_t; typedef struct pcie_device_s { @@ -82,11 +81,11 @@ typedef struct pcie_error_s { } pcie_error_t; static llist_t *pcie_dev_list; -static pcie_config_t pcie_config = {.access_dir = "", .use_sysfs = 1}; +static pcie_config_t pcie_config = {.access_dir = "", .use_sysfs = true}; static pcie_fops_t pcie_fops; /* Device Error Status */ -static pcie_error_t pcie_base_errors[] = { +static const pcie_error_t pcie_base_errors[] = { {PCI_EXP_DEVSTA_CED, "Correctable Error"}, {PCI_EXP_DEVSTA_NFED, "Non-Fatal Error"}, {PCI_EXP_DEVSTA_FED, "Fatal Error"}, @@ -94,7 +93,7 @@ static pcie_error_t pcie_base_errors[] = { static const int pcie_base_errors_num = STATIC_ARRAY_SIZE(pcie_base_errors); /* Uncorrectable Error Status */ -static pcie_error_t pcie_aer_ues[] = { +static const pcie_error_t pcie_aer_ues[] = { #ifdef PCI_ERR_UNC_DLP {PCI_ERR_UNC_DLP, "Data Link Protocol"}, #endif @@ -147,7 +146,7 @@ static pcie_error_t pcie_aer_ues[] = { static const int pcie_aer_ues_num = STATIC_ARRAY_SIZE(pcie_aer_ues); /* Correctable Error Status */ -static pcie_error_t pcie_aer_ces[] = { +static const pcie_error_t pcie_aer_ces[] = { #ifdef PCI_ERR_COR_RCVR {PCI_ERR_COR_RCVR, "Receiver Error Status"}, #endif @@ -223,7 +222,13 @@ static int pcie_list_devices_proc(llist_t *dev_list) { if (dev_list == NULL) return -EINVAL; - snprintf(file_name, sizeof(file_name), "%s/devices", pcie_config.access_dir); + ret = snprintf(file_name, sizeof(file_name), "%s/devices", + pcie_config.access_dir); + if (ret < 1 || (size_t)ret >= sizeof(file_name)) { + ERROR(PCIE_ERRORS_PLUGIN ": Access dir `%s' is too long (%d)", + pcie_config.access_dir, ret); + return -EINVAL; + } fd = fopen(file_name, "r"); if (!fd) { char errbuf[PCIE_BUFF_SIZE]; @@ -234,7 +239,6 @@ static int pcie_list_devices_proc(llist_t *dev_list) { while (fgets(buf, sizeof(buf), fd)) { unsigned int slot; - uint8_t bus, dev, fn; if (sscanf(buf, "%x", &slot) != 1) { ERROR(PCIE_ERRORS_PLUGIN ": Failed to read line %u from %s", i + 1, @@ -242,9 +246,9 @@ static int pcie_list_devices_proc(llist_t *dev_list) { continue; } - bus = slot >> 8U; - dev = PCIE_DEV(slot); - fn = PCIE_FN(slot); + uint8_t bus = slot >> 8U; + uint8_t dev = PCIE_DEV(slot); + uint8_t fn = PCIE_FN(slot); ret = pcie_add_device(dev_list, 0, bus, dev, fn); if (ret) break; @@ -265,7 +269,13 @@ static int pcie_list_devices_sysfs(llist_t *dev_list) { if (dev_list == NULL) return -EINVAL; - snprintf(dir_name, sizeof(dir_name), "%s/devices", pcie_config.access_dir); + ret = snprintf(dir_name, sizeof(dir_name), "%s/devices", + pcie_config.access_dir); + if (ret < 1 || (size_t)ret >= sizeof(dir_name)) { + ERROR(PCIE_ERRORS_PLUGIN ": Access dir `%s' is too long (%d)", + pcie_config.access_dir, ret); + return -EINVAL; + } dir = opendir(dir_name); if (!dir) { char errbuf[PCIE_BUFF_SIZE]; @@ -322,8 +332,14 @@ static int pcie_open(pcie_device_t *dev, const char *name) { static int pcie_open_proc(pcie_device_t *dev) { char file_name[PCIE_NAME_LEN]; - snprintf(file_name, sizeof(file_name), "%s/%02x/%02x.%d", - pcie_config.access_dir, dev->bus, dev->device, dev->function); + int ret = + snprintf(file_name, sizeof(file_name), "%s/%02x/%02x.%d", + pcie_config.access_dir, dev->bus, dev->device, dev->function); + if (ret < 1 || (size_t)ret >= sizeof(file_name)) { + ERROR(PCIE_ERRORS_PLUGIN ": Access dir `%s' is too long (%d)", + pcie_config.access_dir, ret); + return -EINVAL; + } return pcie_open(dev, file_name); } @@ -331,9 +347,15 @@ static int pcie_open_proc(pcie_device_t *dev) { static int pcie_open_sysfs(pcie_device_t *dev) { char file_name[PCIE_NAME_LEN]; - snprintf(file_name, sizeof(file_name), "%s/devices/%04x:%02x:%02x.%d/config", - pcie_config.access_dir, dev->domain, dev->bus, dev->device, - dev->function); + int ret = + snprintf(file_name, sizeof(file_name), + "%s/devices/%04x:%02x:%02x.%d/config", pcie_config.access_dir, + dev->domain, dev->bus, dev->device, dev->function); + if (ret < 1 || (size_t)ret >= sizeof(file_name)) { + ERROR(PCIE_ERRORS_PLUGIN ": Access dir `%s' is too long (%d)", + pcie_config.access_dir, ret); + return -EINVAL; + } return pcie_open(dev, file_name); } @@ -393,7 +415,7 @@ static void pcie_dispatch_notification(pcie_device_t *dev, notification_t *n, static void pcie_dispatch_correctable_errors(pcie_device_t *dev, uint32_t errors, uint32_t masked) { for (int i = 0; i < pcie_aer_ces_num; i++) { - pcie_error_t *err = pcie_aer_ces + i; + const pcie_error_t *err = pcie_aer_ces + i; notification_t n = {.severity = NOTIF_WARNING, .time = cdtime(), .plugin = PCIE_ERRORS_PLUGIN, @@ -431,7 +453,7 @@ static void pcie_dispatch_uncorrectable_errors(pcie_device_t *dev, uint32_t errors, uint32_t masked, uint32_t severity) { for (int i = 0; i < pcie_aer_ues_num; i++) { - pcie_error_t *err = pcie_aer_ues + i; + const pcie_error_t *err = pcie_aer_ues + i; const char *type_instance = (severity & err->mask) ? PCIE_SEV_FATAL : PCIE_SEV_NOFATAL; notification_t n = { @@ -533,13 +555,13 @@ static void pcie_check_dev_status(pcie_device_t *dev, int pos) { /* Report errors found in Device Status register */ for (int i = 0; i < pcie_base_errors_num; i++) { - pcie_error_t *err = pcie_base_errors + i; + const pcie_error_t *err = pcie_base_errors + i; const char *type_instance = (err->mask == PCI_EXP_DEVSTA_FED) ? PCIE_SEV_FATAL : (err->mask == PCI_EXP_DEVSTA_CED) ? PCIE_SEV_CE : PCIE_SEV_NOFATAL; - const int severity = + int severity = (err->mask == PCI_EXP_DEVSTA_FED) ? NOTIF_FAILURE : NOTIF_WARNING; notification_t n = {.severity = severity, .time = cdtime(), @@ -629,7 +651,7 @@ static void pcie_preprocess_devices(llist_t *devs) { for (llentry_t *e = llist_head(devs); e != NULL; e = e_next) { pcie_device_t *dev = e->value; - _Bool del = 0; + bool del = false; if (pcie_fops.open(dev) == 0) { uint16_t status = pcie_read16(dev, PCI_STATUS); @@ -640,7 +662,7 @@ static void pcie_preprocess_devices(llist_t *devs) { if (dev->cap_exp == -1) { DEBUG(PCIE_ERRORS_PLUGIN ": Not PCI Express device: %04x:%02x:%02x.%d", dev->domain, dev->bus, dev->device, dev->function); - del = 1; + del = true; } else { dev->ecap_aer = pcie_find_ecap_aer(dev); if (dev->ecap_aer == -1) @@ -653,7 +675,7 @@ static void pcie_preprocess_devices(llist_t *devs) { } else { ERROR(PCIE_ERRORS_PLUGIN ": %04x:%02x:%02x.%d: failed to open", dev->domain, dev->bus, dev->device, dev->function); - del = 1; + del = true; } e_next = e->next; @@ -697,17 +719,17 @@ static void pcie_access_config(void) { } static int pcie_plugin_config(oconfig_item_t *ci) { + int status = 0; for (int i = 0; i < ci->children_num; i++) { oconfig_item_t *child = ci->children + i; - int status = 0; if (strcasecmp("Source", child->key) == 0) { if ((child->values_num != 1) || (child->values[0].type != OCONFIG_TYPE_STRING)) { status = -1; } else if (strcasecmp("proc", child->values[0].value.string) == 0) { - pcie_config.use_sysfs = 0; + pcie_config.use_sysfs = false; } else if (strcasecmp("sysfs", child->values[0].value.string) != 0) { ERROR(PCIE_ERRORS_PLUGIN ": Allowed sources are 'proc' or 'sysfs'."); status = -1; @@ -722,19 +744,18 @@ static int pcie_plugin_config(oconfig_item_t *ci) { } else { ERROR(PCIE_ERRORS_PLUGIN ": Invalid configuration option \"%s\".", child->key); - pcie_config.config_error = 1; + status = -1; break; } if (status) { ERROR(PCIE_ERRORS_PLUGIN ": Invalid configuration parameter \"%s\".", child->key); - pcie_config.config_error = 1; break; } } - return 0; + return status; } static int pcie_shutdown(void) { @@ -745,11 +766,6 @@ static int pcie_shutdown(void) { } static int pcie_init(void) { - if (pcie_config.config_error) { - ERROR(PCIE_ERRORS_PLUGIN - ": Error in configuration, failed to init plugin."); - return -1; - } pcie_access_config(); pcie_dev_list = llist_create(); diff --git a/src/pcie_errors_test.c b/src/pcie_errors_test.c index 48b01b14..5cb95fa4 100644 --- a/src/pcie_errors_test.c +++ b/src/pcie_errors_test.c @@ -25,6 +25,8 @@ * Kamil Wiatrowski **/ +#define plugin_dispatch_notification plugin_dispatch_notification_pcie_test + #include "pcie_errors.c" /* sic */ #include "testing.h" @@ -40,7 +42,7 @@ static notification_t last_notif; static char g_buff[G_BUFF_LEN]; /* mock functions */ -int plugin_dispatch_notification(const notification_t *notif) { +int plugin_dispatch_notification_pcie_test(const notification_t *notif) { last_notif = *notif; return ENOTSUP; } @@ -70,7 +72,7 @@ DEF_TEST(clear_dev_list) { llist_append(test_list, entry); for (llentry_t *e = llist_head(test_list); e != NULL; e = e->next) { - EXPECT_EQ_UINT64(dev, e->value); + EXPECT_EQ_PTR(dev, e->value); } pcie_clear_list(test_list); @@ -169,10 +171,10 @@ DEF_TEST(dispatch_notification) { DEF_TEST(access_config) { pcie_config.use_sysfs = 0; pcie_access_config(); - EXPECT_EQ_UINT64(pcie_list_devices_proc, pcie_fops.list_devices); - EXPECT_EQ_UINT64(pcie_open_proc, pcie_fops.open); - EXPECT_EQ_UINT64(pcie_close, pcie_fops.close); - EXPECT_EQ_UINT64(pcie_read, pcie_fops.read); + EXPECT_EQ_PTR(pcie_list_devices_proc, pcie_fops.list_devices); + EXPECT_EQ_PTR(pcie_open_proc, pcie_fops.open); + EXPECT_EQ_PTR(pcie_close, pcie_fops.close); + EXPECT_EQ_PTR(pcie_read, pcie_fops.read); EXPECT_EQ_STR(PCIE_DEFAULT_PROCDIR, pcie_config.access_dir); sstrncpy(pcie_config.access_dir, "Test", sizeof(pcie_config.access_dir)); @@ -181,10 +183,10 @@ DEF_TEST(access_config) { pcie_config.use_sysfs = 1; pcie_access_config(); - EXPECT_EQ_UINT64(pcie_list_devices_sysfs, pcie_fops.list_devices); - EXPECT_EQ_UINT64(pcie_open_sysfs, pcie_fops.open); - EXPECT_EQ_UINT64(pcie_close, pcie_fops.close); - EXPECT_EQ_UINT64(pcie_read, pcie_fops.read); + EXPECT_EQ_PTR(pcie_list_devices_sysfs, pcie_fops.list_devices); + EXPECT_EQ_PTR(pcie_open_sysfs, pcie_fops.open); + EXPECT_EQ_PTR(pcie_close, pcie_fops.close); + EXPECT_EQ_PTR(pcie_read, pcie_fops.read); EXPECT_EQ_STR("Test", pcie_config.access_dir); pcie_config.access_dir[0] = '\0'; @@ -206,28 +208,20 @@ DEF_TEST(plugin_config_fail) { test_cfg_parent.children_num = 1; int ret = pcie_plugin_config(&test_cfg_parent); - EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(1, pcie_config.config_error); - pcie_config.config_error = 0; + EXPECT_EQ_INT(-1, ret); sstrncpy(key_buff, "Source", sizeof(key_buff)); ret = pcie_plugin_config(&test_cfg_parent); - EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(1, pcie_config.config_error); - pcie_config.config_error = 0; + EXPECT_EQ_INT(-1, ret); sstrncpy(value_buff, "proc", sizeof(value_buff)); test_cfg_value.type = OCONFIG_TYPE_NUMBER; ret = pcie_plugin_config(&test_cfg_parent); - EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(1, pcie_config.config_error); - pcie_config.config_error = 0; + EXPECT_EQ_INT(-1, ret); sstrncpy(key_buff, "AccessDir", sizeof(key_buff)); ret = pcie_plugin_config(&test_cfg_parent); - EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(1, pcie_config.config_error); - pcie_config.config_error = 0; + EXPECT_EQ_INT(-1, ret); return 0; } @@ -246,21 +240,18 @@ DEF_TEST(plugin_config) { pcie_config.use_sysfs = 1; int ret = pcie_plugin_config(&test_cfg_parent); EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(0, pcie_config.config_error); EXPECT_EQ_INT(0, pcie_config.use_sysfs); pcie_config.use_sysfs = 1; sstrncpy(value_buff, "sysfs", sizeof(value_buff)); ret = pcie_plugin_config(&test_cfg_parent); EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(0, pcie_config.config_error); EXPECT_EQ_INT(1, pcie_config.use_sysfs); sstrncpy(key_buff, "AccessDir", sizeof(key_buff)); sstrncpy(value_buff, "some/test/value", sizeof(value_buff)); ret = pcie_plugin_config(&test_cfg_parent); EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(0, pcie_config.config_error); EXPECT_EQ_STR("some/test/value", pcie_config.access_dir); memset(&test_cfg_value.value, 0, sizeof(test_cfg_value.value)); @@ -269,13 +260,11 @@ DEF_TEST(plugin_config) { sstrncpy(key_buff, "ReportMasked", sizeof(key_buff)); ret = pcie_plugin_config(&test_cfg_parent); EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(0, pcie_config.config_error); EXPECT_EQ_INT(1, pcie_config.notif_masked); sstrncpy(key_buff, "PersistentNotifications", sizeof(key_buff)); ret = pcie_plugin_config(&test_cfg_parent); EXPECT_EQ_INT(0, ret); - EXPECT_EQ_INT(0, pcie_config.config_error); EXPECT_EQ_INT(1, pcie_config.persistent); return 0; diff --git a/src/testing.h b/src/testing.h index 5cf69559..fd7e6c66 100644 --- a/src/testing.h +++ b/src/testing.h @@ -100,6 +100,18 @@ static int check_count__; printf("ok %i - %s = %" PRIu64 "\n", ++check_count__, #actual, got__); \ } while (0) +#define EXPECT_EQ_PTR(expect, actual) \ + do { \ + void *want__ = expect; \ + void *got__ = actual; \ + if (got__ != want__) { \ + printf("not ok %i - %s = %p, want %p\n", ++check_count__, #actual, \ + got__, want__); \ + return -1; \ + } \ + printf("ok %i - %s = %p\n", ++check_count__, #actual, got__); \ + } while (0) + #define EXPECT_EQ_DOUBLE(expect, actual) \ do { \ double want__ = (double)expect; \