[PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Qu Wenruo]

[BUG]
Commit ("btrfs-progs: use raid table for profile names in
print-tree.c") introduced one bug in block group and chunk flags output
and changed the behavior:

	item 1 key (FIRST_CHUNK_TREE CHUNK_ITEM 13631488) itemoff 16105 itemsize 80
		length 8388608 owner 2 stripe_len 65536 type SINGLE
		...
	item 2 key (FIRST_CHUNK_TREE CHUNK_ITEM 22020096) itemoff 15993 itemsize 112
		length 8388608 owner 2 stripe_len 65536 type DUP
		...
	item 3 key (FIRST_CHUNK_TREE CHUNK_ITEM 30408704) itemoff 15881 itemsize 112
		length 268435456 owner 2 stripe_len 65536 type DUP
		...

Note that, the flag string only contains the profile (SINGLE/DUP/etc...)
no type (DATA/METADATA/SYSTEM).

And we have new "SINGLE" string, even that profile has no extra bit to
indicate that.

[CAUSE]
The "SINGLE" part is caused by the raid array which has a name for
SINGLE profile, even it doesn't have corresponding bit.

The missing type string is caused by a code bug:

		strcpy(buf, name);
		while (*tmp) {
			*tmp = toupper(*tmp);
			tmp++;
		}
		strcpy(ret, buf);

The last strcpy() call overrides the existing string in @ret.

[FIX]
- Enhance string handling using strn*()/snprintf()

- Add extra "UKNOWN.0x%llx" output for unknown profiles

- Call proper strncat() to merge type and profile

- Add extra handling for "SINGLE" to keep the old output

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/print-tree.c | 47 +++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index 67b654e6d2d5..39655590272e 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c
@@ -159,40 +159,51 @@ static void print_inode_ref_item(struct extent_buffer *eb, u32 size,
 	}
 }
 
-/* Caller should ensure sizeof(*ret)>=21 "DATA|METADATA|RAID10" */
+/* The minimal length for the string buffer of block group/chunk flags */
+#define BG_FLAG_STRING_LEN	64
+
 static void bg_flags_to_str(u64 flags, char *ret)
 {
 	int empty = 1;
+	char profile[BG_FLAG_STRING_LEN] = {};
 	const char *name;
 
+	ret[0] = '\0';
 	if (flags & BTRFS_BLOCK_GROUP_DATA) {
 		empty = 0;
-		strcpy(ret, "DATA");
+		strncpy(ret, "DATA", BG_FLAG_STRING_LEN);
 	}
 	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
 		if (!empty)
-			strcat(ret, "|");
-		strcat(ret, "METADATA");
+			strncat(ret, "|", BG_FLAG_STRING_LEN);
+		strncat(ret, "METADATA", BG_FLAG_STRING_LEN);
 	}
 	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
 		if (!empty)
-			strcat(ret, "|");
-		strcat(ret, "SYSTEM");
+			strncat(ret, "|", BG_FLAG_STRING_LEN);
+		strncat(ret, "SYSTEM", BG_FLAG_STRING_LEN);
 	}
-	strcat(ret, "|");
 	name = btrfs_bg_type_to_raid_name(flags);
 	if (!name) {
-		strcat(ret, "UNKNOWN");
+		snprintf(profile, BG_FLAG_STRING_LEN, "UNKNOWN.0x%llx",
+			 flags & BTRFS_BLOCK_GROUP_PROFILE_MASK);
 	} else {
-		char buf[32];
-		char *tmp = buf;
+		int i;
 
-		strcpy(buf, name);
-		while (*tmp) {
-			*tmp = toupper(*tmp);
-			tmp++;
-		}
-		strcpy(ret, buf);
+		/*
+		 * Special handing for SINGLE profile, we don't output "SINGLE"
+		 * for SINGLE profile, since there is no such bit for it.
+		 * Thus here we only fill @profile if it's not single.
+		 */
+		if (strncmp(name, "single", strlen("single")) != 0)
+			strncpy(profile, name, BG_FLAG_STRING_LEN);
+
+		for (i = 0; i < BG_FLAG_STRING_LEN && profile[i]; i++)
+			profile[i] = toupper(profile[i]);
+	}
+	if (profile[0]) {
+		strncat(ret, "|", BG_FLAG_STRING_LEN);
+		strncat(ret, profile, BG_FLAG_STRING_LEN);
 	}
 }
 
@@ -215,7 +226,7 @@ void print_chunk_item(struct extent_buffer *eb, struct btrfs_chunk *chunk)
 	u16 num_stripes = btrfs_chunk_num_stripes(eb, chunk);
 	int i;
 	u32 chunk_item_size;
-	char chunk_flags_str[32] = {0};
+	char chunk_flags_str[BG_FLAG_STRING_LEN] = {};
 
 	/* The chunk must contain at least one stripe */
 	if (num_stripes < 1) {
@@ -986,7 +997,7 @@ static void print_block_group_item(struct extent_buffer *eb,
 		struct btrfs_block_group_item *bgi)
 {
 	struct btrfs_block_group_item bg_item;
-	char flags_str[256];
+	char flags_str[BG_FLAG_STRING_LEN] = {};
 
 	read_extent_buffer(eb, &bg_item, (unsigned long)bgi, sizeof(bg_item));
 	memset(flags_str, 0, sizeof(flags_str));
-- 
2.33.0

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[David Sterba]

On Tue, Oct 12, 2021 at 10:17:19AM +0800, Qu Wenruo wrote:
> [BUG]
> Commit ("btrfs-progs: use raid table for profile names in
> print-tree.c") introduced one bug in block group and chunk flags output
> and changed the behavior:
> 
> 	item 1 key (FIRST_CHUNK_TREE CHUNK_ITEM 13631488) itemoff 16105 itemsize 80
> 		length 8388608 owner 2 stripe_len 65536 type SINGLE
> 		...
> 	item 2 key (FIRST_CHUNK_TREE CHUNK_ITEM 22020096) itemoff 15993 itemsize 112
> 		length 8388608 owner 2 stripe_len 65536 type DUP
> 		...
> 	item 3 key (FIRST_CHUNK_TREE CHUNK_ITEM 30408704) itemoff 15881 itemsize 112
> 		length 268435456 owner 2 stripe_len 65536 type DUP
> 		...
> 
> Note that, the flag string only contains the profile (SINGLE/DUP/etc...)
> no type (DATA/METADATA/SYSTEM).
> 
> And we have new "SINGLE" string, even that profile has no extra bit to
> indicate that.
> 
> [CAUSE]
> The "SINGLE" part is caused by the raid array which has a name for
> SINGLE profile, even it doesn't have corresponding bit.
> 
> The missing type string is caused by a code bug:
> 
> 		strcpy(buf, name);
> 		while (*tmp) {
> 			*tmp = toupper(*tmp);
> 			tmp++;
> 		}
> 		strcpy(ret, buf);
> 
> The last strcpy() call overrides the existing string in @ret.
> 
> [FIX]
> - Enhance string handling using strn*()/snprintf()
> 
> - Add extra "UKNOWN.0x%llx" output for unknown profiles
> 
> - Call proper strncat() to merge type and profile
> 
> - Add extra handling for "SINGLE" to keep the old output
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to devel, thanks.

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Nikolay Borisov]

<snip>

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  kernel-shared/print-tree.c | 47 +++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
> index 67b654e6d2d5..39655590272e 100644
> --- a/kernel-shared/print-tree.c
> +++ b/kernel-shared/print-tree.c
> @@ -159,40 +159,51 @@ static void print_inode_ref_item(struct extent_buffer *eb, u32 size,
>  	}
>  }
>  
> -/* Caller should ensure sizeof(*ret)>=21 "DATA|METADATA|RAID10" */
> +/* The minimal length for the string buffer of block group/chunk flags */
> +#define BG_FLAG_STRING_LEN	64
> +
>  static void bg_flags_to_str(u64 flags, char *ret)
>  {
>  	int empty = 1;
> +	char profile[BG_FLAG_STRING_LEN] = {};
>  	const char *name;
>  
> +	ret[0] = '\0';
>  	if (flags & BTRFS_BLOCK_GROUP_DATA) {
>  		empty = 0;
> -		strcpy(ret, "DATA");
> +		strncpy(ret, "DATA", BG_FLAG_STRING_LEN);

I find using strncpy rather odd, it guarantees it will copy num
characters, and if source is smaller than dest, it will overwrite the
rest with 0. So what happens is you are copying 4 chars here, and
writing 60 zeros. Frankly I think it's better to use

snprintf(ret, BG_FLAG_STRING_LEN, "DATA");

>  	}
>  	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>  		if (!empty)
> -			strcat(ret, "|");
> -		strcat(ret, "METADATA");
> +			strncat(ret, "|", BG_FLAG_STRING_LEN);
> +		strncat(ret, "METADATA", BG_FLAG_STRING_LEN);
>  	}
>  	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>  		if (!empty)
> -			strcat(ret, "|");
> -		strcat(ret, "SYSTEM");
> +			strncat(ret, "|", BG_FLAG_STRING_LEN);
> +		strncat(ret, "SYSTEM", BG_FLAG_STRING_LEN);
>  	}
> -	strcat(ret, "|");
>  	name = btrfs_bg_type_to_raid_name(flags);
>  	if (!name) {
> -		strcat(ret, "UNKNOWN");
> +		snprintf(profile, BG_FLAG_STRING_LEN, "UNKNOWN.0x%llx",
> +			 flags & BTRFS_BLOCK_GROUP_PROFILE_MASK);
>  	} else {
> -		char buf[32];
> -		char *tmp = buf;
> +		int i;
>  
> -		strcpy(buf, name);
> -		while (*tmp) {
> -			*tmp = toupper(*tmp);
> -			tmp++;
> -		}
> -		strcpy(ret, buf);
> +		/*
> +		 * Special handing for SINGLE profile, we don't output "SINGLE"
> +		 * for SINGLE profile, since there is no such bit for it.
> +		 * Thus here we only fill @profile if it's not single.
> +		 */
> +		if (strncmp(name, "single", strlen("single")) != 0)
> +			strncpy(profile, name, BG_FLAG_STRING_LEN);
> +
> +		for (i = 0; i < BG_FLAG_STRING_LEN && profile[i]; i++)

nit: It's guaranteed that the profile is shorted than
BG_FLAG_STRING_LEN, then this check can simply be profile[i] without the
i/BG_FLAG_STRING_LEN constant comparison.

> +			profile[i] = toupper(profile[i]);
> +	}
> +	if (profile[0]) {
> +		strncat(ret, "|", BG_FLAG_STRING_LEN);
> +		strncat(ret, profile, BG_FLAG_STRING_LEN);
>  	}

This if can really be put in the above 'else' branch and eliminate the
check altogether.

>  }

<snip>

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Nikolay Borisov]

On 12.10.21 г. 13:35, Nikolay Borisov wrote:
> <snip>
> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  kernel-shared/print-tree.c | 47 +++++++++++++++++++++++---------------
>>  1 file changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
>> index 67b654e6d2d5..39655590272e 100644
>> --- a/kernel-shared/print-tree.c
>> +++ b/kernel-shared/print-tree.c
>> @@ -159,40 +159,51 @@ static void print_inode_ref_item(struct extent_buffer *eb, u32 size,
>>  	}
>>  }
>>  
>> -/* Caller should ensure sizeof(*ret)>=21 "DATA|METADATA|RAID10" */
>> +/* The minimal length for the string buffer of block group/chunk flags */
>> +#define BG_FLAG_STRING_LEN	64
>> +
>>  static void bg_flags_to_str(u64 flags, char *ret)
>>  {
>>  	int empty = 1;
>> +	char profile[BG_FLAG_STRING_LEN] = {};
>>  	const char *name;
>>  
>> +	ret[0] = '\0';
>>  	if (flags & BTRFS_BLOCK_GROUP_DATA) {
>>  		empty = 0;
>> -		strcpy(ret, "DATA");
>> +		strncpy(ret, "DATA", BG_FLAG_STRING_LEN);
> 
> I find using strncpy rather odd, it guarantees it will copy num
> characters, and if source is smaller than dest, it will overwrite the
> rest with 0. So what happens is you are copying 4 chars here, and
> writing 60 zeros. Frankly I think it's better to use
> 
> snprintf(ret, BG_FLAG_STRING_LEN, "DATA");
> 
>>  	}
>>  	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>>  		if (!empty)
>> -			strcat(ret, "|");
>> -		strcat(ret, "METADATA");
>> +			strncat(ret, "|", BG_FLAG_STRING_LEN);
>> +		strncat(ret, "METADATA", BG_FLAG_STRING_LEN);
>>  	}
>>  	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>>  		if (!empty)
>> -			strcat(ret, "|");
>> -		strcat(ret, "SYSTEM");
>> +			strncat(ret, "|", BG_FLAG_STRING_LEN);
>> +		strncat(ret, "SYSTEM", BG_FLAG_STRING_LEN);
>>  	}
>> -	strcat(ret, "|");
>>  	name = btrfs_bg_type_to_raid_name(flags);
>>  	if (!name) {
>> -		strcat(ret, "UNKNOWN");
>> +		snprintf(profile, BG_FLAG_STRING_LEN, "UNKNOWN.0x%llx",
>> +			 flags & BTRFS_BLOCK_GROUP_PROFILE_MASK);
>>  	} else {
>> -		char buf[32];
>> -		char *tmp = buf;
>> +		int i;
>>  
>> -		strcpy(buf, name);
>> -		while (*tmp) {
>> -			*tmp = toupper(*tmp);
>> -			tmp++;
>> -		}
>> -		strcpy(ret, buf);
>> +		/*
>> +		 * Special handing for SINGLE profile, we don't output "SINGLE"
>> +		 * for SINGLE profile, since there is no such bit for it.
>> +		 * Thus here we only fill @profile if it's not single.
>> +		 */
>> +		if (strncmp(name, "single", strlen("single")) != 0)
>> +			strncpy(profile, name, BG_FLAG_STRING_LEN);
>> +
>> +		for (i = 0; i < BG_FLAG_STRING_LEN && profile[i]; i++)
> 
> nit: It's guaranteed that the profile is shorted than
> BG_FLAG_STRING_LEN, then this check can simply be profile[i] without the
> i/BG_FLAG_STRING_LEN constant comparison.
> 
>> +			profile[i] = toupper(profile[i]);
>> +	}
>> +	if (profile[0]) {

Actually profile[0] here is guaranteed to be nonul - it's either
UNKNOWN... or whatever btrfs_bg_type_to_raid_name returned. So you can
simply use the strncat functions without needing the if.

>> +		strncat(ret, "|", BG_FLAG_STRING_LEN);
>> +		strncat(ret, profile, BG_FLAG_STRING_LEN);
>>  	}
> 
> This if can really be put in the above 'else' branch and eliminate the
> check altogether.
> 
>>  }
> 
> <snip>
> 

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Qu Wenruo]

On 2021/10/12 18:38, Nikolay Borisov wrote:
>
>
> On 12.10.21 г. 13:35, Nikolay Borisov wrote:
>> <snip>
>>
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   kernel-shared/print-tree.c | 47 +++++++++++++++++++++++---------------
>>>   1 file changed, 29 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
>>> index 67b654e6d2d5..39655590272e 100644
>>> --- a/kernel-shared/print-tree.c
>>> +++ b/kernel-shared/print-tree.c
>>> @@ -159,40 +159,51 @@ static void print_inode_ref_item(struct extent_buffer *eb, u32 size,
>>>   	}
>>>   }
>>>
>>> -/* Caller should ensure sizeof(*ret)>=21 "DATA|METADATA|RAID10" */
>>> +/* The minimal length for the string buffer of block group/chunk flags */
>>> +#define BG_FLAG_STRING_LEN	64
>>> +
>>>   static void bg_flags_to_str(u64 flags, char *ret)
>>>   {
>>>   	int empty = 1;
>>> +	char profile[BG_FLAG_STRING_LEN] = {};
>>>   	const char *name;
>>>
>>> +	ret[0] = '\0';
>>>   	if (flags & BTRFS_BLOCK_GROUP_DATA) {
>>>   		empty = 0;
>>> -		strcpy(ret, "DATA");
>>> +		strncpy(ret, "DATA", BG_FLAG_STRING_LEN);
>>
>> I find using strncpy rather odd, it guarantees it will copy num
>> characters, and if source is smaller than dest, it will overwrite the
>> rest with 0. So what happens is you are copying 4 chars here, and
>> writing 60 zeros. Frankly I think it's better to use >>
>> snprintf(ret, BG_FLAG_STRING_LEN, "DATA");

Well, you just told me a new fact, strncpy() would set the the rest bytes.

I thought it would just add the terminal '\0' if it's not reaching the
size limit.

But you're right, strncpy() would reset the padding bytes to zero.

>>
>>>   	}
>>>   	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>>>   		if (!empty)
>>> -			strcat(ret, "|");
>>> -		strcat(ret, "METADATA");
>>> +			strncat(ret, "|", BG_FLAG_STRING_LEN);
>>> +		strncat(ret, "METADATA", BG_FLAG_STRING_LEN);
>>>   	}
>>>   	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>>>   		if (!empty)
>>> -			strcat(ret, "|");
>>> -		strcat(ret, "SYSTEM");
>>> +			strncat(ret, "|", BG_FLAG_STRING_LEN);
>>> +		strncat(ret, "SYSTEM", BG_FLAG_STRING_LEN);
>>>   	}
>>> -	strcat(ret, "|");
>>>   	name = btrfs_bg_type_to_raid_name(flags);
>>>   	if (!name) {
>>> -		strcat(ret, "UNKNOWN");
>>> +		snprintf(profile, BG_FLAG_STRING_LEN, "UNKNOWN.0x%llx",
>>> +			 flags & BTRFS_BLOCK_GROUP_PROFILE_MASK);
>>>   	} else {
>>> -		char buf[32];
>>> -		char *tmp = buf;
>>> +		int i;
>>>
>>> -		strcpy(buf, name);
>>> -		while (*tmp) {
>>> -			*tmp = toupper(*tmp);
>>> -			tmp++;
>>> -		}
>>> -		strcpy(ret, buf);
>>> +		/*
>>> +		 * Special handing for SINGLE profile, we don't output "SINGLE"
>>> +		 * for SINGLE profile, since there is no such bit for it.
>>> +		 * Thus here we only fill @profile if it's not single.
>>> +		 */
>>> +		if (strncmp(name, "single", strlen("single")) != 0)
>>> +			strncpy(profile, name, BG_FLAG_STRING_LEN);
>>> +
>>> +		for (i = 0; i < BG_FLAG_STRING_LEN && profile[i]; i++)
>>
>> nit: It's guaranteed that the profile is shorted than
>> BG_FLAG_STRING_LEN, then this check can simply be profile[i] without the
>> i/BG_FLAG_STRING_LEN constant comparison.

I don't want to do any assumption here.

In fact I originally wanted to choose 32 as BG_FLAG_STRING_LEN, but it's
not safe already.

Considering the following output:
"DATA|METADATA|UNKNOWN.0xffffffff00000000"

Which is already 40 chars.

Since we're already doing all the strn*() calls just to avoid such
pitfalls, I tend to be extra safe here, thus I hope to keep the extra
check against BG_FLAG_STRING_LEN.

>>
>>> +			profile[i] = toupper(profile[i]);
>>> +	}
>>> +	if (profile[0]) {
>
> Actually profile[0] here is guaranteed to be nonul - it's either
> UNKNOWN... or whatever btrfs_bg_type_to_raid_name returned. So you can
> simply use the strncat functions without needing the if.

You forgot SINGLE type.

In that case, profile[0] can be "\0".

>
>>> +		strncat(ret, "|", BG_FLAG_STRING_LEN);
>>> +		strncat(ret, profile, BG_FLAG_STRING_LEN);
>>>   	}
>>
>> This if can really be put in the above 'else' branch and eliminate the
>> check altogether.

Then these lines needs to be copied to two branches:

- UNKNOWN branch
- Non-SINGLE branch

Thus it's better to kept inside the if (profile[0]) branch, as it covers
two cases.

Thanks,
Qu

>>
>>>   }
>>
>> <snip>
>>

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Nikolay Borisov]

On 12.10.21 г. 14:42, Qu Wenruo wrote:
> 
> 
> On 2021/10/12 18:38, Nikolay Borisov wrote:
>>
>>
>> On 12.10.21 г. 13:35, Nikolay Borisov wrote:
>>> <snip>
>>>
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>   kernel-shared/print-tree.c | 47
>>>> +++++++++++++++++++++++---------------
>>>>   1 file changed, 29 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
>>>> index 67b654e6d2d5..39655590272e 100644
>>>> --- a/kernel-shared/print-tree.c
>>>> +++ b/kernel-shared/print-tree.c
>>>> @@ -159,40 +159,51 @@ static void print_inode_ref_item(struct
>>>> extent_buffer *eb, u32 size,
>>>>       }
>>>>   }
>>>>
>>>> -/* Caller should ensure sizeof(*ret)>=21 "DATA|METADATA|RAID10" */
>>>> +/* The minimal length for the string buffer of block group/chunk
>>>> flags */
>>>> +#define BG_FLAG_STRING_LEN    64
>>>> +
>>>>   static void bg_flags_to_str(u64 flags, char *ret)
>>>>   {
>>>>       int empty = 1;
>>>> +    char profile[BG_FLAG_STRING_LEN] = {};
>>>>       const char *name;
>>>>
>>>> +    ret[0] = '\0';
>>>>       if (flags & BTRFS_BLOCK_GROUP_DATA) {
>>>>           empty = 0;
>>>> -        strcpy(ret, "DATA");
>>>> +        strncpy(ret, "DATA", BG_FLAG_STRING_LEN);
>>>
>>> I find using strncpy rather odd, it guarantees it will copy num
>>> characters, and if source is smaller than dest, it will overwrite the
>>> rest with 0. So what happens is you are copying 4 chars here, and
>>> writing 60 zeros. Frankly I think it's better to use >>
>>> snprintf(ret, BG_FLAG_STRING_LEN, "DATA");
> 
> Well, you just told me a new fact, strncpy() would set the the rest bytes.
> 
> I thought it would just add the terminal '\0' if it's not reaching the
> size limit.
> 
> But you're right, strncpy() would reset the padding bytes to zero.

The thing is strncpy doesn't really set final NULL by definition. I.e if
source is larger than N, then dest won't be null terminated.

<snip>

> 
>>>
>>>> +            profile[i] = toupper(profile[i]);
>>>> +    }
>>>> +    if (profile[0]) {
>>
>> Actually profile[0] here is guaranteed to be nonul - it's either
>> UNKNOWN... or whatever btrfs_bg_type_to_raid_name returned. So you can
>> simply use the strncat functions without needing the if.
> 
> You forgot SINGLE type.
> 
> In that case, profile[0] can be "\0".

How does that happen? If btrfs_bg_type_to_raid_name return NULL then
prfile contains UNKNWON. OTOH if the 'else' is executed then either
profile contains "single" or whatever btrfs_bg_type_to_raid_name
returned. So profile can never be NULL. What am I missing?

<snip>

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Qu Wenruo]

On 2021/10/12 19:56, Nikolay Borisov wrote:
>
>
> On 12.10.21 г. 14:42, Qu Wenruo wrote:
>>
>>
>> On 2021/10/12 18:38, Nikolay Borisov wrote:
>>>
>>>
>>> On 12.10.21 г. 13:35, Nikolay Borisov wrote:
>>>> <snip>
>>>>
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>>    kernel-shared/print-tree.c | 47
>>>>> +++++++++++++++++++++++---------------
>>>>>    1 file changed, 29 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
>>>>> index 67b654e6d2d5..39655590272e 100644
>>>>> --- a/kernel-shared/print-tree.c
>>>>> +++ b/kernel-shared/print-tree.c
>>>>> @@ -159,40 +159,51 @@ static void print_inode_ref_item(struct
>>>>> extent_buffer *eb, u32 size,
>>>>>        }
>>>>>    }
>>>>>
>>>>> -/* Caller should ensure sizeof(*ret)>=21 "DATA|METADATA|RAID10" */
>>>>> +/* The minimal length for the string buffer of block group/chunk
>>>>> flags */
>>>>> +#define BG_FLAG_STRING_LEN    64
>>>>> +
>>>>>    static void bg_flags_to_str(u64 flags, char *ret)
>>>>>    {
>>>>>        int empty = 1;
>>>>> +    char profile[BG_FLAG_STRING_LEN] = {};
>>>>>        const char *name;
>>>>>
>>>>> +    ret[0] = '\0';
>>>>>        if (flags & BTRFS_BLOCK_GROUP_DATA) {
>>>>>            empty = 0;
>>>>> -        strcpy(ret, "DATA");
>>>>> +        strncpy(ret, "DATA", BG_FLAG_STRING_LEN);
>>>>
>>>> I find using strncpy rather odd, it guarantees it will copy num
>>>> characters, and if source is smaller than dest, it will overwrite the
>>>> rest with 0. So what happens is you are copying 4 chars here, and
>>>> writing 60 zeros. Frankly I think it's better to use >>
>>>> snprintf(ret, BG_FLAG_STRING_LEN, "DATA");
>>
>> Well, you just told me a new fact, strncpy() would set the the rest bytes.
>>
>> I thought it would just add the terminal '\0' if it's not reaching the
>> size limit.
>>
>> But you're right, strncpy() would reset the padding bytes to zero.
>
> The thing is strncpy doesn't really set final NULL by definition. I.e if
> source is larger than N, then dest won't be null terminated.
>
> <snip>
>
>>
>>>>
>>>>> +            profile[i] = toupper(profile[i]);
>>>>> +    }
>>>>> +    if (profile[0]) {
>>>
>>> Actually profile[0] here is guaranteed to be nonul - it's either
>>> UNKNOWN... or whatever btrfs_bg_type_to_raid_name returned. So you can
>>> simply use the strncat functions without needing the if.
>>
>> You forgot SINGLE type.
>>
>> In that case, profile[0] can be "\0".
>
> How does that happen? If btrfs_bg_type_to_raid_name return NULL then
> prfile contains UNKNWON. OTOH if the 'else' is executed then either
> profile contains "single" or whatever btrfs_bg_type_to_raid_name
> returned. So profile can never be NULL. What am I missing?

There is a special handling for SINGLE:

+               /*
+                * Special handing for SINGLE profile, we don't output
"SINGLE"
+                * for SINGLE profile, since there is no such bit for it.
+                * Thus here we only fill @profile if it's not single.
+                */
+               if (strncmp(name, "single", strlen("single")) != 0)
+                       strncpy(profile, name, BG_FLAG_STRING_LEN);

See, if we hit SINGLE profile, we won't populate @profile array.

Thanks,
Qu

>
> <snip>
>

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Nikolay Borisov]

On 12.10.21 г. 15:24, Qu Wenruo wrote:
> 
> 
> On 2021/10/12 19:56, Nikolay Borisov wrote:
>>
>>
>> On 12.10.21 г. 14:42, Qu Wenruo wrote:
>>>

<snip>

> 
> There is a special handling for SINGLE:
> 
> +               /*
> +                * Special handing for SINGLE profile, we don't output
> "SINGLE"
> +                * for SINGLE profile, since there is no such bit for it.
> +                * Thus here we only fill @profile if it's not single.
> +                */
> +               if (strncmp(name, "single", strlen("single")) != 0)
> +                       strncpy(profile, name, BG_FLAG_STRING_LEN);
> 
> See, if we hit SINGLE profile, we won't populate @profile array.

Fair enough, I had misread the != 0 check ... However, I'm wondering,
since this is only used during output, why don't we, for the sake of
consistency, output SINGLE, despite not having an exclusive bit for it?
The point of the human readable output is to be useful for users, so
instead of me having to know an implementation detail that SINGLE is not
represented by any bit, it will be much more useful if I see that
something is single profile, no ?

> 
> Thanks,
> Qu
> 
>>
>> <snip>
>>
> 

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[Qu Wenruo]

On 2021/10/12 21:59, Nikolay Borisov wrote:
> 
> 
> On 12.10.21 г. 15:24, Qu Wenruo wrote:
>>
>>
>> On 2021/10/12 19:56, Nikolay Borisov wrote:
>>>
>>>
>>> On 12.10.21 г. 14:42, Qu Wenruo wrote:
>>>>
> 
> <snip>
> 
>>
>> There is a special handling for SINGLE:
>>
>> +               /*
>> +                * Special handing for SINGLE profile, we don't output
>> "SINGLE"
>> +                * for SINGLE profile, since there is no such bit for it.
>> +                * Thus here we only fill @profile if it's not single.
>> +                */
>> +               if (strncmp(name, "single", strlen("single")) != 0)
>> +                       strncpy(profile, name, BG_FLAG_STRING_LEN);
>>
>> See, if we hit SINGLE profile, we won't populate @profile array.
> 
> Fair enough, I had misread the != 0 check ... However, I'm wondering,
> since this is only used during output, why don't we, for the sake of
> consistency, output SINGLE, despite not having an exclusive bit for it?
> The point of the human readable output is to be useful for users, so
> instead of me having to know an implementation detail that SINGLE is not
> represented by any bit, it will be much more useful if I see that
> something is single profile, no ?

On the other hand, this breaks the consistency of flags output.

We only output flags we have on-disk.

Showing another flag which doesn't have on-disk bit can also be 
confusing, and break the existing output format.

Thanks,
Qu
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> <snip>
>>>
>>
> 

Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

[David Sterba]

On Wed, Oct 13, 2021 at 07:53:48AM +0800, Qu Wenruo wrote:
> >> See, if we hit SINGLE profile, we won't populate @profile array.
> > 
> > Fair enough, I had misread the != 0 check ... However, I'm wondering,
> > since this is only used during output, why don't we, for the sake of
> > consistency, output SINGLE, despite not having an exclusive bit for it?
> > The point of the human readable output is to be useful for users, so
> > instead of me having to know an implementation detail that SINGLE is not
> > represented by any bit, it will be much more useful if I see that
> > something is single profile, no ?
> 
> On the other hand, this breaks the consistency of flags output.
> 
> We only output flags we have on-disk.
> 
> Showing another flag which doesn't have on-disk bit can also be 
> confusing, and break the existing output format.

Agreed, the dump utility should print only what is on-disk.