[PATCH V2 1/2] Btrfs-progs: make pretty_sizes() work less error prone

[PATCH V2 1/2] Btrfs-progs: make pretty_sizes() work less error prone

[Wang Shilong]

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

In the original code, pretty_sizes() may return NULL in two cases:
<1> Allocating memory dynamically fails
<2> Overflow happens(size exceeds YB)

Since we are limited to 16EB both theoretically and practically
due to everything being 64bit, we can just droy zetta- and yotta-
suffixes.(suggested by David)

The original codes don't handle error gracefully and some places
forget to free memory. We can allocate memory before calling pretty_sizes(),
for example, we can use static memory allocation and we don't have to deal
with memory allocation fails.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
---
V1->V2: remove zetta and yotta suffixes(Thanks to David)
and make changelog more precise
---
 btrfs-calc-size.c | 10 ++++------
 btrfs-fragments.c |  4 +++-
 cmds-filesystem.c | 24 ++++++++++--------------
 cmds-scrub.c      |  5 ++---
 mkfs.c            |  8 ++++----
 utils.c           | 24 ++++++++++++------------
 utils.h           |  3 ++-
 7 files changed, 37 insertions(+), 41 deletions(-)

diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
index c4adfb0..708b0d3 100644
--- a/btrfs-calc-size.c
+++ b/btrfs-calc-size.c
@@ -162,18 +162,16 @@ out_print:
 		       stat.total_inline, stat.total_nodes, stat.total_leaves,
 		       level + 1);
 	} else {
-		char *total_size;
-		char *inline_size;
+		char total_size[MAX_PRETTY_LEN];
+		char inline_size[MAX_PRETTY_LEN];
 
-		total_size = pretty_sizes(stat.total_bytes);
-		inline_size = pretty_sizes(stat.total_inline);
+		pretty_sizes(stat.total_bytes, total_size);
+		pretty_sizes(stat.total_inline, inline_size);
 
 		printf("\t%s total size, %s inline data, %Lu nodes, "
 		       "%Lu leaves, %d levels\n",
 		       total_size, inline_size, stat.total_nodes,
 		       stat.total_leaves, level + 1);
-		free(total_size);
-		free(inline_size);
 	}
 out:
 	btrfs_free_path(path);
diff --git a/btrfs-fragments.c b/btrfs-fragments.c
index a012fe1..56c8683 100644
--- a/btrfs-fragments.c
+++ b/btrfs-fragments.c
@@ -84,10 +84,12 @@ print_bg(FILE *html, char *name, u64 start, u64 len, u64 used, u64 flags,
 	 u64 areas)
 {
 	double frag = (double)areas / (len / 4096) * 2;
+	char str[MAX_PRETTY_LEN];
+	pretty_sizes(len, str);
 
 	fprintf(html, "<p>%s chunk starts at %lld, size is %s, %.2f%% used, "
 		      "%.2f%% fragmented</p>\n", chunk_type(flags), start,
-		      pretty_sizes(len), 100.0 * used / len, 100.0 * frag);
+		      str, 100.0 * used / len, 100.0 * frag);
 	fprintf(html, "<img src=\"%s\" border=\"1\" />\n", name);
 }
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index f41a72a..a80e495 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -111,8 +111,8 @@ static int cmd_df(int argc, char **argv)
 
 	for (i = 0; i < sargs->total_spaces; i++) {
 		char description[80];
-		char *total_bytes;
-		char *used_bytes;
+		char total_bytes[MAX_PRETTY_LEN];
+		char used_bytes[MAX_PRETTY_LEN];
 		int written = 0;
 		u64 flags = sargs->spaces[i].flags;
 
@@ -155,8 +155,8 @@ static int cmd_df(int argc, char **argv)
 			written += 7;
 		}
 
-		total_bytes = pretty_sizes(sargs->spaces[i].total_bytes);
-		used_bytes = pretty_sizes(sargs->spaces[i].used_bytes);
+		pretty_sizes(sargs->spaces[i].total_bytes, total_bytes);
+		pretty_sizes(sargs->spaces[i].used_bytes, used_bytes);
 		printf("%s: total=%s, used=%s\n", description, total_bytes,
 		       used_bytes);
 	}
@@ -192,7 +192,7 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
 	char uuidbuf[37];
 	struct list_head *cur;
 	struct btrfs_device *device;
-	char *super_bytes_used;
+	char super_bytes_used[MAX_PRETTY_LEN];
 	u64 devs_found = 0;
 	u64 total;
 
@@ -204,25 +204,21 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
 	else
 		printf("Label: none ");
 
-	super_bytes_used = pretty_sizes(device->super_bytes_used);
+	pretty_sizes(device->super_bytes_used, super_bytes_used);
 
 	total = device->total_devs;
 	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
 	       (unsigned long long)total, super_bytes_used);
 
-	free(super_bytes_used);
-
 	list_for_each(cur, &fs_devices->devices) {
-		char *total_bytes;
-		char *bytes_used;
+		char total_bytes[MAX_PRETTY_LEN];
+		char bytes_used[MAX_PRETTY_LEN];
 		device = list_entry(cur, struct btrfs_device, dev_list);
-		total_bytes = pretty_sizes(device->total_bytes);
-		bytes_used = pretty_sizes(device->bytes_used);
+		pretty_sizes(device->total_bytes, total_bytes);
+		pretty_sizes(device->bytes_used, bytes_used);
 		printf("\tdevid %4llu size %s used %s path %s\n",
 		       (unsigned long long)device->devid,
 		       total_bytes, bytes_used, device->name);
-		free(total_bytes);
-		free(bytes_used);
 		devs_found++;
 	}
 	if (devs_found < total) {
diff --git a/cmds-scrub.c b/cmds-scrub.c
index c0dc584..99526f5 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -139,7 +139,7 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
 {
 	u64 err_cnt;
 	u64 err_cnt2;
-	char *bytes;
+	char bytes[MAX_PRETTY_LEN];
 
 	err_cnt = p->read_errors +
 			p->csum_errors +
@@ -151,10 +151,9 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
 	if (p->malloc_errors)
 		printf("*** WARNING: memory allocation failed while scrubbing. "
 		       "results may be inaccurate\n");
-	bytes = pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed);
+	pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed, bytes);
 	printf("\ttotal bytes scrubbed: %s with %llu errors\n", bytes,
 		max(err_cnt, err_cnt2));
-	free(bytes);
 	if (err_cnt || err_cnt2) {
 		printf("\terror details:");
 		PRINT_SCRUB_ERROR(p->read_errors, "read");
diff --git a/mkfs.c b/mkfs.c
index b412b7e..a7487d7 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1273,7 +1273,7 @@ int main(int ac, char **av)
 	u64 num_of_meta_chunks = 0;
 	u64 size_of_data = 0;
 	u64 source_dir_size = 0;
-	char *pretty_buf;
+	char pretty_buf[MAX_PRETTY_LEN];
 	struct btrfs_super_block *super;
 	u64 flags;
 	int dev_cnt = 0;
@@ -1522,11 +1522,11 @@ raid_groups:
 		printf("Setting RAID5/6 feature flag\n");
 	}
 
+	pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy),
+		     pretty_buf);
 	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
 	    "sectorsize %u size %s\n",
-	    label, first_file, nodesize, leafsize, sectorsize,
-	    pretty_buf = pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
-	free(pretty_buf);
+	    label, first_file, nodesize, leafsize, sectorsize, pretty_buf);
 
 	printf("%s\n", BTRFS_BUILD_VERSION);
 	btrfs_commit_transaction(trans, root);
diff --git a/utils.c b/utils.c
index 7b4cd74..354c206 100644
--- a/utils.c
+++ b/utils.c
@@ -1152,13 +1152,11 @@ out:
 }
 
 static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
-			    "PB", "EB", "ZB", "YB"};
-char *pretty_sizes(u64 size)
+			    "PB", "EB"};
+int pretty_sizes(u64 size, char *pretty)
 {
 	int num_divs = 0;
-        int pretty_len = 16;
-	float fraction;
-	char *pretty;
+	double fraction;
 
 	if( size < 1024 ){
 		fraction = size;
@@ -1171,14 +1169,16 @@ char *pretty_sizes(u64 size)
 			size /= 1024;
 			num_divs ++;
 		}
-
-		if (num_divs >= ARRAY_SIZE(size_strs))
-			return NULL;
-		fraction = (float)last_size / 1024;
+		/* size is up to 16EB, we should not come here.
+		 * add a santity check here in case we may modify
+		 * arrary size_strs[] in the future.
+		 */
+		if (num_divs >= (ARRAY_SIZE(size_strs)))
+			num_divs = ARRAY_SIZE(size_strs) - 1;
+		fraction = (double)last_size / 1024;
 	}
-	pretty = malloc(pretty_len);
-	snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs]);
-	return pretty;
+	return snprintf(pretty, MAX_PRETTY_LEN, "%.2lf%s",
+			fraction, size_strs[num_divs]);
 }
 
 /*
diff --git a/utils.h b/utils.h
index 3c17e14..d43e790 100644
--- a/utils.h
+++ b/utils.h
@@ -23,6 +23,7 @@
 #include "ctree.h"
 
 #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024)
+#define MAX_PRETTY_LEN 30
 
 int make_btrfs(int fd, const char *device, const char *label,
 	       u64 blocks[6], u64 num_bytes, u32 nodesize,
@@ -44,7 +45,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 			struct btrfs_fs_devices **fs_devices_mnt);
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset);
-char *pretty_sizes(u64 size);
+int pretty_sizes(u64 size, char *pretty);
 int get_mountpt(char *dev, char *mntpt, size_t size);
 int btrfs_scan_block_devices(int run_ioctl);
 u64 parse_size(char *s);
-- 
1.7.11.7

Re: [PATCH V2 1/2] Btrfs-progs: make pretty_sizes() work less error prone

[Zach Brown]

> The original codes don't handle error gracefully and some places
> forget to free memory. We can allocate memory before calling pretty_sizes(),
> for example, we can use static memory allocation and we don't have to deal
> with memory allocation fails.

I agree that callers shouldn't have to know to free allocated memory.

But I think that we can do better and not have callers need to worry
about per-call string storage at all.

How about something like this?

- z

>From bea92d06d98827af30518aa800428c0b2a8be101 Mon Sep 17 00:00:00 2001
From: Zach Brown <zab@redhat.com>
Date: Tue, 9 Jul 2013 12:43:57 -0700
Subject: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

We don't need callers to manage string storage for each pretty_sizes()
call.  We can use a macro to have per-thread and per-call static storage
so that pretty_sizes() can be used as many times as needed in printf()
arguments without requiring a bunch of supporting variables.

This lets us have a natural interface at the cost of requiring __thread
and TLS from gcc and a small amount of static storage.  This seems
better than the current code or doing something with illegible format
specifier macros.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 cmds-filesystem.c | 27 +++++++++------------------
 cmds-scrub.c      |  8 ++++----
 mkfs.c            |  4 +---
 utils.c           | 17 +++++++++--------
 utils.h           | 10 +++++++++-
 5 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index f41a72a..8d1c5c2 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -111,8 +111,6 @@ static int cmd_df(int argc, char **argv)
 
 	for (i = 0; i < sargs->total_spaces; i++) {
 		char description[80];
-		char *total_bytes;
-		char *used_bytes;
 		int written = 0;
 		u64 flags = sargs->spaces[i].flags;
 
@@ -155,10 +153,9 @@ static int cmd_df(int argc, char **argv)
 			written += 7;
 		}
 
-		total_bytes = pretty_sizes(sargs->spaces[i].total_bytes);
-		used_bytes = pretty_sizes(sargs->spaces[i].used_bytes);
-		printf("%s: total=%s, used=%s\n", description, total_bytes,
-		       used_bytes);
+		printf("%s: total=%s, used=%s\n", description,
+			pretty_sizes(sargs->spaces[i].total_bytes),
+			pretty_sizes(sargs->spaces[i].used_bytes));
 	}
 	close(fd);
 	free(sargs);
@@ -192,7 +189,6 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
 	char uuidbuf[37];
 	struct list_head *cur;
 	struct btrfs_device *device;
-	char *super_bytes_used;
 	u64 devs_found = 0;
 	u64 total;
 
@@ -204,25 +200,20 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
 	else
 		printf("Label: none ");
 
-	super_bytes_used = pretty_sizes(device->super_bytes_used);
 
 	total = device->total_devs;
 	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
-	       (unsigned long long)total, super_bytes_used);
-
-	free(super_bytes_used);
+	       (unsigned long long)total,
+	       pretty_sizes(device->super_bytes_used));
 
 	list_for_each(cur, &fs_devices->devices) {
-		char *total_bytes;
-		char *bytes_used;
 		device = list_entry(cur, struct btrfs_device, dev_list);
-		total_bytes = pretty_sizes(device->total_bytes);
-		bytes_used = pretty_sizes(device->bytes_used);
+
 		printf("\tdevid %4llu size %s used %s path %s\n",
 		       (unsigned long long)device->devid,
-		       total_bytes, bytes_used, device->name);
-		free(total_bytes);
-		free(bytes_used);
+		       pretty_sizes(device->total_bytes),
+		       pretty_sizes(device->bytes_used), device->name);
+
 		devs_found++;
 	}
 	if (devs_found < total) {
diff --git a/cmds-scrub.c b/cmds-scrub.c
index c0dc584..25f9ffd 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -139,7 +139,6 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
 {
 	u64 err_cnt;
 	u64 err_cnt2;
-	char *bytes;
 
 	err_cnt = p->read_errors +
 			p->csum_errors +
@@ -151,10 +150,11 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
 	if (p->malloc_errors)
 		printf("*** WARNING: memory allocation failed while scrubbing. "
 		       "results may be inaccurate\n");
-	bytes = pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed);
-	printf("\ttotal bytes scrubbed: %s with %llu errors\n", bytes,
+
+	printf("\ttotal bytes scrubbed: %s with %llu errors\n",
+		pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
 		max(err_cnt, err_cnt2));
-	free(bytes);
+
 	if (err_cnt || err_cnt2) {
 		printf("\terror details:");
 		PRINT_SCRUB_ERROR(p->read_errors, "read");
diff --git a/mkfs.c b/mkfs.c
index b412b7e..1a09d31 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1273,7 +1273,6 @@ int main(int ac, char **av)
 	u64 num_of_meta_chunks = 0;
 	u64 size_of_data = 0;
 	u64 source_dir_size = 0;
-	char *pretty_buf;
 	struct btrfs_super_block *super;
 	u64 flags;
 	int dev_cnt = 0;
@@ -1525,8 +1524,7 @@ raid_groups:
 	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
 	    "sectorsize %u size %s\n",
 	    label, first_file, nodesize, leafsize, sectorsize,
-	    pretty_buf = pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
-	free(pretty_buf);
+	    pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
 
 	printf("%s\n", BTRFS_BUILD_VERSION);
 	btrfs_commit_transaction(trans, root);
diff --git a/utils.c b/utils.c
index 7b4cd74..2688eb8 100644
--- a/utils.c
+++ b/utils.c
@@ -1153,12 +1153,13 @@ out:
 
 static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
 			    "PB", "EB", "ZB", "YB"};
-char *pretty_sizes(u64 size)
+void pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
 {
 	int num_divs = 0;
-        int pretty_len = 16;
 	float fraction;
-	char *pretty;
+
+	if (str_bytes == 0)
+		return;
 
 	if( size < 1024 ){
 		fraction = size;
@@ -1172,13 +1173,13 @@ char *pretty_sizes(u64 size)
 			num_divs ++;
 		}
 
-		if (num_divs >= ARRAY_SIZE(size_strs))
-			return NULL;
+		if (num_divs >= ARRAY_SIZE(size_strs)) {
+			str[0] = '\0';
+			return;
+		}
 		fraction = (float)last_size / 1024;
 	}
-	pretty = malloc(pretty_len);
-	snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs]);
-	return pretty;
+	snprintf(str, str_bytes, "%.2f%s", fraction, size_strs[num_divs]);
 }
 
 /*
diff --git a/utils.h b/utils.h
index 3c17e14..03e4aee 100644
--- a/utils.h
+++ b/utils.h
@@ -44,7 +44,15 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 			struct btrfs_fs_devices **fs_devices_mnt);
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset);
-char *pretty_sizes(u64 size);
+
+void pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
+#define pretty_sizes(size) 					\
+	({							\
+		static __thread char _str[16];			\
+		pretty_size_snprintf(size, _str, sizeof(_str));	\
+		_str;						\
+	})
+
 int get_mountpt(char *dev, char *mntpt, size_t size);
 int btrfs_scan_block_devices(int run_ioctl);
 u64 parse_size(char *s);
-- 
1.7.11.7

Re: [PATCH V2 1/2] Btrfs-progs: make pretty_sizes() work less error prone

[Wang Shilong]

Hello, Zach

>> The original codes don't handle error gracefully and some places
>> forget to free memory. We can allocate memory before calling pretty_sizes(),
>> for example, we can use static memory allocation and we don't have to deal
>> with memory allocation fails.
> 
> I agree that callers shouldn't have to know to free allocated memory.
> 
> But I think that we can do better and not have callers need to worry
> about per-call string storage at all.
> 
> How about something like this?

Yeah, much better than mine! 

Acked-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>

Thanks,
Wang
> 
> - z
> 
> From bea92d06d98827af30518aa800428c0b2a8be101 Mon Sep 17 00:00:00 2001
> From: Zach Brown <zab@redhat.com>
> Date: Tue, 9 Jul 2013 12:43:57 -0700
> Subject: [PATCH] btrfs-progs: per-thread, per-call pretty buffer
> 
> We don't need callers to manage string storage for each pretty_sizes()
> call.  We can use a macro to have per-thread and per-call static storage
> so that pretty_sizes() can be used as many times as needed in printf()
> arguments without requiring a bunch of supporting variables.
> 
> This lets us have a natural interface at the cost of requiring __thread
> and TLS from gcc and a small amount of static storage.  This seems
> better than the current code or doing something with illegible format
> specifier macros.
> 
> Signed-off-by: Zach Brown <zab@redhat.com>
> ---
> cmds-filesystem.c | 27 +++++++++------------------
> cmds-scrub.c      |  8 ++++----
> mkfs.c            |  4 +---
> utils.c           | 17 +++++++++--------
> utils.h           | 10 +++++++++-
> 5 files changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index f41a72a..8d1c5c2 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -111,8 +111,6 @@ static int cmd_df(int argc, char **argv)
> 
> 	for (i = 0; i < sargs->total_spaces; i++) {
> 		char description[80];
> -		char *total_bytes;
> -		char *used_bytes;
> 		int written = 0;
> 		u64 flags = sargs->spaces[i].flags;
> 
> @@ -155,10 +153,9 @@ static int cmd_df(int argc, char **argv)
> 			written += 7;
> 		}
> 
> -		total_bytes = pretty_sizes(sargs->spaces[i].total_bytes);
> -		used_bytes = pretty_sizes(sargs->spaces[i].used_bytes);
> -		printf("%s: total=%s, used=%s\n", description, total_bytes,
> -		       used_bytes);
> +		printf("%s: total=%s, used=%s\n", description,
> +			pretty_sizes(sargs->spaces[i].total_bytes),
> +			pretty_sizes(sargs->spaces[i].used_bytes));
> 	}
> 	close(fd);
> 	free(sargs);
> @@ -192,7 +189,6 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
> 	char uuidbuf[37];
> 	struct list_head *cur;
> 	struct btrfs_device *device;
> -	char *super_bytes_used;
> 	u64 devs_found = 0;
> 	u64 total;
> 
> @@ -204,25 +200,20 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
> 	else
> 		printf("Label: none ");
> 
> -	super_bytes_used = pretty_sizes(device->super_bytes_used);
> 
> 	total = device->total_devs;
> 	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
> -	       (unsigned long long)total, super_bytes_used);
> -
> -	free(super_bytes_used);
> +	       (unsigned long long)total,
> +	       pretty_sizes(device->super_bytes_used));
> 
> 	list_for_each(cur, &fs_devices->devices) {
> -		char *total_bytes;
> -		char *bytes_used;
> 		device = list_entry(cur, struct btrfs_device, dev_list);
> -		total_bytes = pretty_sizes(device->total_bytes);
> -		bytes_used = pretty_sizes(device->bytes_used);
> +
> 		printf("\tdevid %4llu size %s used %s path %s\n",
> 		       (unsigned long long)device->devid,
> -		       total_bytes, bytes_used, device->name);
> -		free(total_bytes);
> -		free(bytes_used);
> +		       pretty_sizes(device->total_bytes),
> +		       pretty_sizes(device->bytes_used), device->name);
> +
> 		devs_found++;
> 	}
> 	if (devs_found < total) {
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index c0dc584..25f9ffd 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -139,7 +139,6 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
> {
> 	u64 err_cnt;
> 	u64 err_cnt2;
> -	char *bytes;
> 
> 	err_cnt = p->read_errors +
> 			p->csum_errors +
> @@ -151,10 +150,11 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
> 	if (p->malloc_errors)
> 		printf("*** WARNING: memory allocation failed while scrubbing. "
> 		       "results may be inaccurate\n");
> -	bytes = pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed);
> -	printf("\ttotal bytes scrubbed: %s with %llu errors\n", bytes,
> +
> +	printf("\ttotal bytes scrubbed: %s with %llu errors\n",
> +		pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
> 		max(err_cnt, err_cnt2));
> -	free(bytes);
> +
> 	if (err_cnt || err_cnt2) {
> 		printf("\terror details:");
> 		PRINT_SCRUB_ERROR(p->read_errors, "read");
> diff --git a/mkfs.c b/mkfs.c
> index b412b7e..1a09d31 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -1273,7 +1273,6 @@ int main(int ac, char **av)
> 	u64 num_of_meta_chunks = 0;
> 	u64 size_of_data = 0;
> 	u64 source_dir_size = 0;
> -	char *pretty_buf;
> 	struct btrfs_super_block *super;
> 	u64 flags;
> 	int dev_cnt = 0;
> @@ -1525,8 +1524,7 @@ raid_groups:
> 	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
> 	    "sectorsize %u size %s\n",
> 	    label, first_file, nodesize, leafsize, sectorsize,
> -	    pretty_buf = pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
> -	free(pretty_buf);
> +	    pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
> 
> 	printf("%s\n", BTRFS_BUILD_VERSION);
> 	btrfs_commit_transaction(trans, root);
> diff --git a/utils.c b/utils.c
> index 7b4cd74..2688eb8 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1153,12 +1153,13 @@ out:
> 
> static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
> 			    "PB", "EB", "ZB", "YB"};
> -char *pretty_sizes(u64 size)
> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
> {
> 	int num_divs = 0;
> -        int pretty_len = 16;
> 	float fraction;
> -	char *pretty;
> +
> +	if (str_bytes == 0)
> +		return;
> 
> 	if( size < 1024 ){
> 		fraction = size;
> @@ -1172,13 +1173,13 @@ char *pretty_sizes(u64 size)
> 			num_divs ++;
> 		}
> 
> -		if (num_divs >= ARRAY_SIZE(size_strs))
> -			return NULL;
> +		if (num_divs >= ARRAY_SIZE(size_strs)) {
> +			str[0] = '\0';
> +			return;
> +		}
> 		fraction = (float)last_size / 1024;
> 	}
> -	pretty = malloc(pretty_len);
> -	snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs]);
> -	return pretty;
> +	snprintf(str, str_bytes, "%.2f%s", fraction, size_strs[num_divs]);
> }
> 
> /*
> diff --git a/utils.h b/utils.h
> index 3c17e14..03e4aee 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -44,7 +44,15 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
> 			struct btrfs_fs_devices **fs_devices_mnt);
> int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
> 				 int super_offset);
> -char *pretty_sizes(u64 size);
> +
> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
> +#define pretty_sizes(size) 					\
> +	({							\
> +		static __thread char _str[16];			\
> +		pretty_size_snprintf(size, _str, sizeof(_str));	\
> +		_str;						\
> +	})
> +
> int get_mountpt(char *dev, char *mntpt, size_t size);
> int btrfs_scan_block_devices(int run_ioctl);
> u64 parse_size(char *s);
> -- 
> 1.7.11.7
> 

Re: [PATCH V2 1/2] Btrfs-progs: make pretty_sizes() work less error prone

[David Sterba]

On Tue, Jul 09, 2013 at 01:24:43PM -0700, Zach Brown wrote:
> > The original codes don't handle error gracefully and some places
> > forget to free memory. We can allocate memory before calling pretty_sizes(),
> > for example, we can use static memory allocation and we don't have to deal
> > with memory allocation fails.
> 
> I agree that callers shouldn't have to know to free allocated memory.
> 
> But I think that we can do better and not have callers need to worry
> about per-call string storage at all.
> 
> How about something like this?

Neat trick! A few neat-picks below. Besides, I guess we can use this
sort of trick with the fi-df patches.

> --- a/utils.c
> +++ b/utils.c
> @@ -1153,12 +1153,13 @@ out:
>  
>  static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
>  			    "PB", "EB", "ZB", "YB"};

I'll drop the ZB, YB suffixes.

> --- a/utils.h
> +++ b/utils.h
> @@ -44,7 +44,15 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
>  			struct btrfs_fs_devices **fs_devices_mnt);
>  int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
>  				 int super_offset);
> -char *pretty_sizes(u64 size);
> +
> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
> +#define pretty_sizes(size) 					\

and rename it to pretty_size as it takes only one number

> +	({							\
> +		static __thread char _str[16];			\

16 is not enough for exabyte scale, that needs at least 20 bytes + 1 for 0.

len(str(2**64)) = 20

-> 24

> +		pretty_size_snprintf(size, _str, sizeof(_str));	\

		pretty_size_snprintf((size), _str, sizeof(_str));	\

As these are only trivial changes I'll fix them at commit time.

> +		_str;						\
> +	})
> +
>  int get_mountpt(char *dev, char *mntpt, size_t size);
>  int btrfs_scan_block_devices(int run_ioctl);
>  u64 parse_size(char *s);
> -- 
> 1.7.11.7
> 

[PATCH] btrfs-progs: per-thread, per-call pretty buffer

[David Sterba]

From: Zach Brown <zab@redhat.com>

From: Zach Brown <zab@redhat.com>

We don't need callers to manage string storage for each pretty_sizes()
call.  We can use a macro to have per-thread and per-call static storage
so that pretty_sizes() can be used as many times as needed in printf()
arguments without requiring a bunch of supporting variables.

This lets us have a natural interface at the cost of requiring __thread
and TLS from gcc and a small amount of static storage.  This seems
better than the current code or doing something with illegible format
specifier macros.

Signed-off-by: Zach Brown <zab@redhat.com>
Signed-off-by: David Sterba <dsterba@suse.cz>
---

I've updated the rest of pretty_size callers in targets that were not built by
default.

 btrfs-calc-size.c | 13 +++----------
 btrfs-fragments.c |  2 +-
 cmds-filesystem.c | 27 +++++++++------------------
 cmds-scrub.c      |  8 ++++----
 mkfs.c            |  4 +---
 utils.c           | 19 ++++++++++---------
 utils.h           | 10 +++++++++-
 7 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
index c4adfb0..5aa0b70 100644
--- a/btrfs-calc-size.c
+++ b/btrfs-calc-size.c
@@ -162,18 +162,11 @@ out_print:
 		       stat.total_inline, stat.total_nodes, stat.total_leaves,
 		       level + 1);
 	} else {
-		char *total_size;
-		char *inline_size;
-
-		total_size = pretty_sizes(stat.total_bytes);
-		inline_size = pretty_sizes(stat.total_inline);
-
 		printf("\t%s total size, %s inline data, %Lu nodes, "
 		       "%Lu leaves, %d levels\n",
-		       total_size, inline_size, stat.total_nodes,
-		       stat.total_leaves, level + 1);
-		free(total_size);
-		free(inline_size);
+		       pretty_size(stat.total_bytes),
+		       pretty_size(stat.total_inline),
+		       stat.total_nodes, stat.total_leaves, level + 1);
 	}
 out:
 	btrfs_free_path(path);
diff --git a/btrfs-fragments.c b/btrfs-fragments.c
index a012fe1..7ec77e7 100644
--- a/btrfs-fragments.c
+++ b/btrfs-fragments.c
@@ -87,7 +87,7 @@ print_bg(FILE *html, char *name, u64 start, u64 len, u64 used, u64 flags,
 
 	fprintf(html, "<p>%s chunk starts at %lld, size is %s, %.2f%% used, "
 		      "%.2f%% fragmented</p>\n", chunk_type(flags), start,
-		      pretty_sizes(len), 100.0 * used / len, 100.0 * frag);
+		      pretty_size(len), 100.0 * used / len, 100.0 * frag);
 	fprintf(html, "<img src=\"%s\" border=\"1\" />\n", name);
 }
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index f41a72a..222e458 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -111,8 +111,6 @@ static int cmd_df(int argc, char **argv)
 
 	for (i = 0; i < sargs->total_spaces; i++) {
 		char description[80];
-		char *total_bytes;
-		char *used_bytes;
 		int written = 0;
 		u64 flags = sargs->spaces[i].flags;
 
@@ -155,10 +153,9 @@ static int cmd_df(int argc, char **argv)
 			written += 7;
 		}
 
-		total_bytes = pretty_sizes(sargs->spaces[i].total_bytes);
-		used_bytes = pretty_sizes(sargs->spaces[i].used_bytes);
-		printf("%s: total=%s, used=%s\n", description, total_bytes,
-		       used_bytes);
+		printf("%s: total=%s, used=%s\n", description,
+			pretty_size(sargs->spaces[i].total_bytes),
+			pretty_size(sargs->spaces[i].used_bytes));
 	}
 	close(fd);
 	free(sargs);
@@ -192,7 +189,6 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
 	char uuidbuf[37];
 	struct list_head *cur;
 	struct btrfs_device *device;
-	char *super_bytes_used;
 	u64 devs_found = 0;
 	u64 total;
 
@@ -204,25 +200,20 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
 	else
 		printf("Label: none ");
 
-	super_bytes_used = pretty_sizes(device->super_bytes_used);
 
 	total = device->total_devs;
 	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
-	       (unsigned long long)total, super_bytes_used);
-
-	free(super_bytes_used);
+	       (unsigned long long)total,
+	       pretty_size(device->super_bytes_used));
 
 	list_for_each(cur, &fs_devices->devices) {
-		char *total_bytes;
-		char *bytes_used;
 		device = list_entry(cur, struct btrfs_device, dev_list);
-		total_bytes = pretty_sizes(device->total_bytes);
-		bytes_used = pretty_sizes(device->bytes_used);
+
 		printf("\tdevid %4llu size %s used %s path %s\n",
 		       (unsigned long long)device->devid,
-		       total_bytes, bytes_used, device->name);
-		free(total_bytes);
-		free(bytes_used);
+		       pretty_size(device->total_bytes),
+		       pretty_size(device->bytes_used), device->name);
+
 		devs_found++;
 	}
 	if (devs_found < total) {
diff --git a/cmds-scrub.c b/cmds-scrub.c
index 95dfee3..bf50650 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -139,7 +139,6 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
 {
 	u64 err_cnt;
 	u64 err_cnt2;
-	char *bytes;
 
 	err_cnt = p->read_errors +
 			p->csum_errors +
@@ -151,10 +150,11 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
 	if (p->malloc_errors)
 		printf("*** WARNING: memory allocation failed while scrubbing. "
 		       "results may be inaccurate\n");
-	bytes = pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed);
-	printf("\ttotal bytes scrubbed: %s with %llu errors\n", bytes,
+
+	printf("\ttotal bytes scrubbed: %s with %llu errors\n",
+		pretty_size(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
 		max(err_cnt, err_cnt2));
-	free(bytes);
+
 	if (err_cnt || err_cnt2) {
 		printf("\terror details:");
 		PRINT_SCRUB_ERROR(p->read_errors, "read");
diff --git a/mkfs.c b/mkfs.c
index 95fceb3..ade85c7 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1356,7 +1356,6 @@ int main(int ac, char **av)
 	u64 num_of_meta_chunks = 0;
 	u64 size_of_data = 0;
 	u64 source_dir_size = 0;
-	char *pretty_buf;
 	struct btrfs_super_block *super;
 	u64 flags;
 	int dev_cnt = 0;
@@ -1629,8 +1628,7 @@ raid_groups:
 	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
 	    "sectorsize %u size %s\n",
 	    label, first_file, nodesize, leafsize, sectorsize,
-	    pretty_buf = pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
-	free(pretty_buf);
+	    pretty_size(btrfs_super_total_bytes(root->fs_info->super_copy)));
 
 	printf("%s\n", BTRFS_BUILD_VERSION);
 	btrfs_commit_transaction(trans, root);
diff --git a/utils.c b/utils.c
index 1eeda0f..ced85aa 100644
--- a/utils.c
+++ b/utils.c
@@ -1152,13 +1152,14 @@ out:
 }
 
 static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
-			    "PB", "EB", "ZB", "YB"};
-char *pretty_sizes(u64 size)
+			    "PB", "EB"};
+void pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
 {
 	int num_divs = 0;
-        int pretty_len = 16;
 	float fraction;
-	char *pretty;
+
+	if (str_bytes == 0)
+		return;
 
 	if( size < 1024 ){
 		fraction = size;
@@ -1172,13 +1173,13 @@ char *pretty_sizes(u64 size)
 			num_divs ++;
 		}
 
-		if (num_divs >= ARRAY_SIZE(size_strs))
-			return NULL;
+		if (num_divs >= ARRAY_SIZE(size_strs)) {
+			str[0] = '\0';
+			return;
+		}
 		fraction = (float)last_size / 1024;
 	}
-	pretty = malloc(pretty_len);
-	snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs]);
-	return pretty;
+	snprintf(str, str_bytes, "%.2f%s", fraction, size_strs[num_divs]);
 }
 
 /*
diff --git a/utils.h b/utils.h
index 3c17e14..36fb591 100644
--- a/utils.h
+++ b/utils.h
@@ -44,7 +44,15 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 			struct btrfs_fs_devices **fs_devices_mnt);
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset);
-char *pretty_sizes(u64 size);
+
+void pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
+#define pretty_size(size) 						\
+	({								\
+		static __thread char _str[24];				\
+		pretty_size_snprintf((size), _str, sizeof(_str));	\
+		_str;							\
+	})
+
 int get_mountpt(char *dev, char *mntpt, size_t size);
 int btrfs_scan_block_devices(int run_ioctl);
 u64 parse_size(char *s);
-- 
1.8.2

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[Wang Shilong]

Hello David,

> From: Zach Brown <zab@redhat.com>
> 
duplicate information.

> From: Zach Brown <zab@redhat.com>
> 
> We don't need callers to manage string storage for each pretty_sizes()
> call.  We can use a macro to have per-thread and per-call static storage
> so that pretty_sizes() can be used as many times as needed in printf()
> arguments without requiring a bunch of supporting variables.
> 
> This lets us have a natural interface at the cost of requiring __thread
> and TLS from gcc and a small amount of static storage.  This seems
> better than the current code or doing something with illegible format
> specifier macros.
> 
> Signed-off-by: Zach Brown <zab@redhat.com>
> Signed-off-by: David Sterba <dsterba@suse.cz>

OK.  please add my tag: Acked-by: Wang Shilong <wangs.fnst@cn.fujitsu.com>
 (I have given my tag in the previous thread to Zach and cc to you!)

Zach gives a better solution,but i at least report and try for it. Isn't it?

Thanks
Wang

> ---
> 
> I've updated the rest of pretty_size callers in targets that were not built by
> default.
> 
> btrfs-calc-size.c | 13 +++----------
> btrfs-fragments.c |  2 +-
> cmds-filesystem.c | 27 +++++++++------------------
> cmds-scrub.c      |  8 ++++----
> mkfs.c            |  4 +---
> utils.c           | 19 ++++++++++---------
> utils.h           | 10 +++++++++-
> 7 files changed, 37 insertions(+), 46 deletions(-)
> 
> diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
> index c4adfb0..5aa0b70 100644
> --- a/btrfs-calc-size.c
> +++ b/btrfs-calc-size.c
> @@ -162,18 +162,11 @@ out_print:
> 		       stat.total_inline, stat.total_nodes, stat.total_leaves,
> 		       level + 1);
> 	} else {
> -		char *total_size;
> -		char *inline_size;
> -
> -		total_size = pretty_sizes(stat.total_bytes);
> -		inline_size = pretty_sizes(stat.total_inline);
> -
> 		printf("\t%s total size, %s inline data, %Lu nodes, "
> 		       "%Lu leaves, %d levels\n",
> -		       total_size, inline_size, stat.total_nodes,
> -		       stat.total_leaves, level + 1);
> -		free(total_size);
> -		free(inline_size);
> +		       pretty_size(stat.total_bytes),
> +		       pretty_size(stat.total_inline),
> +		       stat.total_nodes, stat.total_leaves, level + 1);
> 	}
> out:
> 	btrfs_free_path(path);
> diff --git a/btrfs-fragments.c b/btrfs-fragments.c
> index a012fe1..7ec77e7 100644
> --- a/btrfs-fragments.c
> +++ b/btrfs-fragments.c
> @@ -87,7 +87,7 @@ print_bg(FILE *html, char *name, u64 start, u64 len, u64 used, u64 flags,
> 
> 	fprintf(html, "<p>%s chunk starts at %lld, size is %s, %.2f%% used, "
> 		      "%.2f%% fragmented</p>\n", chunk_type(flags), start,
> -		      pretty_sizes(len), 100.0 * used / len, 100.0 * frag);
> +		      pretty_size(len), 100.0 * used / len, 100.0 * frag);
> 	fprintf(html, "<img src=\"%s\" border=\"1\" />\n", name);
> }
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index f41a72a..222e458 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -111,8 +111,6 @@ static int cmd_df(int argc, char **argv)
> 
> 	for (i = 0; i < sargs->total_spaces; i++) {
> 		char description[80];
> -		char *total_bytes;
> -		char *used_bytes;
> 		int written = 0;
> 		u64 flags = sargs->spaces[i].flags;
> 
> @@ -155,10 +153,9 @@ static int cmd_df(int argc, char **argv)
> 			written += 7;
> 		}
> 
> -		total_bytes = pretty_sizes(sargs->spaces[i].total_bytes);
> -		used_bytes = pretty_sizes(sargs->spaces[i].used_bytes);
> -		printf("%s: total=%s, used=%s\n", description, total_bytes,
> -		       used_bytes);
> +		printf("%s: total=%s, used=%s\n", description,
> +			pretty_size(sargs->spaces[i].total_bytes),
> +			pretty_size(sargs->spaces[i].used_bytes));
> 	}
> 	close(fd);
> 	free(sargs);
> @@ -192,7 +189,6 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
> 	char uuidbuf[37];
> 	struct list_head *cur;
> 	struct btrfs_device *device;
> -	char *super_bytes_used;
> 	u64 devs_found = 0;
> 	u64 total;
> 
> @@ -204,25 +200,20 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
> 	else
> 		printf("Label: none ");
> 
> -	super_bytes_used = pretty_sizes(device->super_bytes_used);
> 
> 	total = device->total_devs;
> 	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
> -	       (unsigned long long)total, super_bytes_used);
> -
> -	free(super_bytes_used);
> +	       (unsigned long long)total,
> +	       pretty_size(device->super_bytes_used));
> 
> 	list_for_each(cur, &fs_devices->devices) {
> -		char *total_bytes;
> -		char *bytes_used;
> 		device = list_entry(cur, struct btrfs_device, dev_list);
> -		total_bytes = pretty_sizes(device->total_bytes);
> -		bytes_used = pretty_sizes(device->bytes_used);
> +
> 		printf("\tdevid %4llu size %s used %s path %s\n",
> 		       (unsigned long long)device->devid,
> -		       total_bytes, bytes_used, device->name);
> -		free(total_bytes);
> -		free(bytes_used);
> +		       pretty_size(device->total_bytes),
> +		       pretty_size(device->bytes_used), device->name);
> +
> 		devs_found++;
> 	}
> 	if (devs_found < total) {
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index 95dfee3..bf50650 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -139,7 +139,6 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
> {
> 	u64 err_cnt;
> 	u64 err_cnt2;
> -	char *bytes;
> 
> 	err_cnt = p->read_errors +
> 			p->csum_errors +
> @@ -151,10 +150,11 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
> 	if (p->malloc_errors)
> 		printf("*** WARNING: memory allocation failed while scrubbing. "
> 		       "results may be inaccurate\n");
> -	bytes = pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed);
> -	printf("\ttotal bytes scrubbed: %s with %llu errors\n", bytes,
> +
> +	printf("\ttotal bytes scrubbed: %s with %llu errors\n",
> +		pretty_size(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
> 		max(err_cnt, err_cnt2));
> -	free(bytes);
> +
> 	if (err_cnt || err_cnt2) {
> 		printf("\terror details:");
> 		PRINT_SCRUB_ERROR(p->read_errors, "read");
> diff --git a/mkfs.c b/mkfs.c
> index 95fceb3..ade85c7 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -1356,7 +1356,6 @@ int main(int ac, char **av)
> 	u64 num_of_meta_chunks = 0;
> 	u64 size_of_data = 0;
> 	u64 source_dir_size = 0;
> -	char *pretty_buf;
> 	struct btrfs_super_block *super;
> 	u64 flags;
> 	int dev_cnt = 0;
> @@ -1629,8 +1628,7 @@ raid_groups:
> 	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
> 	    "sectorsize %u size %s\n",
> 	    label, first_file, nodesize, leafsize, sectorsize,
> -	    pretty_buf = pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
> -	free(pretty_buf);
> +	    pretty_size(btrfs_super_total_bytes(root->fs_info->super_copy)));
> 
> 	printf("%s\n", BTRFS_BUILD_VERSION);
> 	btrfs_commit_transaction(trans, root);
> diff --git a/utils.c b/utils.c
> index 1eeda0f..ced85aa 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1152,13 +1152,14 @@ out:
> }
> 
> static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
> -			    "PB", "EB", "ZB", "YB"};
> -char *pretty_sizes(u64 size)
> +			    "PB", "EB"};
> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
> {
> 	int num_divs = 0;
> -        int pretty_len = 16;
> 	float fraction;
> -	char *pretty;
> +
> +	if (str_bytes == 0)
> +		return;
> 
> 	if( size < 1024 ){
> 		fraction = size;
> @@ -1172,13 +1173,13 @@ char *pretty_sizes(u64 size)
> 			num_divs ++;
> 		}
> 
> -		if (num_divs >= ARRAY_SIZE(size_strs))
> -			return NULL;
> +		if (num_divs >= ARRAY_SIZE(size_strs)) {
> +			str[0] = '\0';
> +			return;
> +		}
> 		fraction = (float)last_size / 1024;
> 	}
> -	pretty = malloc(pretty_len);
> -	snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs]);
> -	return pretty;
> +	snprintf(str, str_bytes, "%.2f%s", fraction, size_strs[num_divs]);
> }
> 
> /*
> diff --git a/utils.h b/utils.h
> index 3c17e14..36fb591 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -44,7 +44,15 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
> 			struct btrfs_fs_devices **fs_devices_mnt);
> int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
> 				 int super_offset);
> -char *pretty_sizes(u64 size);
> +
> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
> +#define pretty_size(size) 						\
> +	({								\
> +		static __thread char _str[24];				\
> +		pretty_size_snprintf((size), _str, sizeof(_str));	\
> +		_str;							\
> +	})
> +
> int get_mountpt(char *dev, char *mntpt, size_t size);
> int btrfs_scan_block_devices(int run_ioctl);
> u64 parse_size(char *s);
> -- 
> 1.8.2
> 

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[David Sterba]

On Wed, Jul 10, 2013 at 11:31:17PM +0800, Wang Shilong wrote:
> Hello David,
> 
> > From: Zach Brown <zab@redhat.com>
> > 
> duplicate information.

git-send-email tricked me, the line is not present in thre tree

> 
> > From: Zach Brown <zab@redhat.com>
> > 
> > We don't need callers to manage string storage for each pretty_sizes()
> > call.  We can use a macro to have per-thread and per-call static storage
> > so that pretty_sizes() can be used as many times as needed in printf()
> > arguments without requiring a bunch of supporting variables.
> > 
> > This lets us have a natural interface at the cost of requiring __thread
> > and TLS from gcc and a small amount of static storage.  This seems
> > better than the current code or doing something with illegible format
> > specifier macros.
> > 
> > Signed-off-by: Zach Brown <zab@redhat.com>
> > Signed-off-by: David Sterba <dsterba@suse.cz>
> 
> OK.  please add my tag: Acked-by: Wang Shilong <wangs.fnst@cn.fujitsu.com>
>  (I have given my tag in the previous thread to Zach and cc to you!)
> 
> Zach gives a better solution,but i at least report and try for it. Isn't it?

Oh sorry, I'll add the tag of course, I was so excited with zach's patch
and missed it.

david

Re: [PATCH V2 1/2] Btrfs-progs: make pretty_sizes() work less error prone

[Zach Brown]

> Neat trick! A few neat-picks below.

Indeed, those are all good fixes.

> As these are only trivial changes I'll fix them at commit time.

Great, thanks David!

- z

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

   Sorry to be a pain in the arse at this late stage of the patch, but
I've only just noticed.

On Wed, Jul 10, 2013 at 04:30:15PM +0200, David Sterba wrote:
>  static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
> -			    "PB", "EB", "ZB", "YB"};
> -char *pretty_sizes(u64 size)
> +			    "PB", "EB"};

   These are SI (power of 10) prefixes...

> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
>  {
>  	int num_divs = 0;
> -        int pretty_len = 16;
>  	float fraction;
> -	char *pretty;
> +
> +	if (str_bytes == 0)
> +		return;
>  
>  	if( size < 1024 ){
>  		fraction = size;
> @@ -1172,13 +1173,13 @@ char *pretty_sizes(u64 size)
>  			num_divs ++;
>  		}
>  
> -		if (num_divs >= ARRAY_SIZE(size_strs))
> -			return NULL;
> +		if (num_divs >= ARRAY_SIZE(size_strs)) {
> +			str[0] = '\0';
> +			return;
> +		}
>  		fraction = (float)last_size / 1024;

   ... and this is working in IEC (power of 2) units.

   Can we fix this discrepancy, please? Also note that SI uses k for
10^3, but IEC uses K for 2^10. Just insert an "i" in the middle of
each element of size_strs should deal with the problem.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
   --- Charting the inexorable advance of Western syphilisation... ---   

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[David Sterba]

On Wed, Jul 10, 2013 at 05:16:23PM +0100, Hugo Mills wrote:
>    Sorry to be a pain in the arse at this late stage of the patch, but
> I've only just noticed.

No worries, good to have this one fixed.

david

[PATCH] btrfs-progs: use IEC units for sizes

[David Sterba]

As implemented now, we use 1024 based units but reporting 1000 based,
let's finally fix that and add optional unit bases later.

Signed-off-by: David Sterba <dsterba@suse.cz>
---
 utils.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/utils.c b/utils.c
index bce06f1..2e24cb0 100644
--- a/utils.c
+++ b/utils.c
@@ -1173,8 +1173,7 @@ out:
 	return ret;
 }
 
-static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
-			    "PB", "EB"};
+static char *size_strs[] = { "", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"};
 void pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
 {
 	int num_divs = 0;
-- 
1.8.2

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[Anand Jain]

 > +		static __thread char _str[24];

  This patch is causing segmentation fault as it reached maximum stack
  depth on the sparc machine. Just earlier method of malloc is better ?
  unless we want to change the stack depth.  Any comments ?

Thanks, Anand


On 07/10/2013 10:30 PM, David Sterba wrote:
> From: Zach Brown <zab@redhat.com>
>
> From: Zach Brown <zab@redhat.com>
>
> We don't need callers to manage string storage for each pretty_sizes()
> call.  We can use a macro to have per-thread and per-call static storage
> so that pretty_sizes() can be used as many times as needed in printf()
> arguments without requiring a bunch of supporting variables.
>
> This lets us have a natural interface at the cost of requiring __thread
> and TLS from gcc and a small amount of static storage.  This seems
> better than the current code or doing something with illegible format
> specifier macros.
>
> Signed-off-by: Zach Brown <zab@redhat.com>
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
>
> I've updated the rest of pretty_size callers in targets that were not built by
> default.
>
>   btrfs-calc-size.c | 13 +++----------
>   btrfs-fragments.c |  2 +-
>   cmds-filesystem.c | 27 +++++++++------------------
>   cmds-scrub.c      |  8 ++++----
>   mkfs.c            |  4 +---
>   utils.c           | 19 ++++++++++---------
>   utils.h           | 10 +++++++++-
>   7 files changed, 37 insertions(+), 46 deletions(-)
>
> diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
> index c4adfb0..5aa0b70 100644
> --- a/btrfs-calc-size.c
> +++ b/btrfs-calc-size.c
> @@ -162,18 +162,11 @@ out_print:
>   		       stat.total_inline, stat.total_nodes, stat.total_leaves,
>   		       level + 1);
>   	} else {
> -		char *total_size;
> -		char *inline_size;
> -
> -		total_size = pretty_sizes(stat.total_bytes);
> -		inline_size = pretty_sizes(stat.total_inline);
> -
>   		printf("\t%s total size, %s inline data, %Lu nodes, "
>   		       "%Lu leaves, %d levels\n",
> -		       total_size, inline_size, stat.total_nodes,
> -		       stat.total_leaves, level + 1);
> -		free(total_size);
> -		free(inline_size);
> +		       pretty_size(stat.total_bytes),
> +		       pretty_size(stat.total_inline),
> +		       stat.total_nodes, stat.total_leaves, level + 1);
>   	}
>   out:
>   	btrfs_free_path(path);
> diff --git a/btrfs-fragments.c b/btrfs-fragments.c
> index a012fe1..7ec77e7 100644
> --- a/btrfs-fragments.c
> +++ b/btrfs-fragments.c
> @@ -87,7 +87,7 @@ print_bg(FILE *html, char *name, u64 start, u64 len, u64 used, u64 flags,
>
>   	fprintf(html, "<p>%s chunk starts at %lld, size is %s, %.2f%% used, "
>   		      "%.2f%% fragmented</p>\n", chunk_type(flags), start,
> -		      pretty_sizes(len), 100.0 * used / len, 100.0 * frag);
> +		      pretty_size(len), 100.0 * used / len, 100.0 * frag);
>   	fprintf(html, "<img src=\"%s\" border=\"1\" />\n", name);
>   }
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index f41a72a..222e458 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -111,8 +111,6 @@ static int cmd_df(int argc, char **argv)
>
>   	for (i = 0; i < sargs->total_spaces; i++) {
>   		char description[80];
> -		char *total_bytes;
> -		char *used_bytes;
>   		int written = 0;
>   		u64 flags = sargs->spaces[i].flags;
>
> @@ -155,10 +153,9 @@ static int cmd_df(int argc, char **argv)
>   			written += 7;
>   		}
>
> -		total_bytes = pretty_sizes(sargs->spaces[i].total_bytes);
> -		used_bytes = pretty_sizes(sargs->spaces[i].used_bytes);
> -		printf("%s: total=%s, used=%s\n", description, total_bytes,
> -		       used_bytes);
> +		printf("%s: total=%s, used=%s\n", description,
> +			pretty_size(sargs->spaces[i].total_bytes),
> +			pretty_size(sargs->spaces[i].used_bytes));
>   	}
>   	close(fd);
>   	free(sargs);
> @@ -192,7 +189,6 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
>   	char uuidbuf[37];
>   	struct list_head *cur;
>   	struct btrfs_device *device;
> -	char *super_bytes_used;
>   	u64 devs_found = 0;
>   	u64 total;
>
> @@ -204,25 +200,20 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
>   	else
>   		printf("Label: none ");
>
> -	super_bytes_used = pretty_sizes(device->super_bytes_used);
>
>   	total = device->total_devs;
>   	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
> -	       (unsigned long long)total, super_bytes_used);
> -
> -	free(super_bytes_used);
> +	       (unsigned long long)total,
> +	       pretty_size(device->super_bytes_used));
>
>   	list_for_each(cur, &fs_devices->devices) {
> -		char *total_bytes;
> -		char *bytes_used;
>   		device = list_entry(cur, struct btrfs_device, dev_list);
> -		total_bytes = pretty_sizes(device->total_bytes);
> -		bytes_used = pretty_sizes(device->bytes_used);
> +
>   		printf("\tdevid %4llu size %s used %s path %s\n",
>   		       (unsigned long long)device->devid,
> -		       total_bytes, bytes_used, device->name);
> -		free(total_bytes);
> -		free(bytes_used);
> +		       pretty_size(device->total_bytes),
> +		       pretty_size(device->bytes_used), device->name);
> +
>   		devs_found++;
>   	}
>   	if (devs_found < total) {
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index 95dfee3..bf50650 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -139,7 +139,6 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
>   {
>   	u64 err_cnt;
>   	u64 err_cnt2;
> -	char *bytes;
>
>   	err_cnt = p->read_errors +
>   			p->csum_errors +
> @@ -151,10 +150,11 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
>   	if (p->malloc_errors)
>   		printf("*** WARNING: memory allocation failed while scrubbing. "
>   		       "results may be inaccurate\n");
> -	bytes = pretty_sizes(p->data_bytes_scrubbed + p->tree_bytes_scrubbed);
> -	printf("\ttotal bytes scrubbed: %s with %llu errors\n", bytes,
> +
> +	printf("\ttotal bytes scrubbed: %s with %llu errors\n",
> +		pretty_size(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
>   		max(err_cnt, err_cnt2));
> -	free(bytes);
> +
>   	if (err_cnt || err_cnt2) {
>   		printf("\terror details:");
>   		PRINT_SCRUB_ERROR(p->read_errors, "read");
> diff --git a/mkfs.c b/mkfs.c
> index 95fceb3..ade85c7 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -1356,7 +1356,6 @@ int main(int ac, char **av)
>   	u64 num_of_meta_chunks = 0;
>   	u64 size_of_data = 0;
>   	u64 source_dir_size = 0;
> -	char *pretty_buf;
>   	struct btrfs_super_block *super;
>   	u64 flags;
>   	int dev_cnt = 0;
> @@ -1629,8 +1628,7 @@ raid_groups:
>   	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
>   	    "sectorsize %u size %s\n",
>   	    label, first_file, nodesize, leafsize, sectorsize,
> -	    pretty_buf = pretty_sizes(btrfs_super_total_bytes(root->fs_info->super_copy)));
> -	free(pretty_buf);
> +	    pretty_size(btrfs_super_total_bytes(root->fs_info->super_copy)));
>
>   	printf("%s\n", BTRFS_BUILD_VERSION);
>   	btrfs_commit_transaction(trans, root);
> diff --git a/utils.c b/utils.c
> index 1eeda0f..ced85aa 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1152,13 +1152,14 @@ out:
>   }
>
>   static char *size_strs[] = { "", "KB", "MB", "GB", "TB",
> -			    "PB", "EB", "ZB", "YB"};
> -char *pretty_sizes(u64 size)
> +			    "PB", "EB"};
> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
>   {
>   	int num_divs = 0;
> -        int pretty_len = 16;
>   	float fraction;
> -	char *pretty;
> +
> +	if (str_bytes == 0)
> +		return;
>
>   	if( size < 1024 ){
>   		fraction = size;
> @@ -1172,13 +1173,13 @@ char *pretty_sizes(u64 size)
>   			num_divs ++;
>   		}
>
> -		if (num_divs >= ARRAY_SIZE(size_strs))
> -			return NULL;
> +		if (num_divs >= ARRAY_SIZE(size_strs)) {
> +			str[0] = '\0';
> +			return;
> +		}
>   		fraction = (float)last_size / 1024;
>   	}
> -	pretty = malloc(pretty_len);
> -	snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs]);
> -	return pretty;
> +	snprintf(str, str_bytes, "%.2f%s", fraction, size_strs[num_divs]);
>   }
>
>   /*
> diff --git a/utils.h b/utils.h
> index 3c17e14..36fb591 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -44,7 +44,15 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
>   			struct btrfs_fs_devices **fs_devices_mnt);
>   int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
>   				 int super_offset);
> -char *pretty_sizes(u64 size);
> +
> +void pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
> +#define pretty_size(size) 						\
> +	({								\
> +		static __thread char _str[24];				\
> +		pretty_size_snprintf((size), _str, sizeof(_str));	\
> +		_str;							\
> +	})
> +
>   int get_mountpt(char *dev, char *mntpt, size_t size);
>   int btrfs_scan_block_devices(int run_ioctl);
>   u64 parse_size(char *s);
>

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[Zach Brown]

On Thu, Sep 04, 2014 at 07:43:08PM +0800, Anand Jain wrote:
> 
> 
> > +		static __thread char _str[24];
> 
>  This patch is causing segmentation fault as it reached maximum stack
>  depth on the sparc machine.

Sigh.  I guess it was inevitable that this would fail somewhere :).

> Just earlier method of malloc is better ?

No. Callers having to allocate per-argument buffers was insane. 

>  unless we want to change the stack depth.

Prooobably not.

gcc still hasn't learned about registered format specifiers so we don't
want to do that.

How about just going back to the idea of using boring old macros for the
format and args?  I kind of remember that being discussed but I don't
remember why we didn't go that route.

Something like this, anyway..

(I even rememebered to fix the binaries that are mysteriously not built
by default because reasons!  Yeah!)

- z

>From 3d132362f4c87b065b63cb38726a030db2277919 Mon Sep 17 00:00:00 2001
From: Zach Brown <zab@zabbo.net>
Date: Thu, 4 Sep 2014 12:32:00 -0700
Subject: [PATCH] btrfs-progs: use pretty printing macros

The original pretty printing code was a mess and required callers to
allocate and free buffers for each argument.  The obvious fix would be
to use glibc's dynamic format specifier registration, but gcc has yet to
learn how to parse them when checking formats.  It'd spew unknown
specifier warnings if we add a pretty printing specifier.

So you'd think we'd just use macros like everyone else.  I don't
remember the details now, but it seemed that people weren't excited by
that. So we rolled some silly thread-local buffer for the pretty string
for each argument.

But that balloons the stack and crashes on sparc.. sure, fine.

So we can go back to the dumb macros that are easy and work.

Signed-off-by: Zach Brown <zab@zabbo.net>
---
 btrfs-calc-size.c | 30 +++++++++++++++---------------
 btrfs-fragments.c |  4 ++--
 cmds-filesystem.c | 26 +++++++++++++-------------
 cmds-scrub.c      |  4 ++--
 mkfs.c            |  4 ++--
 utils.c           | 35 ++++++++++++++++++++++++-----------
 utils.h           | 18 +++++++++++-------
 7 files changed, 69 insertions(+), 52 deletions(-)

diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
index 501111c..bea7c81 100644
--- a/btrfs-calc-size.c
+++ b/btrfs-calc-size.c
@@ -324,6 +324,7 @@ static int calc_root_size(struct btrfs_root *tree_root, struct btrfs_key *key,
 	int level;
 	int ret = 0;
 	int size_fail = 0;
+	u64 avg;
 
 	root = btrfs_read_fs_root(tree_root->fs_info, key);
 	if (IS_ERR(root)) {
@@ -369,14 +370,15 @@ out_print:
 		stat.total_clusters = 1;
 	}
 
+	avg = stat.total_seeks ? stat.total_seek_len / stat.total_seeks : 0;
+
 	if (no_pretty || size_fail) {
 		printf("\tTotal size: %Lu\n", stat.total_bytes);
 		printf("\t\tInline data: %Lu\n", stat.total_inline);
 		printf("\tTotal seeks: %Lu\n", stat.total_seeks);
 		printf("\t\tForward seeks: %Lu\n", stat.forward_seeks);
 		printf("\t\tBackward seeks: %Lu\n", stat.backward_seeks);
-		printf("\t\tAvg seek len: %Lu\n", stat.total_seek_len /
-		       stat.total_seeks);
+		printf("\t\tAvg seek len: %Lu\n", avg);
 		print_seek_histogram(&stat);
 		printf("\tTotal clusters: %Lu\n", stat.total_clusters);
 		printf("\t\tAvg cluster size: %Lu\n", stat.total_cluster_size /
@@ -389,25 +391,23 @@ out_print:
 		       (int)diff.tv_usec);
 		printf("\tLevels: %d\n", level + 1);
 	} else {
-		printf("\tTotal size: %s\n", pretty_size(stat.total_bytes));
-		printf("\t\tInline data: %s\n", pretty_size(stat.total_inline));
+		printf("\tTotal size: "PF"\n", PA(stat.total_bytes));
+		printf("\t\tInline data: "PF"\n", PA(stat.total_inline));
 		printf("\tTotal seeks: %Lu\n", stat.total_seeks);
 		printf("\t\tForward seeks: %Lu\n", stat.forward_seeks);
 		printf("\t\tBackward seeks: %Lu\n", stat.backward_seeks);
-		printf("\t\tAvg seek len: %s\n", stat.total_seeks ?
-			pretty_size(stat.total_seek_len / stat.total_seeks) :
-			pretty_size(0));
+		printf("\t\tAvg seek len: "PF"\n", PA(avg));
 		print_seek_histogram(&stat);
 		printf("\tTotal clusters: %Lu\n", stat.total_clusters);
-		printf("\t\tAvg cluster size: %s\n",
-				pretty_size((stat.total_cluster_size /
+		printf("\t\tAvg cluster size: "PF"\n",
+				PA((stat.total_cluster_size /
 						stat.total_clusters)));
-		printf("\t\tMin cluster size: %s\n",
-				pretty_size(stat.min_cluster_size));
-		printf("\t\tMax cluster size: %s\n",
-				pretty_size(stat.max_cluster_size));
-		printf("\tTotal disk spread: %s\n",
-				pretty_size(stat.highest_bytenr -
+		printf("\t\tMin cluster size: "PF"\n",
+				PA(stat.min_cluster_size));
+		printf("\t\tMax cluster size: "PF"\n",
+				PA(stat.max_cluster_size));
+		printf("\tTotal disk spread: "PF"\n",
+				PA(stat.highest_bytenr -
 					stat.lowest_bytenr));
 		printf("\tTotal read time: %d s %d us\n", (int)diff.tv_sec,
 		       (int)diff.tv_usec);
diff --git a/btrfs-fragments.c b/btrfs-fragments.c
index d03c2c3..fd45822 100644
--- a/btrfs-fragments.c
+++ b/btrfs-fragments.c
@@ -85,9 +85,9 @@ print_bg(FILE *html, char *name, u64 start, u64 len, u64 used, u64 flags,
 {
 	double frag = (double)areas / (len / 4096) * 2;
 
-	fprintf(html, "<p>%s chunk starts at %lld, size is %s, %.2f%% used, "
+	fprintf(html, "<p>%s chunk starts at %lld, size is "PF", %.2f%% used, "
 		      "%.2f%% fragmented</p>\n", chunk_type(flags), start,
-		      pretty_size(len), 100.0 * used / len, 100.0 * frag);
+		      PA(len), 100.0 * used / len, 100.0 * frag);
 	fprintf(html, "<img src=\"%s\" border=\"1\" />\n", name);
 }
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 7e8ca95..d8b6938 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -210,11 +210,11 @@ static void print_df(struct btrfs_ioctl_space_args *sargs)
 	struct btrfs_ioctl_space_info *sp = sargs->spaces;
 
 	for (i = 0; i < sargs->total_spaces; i++, sp++) {
-		printf("%s, %s: total=%s, used=%s\n",
+		printf("%s, %s: total="PF", used="PF"\n",
 			group_type_str(sp->flags),
 			group_profile_str(sp->flags),
-			pretty_size(sp->total_bytes),
-			pretty_size(sp->used_bytes));
+			PA(sp->total_bytes),
+			PA(sp->used_bytes));
 	}
 }
 
@@ -326,18 +326,18 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
 
 
 	total = device->total_devs;
-	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
+	printf(" uuid: %s\n\tTotal devices %llu FS bytes used "PF"\n", uuidbuf,
 	       (unsigned long long)total,
-	       pretty_size(device->super_bytes_used));
+	       PA(device->super_bytes_used));
 
 	list_sort(NULL, &fs_devices->devices, cmp_device_id);
 	list_for_each(cur, &fs_devices->devices) {
 		device = list_entry(cur, struct btrfs_device, dev_list);
 
-		printf("\tdevid %4llu size %s used %s path %s\n",
+		printf("\tdevid %4llu size "PF" used "PF" path %s\n",
 		       (unsigned long long)device->devid,
-		       pretty_size(device->total_bytes),
-		       pretty_size(device->bytes_used), device->name);
+		       PA(device->total_bytes),
+		       PA(device->bytes_used), device->name);
 
 		devs_found++;
 	}
@@ -382,9 +382,9 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
 	else
 		printf("Label: none ");
 
-	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
+	printf(" uuid: %s\n\tTotal devices %llu FS bytes used "PF"\n", uuidbuf,
 			fs_info->num_devices,
-			pretty_size(calc_used_bytes(space_info)));
+			PA(calc_used_bytes(space_info)));
 
 	for (i = 0; i < fs_info->num_devices; i++) {
 		tmp_dev_info = (struct btrfs_ioctl_dev_info_args *)&dev_info[i];
@@ -396,10 +396,10 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
 			continue;
 		}
 		close(fd);
-		printf("\tdevid %4llu size %s used %s path %s\n",
+		printf("\tdevid %4llu size "PF" used "PF" path %s\n",
 			tmp_dev_info->devid,
-			pretty_size(tmp_dev_info->total_bytes),
-			pretty_size(tmp_dev_info->bytes_used),
+			PA(tmp_dev_info->total_bytes),
+			PA(tmp_dev_info->bytes_used),
 			tmp_dev_info->path);
 	}
 
diff --git a/cmds-scrub.c b/cmds-scrub.c
index 731c5c9..10a6692 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -151,8 +151,8 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
 		printf("*** WARNING: memory allocation failed while scrubbing. "
 		       "results may be inaccurate\n");
 
-	printf("\ttotal bytes scrubbed: %s with %llu errors\n",
-		pretty_size(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
+	printf("\ttotal bytes scrubbed: "PF" with %llu errors\n",
+		PA(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
 		max(err_cnt, err_cnt2));
 
 	if (err_cnt || err_cnt2) {
diff --git a/mkfs.c b/mkfs.c
index 9de61e1..d649e03 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1643,9 +1643,9 @@ raid_groups:
 	BUG_ON(ret);
 
 	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
-	    "sectorsize %u size %s\n",
+	    "sectorsize %u size "PF"\n",
 	    label, first_file, nodesize, leafsize, sectorsize,
-	    pretty_size(btrfs_super_total_bytes(root->fs_info->super_copy)));
+	    PA(btrfs_super_total_bytes(root->fs_info->super_copy)));
 
 	btrfs_commit_transaction(trans, root);
 
diff --git a/utils.c b/utils.c
index 6c09366..0064587 100644
--- a/utils.c
+++ b/utils.c
@@ -709,8 +709,8 @@ int btrfs_prepare_device(int fd, char *file, int zero_end, u64 *block_count_ret,
 		 * optimization.
 		 */
 		if (discard_range(fd, 0, 0) == 0) {
-			fprintf(stderr, "Performing full device TRIM (%s) ...\n",
-				pretty_size(block_count));
+			fprintf(stderr, "Performing full device TRIM ("PF") ...\n",
+				PA(block_count));
 			discard_blocks(fd, 0, block_count);
 		}
 	}
@@ -1376,22 +1376,21 @@ out:
 	return ret;
 }
 
-static char *size_strs[] = { "", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"};
-int pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
+static float pretty_calc(u64 size, char **str)
 {
 	int num_divs = 0;
 	float fraction;
+	static char *size_strs[] = {
+		"", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB",
+	};
 
-	if (str_bytes == 0)
-		return 0;
-
-	if( size < 1024 ){
+	if (size < 1024) {
 		fraction = size;
 		num_divs = 0;
 	} else {
 		u64 last_size = size;
 		num_divs = 0;
-		while(size >= 1024){
+		while (size >= 1024) {
 			last_size = size;
 			size /= 1024;
 			num_divs ++;
@@ -1403,8 +1402,22 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
 		}
 		fraction = (float)last_size / 1024;
 	}
-	return snprintf(str, str_bytes, "%.2f%s", fraction,
-			size_strs[num_divs]);
+
+	*str = size_strs[num_divs];
+	return fraction;
+}
+
+float pretty_float(u64 size)
+{
+	char *str;
+	return pretty_calc(size, &str);
+}
+
+char *pretty_suffix(u64 size)
+{
+	char *str;
+	pretty_calc(size, &str);
+	return str;
 }
 
 /*
diff --git a/utils.h b/utils.h
index fd25126..26c767b 100644
--- a/utils.h
+++ b/utils.h
@@ -71,13 +71,17 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset);
 
-int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
-#define pretty_size(size) 						\
-	({								\
-		static __thread char _str[24];				\
-		(void)pretty_size_snprintf((size), _str, sizeof(_str));	\
-		_str;							\
-	})
+/*
+ * It's annoying that gcc hasn't yet figured out how to learn about
+ * formats added by register_printf_specifier().  If that were the case
+ * we could just add some %P type and be done with it.  Some day..
+ *
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47781
+ */
+float pretty_float(u64 size);
+char *pretty_suffix(u64 size);
+#define PF "%.2f%s"
+#define PA(x) pretty_float(x), pretty_suffix(x)
 
 int get_mountpt(char *dev, char *mntpt, size_t size);
 int btrfs_scan_block_devices(int run_ioctl);
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[Anand Jain]

  Great! Thanks Zach for your quick patch. it works.

Anand

On 05/09/2014 03:45, Zach Brown wrote:
> On Thu, Sep 04, 2014 at 07:43:08PM +0800, Anand Jain wrote:
>>
>>
>>> +		static __thread char _str[24];
>>
>>   This patch is causing segmentation fault as it reached maximum stack
>>   depth on the sparc machine.
>
> Sigh.  I guess it was inevitable that this would fail somewhere :).
>
>> Just earlier method of malloc is better ?
>
> No. Callers having to allocate per-argument buffers was insane.
>
>>   unless we want to change the stack depth.
>
> Prooobably not.
>
> gcc still hasn't learned about registered format specifiers so we don't
> want to do that.
>
> How about just going back to the idea of using boring old macros for the
> format and args?  I kind of remember that being discussed but I don't
> remember why we didn't go that route.
>
> Something like this, anyway..
>
> (I even rememebered to fix the binaries that are mysteriously not built
> by default because reasons!  Yeah!)
>
> - z
>
>  From 3d132362f4c87b065b63cb38726a030db2277919 Mon Sep 17 00:00:00 2001
> From: Zach Brown <zab@zabbo.net>
> Date: Thu, 4 Sep 2014 12:32:00 -0700
> Subject: [PATCH] btrfs-progs: use pretty printing macros
>
> The original pretty printing code was a mess and required callers to
> allocate and free buffers for each argument.  The obvious fix would be
> to use glibc's dynamic format specifier registration, but gcc has yet to
> learn how to parse them when checking formats.  It'd spew unknown
> specifier warnings if we add a pretty printing specifier.
>
> So you'd think we'd just use macros like everyone else.  I don't
> remember the details now, but it seemed that people weren't excited by
> that. So we rolled some silly thread-local buffer for the pretty string
> for each argument.
>
> But that balloons the stack and crashes on sparc.. sure, fine.
>
> So we can go back to the dumb macros that are easy and work.
>
> Signed-off-by: Zach Brown <zab@zabbo.net>
> ---
>   btrfs-calc-size.c | 30 +++++++++++++++---------------
>   btrfs-fragments.c |  4 ++--
>   cmds-filesystem.c | 26 +++++++++++++-------------
>   cmds-scrub.c      |  4 ++--
>   mkfs.c            |  4 ++--
>   utils.c           | 35 ++++++++++++++++++++++++-----------
>   utils.h           | 18 +++++++++++-------
>   7 files changed, 69 insertions(+), 52 deletions(-)
>
> diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
> index 501111c..bea7c81 100644
> --- a/btrfs-calc-size.c
> +++ b/btrfs-calc-size.c
> @@ -324,6 +324,7 @@ static int calc_root_size(struct btrfs_root *tree_root, struct btrfs_key *key,
>   	int level;
>   	int ret = 0;
>   	int size_fail = 0;
> +	u64 avg;
>
>   	root = btrfs_read_fs_root(tree_root->fs_info, key);
>   	if (IS_ERR(root)) {
> @@ -369,14 +370,15 @@ out_print:
>   		stat.total_clusters = 1;
>   	}
>
> +	avg = stat.total_seeks ? stat.total_seek_len / stat.total_seeks : 0;
> +
>   	if (no_pretty || size_fail) {
>   		printf("\tTotal size: %Lu\n", stat.total_bytes);
>   		printf("\t\tInline data: %Lu\n", stat.total_inline);
>   		printf("\tTotal seeks: %Lu\n", stat.total_seeks);
>   		printf("\t\tForward seeks: %Lu\n", stat.forward_seeks);
>   		printf("\t\tBackward seeks: %Lu\n", stat.backward_seeks);
> -		printf("\t\tAvg seek len: %Lu\n", stat.total_seek_len /
> -		       stat.total_seeks);
> +		printf("\t\tAvg seek len: %Lu\n", avg);
>   		print_seek_histogram(&stat);
>   		printf("\tTotal clusters: %Lu\n", stat.total_clusters);
>   		printf("\t\tAvg cluster size: %Lu\n", stat.total_cluster_size /
> @@ -389,25 +391,23 @@ out_print:
>   		       (int)diff.tv_usec);
>   		printf("\tLevels: %d\n", level + 1);
>   	} else {
> -		printf("\tTotal size: %s\n", pretty_size(stat.total_bytes));
> -		printf("\t\tInline data: %s\n", pretty_size(stat.total_inline));
> +		printf("\tTotal size: "PF"\n", PA(stat.total_bytes));
> +		printf("\t\tInline data: "PF"\n", PA(stat.total_inline));
>   		printf("\tTotal seeks: %Lu\n", stat.total_seeks);
>   		printf("\t\tForward seeks: %Lu\n", stat.forward_seeks);
>   		printf("\t\tBackward seeks: %Lu\n", stat.backward_seeks);
> -		printf("\t\tAvg seek len: %s\n", stat.total_seeks ?
> -			pretty_size(stat.total_seek_len / stat.total_seeks) :
> -			pretty_size(0));
> +		printf("\t\tAvg seek len: "PF"\n", PA(avg));
>   		print_seek_histogram(&stat);
>   		printf("\tTotal clusters: %Lu\n", stat.total_clusters);
> -		printf("\t\tAvg cluster size: %s\n",
> -				pretty_size((stat.total_cluster_size /
> +		printf("\t\tAvg cluster size: "PF"\n",
> +				PA((stat.total_cluster_size /
>   						stat.total_clusters)));
> -		printf("\t\tMin cluster size: %s\n",
> -				pretty_size(stat.min_cluster_size));
> -		printf("\t\tMax cluster size: %s\n",
> -				pretty_size(stat.max_cluster_size));
> -		printf("\tTotal disk spread: %s\n",
> -				pretty_size(stat.highest_bytenr -
> +		printf("\t\tMin cluster size: "PF"\n",
> +				PA(stat.min_cluster_size));
> +		printf("\t\tMax cluster size: "PF"\n",
> +				PA(stat.max_cluster_size));
> +		printf("\tTotal disk spread: "PF"\n",
> +				PA(stat.highest_bytenr -
>   					stat.lowest_bytenr));
>   		printf("\tTotal read time: %d s %d us\n", (int)diff.tv_sec,
>   		       (int)diff.tv_usec);
> diff --git a/btrfs-fragments.c b/btrfs-fragments.c
> index d03c2c3..fd45822 100644
> --- a/btrfs-fragments.c
> +++ b/btrfs-fragments.c
> @@ -85,9 +85,9 @@ print_bg(FILE *html, char *name, u64 start, u64 len, u64 used, u64 flags,
>   {
>   	double frag = (double)areas / (len / 4096) * 2;
>
> -	fprintf(html, "<p>%s chunk starts at %lld, size is %s, %.2f%% used, "
> +	fprintf(html, "<p>%s chunk starts at %lld, size is "PF", %.2f%% used, "
>   		      "%.2f%% fragmented</p>\n", chunk_type(flags), start,
> -		      pretty_size(len), 100.0 * used / len, 100.0 * frag);
> +		      PA(len), 100.0 * used / len, 100.0 * frag);
>   	fprintf(html, "<img src=\"%s\" border=\"1\" />\n", name);
>   }
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 7e8ca95..d8b6938 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -210,11 +210,11 @@ static void print_df(struct btrfs_ioctl_space_args *sargs)
>   	struct btrfs_ioctl_space_info *sp = sargs->spaces;
>
>   	for (i = 0; i < sargs->total_spaces; i++, sp++) {
> -		printf("%s, %s: total=%s, used=%s\n",
> +		printf("%s, %s: total="PF", used="PF"\n",
>   			group_type_str(sp->flags),
>   			group_profile_str(sp->flags),
> -			pretty_size(sp->total_bytes),
> -			pretty_size(sp->used_bytes));
> +			PA(sp->total_bytes),
> +			PA(sp->used_bytes));
>   	}
>   }
>
> @@ -326,18 +326,18 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices)
>
>
>   	total = device->total_devs;
> -	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
> +	printf(" uuid: %s\n\tTotal devices %llu FS bytes used "PF"\n", uuidbuf,
>   	       (unsigned long long)total,
> -	       pretty_size(device->super_bytes_used));
> +	       PA(device->super_bytes_used));
>
>   	list_sort(NULL, &fs_devices->devices, cmp_device_id);
>   	list_for_each(cur, &fs_devices->devices) {
>   		device = list_entry(cur, struct btrfs_device, dev_list);
>
> -		printf("\tdevid %4llu size %s used %s path %s\n",
> +		printf("\tdevid %4llu size "PF" used "PF" path %s\n",
>   		       (unsigned long long)device->devid,
> -		       pretty_size(device->total_bytes),
> -		       pretty_size(device->bytes_used), device->name);
> +		       PA(device->total_bytes),
> +		       PA(device->bytes_used), device->name);
>
>   		devs_found++;
>   	}
> @@ -382,9 +382,9 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
>   	else
>   		printf("Label: none ");
>
> -	printf(" uuid: %s\n\tTotal devices %llu FS bytes used %s\n", uuidbuf,
> +	printf(" uuid: %s\n\tTotal devices %llu FS bytes used "PF"\n", uuidbuf,
>   			fs_info->num_devices,
> -			pretty_size(calc_used_bytes(space_info)));
> +			PA(calc_used_bytes(space_info)));
>
>   	for (i = 0; i < fs_info->num_devices; i++) {
>   		tmp_dev_info = (struct btrfs_ioctl_dev_info_args *)&dev_info[i];
> @@ -396,10 +396,10 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
>   			continue;
>   		}
>   		close(fd);
> -		printf("\tdevid %4llu size %s used %s path %s\n",
> +		printf("\tdevid %4llu size "PF" used "PF" path %s\n",
>   			tmp_dev_info->devid,
> -			pretty_size(tmp_dev_info->total_bytes),
> -			pretty_size(tmp_dev_info->bytes_used),
> +			PA(tmp_dev_info->total_bytes),
> +			PA(tmp_dev_info->bytes_used),
>   			tmp_dev_info->path);
>   	}
>
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index 731c5c9..10a6692 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -151,8 +151,8 @@ static void print_scrub_summary(struct btrfs_scrub_progress *p)
>   		printf("*** WARNING: memory allocation failed while scrubbing. "
>   		       "results may be inaccurate\n");
>
> -	printf("\ttotal bytes scrubbed: %s with %llu errors\n",
> -		pretty_size(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
> +	printf("\ttotal bytes scrubbed: "PF" with %llu errors\n",
> +		PA(p->data_bytes_scrubbed + p->tree_bytes_scrubbed),
>   		max(err_cnt, err_cnt2));
>
>   	if (err_cnt || err_cnt2) {
> diff --git a/mkfs.c b/mkfs.c
> index 9de61e1..d649e03 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -1643,9 +1643,9 @@ raid_groups:
>   	BUG_ON(ret);
>
>   	printf("fs created label %s on %s\n\tnodesize %u leafsize %u "
> -	    "sectorsize %u size %s\n",
> +	    "sectorsize %u size "PF"\n",
>   	    label, first_file, nodesize, leafsize, sectorsize,
> -	    pretty_size(btrfs_super_total_bytes(root->fs_info->super_copy)));
> +	    PA(btrfs_super_total_bytes(root->fs_info->super_copy)));
>
>   	btrfs_commit_transaction(trans, root);
>
> diff --git a/utils.c b/utils.c
> index 6c09366..0064587 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -709,8 +709,8 @@ int btrfs_prepare_device(int fd, char *file, int zero_end, u64 *block_count_ret,
>   		 * optimization.
>   		 */
>   		if (discard_range(fd, 0, 0) == 0) {
> -			fprintf(stderr, "Performing full device TRIM (%s) ...\n",
> -				pretty_size(block_count));
> +			fprintf(stderr, "Performing full device TRIM ("PF") ...\n",
> +				PA(block_count));
>   			discard_blocks(fd, 0, block_count);
>   		}
>   	}
> @@ -1376,22 +1376,21 @@ out:
>   	return ret;
>   }
>
> -static char *size_strs[] = { "", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"};
> -int pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
> +static float pretty_calc(u64 size, char **str)
>   {
>   	int num_divs = 0;
>   	float fraction;
> +	static char *size_strs[] = {
> +		"", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB",
> +	};
>
> -	if (str_bytes == 0)
> -		return 0;
> -
> -	if( size < 1024 ){
> +	if (size < 1024) {
>   		fraction = size;
>   		num_divs = 0;
>   	} else {
>   		u64 last_size = size;
>   		num_divs = 0;
> -		while(size >= 1024){
> +		while (size >= 1024) {
>   			last_size = size;
>   			size /= 1024;
>   			num_divs ++;
> @@ -1403,8 +1402,22 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes)
>   		}
>   		fraction = (float)last_size / 1024;
>   	}
> -	return snprintf(str, str_bytes, "%.2f%s", fraction,
> -			size_strs[num_divs]);
> +
> +	*str = size_strs[num_divs];
> +	return fraction;
> +}
> +
> +float pretty_float(u64 size)
> +{
> +	char *str;
> +	return pretty_calc(size, &str);
> +}
> +
> +char *pretty_suffix(u64 size)
> +{
> +	char *str;
> +	pretty_calc(size, &str);
> +	return str;
>   }
>
>   /*
> diff --git a/utils.h b/utils.h
> index fd25126..26c767b 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -71,13 +71,17 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
>   int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
>   				 int super_offset);
>
> -int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
> -#define pretty_size(size) 						\
> -	({								\
> -		static __thread char _str[24];				\
> -		(void)pretty_size_snprintf((size), _str, sizeof(_str));	\
> -		_str;							\
> -	})
> +/*
> + * It's annoying that gcc hasn't yet figured out how to learn about
> + * formats added by register_printf_specifier().  If that were the case
> + * we could just add some %P type and be done with it.  Some day..
> + *
> + *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47781
> + */
> +float pretty_float(u64 size);
> +char *pretty_suffix(u64 size);
> +#define PF "%.2f%s"
> +#define PA(x) pretty_float(x), pretty_suffix(x)
>
>   int get_mountpt(char *dev, char *mntpt, size_t size);
>   int btrfs_scan_block_devices(int run_ioctl);
>

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[Zach Brown]

>  Great! Thanks Zach for your quick patch. it works.

Cool.

> > From 3d132362f4c87b065b63cb38726a030db2277919 Mon Sep 17 00:00:00 2001
> >From: Zach Brown <zab@zabbo.net>
> >Date: Thu, 4 Sep 2014 12:32:00 -0700
> >Subject: [PATCH] btrfs-progs: use pretty printing macros

David, let me know if you want me to change anything or resend so you
don't have to pull this out of a reply in a thread.

- z

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[David Sterba]

On Fri, Sep 05, 2014 at 08:55:01AM -0700, Zach Brown wrote:
> David, let me know if you want me to change anything or resend so you
> don't have to pull this out of a reply in a thread.

Thanks, I'm now aware of the patch in the thread.

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[David Sterba]

On Fri, Sep 05, 2014 at 06:20:49PM +0200, David Sterba wrote:
> On Fri, Sep 05, 2014 at 08:55:01AM -0700, Zach Brown wrote:
> > David, let me know if you want me to change anything or resend so you
> > don't have to pull this out of a reply in a thread.
> 
> Thanks, I'm now aware of the patch in the thread.

FYI, I'm going to add the fix to the 3.17 queue but there are other
changes to the unit printing interface that are in conflict with this.
My plan is to apply Zach's changes on top of that.

Re: [PATCH] btrfs-progs: per-thread, per-call pretty buffer

[David Sterba]

On Thu, Sep 04, 2014 at 12:45:45PM -0700, Zach Brown wrote:
> --- a/utils.h
> +++ b/utils.h
> @@ -71,13 +71,17 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
>  int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
>  				 int super_offset);
>  
> -int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
> -#define pretty_size(size) 						\
> -	({								\
> -		static __thread char _str[24];				\
> -		(void)pretty_size_snprintf((size), _str, sizeof(_str));	\
> -		_str;							\
> -	})
> +/*
> + * It's annoying that gcc hasn't yet figured out how to learn about
> + * formats added by register_printf_specifier().  If that were the case
> + * we could just add some %P type and be done with it.  Some day..
> + *
> + *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47781
> + */
> +float pretty_float(u64 size);
> +char *pretty_suffix(u64 size);
> +#define PF "%.2f%s"
> +#define PA(x) pretty_float(x), pretty_suffix(x)

So I've crawled back to that patch and I'm not all happy to update every
use of pretty_size and the printf string. The user-defined printf format
handlers are not an option.

My proposal that would avoid that:

- predefine a static array of N strings where the pretty numbers would
  fit, eg 32 bytes as is now
- pretty_size will:
  - grab the first unused slot
  - fill it with the string
  - increase index to the array
  - wrap at N
  - return pointer to the prev entry

Normally we're using at most 2 pretty_size calls per printf, so if we
make eg. N = 10, we're relatively safe.

We can keep the easy %s + pretty_size, I really find it convenient to
write it that way.