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

[Nikolay Borisov <nborisov@suse.com>]

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