From ea55d0f5cb8062f311891c20f2d5d11c62241cf8 Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Sat, 31 Dec 2016 06:52:44 +0100 Subject: [PATCH] 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 | 99 +++++++++++++++++------------------- collector/devstat_freebsd.go | 42 ++++++++------- collector/devstat_freebsd.h | 2 +- 3 files changed, 71 insertions(+), 72 deletions(-) diff --git a/collector/devstat_freebsd.c b/collector/devstat_freebsd.c index 3a8e6d31..80a24e41 100644 --- a/collector/devstat_freebsd.c +++ b/collector/devstat_freebsd.c @@ -9,66 +9,59 @@ #include #include -int _get_ndevs() { - struct statinfo current; - struct devinfo info = {}; - current.dinfo = &info; - - devstat_checkversion(NULL); - - if (devstat_getdevs(NULL, ¤t) == -1) - return -1; - return current.dinfo->numdevs; -} - -Stats _get_stats(int i) { +int _get_stats(Stats **stats) { struct statinfo current; struct devinfo info = {}; current.dinfo = &info; - devstat_getdevs(NULL, ¤t); + if (devstat_getdevs(NULL, ¤t) == -1) { + return -1; + } - Stats stats; - uint64_t bytes_read, bytes_write, bytes_free; - uint64_t transfers_other, transfers_read, transfers_write, transfers_free; - long double duration_other, duration_read, duration_write, duration_free; - long double busy_time; - uint64_t blocks; + Stats *p = (Stats*)calloc(current.dinfo->numdevs, sizeof(Stats)); + for (int i = 0; i < current.dinfo->numdevs; i++) { + uint64_t bytes_read, bytes_write, bytes_free; + uint64_t transfers_other, transfers_read, transfers_write, transfers_free; + long double duration_other, duration_read, duration_write, duration_free; + long double busy_time; + uint64_t blocks; - strcpy(stats.device, current.dinfo->devices[i].device_name); - stats.unit = current.dinfo->devices[i].unit_number; - devstat_compute_statistics(¤t.dinfo->devices[i], - NULL, - 1.0, - DSM_TOTAL_BYTES_READ, &bytes_read, - DSM_TOTAL_BYTES_WRITE, &bytes_write, - DSM_TOTAL_BYTES_FREE, &bytes_free, - DSM_TOTAL_TRANSFERS_OTHER, &transfers_other, - DSM_TOTAL_TRANSFERS_READ, &transfers_read, - DSM_TOTAL_TRANSFERS_WRITE, &transfers_write, - DSM_TOTAL_TRANSFERS_FREE, &transfers_free, - DSM_TOTAL_DURATION_OTHER, &duration_other, - DSM_TOTAL_DURATION_READ, &duration_read, - DSM_TOTAL_DURATION_WRITE, &duration_write, - DSM_TOTAL_DURATION_FREE, &duration_free, - DSM_TOTAL_BUSY_TIME, &busy_time, - DSM_TOTAL_BLOCKS, &blocks, - DSM_NONE); + strcpy(p[i].device, current.dinfo->devices[i].device_name); + p[i].unit = current.dinfo->devices[i].unit_number; + devstat_compute_statistics(¤t.dinfo->devices[i], + NULL, + 1.0, + DSM_TOTAL_BYTES_READ, &bytes_read, + DSM_TOTAL_BYTES_WRITE, &bytes_write, + DSM_TOTAL_BYTES_FREE, &bytes_free, + DSM_TOTAL_TRANSFERS_OTHER, &transfers_other, + DSM_TOTAL_TRANSFERS_READ, &transfers_read, + DSM_TOTAL_TRANSFERS_WRITE, &transfers_write, + DSM_TOTAL_TRANSFERS_FREE, &transfers_free, + DSM_TOTAL_DURATION_OTHER, &duration_other, + DSM_TOTAL_DURATION_READ, &duration_read, + DSM_TOTAL_DURATION_WRITE, &duration_write, + DSM_TOTAL_DURATION_FREE, &duration_free, + DSM_TOTAL_BUSY_TIME, &busy_time, + DSM_TOTAL_BLOCKS, &blocks, + DSM_NONE); - stats.bytes.read = bytes_read; - stats.bytes.write = bytes_write; - stats.bytes.free = bytes_free; - stats.transfers.other = transfers_other; - stats.transfers.read = transfers_read; - stats.transfers.write = transfers_write; - stats.transfers.free = transfers_free; - stats.duration.other = duration_other; - stats.duration.read = duration_read; - stats.duration.write = duration_write; - stats.duration.free = duration_free; - stats.busyTime = busy_time; - stats.blocks = blocks; + p[i].bytes.read = bytes_read; + p[i].bytes.write = bytes_write; + p[i].bytes.free = bytes_free; + p[i].transfers.other = transfers_other; + p[i].transfers.read = transfers_read; + p[i].transfers.write = transfers_write; + p[i].transfers.free = transfers_free; + p[i].duration.other = duration_other; + p[i].duration.read = duration_read; + p[i].duration.write = duration_write; + p[i].duration.free = duration_free; + p[i].busyTime = busy_time; + p[i].blocks = blocks; + } - return stats; + *stats = p; + return current.dinfo->numdevs; } diff --git a/collector/devstat_freebsd.go b/collector/devstat_freebsd.go index 6f3b3e91..34bc53c8 100644 --- a/collector/devstat_freebsd.go +++ b/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) { - count := C._get_ndevs() - if count == -1 { - return errors.New("devstat_getdevs() failed") +func (c *devstatCollector) Update(ch chan<- prometheus.Metric) error { + var stats *C.Stats + n := C._get_stats(&stats) + if n == -1 { + return errors.New("devstat_getdevs failed") } - for i := C.int(0); i < count; i++ { - stats := C._get_stats(i) - device := fmt.Sprintf("%s%d", C.GoString(&stats.device[0]), stats.unit) - ch <- c.bytes.mustNewConstMetric(float64(stats.bytes.read), device, "read") - ch <- c.bytes.mustNewConstMetric(float64(stats.bytes.write), device, "write") - ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.other), device, "other") - ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.read), device, "read") - ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.write), device, "write") - ch <- c.duration.mustNewConstMetric(float64(stats.duration.other), device, "other") - ch <- c.duration.mustNewConstMetric(float64(stats.duration.read), device, "read") - ch <- c.duration.mustNewConstMetric(float64(stats.duration.write), device, "write") - ch <- c.busyTime.mustNewConstMetric(float64(stats.busyTime), device) - ch <- c.blocks.mustNewConstMetric(float64(stats.blocks), device) + base := unsafe.Pointer(stats) + for i := C.int(0); i < n; i++ { + offset := i * C.int(C.sizeof_Stats) + stat := (*C.Stats)(unsafe.Pointer(uintptr(base) + uintptr(offset))) + + device := fmt.Sprintf("%s%d", C.GoString(&stat.device[0]), stat.unit) + ch <- c.bytes.mustNewConstMetric(float64(stat.bytes.read), device, "read") + ch <- c.bytes.mustNewConstMetric(float64(stat.bytes.write), device, "write") + ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.other), device, "other") + ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.read), device, "read") + ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.write), device, "write") + ch <- c.duration.mustNewConstMetric(float64(stat.duration.other), device, "other") + 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 } diff --git a/collector/devstat_freebsd.h b/collector/devstat_freebsd.h index 13ea4bc6..87f09f0b 100644 --- a/collector/devstat_freebsd.h +++ b/collector/devstat_freebsd.h @@ -38,4 +38,4 @@ typedef struct { int _get_ndevs(); -Stats _get_stats(int i); +int _get_stats(Stats **stats);