btrfs-progs: dump-super: Refactor print function and add extra check

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 <[email protected]>

---
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(-)

cmds-inspect-dump-super.c +11 -0

index 24588c87cce6..68d6f59ad727 100644
@@ -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);
+}

cmds-inspect-dump-super.c +19 -0

+})
+
+/* 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)			\

cmds-inspect-dump-super.c +87 -69

+	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);

Hi Qu,

Quoted 3 lines · Qu Wenruo

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

[...]

Quoted 32 lines

Signed-off-by Qu Wenruo <[email protected]>

---
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(-)

cmds-inspect-dump-super.c +11 -0

index 24588c87cce6..68d6f59ad727 100644
@@ -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);
+}

cmds-inspect-dump-super.c static void print_readable_super_flag(u64 flag) · line 335

Quoted 10 lines

cmds-inspect-dump-super.c +10 -0

+
+/* 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

Quoted 20 lines

cmds-inspect-dump-super.c +19 -0

+})
+
+/* 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)			\

cmds-inspect-dump-super.c static void print_readable_super_flag(u64 flag) · line 364

Quoted 10 lines

cmds-inspect-dump-super.c +10 -0

+({									\
+	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

(quoted diff) +2 -0

+	if (bad_condition)						\
+		printf(" [INVALID '%s']", #bad_condition);		\

it would be even better a "good_condition":

so instead of:

cmds-inspect-dump-super.c +1 -0

+	print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));

do

(quoted diff) +1 -0

+	print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));

and

(quoted diff) +2 -0

+	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)']

Quoted 190 lines

cmds-inspect-dump-super.c +87 -69

+	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);

Quoted 61 lines · Goffredo Baroncelli

Hi Qu,

Quoted 3 lines · Qu Wenruo

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

[...]

Quoted 32 lines

Signed-off-by Qu Wenruo <[email protected]>

---
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(-)

cmds-inspect-dump-super.c +11 -0

index 24588c87cce6..68d6f59ad727 100644
@@ -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);
+}

cmds-inspect-dump-super.c static void print_readable_super_flag(u64 flag) · line 335

Quoted 10 lines

cmds-inspect-dump-super.c +10 -0

+
+/* 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);  

Quoted 1 line

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.

Quoted 18 lines

cmds-inspect-dump-super.c static void print_readable_super_flag(u64 flag) · line 364

Quoted 12 lines

cmds-inspect-dump-super.c +12 -0

+ */
+#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

(quoted diff) +2 -0

+	if (bad_condition)						\
+		printf(" [INVALID '%s']", #bad_condition);		\

Quoted 1 line

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

Quoted 12 lines

so instead of:

cmds-inspect-dump-super.c +1 -0

+	print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));

do

(quoted diff) +1 -0

+	print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));

and

(quoted diff) +2 -0

+	if (!good_condition)						\
+		printf(" [ERROR: required '%s']", #good_condition);	\

Quoted 8 lines

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);                       \

Quoted 5 lines

            u64 value = (u64)btrfs_super_##member(sb);    \
            printf("%s%s" format,                         \
                #member, tabs+i, value);                  \
            if (!(expected))                              \
                printf(" [ERROR: expected '%s']", #expected);    \

Quoted 6 lines

            printf("\n");                           \
         } while(0)

  #define print_super(sb, member) \
    __print_and_check(sb, member, "%llu", 1);

Quoted 6 lines

  #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);

Quoted 203 lines

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)']

Quoted 190 lines

cmds-inspect-dump-super.c +87 -69

+	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);

Hi Qu,

Quoted 64 lines · Qu Wenruo

Quoted 61 lines · Goffredo Baroncelli

Hi Qu,

Quoted 3 lines · Qu Wenruo

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

[...]

Quoted 32 lines

Signed-off-by Qu Wenruo <[email protected]>

---
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(-)

cmds-inspect-dump-super.c +11 -0

index 24588c87cce6..68d6f59ad727 100644
@@ -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);
+}

cmds-inspect-dump-super.c static void print_readable_super_flag(u64 flag) · line 335

Quoted 10 lines

cmds-inspect-dump-super.c +10 -0

+
+/* 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);  

Quoted 4 lines

Quoted 1 line

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.

[...]

Quoted 18 lines

Quoted 18 lines

cmds-inspect-dump-super.c static void print_readable_super_flag(u64 flag) · line 364

Quoted 12 lines

+ */

cmds-inspect-dump-super.c +11 -0

+#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

(quoted diff) +2 -0

+	if (bad_condition)						\
+		printf(" [INVALID '%s']", #bad_condition);		\

Quoted 7 lines

Quoted 1 line

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 :-)

Quoted 17 lines

Thanks,
Qu

Quoted 12 lines

so instead of:

cmds-inspect-dump-super.c +1 -0

+	print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));

do

(quoted diff) +1 -0

+	print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));

and

(quoted diff) +2 -0

+	if (!good_condition)						\
+		printf(" [ERROR: required '%s']", #good_condition);	\

Quoted 8 lines

Quoted 8 lines

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;" :-)

Quoted 5 lines

Quoted 5 lines

            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

Quoted 6 lines

Quoted 6 lines

            printf("\n");                           \
         } while(0)

  #define print_super(sb, member) \
    __print_and_check(sb, member, "%llu", 1);

Quoted 6 lines

Quoted 6 lines

  #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

Quoted 204 lines

Quoted 203 lines

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)']

Quoted 190 lines

cmds-inspect-dump-super.c +87 -69

+	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);

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 1 line

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

On 04/11/2018 02:32 AM, Qu Wenruo wrote:
[...]

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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 ?

[...]

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

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 :-)

Quoted 6 lines

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) ?

On 04/11/2018 02:32 AM, Qu Wenruo wrote:
[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

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 ?

[...]

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

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 :-)

Quoted 6 lines

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) ?

Quoted 6 lines

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

Quoted 5 lines

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

[...]

Quoted 3 lines

Thanks,
Qu

BR
G.Baroncelli