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

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

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

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