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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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