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

On 2021/10/12 19:56, Nikolay Borisov wrote:

On 12.10.21 г. 14:42, Qu Wenruo wrote:

quoted

On 2021/10/12 18:38, Nikolay Borisov wrote:

quoted

On 12.10.21 г. 13:35, Nikolay Borisov wrote:

quoted

<snip>

quoted

Signed-off-by: Qu Wenruo <redacted>
---
   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>

quoted

quoted

quoted

quoted

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