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 <kamilx.wiatrowski@intel.com>

diff --git a/src/collectd.conf.pod b/src/collectd.conf.pod
index 20605f5..87b92e3 100644 (file)
--- 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<NOTIF_FAILURE> and all others as I<NOTIF_WARNING>.
+Fatal errors are reported as I<NOTIF_FAILURE> and all others as I<NOTIF_WARNING>.
 
 B<Synopsis:>
 
@@ -6301,13 +6301,12 @@ Directory used to access device config space. It is optional and defaults to
 
 =item B<ReportMasked> B<false>|B<true>
 
-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<false>.
+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<false>.
 
 =item B<PersistentNotifications> B<false>|B<true>
 
-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<false>.
 
 =back

diff --git a/src/pcie_errors.c b/src/pcie_errors.c
index 1f7837d..b239a8c 100644 (file)
--- 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 48b01b1..5cb95fa 100644 (file)
--- a/src/pcie_errors_test.c
+++ b/src/pcie_errors_test.c
@@ -25,6 +25,8 @@
  *   Kamil Wiatrowski <kamilx.wiatrowski@intel.com>
  **/
 
+#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 5cf6955..fd7e6c6 100644 (file)
--- 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;                                            \