Merge pull request #396 from dominikh/bsd-memleak

Don't leak or race in FreeBSD devstat collector

collector/devstat_freebsd.c

@@ -0,0 +1,79 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build !nodevstat
+
+#include <devstat.h>
+#include <fcntl.h>
+#include <libgeom.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <devstat_freebsd.h>
+
+
+int _get_stats(struct devinfo *info, Stats **stats) {
+	struct statinfo current;
+	current.dinfo = info;
+
+	if (devstat_getdevs(NULL, &current) == -1) {
+		return -1;
+	}
+
+	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(p[i].device, current.dinfo->devices[i].device_name);
+		p[i].unit = current.dinfo->devices[i].unit_number;
+		devstat_compute_statistics(&current.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);
+
+		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;
+	}
+
+	*stats = p;
+	return current.dinfo->numdevs;
+}

collector/devstat_freebsd.go

@@ -18,118 +18,14 @@ package collector
 import (
 	"errors"
 	"fmt"
+	"sync"
+	"unsafe"
 
 	"github.com/prometheus/client_golang/prometheus"
 )
 
-/*
+// #cgo LDFLAGS: -ldevstat -lkvm
-#cgo LDFLAGS: -ldevstat -lkvm
+// #include "devstat_freebsd.h"
-#include <devstat.h>
-#include <fcntl.h>
-#include <libgeom.h>
-#include <limits.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-typedef struct {
-	uint64_t	read;
-	uint64_t	write;
-	uint64_t	free;
-} Bytes;
-
-typedef struct {
-	uint64_t	other;
-	uint64_t	read;
-	uint64_t	write;
-	uint64_t	free;
-} Transfers;
-
-typedef struct {
-	double		other;
-	double		read;
-	double		write;
-	double		free;
-} Duration;
-
-typedef struct {
-	char		device[DEVSTAT_NAME_LEN];
-	int		unit;
-	Bytes		bytes;
-	Transfers	transfers;
-	Duration	duration;
-	long		busyTime;
-	uint64_t	blocks;
-} Stats;
-
-int _get_ndevs() {
-	struct statinfo current;
-	int num_devices;
-
-	current.dinfo = (struct devinfo *)calloc(1, sizeof(struct devinfo));
-	if (current.dinfo == NULL)
-		return -2;
-
-	devstat_checkversion(NULL);
-
-	if (devstat_getdevs(NULL, &current) == -1)
-		return -1;
-
-	return current.dinfo->numdevs;
-}
-
-Stats _get_stats(int i) {
-	struct statinfo current;
-	int num_devices;
-
-	current.dinfo = (struct devinfo *)calloc(1, sizeof(struct devinfo));
-	devstat_getdevs(NULL, &current);
-
-	num_devices = current.dinfo->numdevs;
-	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;
-
-	strcpy(stats.device, current.dinfo->devices[i].device_name);
-	stats.unit = current.dinfo->devices[i].unit_number;
-	devstat_compute_statistics(&current.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;
-
-	return stats;
-}
-*/
 import "C"
 
 const (
@@ -137,6 +33,9 @@ const (
 )
 
 type devstatCollector struct {
+	mu      sync.Mutex
+	devinfo *C.struct_devinfo
+
 	bytes       typedDesc
 	bytes_total typedDesc
 	transfers   typedDesc
@@ -153,6 +52,7 @@ func init() {
 // Device stats.
 func NewDevstatCollector() (Collector, error) {
 	return &devstatCollector{
+		devinfo: &C.struct_devinfo{},
 		bytes: typedDesc{prometheus.NewDesc(
 			prometheus.BuildFQName(Namespace, devstatSubsystem, "bytes_total"),
 			"The total number of bytes in transactions.",
@@ -181,28 +81,33 @@ 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()
+	c.mu.Lock()
-	if count == -1 {
+	defer c.mu.Unlock()
-		return errors.New("devstat_getdevs() failed")
+
-	}
+	var stats *C.Stats
-	if count == -2 {
+	n := C._get_stats(c.devinfo, &stats)
-		return errors.New("calloc() 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

@@ -0,0 +1,54 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <devstat.h>
+#include <fcntl.h>
+#include <libgeom.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+typedef struct {
+	uint64_t	read;
+	uint64_t	write;
+	uint64_t	free;
+} Bytes;
+
+typedef struct {
+	uint64_t	other;
+	uint64_t	read;
+	uint64_t	write;
+	uint64_t	free;
+} Transfers;
+
+typedef struct {
+	double		other;
+	double		read;
+	double		write;
+	double		free;
+} Duration;
+
+typedef struct {
+	char		device[DEVSTAT_NAME_LEN];
+	int		unit;
+	Bytes		bytes;
+	Transfers	transfers;
+	Duration	duration;
+	long		busyTime;
+	uint64_t	blocks;
+} Stats;
+
+
+int _get_ndevs();
+int _get_stats(struct devinfo *info, Stats **stats);