thr3ads.net - Btrfs devel - [PATCH V2 1/2] Btrfs-progs: make pretty

If this information is useful, please help other people find it:
Share via:

Wang Shilong

2013-Jul-07 09:58 UTC

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

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

--
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-info.html

Zach Brown

2013-Jul-09 20:24 UTC

head link

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

> 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

--
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-info.html

Wang Shilong

2013-Jul-09 23:05 UTC

head link

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

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
> 
--
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-info.html

David Sterba

2013-Jul-10 12:49 UTC

head link

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

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
> --
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-info.html

David Sterba

2013-Jul-10 14:30 UTC

head link

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

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

--
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-info.html

Wang Shilong

2013-Jul-10 15:31 UTC

head link

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

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
> 
--
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-info.html

David Sterba

2013-Jul-10 15:51 UTC

head link

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

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
--
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-info.html

Zach Brown

2013-Jul-10 15:59 UTC

head link

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

> 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
--
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-info.html

Hugo Mills

2013-Jul-10 16:16 UTC

head link

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

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... ---

David Sterba

2013-Jul-10 17:39 UTC

head link

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

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
--
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-info.html

David Sterba

2013-Jul-10 17:40 UTC

head link

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

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

--
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-info.html

Btrfs devel - Jul 2013 - [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

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

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

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

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

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

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

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

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

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

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