libdm/libdm-stats.c | 223 ++++++++++++++++++++++++++++++----------------------

 tools/dmsetup.c     |   1 +

 2 files changed, 131 insertions(+), 93 deletions(-)

diff --git a/libdm/libdm-stats.c b/libdm/libdm-stats.c

index 93c7760..ee62fe6 100644

--- a/libdm/libdm-stats.c

+++ b/libdm/libdm-stats.c

@@ -144,17 +144,24 @@ struct dm_stats *dm_stats_create(const char *program_id)

 		return_NULL;

 	/* FIXME: better hint. */

-	if (!(dms->mem = dm_pool_create("stats_pool", 4096)))

-		goto_bad;

+	if (!(dms->mem = dm_pool_create("stats_pool", 4096))) {

+		dm_free(dms);

+		return_NULL;

+	}

 	if (!(dms->hist_mem = dm_pool_create("histogram_pool", hist_hint)))

-		return_0;

+		goto_bad;

 	if (!program_id || !strlen(program_id))

 		dms->program_id = _program_id_from_proc();

 	else

 		dms->program_id = dm_strdup(program_id);

+	if (!dms->program_id) {

+		dm_pool_destroy(dms->hist_mem);

+		goto_bad;

+	}

+

 	dms->major = -1;

 	dms->minor = -1;

 	dms->name = NULL;

@@ -171,6 +178,7 @@ struct dm_stats *dm_stats_create(const char *program_id)

 	return dms;

 bad:

+	dm_pool_destroy(dms->mem);

 	dm_free(dms);

 	return NULL;

 }

@@ -186,15 +194,15 @@ static int _stats_region_present(const struct dm_stats_region *region)

 static void _stats_histograms_destroy(struct dm_pool *mem,

 				      struct dm_stats_region *region)

 {

-	uint64_t n;

-

 	/* Unpopulated handle. */

 	if (!region->counters)

 		return;

-	for (n = _nr_areas_region(region) - 1; n; n--)

-		if (region->counters[n].histogram)

-			dm_pool_free(mem, region->counters[n].histogram);

+	/*

+	 * Only the first histogram needs to be freed explicitly.

+	 */

+	if (region->counters[0].histogram)

+		dm_pool_free(mem, region->counters[0].histogram);

 }

 static void _stats_region_destroy(struct dm_stats_region *region)

@@ -316,11 +324,54 @@ int dm_stats_driver_supports_histogram(void)

 	return _stats_check_precise_timestamps(NULL);

 }

+static int _fill_hist_arg(char *hist_arg, size_t hist_len, uint64_t scale,

+			  struct dm_histogram *bounds)

+{

+	int i, l, len = 0, nr_bins;

+	char *arg = hist_arg;

+	uint64_t value;

+

+	nr_bins = bounds->nr_bins;

+

+	for (i = 0; i < nr_bins; i++) {

+		value = bounds->bins[i].upper / scale;

+		if ((l = dm_snprintf(arg, hist_len - len, FMTu64"%s", value,

+				     (i == (nr_bins - 1)) ? "" : ",")) < 0)

+			return_0;

+		len += l;

+		arg += l;

+	}

+	return 1;

+}

+

+static void *_get_hist_arg(struct dm_histogram *bounds, uint64_t scale,

+			   size_t *len)

+{

+	struct dm_histogram_bin *entry, *bins;

+	size_t hist_len = 1; /* terminating '\0' */

+	double value;

+

+	entry = bins = bounds->bins;

+

+	entry += bounds->nr_bins - 1;

+	while(entry >= bins) {

+		value = (double) (entry--)->upper;

+		/* Use lround to avoid size_t -> double cast warning. */

+		hist_len += 1 + (size_t) lround(log10(value / scale));

+		if (entry != bins)

+			hist_len++; /* ',' */

+	}

+

+	*len = hist_len;

+

+	return dm_zalloc(hist_len);

+}

+

 static char *_build_histogram_arg(struct dm_histogram *bounds, int *precise)

 {

 	struct dm_histogram_bin *entry, *bins;

-	size_t hist_len = 1, len = 0;

-	char *hist_arg, *arg = NULL;

+	size_t hist_len;

+	char *hist_arg;

 	uint64_t scale;

 	entry = bins = bounds->bins;

@@ -331,53 +382,37 @@ static char *_build_histogram_arg(struct dm_histogram *bounds, int *precise)

 		return NULL;

 	}

+	/* Validate entries and set *precise if precision < 1ms. */

 	entry += bounds->nr_bins - 1;

-	while(entry >= bins) {

-		double value;

+	while (entry >= bins) {

 		if (entry != bins) {

 			if (entry->upper < (entry - 1)->upper) {

 				log_error("Histogram boundaries must be in "

 					  "order of increasing magnitude.");

 				return 0;

 			}

-			hist_len++; /* ',' */

 		}

 		/*

 		 * Only enable precise_timestamps automatically if any

 		 * value in the histogram bounds uses precision < 1ms.

 		 */

-		if (!*precise && (entry->upper % NSEC_PER_MSEC))

+		if (((entry--)->upper % NSEC_PER_MSEC) && !*precise)

 			*precise = 1;

-

-		value = (double) (entry--)->upper;

-		/* Use lround to avoid size_t -> double cast warning. */

-		hist_len += 1 + (size_t) lround(log10(value));

 	}

-	if (!(hist_arg = dm_zalloc(hist_len))) {

+	scale = (*precise) ? 1 : NSEC_PER_MSEC;

+

+	/* Calculate hist_len and allocate a character buffer. */

+	if (!(hist_arg = _get_hist_arg(bounds, scale, &hist_len))) {

 		log_error("Could not allocate memory for histogram argument.");

 		return 0;

 	}

-	arg = hist_arg;

-

-	if (*precise)

-		scale = 1;

-	else

-		scale = (*precise) ? 1 : NSEC_PER_MSEC;

+	/* Fill hist_arg with boundary strings. */

+	if (!_fill_hist_arg(hist_arg, hist_len, scale, bounds))

+		goto_bad;

-	for (entry = bins; entry < (bins + bounds->nr_bins); entry++) {

-		uint64_t value;

-		ssize_t l = 0;

-		int last = !(entry < (bins + bounds->nr_bins - 1));

-		value = entry->upper / scale;

-		if ((l = dm_snprintf(arg, hist_len - len, FMTu64"%s", value,

-				     (last) ? "" : ",")) < 0)

-			goto_bad;

-		len += (size_t) l;

-		arg += (size_t) l;

-	}

 	return hist_arg;

 bad:

@@ -419,13 +454,13 @@ static int _stats_parse_histogram_spec(struct dm_stats *dms,

 				       const char *histogram)

 {

 	static const char *_valid_chars = "0123456789,";

-	uint64_t scale = region->timescale;

+	uint64_t scale = region->timescale, this_val = 0;

 	struct dm_pool *mem = dms->hist_mem;

 	struct dm_histogram_bin cur;

 	struct dm_histogram hist;

 	int nr_bins = 1;

-	const char *c, *v;

-	char *p;

+	const char *c, *v, *val_start;

+	char *p, *endptr = NULL;

 	/* Advance past "histogram:". */

 	histogram = strchr(histogram, ':');

@@ -466,9 +501,8 @@ static int _stats_parse_histogram_spec(struct dm_stats *dms,

 				  histogram);

 			goto bad;

 		} else {

-			const char *val_start = c;

-			char *endptr = NULL;

-			uint64_t this_val = 0;

+			val_start = c;

+			endptr = NULL;

 			this_val = strtoull(val_start, &endptr, 10);

 			if (!endptr) {

@@ -592,11 +626,11 @@ static int _stats_parse_list_region(struct dm_stats *dms,

 static int _stats_parse_list(struct dm_stats *dms, const char *resp)

 {

-	struct dm_pool *mem = dms->mem;

-	struct dm_stats_region cur;

 	uint64_t max_region = 0, nr_regions = 0;

+	struct dm_stats_region cur, fill;

+	struct dm_pool *mem = dms->mem;

 	FILE *list_rows;

-	/* FIXME: determine correct maximum line length based on kernel format */

+	/* FIXME: use correct maximum line length for kernel format */

 	char line[256];

 	if (!resp) {

@@ -631,7 +665,6 @@ static int _stats_parse_list(struct dm_stats *dms, const char *resp)

 		/* handle holes in the list of region_ids */

 		if (cur.region_id > max_region) {

-			struct dm_stats_region fill;

 			memset(&fill, 0, sizeof(fill));

 			fill.region_id = DM_STATS_REGION_NOT_PRESENT;

 			do {

@@ -707,54 +740,51 @@ static int _stats_parse_histogram(struct dm_pool *mem, char *hist_str,

 				  struct dm_histogram **histogram,

 				  struct dm_stats_region *region)

 {

+	struct dm_histogram hist, *bounds = region->bounds;

 	static const char *_valid_chars = "0123456789:";

 	int nr_bins = region->bounds->nr_bins;

-	struct dm_histogram hist, *bounds = region->bounds;

+	const char *c, *v, *val_start;

 	struct dm_histogram_bin cur;

-	uint64_t sum = 0;

-	const char *c, *v;

+	uint64_t sum = 0, this_val;

+	char *endptr = NULL;

 	int bin = 0;

 	c = hist_str;

-	dm_pool_begin_object(mem, sizeof(cur));

+	if (!dm_pool_begin_object(mem, sizeof(cur)))

+		return_0;

 	hist.nr_bins = nr_bins;

-	dm_pool_grow_object(mem, &hist, sizeof(hist));

+	if (!dm_pool_grow_object(mem, &hist, sizeof(hist)))

+		goto_bad;

 	do {

 		memset(&cur, 0, sizeof(cur));

 		for (v = _valid_chars; *v; v++)

 			if (*c == *v)

 				break;

-		if (!*v) {

-			stack;

+		if (!*v)

 			goto badchar;

-		}

-		if (*c == ',') {

-			log_error("Invalid histogram: %s", hist_str);

-			return 0;

-		} else {

-			const char *val_start = c;

-			char *endptr = NULL;

-			uint64_t this_val = 0;

+		if (*c == ',')

+			goto badchar;

+		else {

+			val_start = c;

+			endptr = NULL;

 			this_val = strtoull(val_start, &endptr, 10);

 			if (!endptr) {

 				log_error("Could not parse histogram value.");

-				return 0;

+				goto bad;

 			}

 			c = endptr; /* Advance to colon, or end. */

 			if (*c == ':')

 				c++;

-			else if (*c & (*c != '\n')) {

+			else if (*c & (*c != '\n'))

 				/* Expected ':', '\n', or NULL. */

-				stack;

 				goto badchar;

-			}

 			if (*c == ':')

 				c++;

@@ -763,7 +793,8 @@ static int _stats_parse_histogram(struct dm_pool *mem, char *hist_str,

 			cur.count = this_val;

 			sum += this_val;

-			dm_pool_grow_object(mem, &cur, sizeof(cur));

+			if (!dm_pool_grow_object(mem, &cur, sizeof(cur)))

+				goto_bad;

 			bin++;

 		}

@@ -778,6 +809,8 @@ static int _stats_parse_histogram(struct dm_pool *mem, char *hist_str,

 badchar:

 	log_error("Invalid character in histogram data: '%c' (0x%x)", *c, *c);

+bad:

+	dm_pool_abandon_object(mem);

 	return 0;

 }

@@ -786,8 +819,8 @@ static int _stats_parse_region(struct dm_stats *dms, const char *resp,

 			       uint64_t timescale)

 {

 	struct dm_histogram *hist = NULL;

-	struct dm_stats_counters cur;

 	struct dm_pool *mem = dms->mem;

+	struct dm_stats_counters cur;

 	FILE *stats_rows = NULL;

 	uint64_t start, len;

 	char row[256];

@@ -1040,12 +1073,12 @@ static int _stats_create_region(struct dm_stats *dms, uint64_t *region_id,

 				int precise, const char *hist_arg,

 				const char *program_id,	const char *aux_data)

 {

-	struct dm_task *dmt = NULL;

-	char msg[1024], range[64];

 	const char *err_fmt = "Could not prepare @stats_create %s.";

 	const char *precise_str = PRECISE_ARG;

 	const char *resp, *opt_args = NULL;

-	int r = 0, nr_opt = 0; /* number of optional args. */

+	char msg[1024], range[64], *endptr = NULL;

+	struct dm_task *dmt = NULL;

+	int r = 0, nr_opt = 0;

 	if (!_stats_bound(dms))

 		return_0;

@@ -1105,7 +1138,6 @@ static int _stats_create_region(struct dm_stats *dms, uint64_t *region_id,

 	}

 	if (region_id) {

-		char *endptr = NULL;

 		*region_id = strtoull(resp, &endptr, 10);

 		if (resp == endptr)

 			goto_out;

@@ -1195,11 +1227,11 @@ static struct dm_task *_stats_print_region(struct dm_stats *dms,

 				    uint64_t region_id, unsigned start_line,

 				    unsigned num_lines, unsigned clear)

 {

-	struct dm_task *dmt = NULL;

 	/* @stats_print[_clear] <region_id> [<start_line> <num_lines>] */

 	const char *clear_str = "_clear", *lines_fmt = "%u %u";

 	const char *msg_fmt = "@stats_print%s " FMTu64 " %s";

 	const char *err_fmt = "Could not prepare @stats_print %s.";

+	struct dm_task *dmt = NULL;

 	char msg[1024], lines[64];

 	if (start_line || num_lines)

@@ -1292,6 +1324,8 @@ int dm_stats_populate(struct dm_stats *dms, const char *program_id,

 		      uint64_t region_id)

 {

 	int all_regions = (region_id == DM_STATS_REGIONS_ALL);

+	struct dm_task *dmt = NULL; /* @stats_print task */

+	const char *resp;

 	if (!_stats_bound(dms))

 		return_0;

@@ -1311,9 +1345,6 @@ int dm_stats_populate(struct dm_stats *dms, const char *program_id,

 	dm_stats_walk_start(dms);

 	do {

-		struct dm_task *dmt = NULL; /* @stats_print task */

-		const char *resp;

-

 		region_id = (all_regions)

 			     ? dm_stats_get_current_region(dms) : region_id;

@@ -1967,10 +1998,12 @@ static struct dm_histogram *_alloc_dm_histogram(int nr_bins)

 struct dm_histogram *dm_histogram_bounds_from_string(const char *bounds_str)

 {

 	static const char *_valid_chars = "0123456789,muns";

-	struct dm_histogram *dmh;

+	uint64_t this_val = 0, mult = 1;

+	const char *c, *v, *val_start;

 	struct dm_histogram_bin *cur;

-	const char *c, *v;

+	struct dm_histogram *dmh;

 	int nr_entries = 1;

+	char *endptr;

 	c = bounds_str;

@@ -2003,9 +2036,8 @@ struct dm_histogram *dm_histogram_bounds_from_string(const char *bounds_str)

 				  bounds_str);

 			goto bad;

 		} else {

-			const char *val_start = c;

-			char *endptr = NULL;

-			uint64_t this_val = 0, mult = 1;

+			val_start = c;

+			endptr = NULL;

 			this_val = strtoull(val_start, &endptr, 10);

 			if (!endptr) {

@@ -2058,10 +2090,10 @@ bad:

 struct dm_histogram *dm_histogram_bounds_from_uint64(const uint64_t *bounds)

 {

-	struct dm_histogram *dmh;

+	const uint64_t *entry = bounds;

 	struct dm_histogram_bin *cur;

+	struct dm_histogram *dmh;

 	int nr_entries = 1;

-	const uint64_t *entry = bounds;

 	if (!bounds || !bounds[0]) {

 		log_error("Could not parse empty histogram bounds array");

@@ -2113,6 +2145,7 @@ void dm_histogram_bounds_destroy(struct dm_histogram *bounds)

  */

 static void _scale_bound_value_to_suffix(uint64_t *bound, const char **suffix)

 {

+	*suffix = "ns";

 	if (!(*bound % NSEC_PER_SEC)) {

 		*bound /= NSEC_PER_SEC;

 		*suffix = "s";

@@ -2191,12 +2224,14 @@ const char *dm_histogram_to_string(const struct dm_histogram *dmh, int bin,

 				   int width, int flags)

 {

 	int minwidth, bounds, values, start, last;

-	uint64_t lower, upper; /* bounds of the current bin. */

+	uint64_t lower, upper, val_u64; /* bounds of the current bin. */

 	/* Use the histogram pool for string building. */

 	struct dm_pool *mem = dmh->dms->hist_mem;

 	char buf[64], bounds_buf[64];

 	const char *sep = "";

+	int bounds_width;

 	ssize_t len = 0;

+	float val_flt;

 	bounds = flags & DM_HISTOGRAM_BOUNDS_MASK;

 	values = flags & DM_HISTOGRAM_VALUES;

@@ -2222,12 +2257,11 @@ const char *dm_histogram_to_string(const struct dm_histogram *dmh, int bin,

 	/* Set bounds string to the empty string. */

 	bounds_buf[0] = '\0';

-	dm_pool_begin_object(mem, 64);

+	if (!dm_pool_begin_object(mem, 64))

+		return_0;

 	for (bin = start; bin <= last; bin++) {

 		if (bounds) {

-			int bounds_width;

-

 			/* Default bounds width depends on time suffixes. */

 			bounds_width = (!(flags & DM_HISTOGRAM_SUFFIX))

 					? BOUND_WIDTH_NOSUFFIX

@@ -2260,15 +2294,14 @@ const char *dm_histogram_to_string(const struct dm_histogram *dmh, int bin,

 		if (flags & DM_HISTOGRAM_PERCENT) {

 			dm_percent_t pr;

-			float value;

 			pr = dm_histogram_get_bin_percent(dmh, bin);

-			value = dm_percent_to_float(pr);

+			val_flt = dm_percent_to_float(pr);

 			len = dm_snprintf(buf, sizeof(buf), "%s%*.2f%%%s",

-					  bounds_buf, width, value, sep);

+					  bounds_buf, width, val_flt, sep);

 		} else if (values) {

-			uint64_t value = dmh->bins[bin].count;

+			val_u64 = dmh->bins[bin].count;

 			len = dm_snprintf(buf, sizeof(buf), "%s%*"PRIu64"%s",

-					  bounds_buf, width, value, sep);

+					  bounds_buf, width, val_u64, sep);

 		} else if (bounds)

 			len = dm_snprintf(buf, sizeof(buf), "%s%s", bounds_buf,

 					  sep);

@@ -2277,9 +2310,13 @@ const char *dm_histogram_to_string(const struct dm_histogram *dmh, int bin,

 			goto_bad;

 		width = minwidth; /* re-set histogram column width. */

-		dm_pool_grow_object(mem, buf, (size_t) len);

+		if (!dm_pool_grow_object(mem, buf, (size_t) len))

+			goto_bad;

 	}

-	dm_pool_grow_object(mem, "\0", 1);

+

+	if (!dm_pool_grow_object(mem, "\0", 1))

+		goto_bad;

+

 	return (const char *) dm_pool_end_object(mem);

 bad:

diff --git a/tools/dmsetup.c b/tools/dmsetup.c

index 61ad5a9..8b7ad74 100644

--- a/tools/dmsetup.c

+++ b/tools/dmsetup.c

@@ -4674,6 +4674,7 @@ static int _do_stats_create_regions(struct dm_stats *dms,

 		return_0;

 	if (!(dmt = dm_task_create(DM_DEVICE_TABLE))) {

+		dm_histogram_bounds_destroy(bounds);

 		dm_stats_destroy(dms);

 		return_0;

 	}