Re: [PATCH] btrfs-progs: dump-super: Refactor print function and add extra check

2018-04-09 Thread Qu Wenruo 


On 2018年04月09日 16:06, Nikolay Borisov wrote:
> 
> 
> On  9.04.2018 10:47, 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).
>>
>> 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.
>>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 
>> ---
>>  cmds-inspect-dump-super.c | 168 ++
>>  1 file changed, 99 insertions(+), 69 deletions(-)
>>
>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
>> index 24588c87cce6..8000d2ace663 100644
>> --- a/cmds-inspect-dump-super.c
>> +++ b/cmds-inspect-dump-super.c
>> @@ -312,11 +312,42 @@ static void print_readable_super_flag(u64 flag)
>>   super_flags_num, BTRFS_SUPER_FLAG_SUPP);
>>  }
>>  
>> +/* Helper to print member in %llu */
>> +#define print_super(sb, member, spacer) 
>> \
>> +printf("%s%s%llu\n", #member, spacer, (u64) btrfs_super_##member(sb));
>> +
>> +/* Helper to print member in %llx */
>> +#define print_super_hex(sb, member, spacer) \
>> +printf("%s%s%llx\n", #member, spacer, (u64) btrfs_super_##member(sb));
> 
> nit: I like the idea of the patch, however, I think the presence of the
> "spacer" argument is a bit annoying. Why don't you make all the print
> out to be aligned to the same level i.e hardcode \t\t\ or \t\t or whatever?

Makes sense.
Since currently all data output is aligned to "\t\t\t", it's not that
hard to do alignment calculation in the macro.

Thanks,
Qu

>> +
>> +/*
>> + * Helper macro to wrap member checker
>> + *
>> + * Only support %llu output for member.
>> + */
>> +#define print_check_super(sb, member, spacer, bad_condition)
>> \
>> +({  \
>> +u64 value;  \
>> +\
>> +value = btrfs_super_##member(sb);   \
>> +printf("%s%s%llu", #member, spacer, (u64) value);   \
>> +if (bad_condition)  \
>> +printf(" [INVALID]");   \
>> +printf("\n");   \
>> +})
>> +
>> +/* Helper to print sb->dev_item members */
>> +#define print_super_dev(sb, member, spacer) \
>> +printf("dev_item.%s%s%llu\n", #member, spacer,  \
>> +   (u64) btrfs_stack_device_##member(>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 +379,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

Re: [PATCH] btrfs-progs: dump-super: Refactor print function and add extra check

2018-04-09 Thread Nikolay Borisov 


On  9.04.2018 10:47, 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).
> 
> 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.
>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 
> ---
>  cmds-inspect-dump-super.c | 168 ++
>  1 file changed, 99 insertions(+), 69 deletions(-)
> 
> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
> index 24588c87cce6..8000d2ace663 100644
> --- a/cmds-inspect-dump-super.c
> +++ b/cmds-inspect-dump-super.c
> @@ -312,11 +312,42 @@ static void print_readable_super_flag(u64 flag)
>super_flags_num, BTRFS_SUPER_FLAG_SUPP);
>  }
>  
> +/* Helper to print member in %llu */
> +#define print_super(sb, member, spacer)  
> \
> + printf("%s%s%llu\n", #member, spacer, (u64) btrfs_super_##member(sb));
> +
> +/* Helper to print member in %llx */
> +#define print_super_hex(sb, member, spacer)  \
> + printf("%s%s%llx\n", #member, spacer, (u64) btrfs_super_##member(sb));

nit: I like the idea of the patch, however, I think the presence of the
"spacer" argument is a bit annoying. Why don't you make all the print
out to be aligned to the same level i.e hardcode \t\t\ or \t\t or whatever?
> +
> +/*
> + * Helper macro to wrap member checker
> + *
> + * Only support %llu output for member.
> + */
> +#define print_check_super(sb, member, spacer, bad_condition) \
> +({   \
> + u64 value;  \
> + \
> + value = btrfs_super_##member(sb);   \
> + printf("%s%s%llu", #member, spacer, (u64) value);   \
> + if (bad_condition)  \
> + printf(" [INVALID]");   \
> + printf("\n");   \
> +})
> +
> +/* Helper to print sb->dev_item members */
> +#define print_super_dev(sb, member, spacer)  \
> + printf("dev_item.%s%s%llu\n", #member, spacer,  \
> +(u64) btrfs_stack_device_##member(>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 +379,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

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

2018-04-09 Thread 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).

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.
   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 
---
 cmds-inspect-dump-super.c | 168 ++
 1 file changed, 99 insertions(+), 69 deletions(-)

diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
index 24588c87cce6..8000d2ace663 100644
--- a/cmds-inspect-dump-super.c
+++ b/cmds-inspect-dump-super.c
@@ -312,11 +312,42 @@ static void print_readable_super_flag(u64 flag)
 super_flags_num, BTRFS_SUPER_FLAG_SUPP);
 }
 
+/* Helper to print member in %llu */
+#define print_super(sb, member, spacer)
\
+   printf("%s%s%llu\n", #member, spacer, (u64) btrfs_super_##member(sb));
+
+/* Helper to print member in %llx */
+#define print_super_hex(sb, member, spacer)\
+   printf("%s%s%llx\n", #member, spacer, (u64) btrfs_super_##member(sb));
+
+/*
+ * Helper macro to wrap member checker
+ *
+ * Only support %llu output for member.
+ */
+#define print_check_super(sb, member, spacer, bad_condition)   \
+({ \
+   u64 value;  \
+   \
+   value = btrfs_super_##member(sb);   \
+   printf("%s%s%llu", #member, spacer, (u64) value);   \
+   if (bad_condition)  \
+   printf(" [INVALID]");   \
+   printf("\n");   \
+})
+
+/* Helper to print sb->dev_item members */
+#define print_super_dev(sb, member, spacer)\
+   printf("dev_item.%s%s%llu\n", #member, spacer,  \
+  (u64) btrfs_stack_device_##member(>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 +379,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, "\t\t\t", (!IS_ALIGNED(value, SZ_4K)));
+   print_super_hex(sb, flags, "\t\t\t");