* [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
@ 2018-04-09 9:19 Qu Wenruo
2018-04-09 21:50 ` Goffredo Baroncelli
0 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2018-04-09 9:19 UTC (permalink / raw)
To: linux-btrfs
When manually patching super blocks, current validation check is pretty
weak (limited to magic number and csum) and doesn't provide extra check
for some obvious corruption (like invalid sectorsize).
This patch will enhance dump-super by:
1) Refactor print function and add checker helper macro
Instead of manually call printf() and takes 2 lines to just print one
member, introduce several helper macros to print/check value.
And now caller don't need to calculate the indent manually, it will
be calculated automatically in the helper macros.
Most of the callers just need 1 line.
(Although the macro and helper itself offsets the saved lines)
2) Add extra check for some members
The following members will be checked, and if invalid the suffix
" [INVALID]" will be pended:
bytenr: alignment check
sys_array_size: maximum check
root: alignment check
root_level: maximum check
chunk_root: alignment check
chunk_root_level: maximum check
chunk_root_generation: maximum check against root generation
log_root: alignment check
log_root_level: maximum check
log_root_transid: maximum check against root generation + 1
bytes_used: alignment check
sectorsize: power of 2 and 4K~64K range check
nodesize: the same as sectorsize, plus minimum check
stripesize: the same as kernel check
cache_generation: maximum check against root generation
uuid_tree_generation: maximum check against root generation
3) output sequence change
Put bytenr/level/generation together for the same root (tree root,
chunk root, and log root).
4) Minor output change
To make dev_item macro easier, the following output is changed:
dev_item.devid -> dev_item.id
dev_item.dev_group -> dev_item.group
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
changelog:
v2:
Generate tab based indent string in helper macro instead of passing spacer
string from outside, suggested by Nikolay.
(In fact, if using %*s it would be much easier, however it needs extra
rework for existing code as they still use tab as indent)
---
cmds-inspect-dump-super.c | 206 +++++++++++++++++++++++++-------------
1 file changed, 137 insertions(+), 69 deletions(-)
diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
index 24588c87cce6..68d6f59ad727 100644
--- a/cmds-inspect-dump-super.c
+++ b/cmds-inspect-dump-super.c
@@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag)
super_flags_num, BTRFS_SUPER_FLAG_SUPP);
}
+#define INDENT_BUFFER_LEN 80
+#define TAB_LEN 8
+#define SUPER_INDENT 24
+
+/* helper to generate tab based indent string */
+static void generate_tab_indent(char *buf, unsigned int indent)
+{
+ buf[0] = '\0';
+ for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN)
+ strncat(buf, "\t", INDENT_BUFFER_LEN);
+}
+
+/* Helper to print member in %llu */
+#define print_super(sb, member) \
+({ \
+ int indent = SUPER_INDENT - strlen(#member); \
+ char indent_str[INDENT_BUFFER_LEN]; \
+ \
+ generate_tab_indent(indent_str, indent); \
+ printf("%s%s%llu\n", #member, indent_str, \
+ (u64) btrfs_super_##member(sb)); \
+})
+
+/* Helper to print member in %llx */
+#define print_super_hex(sb, member) \
+({ \
+ int indent = SUPER_INDENT - strlen(#member); \
+ char indent_str[INDENT_BUFFER_LEN]; \
+ \
+ generate_tab_indent(indent_str, indent); \
+ printf("%s%s0x%llx\n", #member, indent_str, \
+ (u64) btrfs_super_##member(sb)); \
+})
+
+/*
+ * Helper macro to wrap member checker
+ *
+ * Only support %llu output for member.
+ */
+#define print_check_super(sb, member, bad_condition) \
+({ \
+ int indent = SUPER_INDENT - strlen(#member); \
+ char indent_str[INDENT_BUFFER_LEN]; \
+ u64 value; \
+ \
+ generate_tab_indent(indent_str, indent); \
+ value = btrfs_super_##member(sb); \
+ printf("%s%s%llu", #member, indent_str, (u64) value); \
+ if (bad_condition) \
+ printf(" [INVALID]"); \
+ printf("\n"); \
+})
+
+#define DEV_INDENT (SUPER_INDENT - strlen("dev_item."))
+BUILD_ASSERT(DEV_INDENT > 0);
+
+/* Helper to print sb->dev_item members */
+#define print_super_dev(sb, member) \
+({ \
+ int indent = DEV_INDENT - strlen(#member); \
+ char indent_str[INDENT_BUFFER_LEN]; \
+ \
+ generate_tab_indent(indent_str, indent); \
+ printf("dev_item.%s%s%llu\n", #member, indent_str, \
+ (u64) btrfs_stack_device_##member(&sb->dev_item)); \
+})
+
static void dump_superblock(struct btrfs_super_block *sb, int full)
{
int i;
char *s, buf[BTRFS_UUID_UNPARSED_SIZE];
+ int max_level = BTRFS_MAX_LEVEL; /* to save several chars */
u8 *p;
+ u64 super_gen;
u32 csum_size;
u16 csum_type;
@@ -348,10 +417,16 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
printf(" [DON'T MATCH]");
putchar('\n');
- printf("bytenr\t\t\t%llu\n",
- (unsigned long long)btrfs_super_bytenr(sb));
- printf("flags\t\t\t0x%llx\n",
- (unsigned long long)btrfs_super_flags(sb));
+ /*
+ * Use btrfs minimal sector size as basic check for bytenr, since we
+ * can't trust sector size from super block.
+ * This 4K check should works fine for most architecture, and will be
+ * just a little loose for 64K page system.
+ *
+ * And such 4K check will be used for other members too.
+ */
+ print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
+ print_super_hex(sb, flags);
print_readable_super_flag(btrfs_super_flags(sb));
printf("magic\t\t\t");
@@ -372,53 +447,56 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
putchar(isprint(s[i]) ? s[i] : '.');
putchar('\n');
- printf("generation\t\t%llu\n",
- (unsigned long long)btrfs_super_generation(sb));
- printf("root\t\t\t%llu\n", (unsigned long long)btrfs_super_root(sb));
- printf("sys_array_size\t\t%llu\n",
- (unsigned long long)btrfs_super_sys_array_size(sb));
- printf("chunk_root_generation\t%llu\n",
- (unsigned long long)btrfs_super_chunk_root_generation(sb));
- printf("root_level\t\t%llu\n",
- (unsigned long long)btrfs_super_root_level(sb));
- printf("chunk_root\t\t%llu\n",
- (unsigned long long)btrfs_super_chunk_root(sb));
- printf("chunk_root_level\t%llu\n",
- (unsigned long long)btrfs_super_chunk_root_level(sb));
- printf("log_root\t\t%llu\n",
- (unsigned long long)btrfs_super_log_root(sb));
- printf("log_root_transid\t%llu\n",
- (unsigned long long)btrfs_super_log_root_transid(sb));
- printf("log_root_level\t\t%llu\n",
- (unsigned long long)btrfs_super_log_root_level(sb));
- printf("total_bytes\t\t%llu\n",
- (unsigned long long)btrfs_super_total_bytes(sb));
- printf("bytes_used\t\t%llu\n",
- (unsigned long long)btrfs_super_bytes_used(sb));
- printf("sectorsize\t\t%llu\n",
- (unsigned long long)btrfs_super_sectorsize(sb));
- printf("nodesize\t\t%llu\n",
- (unsigned long long)btrfs_super_nodesize(sb));
+ print_check_super(sb, sys_array_size,
+ (value > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE));
+
+ super_gen = btrfs_super_generation(sb);
+ print_check_super(sb, root, (!IS_ALIGNED(value, SZ_4K)));
+ print_check_super(sb, root_level, (value >= max_level));
+ print_super(sb, generation);
+
+ print_check_super(sb, chunk_root, (!IS_ALIGNED(value, SZ_4K)));
+ print_check_super(sb, chunk_root_level, (value >= max_level));
+ /*
+ * Here we trust super generation, and use it as checker for other
+ * tree roots. Applies to all other trees.
+ */
+ print_check_super(sb, chunk_root_generation, (value > super_gen));
+
+ print_check_super(sb, log_root, (!IS_ALIGNED(value, SZ_4K)));
+ print_check_super(sb, log_root_level, (value >= max_level));
+ print_check_super(sb, log_root_transid, (value > super_gen + 1));
+
+ /*
+ * For total bytes, it's possible that old kernel is using unaligned
+ * size, not critical so don't do 4K check here.
+ */
+ print_super(sb, total_bytes);
+
+ /* Used bytes must be aligned to 4K */
+ print_check_super(sb, bytes_used, (!IS_ALIGNED(value, SZ_4K)));
+ print_check_super(sb, sectorsize,
+ (!(is_power_of_2(value) && value >= SZ_4K &&
+ value <= SZ_64K)));
+ print_check_super(sb, nodesize,
+ (!(is_power_of_2(value) && value >= SZ_4K &&
+ value <= SZ_64K &&
+ value > btrfs_super_sectorsize(sb))));
printf("leafsize (deprecated)\t%u\n",
le32_to_cpu(sb->__unused_leafsize));
- printf("stripesize\t\t%llu\n",
- (unsigned long long)btrfs_super_stripesize(sb));
- printf("root_dir\t\t%llu\n",
- (unsigned long long)btrfs_super_root_dir(sb));
- printf("num_devices\t\t%llu\n",
- (unsigned long long)btrfs_super_num_devices(sb));
- printf("compat_flags\t\t0x%llx\n",
- (unsigned long long)btrfs_super_compat_flags(sb));
- printf("compat_ro_flags\t\t0x%llx\n",
- (unsigned long long)btrfs_super_compat_ro_flags(sb));
+
+ /* Not really used, just check it the same way as kernel */
+ print_check_super(sb, stripesize, (!is_power_of_2(value)));
+ print_super(sb, root_dir);
+ print_super(sb, num_devices);
+ print_super_hex(sb, compat_flags);
+ print_super_hex(sb, compat_ro_flags);
print_readable_compat_ro_flag(btrfs_super_compat_ro_flags(sb));
- printf("incompat_flags\t\t0x%llx\n",
- (unsigned long long)btrfs_super_incompat_flags(sb));
+ print_super_hex(sb, incompat_flags);
print_readable_incompat_flag(btrfs_super_incompat_flags(sb));
- printf("cache_generation\t%llu\n",
- (unsigned long long)btrfs_super_cache_generation(sb));
- printf("uuid_tree_generation\t%llu\n",
- (unsigned long long)btrfs_super_uuid_tree_generation(sb));
+ print_check_super(sb, cache_generation,
+ (value > super_gen && value != (u64)-1));
+ print_check_super(sb, uuid_tree_generation, (value > super_gen));
uuid_unparse(sb->dev_item.uuid, buf);
printf("dev_item.uuid\t\t%s\n", buf);
@@ -428,28 +506,18 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
!memcmp(sb->dev_item.fsid, sb->fsid, BTRFS_FSID_SIZE) ?
"[match]" : "[DON'T MATCH]");
- printf("dev_item.type\t\t%llu\n", (unsigned long long)
- btrfs_stack_device_type(&sb->dev_item));
- printf("dev_item.total_bytes\t%llu\n", (unsigned long long)
- btrfs_stack_device_total_bytes(&sb->dev_item));
- printf("dev_item.bytes_used\t%llu\n", (unsigned long long)
- btrfs_stack_device_bytes_used(&sb->dev_item));
- printf("dev_item.io_align\t%u\n", (unsigned int)
- btrfs_stack_device_io_align(&sb->dev_item));
- printf("dev_item.io_width\t%u\n", (unsigned int)
- btrfs_stack_device_io_width(&sb->dev_item));
- printf("dev_item.sector_size\t%u\n", (unsigned int)
- btrfs_stack_device_sector_size(&sb->dev_item));
- printf("dev_item.devid\t\t%llu\n",
- btrfs_stack_device_id(&sb->dev_item));
- printf("dev_item.dev_group\t%u\n", (unsigned int)
- btrfs_stack_device_group(&sb->dev_item));
- printf("dev_item.seek_speed\t%u\n", (unsigned int)
- btrfs_stack_device_seek_speed(&sb->dev_item));
- printf("dev_item.bandwidth\t%u\n", (unsigned int)
- btrfs_stack_device_bandwidth(&sb->dev_item));
- printf("dev_item.generation\t%llu\n", (unsigned long long)
- btrfs_stack_device_generation(&sb->dev_item));
+ /* For embedded device item, don't do extra check, just like kernel */
+ print_super_dev(sb, type);
+ print_super_dev(sb, total_bytes);
+ print_super_dev(sb, bytes_used);
+ print_super_dev(sb, io_align);
+ print_super_dev(sb, io_width);
+ print_super_dev(sb, sector_size);
+ print_super_dev(sb, id);
+ print_super_dev(sb, group);
+ print_super_dev(sb, seek_speed);
+ print_super_dev(sb, bandwidth);
+ print_super_dev(sb, generation);
if (full) {
printf("sys_chunk_array[%d]:\n", BTRFS_SYSTEM_CHUNK_ARRAY_SIZE);
print_sys_chunk_array(sb);
--
2.17.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
2018-04-09 9:19 [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check Qu Wenruo
@ 2018-04-09 21:50 ` Goffredo Baroncelli
2018-04-10 2:00 ` Qu Wenruo
0 siblings, 1 reply; 8+ messages in thread
From: Goffredo Baroncelli @ 2018-04-09 21:50 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
Hi Qu,
On 04/09/2018 11:19 AM, Qu Wenruo wrote:
> When manually patching super blocks, current validation check is pretty
> weak (limited to magic number and csum) and doesn't provide extra check
> for some obvious corruption (like invalid sectorsize).
[...]
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> changelog:
> v2:
> Generate tab based indent string in helper macro instead of passing spacer
> string from outside, suggested by Nikolay.
> (In fact, if using %*s it would be much easier, however it needs extra
> rework for existing code as they still use tab as indent)
> ---
> cmds-inspect-dump-super.c | 206 +++++++++++++++++++++++++-------------
> 1 file changed, 137 insertions(+), 69 deletions(-)
>
> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
> index 24588c87cce6..68d6f59ad727 100644
> --- a/cmds-inspect-dump-super.c
> +++ b/cmds-inspect-dump-super.c
> @@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag)
> super_flags_num, BTRFS_SUPER_FLAG_SUPP);
> }
>
> +#define INDENT_BUFFER_LEN 80
> +#define TAB_LEN 8
> +#define SUPER_INDENT 24
> +
> +/* helper to generate tab based indent string */
> +static void generate_tab_indent(char *buf, unsigned int indent)
> +{
> + buf[0] = '\0';
> + for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN)
> + strncat(buf, "\t", INDENT_BUFFER_LEN);
> +}
> +
> +/* Helper to print member in %llu */
> +#define print_super(sb, member) \
> +({ \
> + int indent = SUPER_INDENT - strlen(#member); \
> + char indent_str[INDENT_BUFFER_LEN]; \
> + \
> + generate_tab_indent(indent_str, indent); \
> + printf("%s%s%llu\n", #member, indent_str, \
> + (u64) btrfs_super_##member(sb)); \
Why not something like:
static const char tabs[] = "\t\t\t\t"; \
int i = (sizeof(#member)+6)/8; \
if (i > sizeof(tabs)-1) \
i = sizeof(tabs-1); \
u64 value = (u64)btrfs_super_##member(sb); \
printf("%s%s" format, \
#member, tabs+i, value);
so to get rid of generate_tab_indent and indent_str
> +})
> +
> +/* Helper to print member in %llx */
> +#define print_super_hex(sb, member) \
> +({ \
> + int indent = SUPER_INDENT - strlen(#member); \
> + char indent_str[INDENT_BUFFER_LEN]; \
> + \
> + generate_tab_indent(indent_str, indent); \
> + printf("%s%s0x%llx\n", #member, indent_str, \
> + (u64) btrfs_super_##member(sb)); \
> +})
> +
> +/*
> + * Helper macro to wrap member checker
> + *
> + * Only support %llu output for member.
> + */
> +#define print_check_super(sb, member, bad_condition) \
> +({ \
> + int indent = SUPER_INDENT - strlen(#member); \
> + char indent_str[INDENT_BUFFER_LEN]; \
> + u64 value; \
> + \
> + generate_tab_indent(indent_str, indent); \
> + value = btrfs_super_##member(sb); \
> + printf("%s%s%llu", #member, indent_str, (u64) value); \
> + if (bad_condition) \
> + printf(" [INVALID]"); \
What about printing also the condition: something like
+ if (bad_condition) \
+ printf(" [INVALID '%s']", #bad_condition); \
it would be even better a "good_condition":
so instead of:
+ print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
do
+ print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));
and
+ if (!good_condition) \
+ printf(" [ERROR: required '%s']", #good_condition); \
All above functions could be written as:
#define __print_and_check(sb, member, format, expected) \
do{ \
static const char tabs[] = "\t\t\t\t"; \
int i = (sizeof(#member)+6)/8; \
if (i > sizeof(tabs)-1) \
i = sizeof(tabs-1); \
u64 value = (u64)btrfs_super_##member(sb); \
printf("%s%s" format, \
#member, tabs+i, value); \
if (!(expected)) \
printf(" [ERROR: expected '%s']", #expected); \
printf("\n"); \
} while(0)
#define print_super(sb, member) \
__print_and_check(sb, member, "%llu", 1);
#define print_super_hex(sb, member) \
__print_and_check(sb, member, "0x%llx", 1);
#define print_check_super(sb, member, condition) \
__print_and_check(sb, member, "0x%llx", condition);
And the value should be printed as (I removed the !):
print_check_super(sb, root, (IS_ALIGNED(value, SZ_4K)));
In case of error:
test 12 [ERROR: expected 'IS_ALIGNED(value, SZ_4K)']
> + printf("\n"); \
> +})
> +
> +#define DEV_INDENT (SUPER_INDENT - strlen("dev_item."))
> +BUILD_ASSERT(DEV_INDENT > 0);
> +
> +/* Helper to print sb->dev_item members */
> +#define print_super_dev(sb, member) \
> +({ \
> + int indent = DEV_INDENT - strlen(#member); \
> + char indent_str[INDENT_BUFFER_LEN]; \
> + \
> + generate_tab_indent(indent_str, indent); \
> + printf("dev_item.%s%s%llu\n", #member, indent_str, \
> + (u64) btrfs_stack_device_##member(&sb->dev_item)); \
> +})
> +
> static void dump_superblock(struct btrfs_super_block *sb, int full)
> {
> int i;
> char *s, buf[BTRFS_UUID_UNPARSED_SIZE];
> + int max_level = BTRFS_MAX_LEVEL; /* to save several chars */
> u8 *p;
> + u64 super_gen;
> u32 csum_size;
> u16 csum_type;
>
> @@ -348,10 +417,16 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
> printf(" [DON'T MATCH]");
> putchar('\n');
>
> - printf("bytenr\t\t\t%llu\n",
> - (unsigned long long)btrfs_super_bytenr(sb));
> - printf("flags\t\t\t0x%llx\n",
> - (unsigned long long)btrfs_super_flags(sb));
> + /*
> + * Use btrfs minimal sector size as basic check for bytenr, since we
> + * can't trust sector size from super block.
> + * This 4K check should works fine for most architecture, and will be
> + * just a little loose for 64K page system.
> + *
> + * And such 4K check will be used for other members too.
> + */
> + print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
> + print_super_hex(sb, flags);
> print_readable_super_flag(btrfs_super_flags(sb));
>
> printf("magic\t\t\t");
> @@ -372,53 +447,56 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
> putchar(isprint(s[i]) ? s[i] : '.');
> putchar('\n');
>
> - printf("generation\t\t%llu\n",
> - (unsigned long long)btrfs_super_generation(sb));
> - printf("root\t\t\t%llu\n", (unsigned long long)btrfs_super_root(sb));
> - printf("sys_array_size\t\t%llu\n",
> - (unsigned long long)btrfs_super_sys_array_size(sb));
> - printf("chunk_root_generation\t%llu\n",
> - (unsigned long long)btrfs_super_chunk_root_generation(sb));
> - printf("root_level\t\t%llu\n",
> - (unsigned long long)btrfs_super_root_level(sb));
> - printf("chunk_root\t\t%llu\n",
> - (unsigned long long)btrfs_super_chunk_root(sb));
> - printf("chunk_root_level\t%llu\n",
> - (unsigned long long)btrfs_super_chunk_root_level(sb));
> - printf("log_root\t\t%llu\n",
> - (unsigned long long)btrfs_super_log_root(sb));
> - printf("log_root_transid\t%llu\n",
> - (unsigned long long)btrfs_super_log_root_transid(sb));
> - printf("log_root_level\t\t%llu\n",
> - (unsigned long long)btrfs_super_log_root_level(sb));
> - printf("total_bytes\t\t%llu\n",
> - (unsigned long long)btrfs_super_total_bytes(sb));
> - printf("bytes_used\t\t%llu\n",
> - (unsigned long long)btrfs_super_bytes_used(sb));
> - printf("sectorsize\t\t%llu\n",
> - (unsigned long long)btrfs_super_sectorsize(sb));
> - printf("nodesize\t\t%llu\n",
> - (unsigned long long)btrfs_super_nodesize(sb));
> + print_check_super(sb, sys_array_size,
> + (value > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE));
> +
> + super_gen = btrfs_super_generation(sb);
> + print_check_super(sb, root, (!IS_ALIGNED(value, SZ_4K)));
> + print_check_super(sb, root_level, (value >= max_level));
> + print_super(sb, generation);
> +
> + print_check_super(sb, chunk_root, (!IS_ALIGNED(value, SZ_4K)));
> + print_check_super(sb, chunk_root_level, (value >= max_level));
> + /*
> + * Here we trust super generation, and use it as checker for other
> + * tree roots. Applies to all other trees.
> + */
> + print_check_super(sb, chunk_root_generation, (value > super_gen));
> +
> + print_check_super(sb, log_root, (!IS_ALIGNED(value, SZ_4K)));
> + print_check_super(sb, log_root_level, (value >= max_level));
> + print_check_super(sb, log_root_transid, (value > super_gen + 1));
> +
> + /*
> + * For total bytes, it's possible that old kernel is using unaligned
> + * size, not critical so don't do 4K check here.
> + */
> + print_super(sb, total_bytes);
> +
> + /* Used bytes must be aligned to 4K */
> + print_check_super(sb, bytes_used, (!IS_ALIGNED(value, SZ_4K)));
> + print_check_super(sb, sectorsize,
> + (!(is_power_of_2(value) && value >= SZ_4K &&
> + value <= SZ_64K)));
> + print_check_super(sb, nodesize,
> + (!(is_power_of_2(value) && value >= SZ_4K &&
> + value <= SZ_64K &&
> + value > btrfs_super_sectorsize(sb))));
> printf("leafsize (deprecated)\t%u\n",
> le32_to_cpu(sb->__unused_leafsize));
> - printf("stripesize\t\t%llu\n",
> - (unsigned long long)btrfs_super_stripesize(sb));
> - printf("root_dir\t\t%llu\n",
> - (unsigned long long)btrfs_super_root_dir(sb));
> - printf("num_devices\t\t%llu\n",
> - (unsigned long long)btrfs_super_num_devices(sb));
> - printf("compat_flags\t\t0x%llx\n",
> - (unsigned long long)btrfs_super_compat_flags(sb));
> - printf("compat_ro_flags\t\t0x%llx\n",
> - (unsigned long long)btrfs_super_compat_ro_flags(sb));
> +
> + /* Not really used, just check it the same way as kernel */
> + print_check_super(sb, stripesize, (!is_power_of_2(value)));
> + print_super(sb, root_dir);
> + print_super(sb, num_devices);
> + print_super_hex(sb, compat_flags);
> + print_super_hex(sb, compat_ro_flags);
> print_readable_compat_ro_flag(btrfs_super_compat_ro_flags(sb));
> - printf("incompat_flags\t\t0x%llx\n",
> - (unsigned long long)btrfs_super_incompat_flags(sb));
> + print_super_hex(sb, incompat_flags);
> print_readable_incompat_flag(btrfs_super_incompat_flags(sb));
> - printf("cache_generation\t%llu\n",
> - (unsigned long long)btrfs_super_cache_generation(sb));
> - printf("uuid_tree_generation\t%llu\n",
> - (unsigned long long)btrfs_super_uuid_tree_generation(sb));
> + print_check_super(sb, cache_generation,
> + (value > super_gen && value != (u64)-1));
> + print_check_super(sb, uuid_tree_generation, (value > super_gen));
>
> uuid_unparse(sb->dev_item.uuid, buf);
> printf("dev_item.uuid\t\t%s\n", buf);
> @@ -428,28 +506,18 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
> !memcmp(sb->dev_item.fsid, sb->fsid, BTRFS_FSID_SIZE) ?
> "[match]" : "[DON'T MATCH]");
>
> - printf("dev_item.type\t\t%llu\n", (unsigned long long)
> - btrfs_stack_device_type(&sb->dev_item));
> - printf("dev_item.total_bytes\t%llu\n", (unsigned long long)
> - btrfs_stack_device_total_bytes(&sb->dev_item));
> - printf("dev_item.bytes_used\t%llu\n", (unsigned long long)
> - btrfs_stack_device_bytes_used(&sb->dev_item));
> - printf("dev_item.io_align\t%u\n", (unsigned int)
> - btrfs_stack_device_io_align(&sb->dev_item));
> - printf("dev_item.io_width\t%u\n", (unsigned int)
> - btrfs_stack_device_io_width(&sb->dev_item));
> - printf("dev_item.sector_size\t%u\n", (unsigned int)
> - btrfs_stack_device_sector_size(&sb->dev_item));
> - printf("dev_item.devid\t\t%llu\n",
> - btrfs_stack_device_id(&sb->dev_item));
> - printf("dev_item.dev_group\t%u\n", (unsigned int)
> - btrfs_stack_device_group(&sb->dev_item));
> - printf("dev_item.seek_speed\t%u\n", (unsigned int)
> - btrfs_stack_device_seek_speed(&sb->dev_item));
> - printf("dev_item.bandwidth\t%u\n", (unsigned int)
> - btrfs_stack_device_bandwidth(&sb->dev_item));
> - printf("dev_item.generation\t%llu\n", (unsigned long long)
> - btrfs_stack_device_generation(&sb->dev_item));
> + /* For embedded device item, don't do extra check, just like kernel */
> + print_super_dev(sb, type);
> + print_super_dev(sb, total_bytes);
> + print_super_dev(sb, bytes_used);
> + print_super_dev(sb, io_align);
> + print_super_dev(sb, io_width);
> + print_super_dev(sb, sector_size);
> + print_super_dev(sb, id);
> + print_super_dev(sb, group);
> + print_super_dev(sb, seek_speed);
> + print_super_dev(sb, bandwidth);
> + print_super_dev(sb, generation);
> if (full) {
> printf("sys_chunk_array[%d]:\n", BTRFS_SYSTEM_CHUNK_ARRAY_SIZE);
> print_sys_chunk_array(sb);
>
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
2018-04-09 21:50 ` Goffredo Baroncelli
@ 2018-04-10 2:00 ` Qu Wenruo
2018-04-10 21:04 ` Goffredo Baroncelli
0 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2018-04-10 2:00 UTC (permalink / raw)
To: kreijack, Qu Wenruo, linux-btrfs
On 2018年04月10日 05:50, Goffredo Baroncelli wrote:
> Hi Qu,
>
> On 04/09/2018 11:19 AM, Qu Wenruo wrote:
>> When manually patching super blocks, current validation check is pretty
>> weak (limited to magic number and csum) and doesn't provide extra check
>> for some obvious corruption (like invalid sectorsize).
> [...]
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> changelog:
>> v2:
>> Generate tab based indent string in helper macro instead of passing spacer
>> string from outside, suggested by Nikolay.
>> (In fact, if using %*s it would be much easier, however it needs extra
>> rework for existing code as they still use tab as indent)
>> ---
>> cmds-inspect-dump-super.c | 206 +++++++++++++++++++++++++-------------
>> 1 file changed, 137 insertions(+), 69 deletions(-)
>>
>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
>> index 24588c87cce6..68d6f59ad727 100644
>> --- a/cmds-inspect-dump-super.c
>> +++ b/cmds-inspect-dump-super.c
>> @@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag)
>> super_flags_num, BTRFS_SUPER_FLAG_SUPP);
>> }
>>
>> +#define INDENT_BUFFER_LEN 80
>> +#define TAB_LEN 8
>> +#define SUPER_INDENT 24
>> +
>> +/* helper to generate tab based indent string */
>> +static void generate_tab_indent(char *buf, unsigned int indent)
>> +{
>> + buf[0] = '\0';
>> + for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN)
>> + strncat(buf, "\t", INDENT_BUFFER_LEN);
>> +}
>> +
>> +/* Helper to print member in %llu */
>> +#define print_super(sb, member) \
>> +({ \
>> + int indent = SUPER_INDENT - strlen(#member); \
>> + char indent_str[INDENT_BUFFER_LEN]; \
>> + \
>> + generate_tab_indent(indent_str, indent); \
>> + printf("%s%s%llu\n", #member, indent_str, \
>> + (u64) btrfs_super_##member(sb)); \
>
> Why not something like:
>
> static const char tabs[] = "\t\t\t\t"; \
> int i = (sizeof(#member)+6)/8; \
> if (i > sizeof(tabs)-1) \
> i = sizeof(tabs-1); \
> u64 value = (u64)btrfs_super_##member(sb); \
> printf("%s%s" format, \
> #member, tabs+i, value);
>
>
> so to get rid of generate_tab_indent and indent_str
And we need to call such functions in each helper macros, with
duplicated codes.
>
>> +})
>> +
>> +/* Helper to print member in %llx */
>> +#define print_super_hex(sb, member) \
>> +({ \
>> + int indent = SUPER_INDENT - strlen(#member); \
>> + char indent_str[INDENT_BUFFER_LEN]; \
>> + \
>> + generate_tab_indent(indent_str, indent); \
>> + printf("%s%s0x%llx\n", #member, indent_str, \
>> + (u64) btrfs_super_##member(sb)); \
>> +})
>> +
>> +/*
>> + * Helper macro to wrap member checker
>> + *
>> + * Only support %llu output for member.
>> + */
>> +#define print_check_super(sb, member, bad_condition) \
>> +({ \
>> + int indent = SUPER_INDENT - strlen(#member); \
>> + char indent_str[INDENT_BUFFER_LEN]; \
>> + u64 value; \
>> + \
>> + generate_tab_indent(indent_str, indent); \
>> + value = btrfs_super_##member(sb); \
>> + printf("%s%s%llu", #member, indent_str, (u64) value); \
>> + if (bad_condition) \
>> + printf(" [INVALID]"); \
>
> What about printing also the condition: something like
>
> + if (bad_condition) \
> + printf(" [INVALID '%s']", #bad_condition); \
>
> it would be even better a "good_condition":
When passing random stream to dump-super, such reason will make output
quite nasty.
So just INVALID to info the user that some of the members don't look
valid is good enough, as the tool is only to help guys who are going to
manually patching superblocks.
Thanks,
Qu
>
> so instead of:
> + print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
> do
> + print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));
>
> and
>
> + if (!good_condition) \
> + printf(" [ERROR: required '%s']", #good_condition); \
>
>
> All above functions could be written as:
>
> #define __print_and_check(sb, member, format, expected) \
> do{ \
> static const char tabs[] = "\t\t\t\t"; \
> int i = (sizeof(#member)+6)/8; \
> if (i > sizeof(tabs)-1) \
> i = sizeof(tabs-1); \
> u64 value = (u64)btrfs_super_##member(sb); \
> printf("%s%s" format, \
> #member, tabs+i, value); \
> if (!(expected)) \
> printf(" [ERROR: expected '%s']", #expected); \
> printf("\n"); \
> } while(0)
>
> #define print_super(sb, member) \
> __print_and_check(sb, member, "%llu", 1);
>
> #define print_super_hex(sb, member) \
> __print_and_check(sb, member, "0x%llx", 1);
>
> #define print_check_super(sb, member, condition) \
> __print_and_check(sb, member, "0x%llx", condition);
>
>
> And the value should be printed as (I removed the !):
>
> print_check_super(sb, root, (IS_ALIGNED(value, SZ_4K)));
>
> In case of error:
>
> test 12 [ERROR: expected 'IS_ALIGNED(value, SZ_4K)']
>
>
>
>
>> + printf("\n"); \
>> +})
>> +
>> +#define DEV_INDENT (SUPER_INDENT - strlen("dev_item."))
>> +BUILD_ASSERT(DEV_INDENT > 0);
>> +
>> +/* Helper to print sb->dev_item members */
>> +#define print_super_dev(sb, member) \
>> +({ \
>> + int indent = DEV_INDENT - strlen(#member); \
>> + char indent_str[INDENT_BUFFER_LEN]; \
>> + \
>> + generate_tab_indent(indent_str, indent); \
>> + printf("dev_item.%s%s%llu\n", #member, indent_str, \
>> + (u64) btrfs_stack_device_##member(&sb->dev_item)); \
>> +})
>> +
>> static void dump_superblock(struct btrfs_super_block *sb, int full)
>> {
>> int i;
>> char *s, buf[BTRFS_UUID_UNPARSED_SIZE];
>> + int max_level = BTRFS_MAX_LEVEL; /* to save several chars */
>> u8 *p;
>> + u64 super_gen;
>> u32 csum_size;
>> u16 csum_type;
>>
>> @@ -348,10 +417,16 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>> printf(" [DON'T MATCH]");
>> putchar('\n');
>>
>> - printf("bytenr\t\t\t%llu\n",
>> - (unsigned long long)btrfs_super_bytenr(sb));
>> - printf("flags\t\t\t0x%llx\n",
>> - (unsigned long long)btrfs_super_flags(sb));
>> + /*
>> + * Use btrfs minimal sector size as basic check for bytenr, since we
>> + * can't trust sector size from super block.
>> + * This 4K check should works fine for most architecture, and will be
>> + * just a little loose for 64K page system.
>> + *
>> + * And such 4K check will be used for other members too.
>> + */
>> + print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
>> + print_super_hex(sb, flags);
>> print_readable_super_flag(btrfs_super_flags(sb));
>>
>> printf("magic\t\t\t");
>> @@ -372,53 +447,56 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>> putchar(isprint(s[i]) ? s[i] : '.');
>> putchar('\n');
>>
>> - printf("generation\t\t%llu\n",
>> - (unsigned long long)btrfs_super_generation(sb));
>> - printf("root\t\t\t%llu\n", (unsigned long long)btrfs_super_root(sb));
>> - printf("sys_array_size\t\t%llu\n",
>> - (unsigned long long)btrfs_super_sys_array_size(sb));
>> - printf("chunk_root_generation\t%llu\n",
>> - (unsigned long long)btrfs_super_chunk_root_generation(sb));
>> - printf("root_level\t\t%llu\n",
>> - (unsigned long long)btrfs_super_root_level(sb));
>> - printf("chunk_root\t\t%llu\n",
>> - (unsigned long long)btrfs_super_chunk_root(sb));
>> - printf("chunk_root_level\t%llu\n",
>> - (unsigned long long)btrfs_super_chunk_root_level(sb));
>> - printf("log_root\t\t%llu\n",
>> - (unsigned long long)btrfs_super_log_root(sb));
>> - printf("log_root_transid\t%llu\n",
>> - (unsigned long long)btrfs_super_log_root_transid(sb));
>> - printf("log_root_level\t\t%llu\n",
>> - (unsigned long long)btrfs_super_log_root_level(sb));
>> - printf("total_bytes\t\t%llu\n",
>> - (unsigned long long)btrfs_super_total_bytes(sb));
>> - printf("bytes_used\t\t%llu\n",
>> - (unsigned long long)btrfs_super_bytes_used(sb));
>> - printf("sectorsize\t\t%llu\n",
>> - (unsigned long long)btrfs_super_sectorsize(sb));
>> - printf("nodesize\t\t%llu\n",
>> - (unsigned long long)btrfs_super_nodesize(sb));
>> + print_check_super(sb, sys_array_size,
>> + (value > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE));
>> +
>> + super_gen = btrfs_super_generation(sb);
>> + print_check_super(sb, root, (!IS_ALIGNED(value, SZ_4K)));
>> + print_check_super(sb, root_level, (value >= max_level));
>> + print_super(sb, generation);
>> +
>> + print_check_super(sb, chunk_root, (!IS_ALIGNED(value, SZ_4K)));
>> + print_check_super(sb, chunk_root_level, (value >= max_level));
>> + /*
>> + * Here we trust super generation, and use it as checker for other
>> + * tree roots. Applies to all other trees.
>> + */
>> + print_check_super(sb, chunk_root_generation, (value > super_gen));
>> +
>> + print_check_super(sb, log_root, (!IS_ALIGNED(value, SZ_4K)));
>> + print_check_super(sb, log_root_level, (value >= max_level));
>> + print_check_super(sb, log_root_transid, (value > super_gen + 1));
>> +
>> + /*
>> + * For total bytes, it's possible that old kernel is using unaligned
>> + * size, not critical so don't do 4K check here.
>> + */
>> + print_super(sb, total_bytes);
>> +
>> + /* Used bytes must be aligned to 4K */
>> + print_check_super(sb, bytes_used, (!IS_ALIGNED(value, SZ_4K)));
>> + print_check_super(sb, sectorsize,
>> + (!(is_power_of_2(value) && value >= SZ_4K &&
>> + value <= SZ_64K)));
>> + print_check_super(sb, nodesize,
>> + (!(is_power_of_2(value) && value >= SZ_4K &&
>> + value <= SZ_64K &&
>> + value > btrfs_super_sectorsize(sb))));
>> printf("leafsize (deprecated)\t%u\n",
>> le32_to_cpu(sb->__unused_leafsize));
>> - printf("stripesize\t\t%llu\n",
>> - (unsigned long long)btrfs_super_stripesize(sb));
>> - printf("root_dir\t\t%llu\n",
>> - (unsigned long long)btrfs_super_root_dir(sb));
>> - printf("num_devices\t\t%llu\n",
>> - (unsigned long long)btrfs_super_num_devices(sb));
>> - printf("compat_flags\t\t0x%llx\n",
>> - (unsigned long long)btrfs_super_compat_flags(sb));
>> - printf("compat_ro_flags\t\t0x%llx\n",
>> - (unsigned long long)btrfs_super_compat_ro_flags(sb));
>> +
>> + /* Not really used, just check it the same way as kernel */
>> + print_check_super(sb, stripesize, (!is_power_of_2(value)));
>> + print_super(sb, root_dir);
>> + print_super(sb, num_devices);
>> + print_super_hex(sb, compat_flags);
>> + print_super_hex(sb, compat_ro_flags);
>> print_readable_compat_ro_flag(btrfs_super_compat_ro_flags(sb));
>> - printf("incompat_flags\t\t0x%llx\n",
>> - (unsigned long long)btrfs_super_incompat_flags(sb));
>> + print_super_hex(sb, incompat_flags);
>> print_readable_incompat_flag(btrfs_super_incompat_flags(sb));
>> - printf("cache_generation\t%llu\n",
>> - (unsigned long long)btrfs_super_cache_generation(sb));
>> - printf("uuid_tree_generation\t%llu\n",
>> - (unsigned long long)btrfs_super_uuid_tree_generation(sb));
>> + print_check_super(sb, cache_generation,
>> + (value > super_gen && value != (u64)-1));
>> + print_check_super(sb, uuid_tree_generation, (value > super_gen));
>>
>> uuid_unparse(sb->dev_item.uuid, buf);
>> printf("dev_item.uuid\t\t%s\n", buf);
>> @@ -428,28 +506,18 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>> !memcmp(sb->dev_item.fsid, sb->fsid, BTRFS_FSID_SIZE) ?
>> "[match]" : "[DON'T MATCH]");
>>
>> - printf("dev_item.type\t\t%llu\n", (unsigned long long)
>> - btrfs_stack_device_type(&sb->dev_item));
>> - printf("dev_item.total_bytes\t%llu\n", (unsigned long long)
>> - btrfs_stack_device_total_bytes(&sb->dev_item));
>> - printf("dev_item.bytes_used\t%llu\n", (unsigned long long)
>> - btrfs_stack_device_bytes_used(&sb->dev_item));
>> - printf("dev_item.io_align\t%u\n", (unsigned int)
>> - btrfs_stack_device_io_align(&sb->dev_item));
>> - printf("dev_item.io_width\t%u\n", (unsigned int)
>> - btrfs_stack_device_io_width(&sb->dev_item));
>> - printf("dev_item.sector_size\t%u\n", (unsigned int)
>> - btrfs_stack_device_sector_size(&sb->dev_item));
>> - printf("dev_item.devid\t\t%llu\n",
>> - btrfs_stack_device_id(&sb->dev_item));
>> - printf("dev_item.dev_group\t%u\n", (unsigned int)
>> - btrfs_stack_device_group(&sb->dev_item));
>> - printf("dev_item.seek_speed\t%u\n", (unsigned int)
>> - btrfs_stack_device_seek_speed(&sb->dev_item));
>> - printf("dev_item.bandwidth\t%u\n", (unsigned int)
>> - btrfs_stack_device_bandwidth(&sb->dev_item));
>> - printf("dev_item.generation\t%llu\n", (unsigned long long)
>> - btrfs_stack_device_generation(&sb->dev_item));
>> + /* For embedded device item, don't do extra check, just like kernel */
>> + print_super_dev(sb, type);
>> + print_super_dev(sb, total_bytes);
>> + print_super_dev(sb, bytes_used);
>> + print_super_dev(sb, io_align);
>> + print_super_dev(sb, io_width);
>> + print_super_dev(sb, sector_size);
>> + print_super_dev(sb, id);
>> + print_super_dev(sb, group);
>> + print_super_dev(sb, seek_speed);
>> + print_super_dev(sb, bandwidth);
>> + print_super_dev(sb, generation);
>> if (full) {
>> printf("sys_chunk_array[%d]:\n", BTRFS_SYSTEM_CHUNK_ARRAY_SIZE);
>> print_sys_chunk_array(sb);
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
2018-04-10 2:00 ` Qu Wenruo
@ 2018-04-10 21:04 ` Goffredo Baroncelli
2018-04-11 0:32 ` Qu Wenruo
0 siblings, 1 reply; 8+ messages in thread
From: Goffredo Baroncelli @ 2018-04-10 21:04 UTC (permalink / raw)
To: Qu Wenruo, Qu Wenruo, linux-btrfs
Hi Qu,
On 04/10/2018 04:00 AM, Qu Wenruo wrote:
>
>
> On 2018年04月10日 05:50, Goffredo Baroncelli wrote:
>> Hi Qu,
>>
>> On 04/09/2018 11:19 AM, Qu Wenruo wrote:
>>> When manually patching super blocks, current validation check is pretty
>>> weak (limited to magic number and csum) and doesn't provide extra check
>>> for some obvious corruption (like invalid sectorsize).
>> [...]
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>> changelog:
>>> v2:
>>> Generate tab based indent string in helper macro instead of passing spacer
>>> string from outside, suggested by Nikolay.
>>> (In fact, if using %*s it would be much easier, however it needs extra
>>> rework for existing code as they still use tab as indent)
>>> ---
>>> cmds-inspect-dump-super.c | 206 +++++++++++++++++++++++++-------------
>>> 1 file changed, 137 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
>>> index 24588c87cce6..68d6f59ad727 100644
>>> --- a/cmds-inspect-dump-super.c
>>> +++ b/cmds-inspect-dump-super.c
>>> @@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag)
>>> super_flags_num, BTRFS_SUPER_FLAG_SUPP);
>>> }
>>>
>>> +#define INDENT_BUFFER_LEN 80
>>> +#define TAB_LEN 8
>>> +#define SUPER_INDENT 24
>>> +
>>> +/* helper to generate tab based indent string */
>>> +static void generate_tab_indent(char *buf, unsigned int indent)
>>> +{
>>> + buf[0] = '\0';
>>> + for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN)
>>> + strncat(buf, "\t", INDENT_BUFFER_LEN);
>>> +}
>>> +
>>> +/* Helper to print member in %llu */
>>> +#define print_super(sb, member) \
>>> +({ \
>>> + int indent = SUPER_INDENT - strlen(#member); \
>>> + char indent_str[INDENT_BUFFER_LEN]; \
>>> + \
>>> + generate_tab_indent(indent_str, indent); \
>>> + printf("%s%s%llu\n", #member, indent_str, \
>>> + (u64) btrfs_super_##member(sb)); \
>>
>> Why not something like:
>>
>> static const char tabs[] = "\t\t\t\t"; \
>> int i = (sizeof(#member)+6)/8; \
>> if (i > sizeof(tabs)-1) \
>> i = sizeof(tabs-1); \
>> u64 value = (u64)btrfs_super_##member(sb); \
>> printf("%s%s" format, \
>> #member, tabs+i, value);
>>
>>
>> so to get rid of generate_tab_indent and indent_str
>
> And we need to call such functions in each helper macros, with
> duplicated codes.
Please look at the asm generated: even if the "source generated" by the expansion of the macro is bigger, the binary code is smaller.
E.g. the code below
static const char tabs[] = "\t\t\t\t"; \
int i = (sizeof(#member)+6)/8; \
if (i > sizeof(tabs)-1) \
i = sizeof(tabs)-1; \
is translate as single move (see https://godbolt.org/g/4oxmAZ)
main::tabs:
.string "\t\t\t\t"
movl main::tabs+1, %edx
These days, the compilers are smart enough to evaluate the code above at compile time. This is not true for generate_tab_indent(), which is too complex.
Of course all the above consideration are meaningless, because I don't think that the size (or the speed) matters in the specific case of dump_superblock(). However I want to point out that the compiler sometime are smarter than the programmer (or almost smarter than me :-) ) in a surprising way, and what would seems bigger at the end results smaller.
[...]
>>> + */
>>> +#define print_check_super(sb, member, bad_condition) \
>>> +({ \
>>> + int indent = SUPER_INDENT - strlen(#member); \
>>> + char indent_str[INDENT_BUFFER_LEN]; \
>>> + u64 value; \
>>> + \
>>> + generate_tab_indent(indent_str, indent); \
>>> + value = btrfs_super_##member(sb); \
>>> + printf("%s%s%llu", #member, indent_str, (u64) value); \
>>> + if (bad_condition) \
>>> + printf(" [INVALID]"); \
>>
>> What about printing also the condition: something like
>>
>> + if (bad_condition) \
>> + printf(" [INVALID '%s']", #bad_condition); \
>>
>> it would be even better a "good_condition":
>
> When passing random stream to dump-super, such reason will make output
> quite nasty.
> So just INVALID to info the user that some of the members don't look
> valid is good enough, as the tool is only to help guys who are going to
> manually patching superblocks.
I think that we should increase the possible target also to who want to make some debugging :-)
>
> Thanks,
> Qu
>
>>
>> so instead of:
>> + print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
>> do
>> + print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));
>>
>> and
>>
>> + if (!good_condition) \
>> + printf(" [ERROR: required '%s']", #good_condition); \
>>
>>
>> All above functions could be written as:
>>
>> #define __print_and_check(sb, member, format, expected) \
>> do{ \
>> static const char tabs[] = "\t\t\t\t"; \
>> int i = (sizeof(#member)+6)/8; \
>> if (i > sizeof(tabs)-1) \
>> i = sizeof(tabs-1); \
The line above obviously is wrong: it should be "i = sizeof(tabs) -1;" :-)
>> u64 value = (u64)btrfs_super_##member(sb); \
>> printf("%s%s" format, \
>> #member, tabs+i, value); \
>> if (!(expected)) \
>> printf(" [ERROR: expected '%s']", #expected); \
As further example about the compiler smartness, if "expected" is evaluate as false at compile time, the "if" above is skipped
>> printf("\n"); \
>> } while(0)
>>
>> #define print_super(sb, member) \
>> __print_and_check(sb, member, "%llu", 1);
>>
>> #define print_super_hex(sb, member) \
>> __print_and_check(sb, member, "0x%llx", 1);
>>
>> #define print_check_super(sb, member, condition) \
>> __print_and_check(sb, member, "0x%llx", condition);
>>
BR
G.Baroncelli
>> And the value should be printed as (I removed the !):
>>
>> print_check_super(sb, root, (IS_ALIGNED(value, SZ_4K)));
>>
>> In case of error:
>>
>> test 12 [ERROR: expected 'IS_ALIGNED(value, SZ_4K)']
>>
>>
>>
>>
>>> + printf("\n"); \
>>> +})
>>> +
>>> +#define DEV_INDENT (SUPER_INDENT - strlen("dev_item."))
>>> +BUILD_ASSERT(DEV_INDENT > 0);
>>> +
>>> +/* Helper to print sb->dev_item members */
>>> +#define print_super_dev(sb, member) \
>>> +({ \
>>> + int indent = DEV_INDENT - strlen(#member); \
>>> + char indent_str[INDENT_BUFFER_LEN]; \
>>> + \
>>> + generate_tab_indent(indent_str, indent); \
>>> + printf("dev_item.%s%s%llu\n", #member, indent_str, \
>>> + (u64) btrfs_stack_device_##member(&sb->dev_item)); \
>>> +})
>>> +dump_superblock(
>>> static void dump_superblock(struct btrfs_super_block *sb, int full)
>>> {
>>> int i;
>>> char *s, buf[BTRFS_UUID_UNPARSED_SIZE];
>>> + int max_level = BTRFS_MAX_LEVEL; /* to save several chars */
>>> u8 *p;
>>> + u64 super_gen;
>>> u32 csum_size;
>>> u16 csum_type;
>>>
>>> @@ -348,10 +417,16 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>> printf(" [DON'T MATCH]");
>>> putchar('\n');
>>>
>>> - printf("bytenr\t\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_bytenr(sb));
>>> - printf("flags\t\t\t0x%llx\n",
>>> - (unsigned long long)btrfs_super_flags(sb));
>>> + /*
>>> + * Use btrfs minimal sector size as basic check for bytenr, since we
>>> + * can't trust sector size from super block.
>>> + * This 4K check should works fine for most architecture, and will be
>>> + * just a little loose for 64K page system.
>>> + *
>>> + * And such 4K check will be used for other members too.
>>> + */
>>> + print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
>>> + print_super_hex(sb, flags);
>>> print_readable_super_flag(btrfs_super_flags(sb));
>>>
>>> printf("magic\t\t\t");
>>> @@ -372,53 +447,56 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>> putchar(isprint(s[i]) ? s[i] : '.');
>>> putchar('\n');
>>>
>>> - printf("generation\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_generation(sb));
>>> - printf("root\t\t\t%llu\n", (unsigned long long)btrfs_super_root(sb));
>>> - printf("sys_array_size\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_sys_array_size(sb));
>>> - printf("chunk_root_generation\t%llu\n",
>>> - (unsigned long long)btrfs_super_chunk_root_generation(sb));
>>> - printf("root_level\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_root_level(sb));
>>> - printf("chunk_root\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_chunk_root(sb));
>>> - printf("chunk_root_level\t%llu\n",
>>> - (unsigned long long)btrfs_super_chunk_root_level(sb));
>>> - printf("log_root\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_log_root(sb));
>>> - printf("log_root_transid\t%llu\n",
>>> - (unsigned long long)btrfs_super_log_root_transid(sb));
>>> - printf("log_root_level\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_log_root_level(sb));
>>> - printf("total_bytes\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_total_bytes(sb));
>>> - printf("bytes_used\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_bytes_used(sb));
>>> - printf("sectorsize\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_sectorsize(sb));
>>> - printf("nodesize\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_nodesize(sb));
>>> + print_check_super(sb, sys_array_size,
>>> + (value > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE));
>>> +
>>> + super_gen = btrfs_super_generation(sb);
>>> + print_check_super(sb, root, (!IS_ALIGNED(value, SZ_4K)));
>>> + print_check_super(sb, root_level, (value >= max_level));
>>> + print_super(sb, generation);
>>> +
>>> + print_check_super(sb, chunk_root, (!IS_ALIGNED(value, SZ_4K)));
>>> + print_check_super(sb, chunk_root_level, (value >= max_level));
>>> + /*
>>> + * Here we trust super generation, and use it as checker for other
>>> + * tree roots. Applies to all other trees.
>>> + */
>>> + print_check_super(sb, chunk_root_generation, (value > super_gen));
>>> +
>>> + print_check_super(sb, log_root, (!IS_ALIGNED(value, SZ_4K)));
>>> + print_check_super(sb, log_root_level, (value >= max_level));
>>> + print_check_super(sb, log_root_transid, (value > super_gen + 1));
>>> +
>>> + /*
>>> + * For total bytes, it's possible that old kernel is using unaligned
>>> + * size, not critical so don't do 4K check here.
>>> + */
>>> + print_super(sb, total_bytes);
>>> +
>>> + /* Used bytes must be aligned to 4K */
>>> + print_check_super(sb, bytes_used, (!IS_ALIGNED(value, SZ_4K)));
>>> + print_check_super(sb, sectorsize,
>>> + (!(is_power_of_2(value) && value >= SZ_4K &&
>>> + value <= SZ_64K)));
>>> + print_check_super(sb, nodesize,
>>> + (!(is_power_of_2(value) && value >= SZ_4K &&
>>> + value <= SZ_64K &&
>>> + value > btrfs_super_sectorsize(sb))));
>>> printf("leafsize (deprecated)\t%u\n",
>>> le32_to_cpu(sb->__unused_leafsize));
>>> - printf("stripesize\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_stripesize(sb));
>>> - printf("root_dir\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_root_dir(sb));
>>> - printf("num_devices\t\t%llu\n",
>>> - (unsigned long long)btrfs_super_num_devices(sb));
>>> - printf("compat_flags\t\t0x%llx\n",
>>> - (unsigned long long)btrfs_super_compat_flags(sb));
>>> - printf("compat_ro_flags\t\t0x%llx\n",
>>> - (unsigned long long)btrfs_super_compat_ro_flags(sb));
>>> +
>>> + /* Not really used, just check it the same way as kernel */
>>> + print_check_super(sb, stripesize, (!is_power_of_2(value)));
>>> + print_super(sb, root_dir);
>>> + print_super(sb, num_devices);
>>> + print_super_hex(sb, compat_flags);
>>> + print_super_hex(sb, compat_ro_flags);
>>> print_readable_compat_ro_flag(btrfs_super_compat_ro_flags(sb));
>>> - printf("incompat_flags\t\t0x%llx\n",
>>> - (unsigned long long)btrfs_super_incompat_flags(sb));
>>> + print_super_hex(sb, incompat_flags);
>>> print_readable_incompat_flag(btrfs_super_incompat_flags(sb));
>>> - printf("cache_generation\t%llu\n",
>>> - (unsigned long long)btrfs_super_cache_generation(sb));
>>> - printf("uuid_tree_generation\t%llu\n",
>>> - (unsigned long long)btrfs_super_uuid_tree_generation(sb));
>>> + print_check_super(sb, cache_generation,
>>> + (value > super_gen && value != (u64)-1));
>>> + print_check_super(sb, uuid_tree_generation, (value > super_gen));
>>>
>>> uuid_unparse(sb->dev_item.uuid, buf);
>>> printf("dev_item.uuid\t\t%s\n", buf);
>>> @@ -428,28 +506,18 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>> !memcmp(sb->dev_item.fsid, sb->fsid, BTRFS_FSID_SIZE) ?
>>> "[match]" : "[DON'T MATCH]");
>>>
>>> - printf("dev_item.type\t\t%llu\n", (unsigned long long)
>>> - btrfs_stack_device_type(&sb->dev_item));
>>> - printf("dev_item.total_bytes\t%llu\n", (unsigned long long)
>>> - btrfs_stack_device_total_bytes(&sb->dev_item));
>>> - printf("dev_item.bytes_used\t%llu\n", (unsigned long long)
>>> - btrfs_stack_device_bytes_used(&sb->dev_item));
>>> - printf("dev_item.io_align\t%u\n", (unsigned int)
>>> - btrfs_stack_device_io_align(&sb->dev_item));
>>> - printf("dev_item.io_width\t%u\n", (unsigned int)
>>> - btrfs_stack_device_io_width(&sb->dev_item));
>>> - printf("dev_item.sector_size\t%u\n", (unsigned int)
>>> - btrfs_stack_device_sector_size(&sb->dev_item));
>>> - printf("dev_item.devid\t\t%llu\n",
>>> - btrfs_stack_device_id(&sb->dev_item));
>>> - printf("dev_item.dev_group\t%u\n", (unsigned int)
>>> - btrfs_stack_device_group(&sb->dev_item));
>>> - printf("dev_item.seek_speed\t%u\n", (unsigned int)
>>> - btrfs_stack_device_seek_speed(&sb->dev_item));
>>> - printf("dev_item.bandwidth\t%u\n", (unsigned int)
>>> - btrfs_stack_device_bandwidth(&sb->dev_item));
>>> - printf("dev_item.generation\t%llu\n", (unsigned long long)
>>> - btrfs_stack_device_generation(&sb->dev_item));
>>> + /* For embedded device item, don't do extra check, just like kernel */
>>> + print_super_dev(sb, type);
>>> + print_super_dev(sb, total_bytes);
>>> + print_super_dev(sb, bytes_used);
>>> + print_super_dev(sb, io_align);
>>> + print_super_dev(sb, io_width);
>>> + print_super_dev(sb, sector_size);
>>> + print_super_dev(sb, id);
>>> + print_super_dev(sb, group);
>>> + print_super_dev(sb, seek_speed);
>>> + print_super_dev(sb, bandwidth);
>>> + print_super_dev(sb, generation);
>>> if (full) {
>>> printf("sys_chunk_array[%d]:\n", BTRFS_SYSTEM_CHUNK_ARRAY_SIZE);
>>> print_sys_chunk_array(sb);
>>>
>>
>>
>
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
2018-04-10 21:04 ` Goffredo Baroncelli
@ 2018-04-11 0:32 ` Qu Wenruo
2018-04-11 17:15 ` Goffredo Baroncelli
0 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2018-04-11 0:32 UTC (permalink / raw)
To: kreijack, Qu Wenruo, linux-btrfs
On 2018年04月11日 05:04, Goffredo Baroncelli wrote:
> Hi Qu,
>
> On 04/10/2018 04:00 AM, Qu Wenruo wrote:
>>
>>
>> On 2018年04月10日 05:50, Goffredo Baroncelli wrote:
>>> Hi Qu,
>>>
>>> On 04/09/2018 11:19 AM, Qu Wenruo wrote:
>>>> When manually patching super blocks, current validation check is pretty
>>>> weak (limited to magic number and csum) and doesn't provide extra check
>>>> for some obvious corruption (like invalid sectorsize).
>>> [...]
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>> changelog:
>>>> v2:
>>>> Generate tab based indent string in helper macro instead of passing spacer
>>>> string from outside, suggested by Nikolay.
>>>> (In fact, if using %*s it would be much easier, however it needs extra
>>>> rework for existing code as they still use tab as indent)
>>>> ---
>>>> cmds-inspect-dump-super.c | 206 +++++++++++++++++++++++++-------------
>>>> 1 file changed, 137 insertions(+), 69 deletions(-)
>>>>
>>>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
>>>> index 24588c87cce6..68d6f59ad727 100644
>>>> --- a/cmds-inspect-dump-super.c
>>>> +++ b/cmds-inspect-dump-super.c
>>>> @@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag)
>>>> super_flags_num, BTRFS_SUPER_FLAG_SUPP);
>>>> }
>>>>
>>>> +#define INDENT_BUFFER_LEN 80
>>>> +#define TAB_LEN 8
>>>> +#define SUPER_INDENT 24
>>>> +
>>>> +/* helper to generate tab based indent string */
>>>> +static void generate_tab_indent(char *buf, unsigned int indent)
>>>> +{
>>>> + buf[0] = '\0';
>>>> + for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN)
>>>> + strncat(buf, "\t", INDENT_BUFFER_LEN);
>>>> +}
>>>> +
>>>> +/* Helper to print member in %llu */
>>>> +#define print_super(sb, member) \
>>>> +({ \
>>>> + int indent = SUPER_INDENT - strlen(#member); \
>>>> + char indent_str[INDENT_BUFFER_LEN]; \
>>>> + \
>>>> + generate_tab_indent(indent_str, indent); \
>>>> + printf("%s%s%llu\n", #member, indent_str, \
>>>> + (u64) btrfs_super_##member(sb)); \
>>>
>>> Why not something like:
>>>
>>> static const char tabs[] = "\t\t\t\t"; \
>>> int i = (sizeof(#member)+6)/8; \
>>> if (i > sizeof(tabs)-1) \
>>> i = sizeof(tabs-1); \
>>> u64 value = (u64)btrfs_super_##member(sb); \
>>> printf("%s%s" format, \
>>> #member, tabs+i, value);
>>>
>>>
>>> so to get rid of generate_tab_indent and indent_str
>>
>> And we need to call such functions in each helper macros, with
>> duplicated codes.
>
> Please look at the asm generated: even if the "source generated" by the expansion of the macro is bigger, the binary code is smaller.
> E.g. the code below
No, I don't mean asm code, but C code.
We should reduce duplicated code, as when it get modified we need to
modify all places, other than just one function.
For the generated asm code, it's compiler's work to optimize its size or
speed, not developer.
>
> static const char tabs[] = "\t\t\t\t"; \
> int i = (sizeof(#member)+6)/8; \
> if (i > sizeof(tabs)-1) \
> i = sizeof(tabs)-1; \
>
> is translate as single move (see https://godbolt.org/g/4oxmAZ)
>
> main::tabs:
> .string "\t\t\t\t"
>
> movl main::tabs+1, %edx
>
>
> These days, the compilers are smart enough to evaluate the code above at compile time. This is not true for generate_tab_indent(), which is too complex.
>
> Of course all the above consideration are meaningless, because I don't think that the size (or the speed) matters in the specific case of dump_superblock().
Exactly.
> However I want to point out that the compiler sometime are smarter than the programmer (or almost smarter than me :-) ) in a surprising way, and what would seems bigger at the end results smaller.
That's why our developer should care about C code duplications other
than generated asm code size.
>
> [...]
>
>>>> + */
>>>> +#define print_check_super(sb, member, bad_condition) \
>>>> +({ \
>>>> + int indent = SUPER_INDENT - strlen(#member); \
>>>> + char indent_str[INDENT_BUFFER_LEN]; \
>>>> + u64 value; \
>>>> + \
>>>> + generate_tab_indent(indent_str, indent); \
>>>> + value = btrfs_super_##member(sb); \
>>>> + printf("%s%s%llu", #member, indent_str, (u64) value); \
>>>> + if (bad_condition) \
>>>> + printf(" [INVALID]"); \
>>>
>>> What about printing also the condition: something like
>>>
>>> + if (bad_condition) \
>>> + printf(" [INVALID '%s']", #bad_condition); \
>>>
>>> it would be even better a "good_condition":
>>
>> When passing random stream to dump-super, such reason will make output
>> quite nasty.
>> So just INVALID to info the user that some of the members don't look
>> valid is good enough, as the tool is only to help guys who are going to
>> manually patching superblocks.
>
> I think that we should increase the possible target also to who want to make some debugging :-)
There are several problems here to output the condition
1) Loose condition
for basic alignment check it may looks good to output the condition, but
the fact is, the condition is not 100% correct for 64K pages system.
So when output IS_ALIGN(value, SZ_4K), it's not 100% correct.
2) Too long condition for some case.
!(is_power_of_2(value) && value >= SZ_4K && value <= SZ_64K)
This seems pretty strange in fact.
3) C macro usage
Just a minor problem, macro usage such as SZ_4K/SZ_64K may not looks
good for new developers.
Thanks,
Qu
>
>>
>> Thanks,
>> Qu
>>
>>>
>>> so instead of:
>>> + print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
>>> do
>>> + print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));
>>>
>>> and
>>>
>>> + if (!good_condition) \
>>> + printf(" [ERROR: required '%s']", #good_condition); \
>>>
>>>
>>> All above functions could be written as:
>>>
>>> #define __print_and_check(sb, member, format, expected) \
>>> do{ \
>>> static const char tabs[] = "\t\t\t\t"; \
>>> int i = (sizeof(#member)+6)/8; \
>>> if (i > sizeof(tabs)-1) \
>>> i = sizeof(tabs-1); \
>
> The line above obviously is wrong: it should be "i = sizeof(tabs) -1;" :-)
>
>>> u64 value = (u64)btrfs_super_##member(sb); \
>>> printf("%s%s" format, \
>>> #member, tabs+i, value); \
>>> if (!(expected)) \
>>> printf(" [ERROR: expected '%s']", #expected); \
>
> As further example about the compiler smartness, if "expected" is evaluate as false at compile time, the "if" above is skipped
>
>>> printf("\n"); \
>>> } while(0)
>>>
>>> #define print_super(sb, member) \
>>> __print_and_check(sb, member, "%llu", 1);
>>>
>>> #define print_super_hex(sb, member) \
>>> __print_and_check(sb, member, "0x%llx", 1);
>>>
>>> #define print_check_super(sb, member, condition) \
>>> __print_and_check(sb, member, "0x%llx", condition);
>>>
>
> BR
> G.Baroncelli
>
>
>>> And the value should be printed as (I removed the !):
>>>
>>> print_check_super(sb, root, (IS_ALIGNED(value, SZ_4K)));
>>>
>>> In case of error:
>>>
>>> test 12 [ERROR: expected 'IS_ALIGNED(value, SZ_4K)']
>>>
>>>
>>>
>>>
>>>> + printf("\n"); \
>>>> +})
>>>> +
>>>> +#define DEV_INDENT (SUPER_INDENT - strlen("dev_item."))
>>>> +BUILD_ASSERT(DEV_INDENT > 0);
>>>> +
>>>> +/* Helper to print sb->dev_item members */
>>>> +#define print_super_dev(sb, member) \
>>>> +({ \
>>>> + int indent = DEV_INDENT - strlen(#member); \
>>>> + char indent_str[INDENT_BUFFER_LEN]; \
>>>> + \
>>>> + generate_tab_indent(indent_str, indent); \
>>>> + printf("dev_item.%s%s%llu\n", #member, indent_str, \
>>>> + (u64) btrfs_stack_device_##member(&sb->dev_item)); \
>>>> +})
>>>> +dump_superblock(
>>>> static void dump_superblock(struct btrfs_super_block *sb, int full)
>>>> {
>>>> int i;
>>>> char *s, buf[BTRFS_UUID_UNPARSED_SIZE];
>>>> + int max_level = BTRFS_MAX_LEVEL; /* to save several chars */
>>>> u8 *p;
>>>> + u64 super_gen;
>>>> u32 csum_size;
>>>> u16 csum_type;
>>>>
>>>> @@ -348,10 +417,16 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>>> printf(" [DON'T MATCH]");
>>>> putchar('\n');
>>>>
>>>> - printf("bytenr\t\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_bytenr(sb));
>>>> - printf("flags\t\t\t0x%llx\n",
>>>> - (unsigned long long)btrfs_super_flags(sb));
>>>> + /*
>>>> + * Use btrfs minimal sector size as basic check for bytenr, since we
>>>> + * can't trust sector size from super block.
>>>> + * This 4K check should works fine for most architecture, and will be
>>>> + * just a little loose for 64K page system.
>>>> + *
>>>> + * And such 4K check will be used for other members too.
>>>> + */
>>>> + print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
>>>> + print_super_hex(sb, flags);
>>>> print_readable_super_flag(btrfs_super_flags(sb));
>>>>
>>>> printf("magic\t\t\t");
>>>> @@ -372,53 +447,56 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>>> putchar(isprint(s[i]) ? s[i] : '.');
>>>> putchar('\n');
>>>>
>>>> - printf("generation\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_generation(sb));
>>>> - printf("root\t\t\t%llu\n", (unsigned long long)btrfs_super_root(sb));
>>>> - printf("sys_array_size\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_sys_array_size(sb));
>>>> - printf("chunk_root_generation\t%llu\n",
>>>> - (unsigned long long)btrfs_super_chunk_root_generation(sb));
>>>> - printf("root_level\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_root_level(sb));
>>>> - printf("chunk_root\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_chunk_root(sb));
>>>> - printf("chunk_root_level\t%llu\n",
>>>> - (unsigned long long)btrfs_super_chunk_root_level(sb));
>>>> - printf("log_root\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_log_root(sb));
>>>> - printf("log_root_transid\t%llu\n",
>>>> - (unsigned long long)btrfs_super_log_root_transid(sb));
>>>> - printf("log_root_level\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_log_root_level(sb));
>>>> - printf("total_bytes\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_total_bytes(sb));
>>>> - printf("bytes_used\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_bytes_used(sb));
>>>> - printf("sectorsize\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_sectorsize(sb));
>>>> - printf("nodesize\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_nodesize(sb));
>>>> + print_check_super(sb, sys_array_size,
>>>> + (value > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE));
>>>> +
>>>> + super_gen = btrfs_super_generation(sb);
>>>> + print_check_super(sb, root, (!IS_ALIGNED(value, SZ_4K)));
>>>> + print_check_super(sb, root_level, (value >= max_level));
>>>> + print_super(sb, generation);
>>>> +
>>>> + print_check_super(sb, chunk_root, (!IS_ALIGNED(value, SZ_4K)));
>>>> + print_check_super(sb, chunk_root_level, (value >= max_level));
>>>> + /*
>>>> + * Here we trust super generation, and use it as checker for other
>>>> + * tree roots. Applies to all other trees.
>>>> + */
>>>> + print_check_super(sb, chunk_root_generation, (value > super_gen));
>>>> +
>>>> + print_check_super(sb, log_root, (!IS_ALIGNED(value, SZ_4K)));
>>>> + print_check_super(sb, log_root_level, (value >= max_level));
>>>> + print_check_super(sb, log_root_transid, (value > super_gen + 1));
>>>> +
>>>> + /*
>>>> + * For total bytes, it's possible that old kernel is using unaligned
>>>> + * size, not critical so don't do 4K check here.
>>>> + */
>>>> + print_super(sb, total_bytes);
>>>> +
>>>> + /* Used bytes must be aligned to 4K */
>>>> + print_check_super(sb, bytes_used, (!IS_ALIGNED(value, SZ_4K)));
>>>> + print_check_super(sb, sectorsize,
>>>> + (!(is_power_of_2(value) && value >= SZ_4K &&
>>>> + value <= SZ_64K)));
>>>> + print_check_super(sb, nodesize,
>>>> + (!(is_power_of_2(value) && value >= SZ_4K &&
>>>> + value <= SZ_64K &&
>>>> + value > btrfs_super_sectorsize(sb))));
>>>> printf("leafsize (deprecated)\t%u\n",
>>>> le32_to_cpu(sb->__unused_leafsize));
>>>> - printf("stripesize\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_stripesize(sb));
>>>> - printf("root_dir\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_root_dir(sb));
>>>> - printf("num_devices\t\t%llu\n",
>>>> - (unsigned long long)btrfs_super_num_devices(sb));
>>>> - printf("compat_flags\t\t0x%llx\n",
>>>> - (unsigned long long)btrfs_super_compat_flags(sb));
>>>> - printf("compat_ro_flags\t\t0x%llx\n",
>>>> - (unsigned long long)btrfs_super_compat_ro_flags(sb));
>>>> +
>>>> + /* Not really used, just check it the same way as kernel */
>>>> + print_check_super(sb, stripesize, (!is_power_of_2(value)));
>>>> + print_super(sb, root_dir);
>>>> + print_super(sb, num_devices);
>>>> + print_super_hex(sb, compat_flags);
>>>> + print_super_hex(sb, compat_ro_flags);
>>>> print_readable_compat_ro_flag(btrfs_super_compat_ro_flags(sb));
>>>> - printf("incompat_flags\t\t0x%llx\n",
>>>> - (unsigned long long)btrfs_super_incompat_flags(sb));
>>>> + print_super_hex(sb, incompat_flags);
>>>> print_readable_incompat_flag(btrfs_super_incompat_flags(sb));
>>>> - printf("cache_generation\t%llu\n",
>>>> - (unsigned long long)btrfs_super_cache_generation(sb));
>>>> - printf("uuid_tree_generation\t%llu\n",
>>>> - (unsigned long long)btrfs_super_uuid_tree_generation(sb));
>>>> + print_check_super(sb, cache_generation,
>>>> + (value > super_gen && value != (u64)-1));
>>>> + print_check_super(sb, uuid_tree_generation, (value > super_gen));
>>>>
>>>> uuid_unparse(sb->dev_item.uuid, buf);
>>>> printf("dev_item.uuid\t\t%s\n", buf);
>>>> @@ -428,28 +506,18 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>>> !memcmp(sb->dev_item.fsid, sb->fsid, BTRFS_FSID_SIZE) ?
>>>> "[match]" : "[DON'T MATCH]");
>>>>
>>>> - printf("dev_item.type\t\t%llu\n", (unsigned long long)
>>>> - btrfs_stack_device_type(&sb->dev_item));
>>>> - printf("dev_item.total_bytes\t%llu\n", (unsigned long long)
>>>> - btrfs_stack_device_total_bytes(&sb->dev_item));
>>>> - printf("dev_item.bytes_used\t%llu\n", (unsigned long long)
>>>> - btrfs_stack_device_bytes_used(&sb->dev_item));
>>>> - printf("dev_item.io_align\t%u\n", (unsigned int)
>>>> - btrfs_stack_device_io_align(&sb->dev_item));
>>>> - printf("dev_item.io_width\t%u\n", (unsigned int)
>>>> - btrfs_stack_device_io_width(&sb->dev_item));
>>>> - printf("dev_item.sector_size\t%u\n", (unsigned int)
>>>> - btrfs_stack_device_sector_size(&sb->dev_item));
>>>> - printf("dev_item.devid\t\t%llu\n",
>>>> - btrfs_stack_device_id(&sb->dev_item));
>>>> - printf("dev_item.dev_group\t%u\n", (unsigned int)
>>>> - btrfs_stack_device_group(&sb->dev_item));
>>>> - printf("dev_item.seek_speed\t%u\n", (unsigned int)
>>>> - btrfs_stack_device_seek_speed(&sb->dev_item));
>>>> - printf("dev_item.bandwidth\t%u\n", (unsigned int)
>>>> - btrfs_stack_device_bandwidth(&sb->dev_item));
>>>> - printf("dev_item.generation\t%llu\n", (unsigned long long)
>>>> - btrfs_stack_device_generation(&sb->dev_item));
>>>> + /* For embedded device item, don't do extra check, just like kernel */
>>>> + print_super_dev(sb, type);
>>>> + print_super_dev(sb, total_bytes);
>>>> + print_super_dev(sb, bytes_used);
>>>> + print_super_dev(sb, io_align);
>>>> + print_super_dev(sb, io_width);
>>>> + print_super_dev(sb, sector_size);
>>>> + print_super_dev(sb, id);
>>>> + print_super_dev(sb, group);
>>>> + print_super_dev(sb, seek_speed);
>>>> + print_super_dev(sb, bandwidth);
>>>> + print_super_dev(sb, generation);
>>>> if (full) {
>>>> printf("sys_chunk_array[%d]:\n", BTRFS_SYSTEM_CHUNK_ARRAY_SIZE);
>>>> print_sys_chunk_array(sb);
>>>>
>>>
>>>
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
2018-04-11 0:32 ` Qu Wenruo
@ 2018-04-11 17:15 ` Goffredo Baroncelli
2018-04-11 19:43 ` Nikolay Borisov
2018-04-11 23:21 ` Qu Wenruo
0 siblings, 2 replies; 8+ messages in thread
From: Goffredo Baroncelli @ 2018-04-11 17:15 UTC (permalink / raw)
To: Qu Wenruo, Qu Wenruo, linux-btrfs
On 04/11/2018 02:32 AM, Qu Wenruo wrote:
[...]
>>>> so to get rid of generate_tab_indent and indent_str
>>>
>>> And we need to call such functions in each helper macros, with
>>> duplicated codes.
>>
>> Please look at the asm generated: even if the "source generated" by the expansion of the macro is bigger, the binary code is smaller.
>> E.g. the code below
>
> No, I don't mean asm code, but C code.
May be that there is some misunderstanding: my code is about 20loc, your one is about 50loc... I am missing something ?
[...]
>>> When passing random stream to dump-super, such reason will make output
>>> quite nasty.
>>> So just INVALID to info the user that some of the members don't look
>>> valid is good enough, as the tool is only to help guys who are going to
>>> manually patching superblocks.
>>
>> I think that we should increase the possible target also to who want to make some debugging :-)
>
> There are several problems here to output the condition
>
> 1) Loose condition
> for basic alignment check it may looks good to output the condition, but
> the fact is, the condition is not 100% correct for 64K pages system.
> So when output IS_ALIGN(value, SZ_4K), it's not 100% correct.
I don't understand your statement: does the alignment is the same for all the system ?. If not, this means that a filesystem created on a x86 might not work on a PPC64 (which IIRC is a 64k page hardware) ?
>
> 2) Too long condition for some case.
> !(is_power_of_2(value) && value >= SZ_4K && value <= SZ_64K)
> This seems pretty strange in fact.
I agree that this is quite verbose... however if this is the constraint, why not print it
>
> 3) C macro usage
> Just a minor problem, macro usage such as SZ_4K/SZ_64K may not looks
> good for new developers.
I find it quite clear and intuitive even if this is the first time that I notice these macros
[...]
>
> Thanks,
> Qu
BR
G.Baroncelli
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
2018-04-11 17:15 ` Goffredo Baroncelli
@ 2018-04-11 19:43 ` Nikolay Borisov
2018-04-11 23:21 ` Qu Wenruo
1 sibling, 0 replies; 8+ messages in thread
From: Nikolay Borisov @ 2018-04-11 19:43 UTC (permalink / raw)
To: kreijack, Qu Wenruo, Qu Wenruo, linux-btrfs
On 11.04.2018 20:15, Goffredo Baroncelli wrote:
> On 04/11/2018 02:32 AM, Qu Wenruo wrote:
> [...]
>>>>> so to get rid of generate_tab_indent and indent_str
>>>>
>>>> And we need to call such functions in each helper macros, with
>>>> duplicated codes.
>>>
>>> Please look at the asm generated: even if the "source generated" by the expansion of the macro is bigger, the binary code is smaller.
>>> E.g. the code below
>>
>> No, I don't mean asm code, but C code.
>
> May be that there is some misunderstanding: my code is about 20loc, your one is about 50loc... I am missing something ?
>
> [...]
>>>> When passing random stream to dump-super, such reason will make output
>>>> quite nasty.
>>>> So just INVALID to info the user that some of the members don't look
>>>> valid is good enough, as the tool is only to help guys who are going to
>>>> manually patching superblocks.
>>>
>>> I think that we should increase the possible target also to who want to make some debugging :-)
>>
>> There are several problems here to output the condition
>>
>> 1) Loose condition
>> for basic alignment check it may looks good to output the condition, but
>> the fact is, the condition is not 100% correct for 64K pages system.
>> So when output IS_ALIGN(value, SZ_4K), it's not 100% correct.
>
> I don't understand your statement: does the alignment is the same for all the system ?. If not, this means that a filesystem created on a x86 might not work on a PPC64 (which IIRC is a 64k page hardware) ?
>
That is true, since btrfs doesn't support subpage blocksizes you cannot
mount an fs created on a system with pagesize A, on a system with
pagesize B.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
2018-04-11 17:15 ` Goffredo Baroncelli
2018-04-11 19:43 ` Nikolay Borisov
@ 2018-04-11 23:21 ` Qu Wenruo
1 sibling, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2018-04-11 23:21 UTC (permalink / raw)
To: kreijack, Qu Wenruo, linux-btrfs
On 2018年04月12日 01:15, Goffredo Baroncelli wrote:
> On 04/11/2018 02:32 AM, Qu Wenruo wrote:
> [...]
>>>>> so to get rid of generate_tab_indent and indent_str
>>>>
>>>> And we need to call such functions in each helper macros, with
>>>> duplicated codes.
>>>
>>> Please look at the asm generated: even if the "source generated" by the expansion of the macro is bigger, the binary code is smaller.
>>> E.g. the code below
>>
>> No, I don't mean asm code, but C code.
>
> May be that there is some misunderstanding: my code is about 20loc, your one is about 50loc... I am missing something ?
Loc is not the core factor, it's duplicated code.
>
> [...]
>>>> When passing random stream to dump-super, such reason will make output
>>>> quite nasty.
>>>> So just INVALID to info the user that some of the members don't look
>>>> valid is good enough, as the tool is only to help guys who are going to
>>>> manually patching superblocks.
>>>
>>> I think that we should increase the possible target also to who want to make some debugging :-)
>>
>> There are several problems here to output the condition
>>
>> 1) Loose condition
>> for basic alignment check it may looks good to output the condition, but
>> the fact is, the condition is not 100% correct for 64K pages system.
>> So when output IS_ALIGN(value, SZ_4K), it's not 100% correct.
>
> I don't understand your statement: does the alignment is the same for all the system ?.
As already mentioned by Nikolay, no.
> If not, this means that a filesystem created on a x86 might not work on a PPC64 (which IIRC is a 64k page hardware) ?
Yep, btrfs created on x86 will not work on any 64K page size system, and
vice verse.
IBM guy is working on such sub-page size support to allow PPC64 to mount
4K sector size btrfs, but I didn't see their update for a long time.
>
>
>>
>> 2) Too long condition for some case.
>> !(is_power_of_2(value) && value >= SZ_4K && value <= SZ_64K)
>> This seems pretty strange in fact.
>
> I agree that this is quite verbose... however if this is the constraint, why not print it
Because when it's invalid, the constraint doesn't help much for
developer/user to patch the value.
Thanks,
Qu
>
>>
>> 3) C macro usage
>> Just a minor problem, macro usage such as SZ_4K/SZ_64K may not looks
>> good for new developers.
>
> I find it quite clear and intuitive even if this is the first time that I notice these macros
>
> [...]
>>
>> Thanks,
>> Qu
> BR
> G.Baroncelli
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-04-11 23:21 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-09 9:19 [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check Qu Wenruo
2018-04-09 21:50 ` Goffredo Baroncelli
2018-04-10 2:00 ` Qu Wenruo
2018-04-10 21:04 ` Goffredo Baroncelli
2018-04-11 0:32 ` Qu Wenruo
2018-04-11 17:15 ` Goffredo Baroncelli
2018-04-11 19:43 ` Nikolay Borisov
2018-04-11 23:21 ` Qu Wenruo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.