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

[Nikolay Borisov <nborisov@suse.com>]

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>