Don't race in FreeBSD devstat collector

Querying the number of devices separately from the device list itself is
racy. Devices may be added or removed between the two calls; and removed
devices would lead to a segfault.

collector/devstat_freebsd.c

@@ -9,66 +9,59 @@
 #include <string.h>
 #include <devstat_freebsd.h>
 
-int _get_ndevs() {
-	struct statinfo current;
-	struct devinfo info = {};
-	current.dinfo = &info;
-
-	devstat_checkversion(NULL);
-
-	if (devstat_getdevs(NULL, &current) == -1)
-		return -1;
 
-	return current.dinfo->numdevs;
+int _get_stats(Stats **stats) {
-}
-
-Stats _get_stats(int i) {
 	struct statinfo current;
 	struct devinfo info = {};
 	current.dinfo = &info;
 
-	devstat_getdevs(NULL, &current);
+	if (devstat_getdevs(NULL, &current) == -1) {
+		return -1;
+	}
 
-	Stats stats;
+	Stats *p = (Stats*)calloc(current.dinfo->numdevs, sizeof(Stats));
-	uint64_t bytes_read, bytes_write, bytes_free;
+	for (int i = 0; i < current.dinfo->numdevs; i++) {
-	uint64_t transfers_other, transfers_read, transfers_write, transfers_free;
+		uint64_t bytes_read, bytes_write, bytes_free;
-	long double duration_other, duration_read, duration_write, duration_free;
+		uint64_t transfers_other, transfers_read, transfers_write, transfers_free;
-	long double busy_time;
+		long double duration_other, duration_read, duration_write, duration_free;
-	uint64_t blocks;
+		long double busy_time;
+		uint64_t blocks;
 
-	strcpy(stats.device, current.dinfo->devices[i].device_name);
+		strcpy(p[i].device, current.dinfo->devices[i].device_name);
-	stats.unit = current.dinfo->devices[i].unit_number;
+		p[i].unit = current.dinfo->devices[i].unit_number;
-	devstat_compute_statistics(&current.dinfo->devices[i],
+		devstat_compute_statistics(&current.dinfo->devices[i],
-		NULL,
+				NULL,
-		1.0,
+				1.0,
-		DSM_TOTAL_BYTES_READ, &bytes_read,
+				DSM_TOTAL_BYTES_READ, &bytes_read,
-		DSM_TOTAL_BYTES_WRITE, &bytes_write,
+				DSM_TOTAL_BYTES_WRITE, &bytes_write,
-		DSM_TOTAL_BYTES_FREE, &bytes_free,
+				DSM_TOTAL_BYTES_FREE, &bytes_free,
-		DSM_TOTAL_TRANSFERS_OTHER, &transfers_other,
+				DSM_TOTAL_TRANSFERS_OTHER, &transfers_other,
-		DSM_TOTAL_TRANSFERS_READ, &transfers_read,
+				DSM_TOTAL_TRANSFERS_READ, &transfers_read,
-		DSM_TOTAL_TRANSFERS_WRITE, &transfers_write,
+				DSM_TOTAL_TRANSFERS_WRITE, &transfers_write,
-		DSM_TOTAL_TRANSFERS_FREE, &transfers_free,
+				DSM_TOTAL_TRANSFERS_FREE, &transfers_free,
-		DSM_TOTAL_DURATION_OTHER, &duration_other,
+				DSM_TOTAL_DURATION_OTHER, &duration_other,
-		DSM_TOTAL_DURATION_READ, &duration_read,
+				DSM_TOTAL_DURATION_READ, &duration_read,
-		DSM_TOTAL_DURATION_WRITE, &duration_write,
+				DSM_TOTAL_DURATION_WRITE, &duration_write,
-		DSM_TOTAL_DURATION_FREE, &duration_free,
+				DSM_TOTAL_DURATION_FREE, &duration_free,
-		DSM_TOTAL_BUSY_TIME, &busy_time,
+				DSM_TOTAL_BUSY_TIME, &busy_time,
-		DSM_TOTAL_BLOCKS, &blocks,
+				DSM_TOTAL_BLOCKS, &blocks,
-		DSM_NONE);
+				DSM_NONE);
 
-	stats.bytes.read = bytes_read;
+		p[i].bytes.read = bytes_read;
-	stats.bytes.write = bytes_write;
+		p[i].bytes.write = bytes_write;
-	stats.bytes.free = bytes_free;
+		p[i].bytes.free = bytes_free;
-	stats.transfers.other = transfers_other;
+		p[i].transfers.other = transfers_other;
-	stats.transfers.read = transfers_read;
+		p[i].transfers.read = transfers_read;
-	stats.transfers.write = transfers_write;
+		p[i].transfers.write = transfers_write;
-	stats.transfers.free = transfers_free;
+		p[i].transfers.free = transfers_free;
-	stats.duration.other = duration_other;
+		p[i].duration.other = duration_other;
-	stats.duration.read = duration_read;
+		p[i].duration.read = duration_read;
-	stats.duration.write = duration_write;
+		p[i].duration.write = duration_write;
-	stats.duration.free = duration_free;
+		p[i].duration.free = duration_free;
-	stats.busyTime = busy_time;
+		p[i].busyTime = busy_time;
-	stats.blocks = blocks;
+		p[i].blocks = blocks;
+	}
 
-	return stats;
+	*stats = p;
+	return current.dinfo->numdevs;
 }

collector/devstat_freebsd.go

@@ -18,6 +18,7 @@ package collector
 import (
 	"errors"
 	"fmt"
+	"unsafe"
 
 	"github.com/prometheus/client_golang/prometheus"
 )
@@ -75,25 +76,30 @@ func NewDevstatCollector() (Collector, error) {
 	}, nil
 }
 
-func (c *devstatCollector) Update(ch chan<- prometheus.Metric) (err error) {
+func (c *devstatCollector) Update(ch chan<- prometheus.Metric) error {
-	count := C._get_ndevs()
+	var stats *C.Stats
-	if count == -1 {
+	n := C._get_stats(&stats)
-		return errors.New("devstat_getdevs() failed")
+	if n == -1 {
+		return errors.New("devstat_getdevs failed")
 	}
 
-	for i := C.int(0); i < count; i++ {
+	base := unsafe.Pointer(stats)
-		stats := C._get_stats(i)
+	for i := C.int(0); i < n; i++ {
-		device := fmt.Sprintf("%s%d", C.GoString(&stats.device[0]), stats.unit)
+		offset := i * C.int(C.sizeof_Stats)
-		ch <- c.bytes.mustNewConstMetric(float64(stats.bytes.read), device, "read")
+		stat := (*C.Stats)(unsafe.Pointer(uintptr(base) + uintptr(offset)))
-		ch <- c.bytes.mustNewConstMetric(float64(stats.bytes.write), device, "write")
+
-		ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.other), device, "other")
+		device := fmt.Sprintf("%s%d", C.GoString(&stat.device[0]), stat.unit)
-		ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.read), device, "read")
+		ch <- c.bytes.mustNewConstMetric(float64(stat.bytes.read), device, "read")
-		ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.write), device, "write")
+		ch <- c.bytes.mustNewConstMetric(float64(stat.bytes.write), device, "write")
-		ch <- c.duration.mustNewConstMetric(float64(stats.duration.other), device, "other")
+		ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.other), device, "other")
-		ch <- c.duration.mustNewConstMetric(float64(stats.duration.read), device, "read")
+		ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.read), device, "read")
-		ch <- c.duration.mustNewConstMetric(float64(stats.duration.write), device, "write")
+		ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.write), device, "write")
-		ch <- c.busyTime.mustNewConstMetric(float64(stats.busyTime), device)
+		ch <- c.duration.mustNewConstMetric(float64(stat.duration.other), device, "other")
-		ch <- c.blocks.mustNewConstMetric(float64(stats.blocks), device)
+		ch <- c.duration.mustNewConstMetric(float64(stat.duration.read), device, "read")
+		ch <- c.duration.mustNewConstMetric(float64(stat.duration.write), device, "write")
+		ch <- c.busyTime.mustNewConstMetric(float64(stat.busyTime), device)
+		ch <- c.blocks.mustNewConstMetric(float64(stat.blocks), device)
 	}
-	return err
+	C.free(unsafe.Pointer(stats))
+	return nil
 }

collector/devstat_freebsd.h

@@ -38,4 +38,4 @@ typedef struct {
 
 
 int _get_ndevs();
-Stats _get_stats(int i);
+int _get_stats(Stats **stats);