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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
Thanks,
Wang
>
> - z
>
> From bea92d06d98827af30518aa800428c0b2a8be101 Mon Sep 17 00:00:00 2001
> From: Zach Brown
> 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
> ---
> 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
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
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
---
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
[PATCH V2 1/2] Btrfs-progs: make pretty_sizes() work less error prone
From: Wang Shilong
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
---
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, "%s chunk starts at %lld, size is %s, %.2f%% used, "
"%.2f%% fragmented\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, "\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);
-
