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

[Qu Wenruo]

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nikolay Borisov]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Goffredo Baroncelli]

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 
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Qu Wenruo]

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

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

[Goffredo Baroncelli]

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