Re: [PATCH v3 1/2] btrfs-progs: export util functions about file extent items

[Sidong Yang <realwakka@gmail.com>]

On Sat, Jul 17, 2021 at 06:56:37AM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/7/17 上午12:30, Sidong Yang wrote:
> > On Thu, Jul 15, 2021 at 06:46:08AM +0800, Qu Wenruo wrote:
> > > 
> > > 
> > > On 2021/7/14 下午10:54, Sidong Yang wrote:
> > > > This patch export two functions that convert enum about file extents to
> > > > string. It can be used in other code like inspect-internal command. And
> > > > this patch also make compress_type_to_str() function more safe by using
> > > > strncpy() than strcpy().
> > > > 
> > > > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > > > ---
> > > > v2:
> > > >    - Prints type and compression
> > > >    - Use the terms from file_extents_item like disk_bytenr not like
> > > >      "physical"
> > > > v3:
> > > >    - export util functions for removing duplication
> > > >    - change the way to loop with search ioctl
> > > > ---
> > > >    kernel-shared/print-tree.c | 18 ++++++++++--------
> > > >    kernel-shared/print-tree.h |  3 +++
> > > >    2 files changed, 13 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
> > > > index e5d4b453..bfef7d26 100644
> > > > --- a/kernel-shared/print-tree.c
> > > > +++ b/kernel-shared/print-tree.c
> > > > @@ -27,6 +27,8 @@
> > > >    #include "kernel-shared/print-tree.h"
> > > >    #include "common/utils.h"
> > > > +#define COMPRESS_STR_LEN 5
> > > 
> > > It's already too small to contain all the strings.
> > > 
> > > We have a default branch:
> > > 
> > >   	default:
> > >   		sprintf(ret, "UNKNOWN.%d", compress_type);
> > > 
> > > In that case, our minimal value should be strlen("UNKNOWN.255") + 1, which
> > > is 12 bytes.
> > > 
> > > Your old rounded up 16 bytes is in fact perfect in this case.
> > > 
> > > Despite that, this patch looks good to me.
> > 
> > Thanks for review.
> > 
> > I've been thought about this. should COMPRESS_STR_LEN be exported?
> 
> I guess yes.

Thanks, I'll apply this.

Thanks,
Sidong
> 
> Thanks,
> Qu
> 
> > Developer that use this function needs to know max length of string.
> > Or we can choose the simple implementation that just return const char*
> > like btrfs_file_extent_type_to_str(). But it will be hard to print
> > unknown compress_type.
> > 
> > Thanks,
> > Sidong
> > > 
> > > Thanks,
> > > Qu
> > > > +
> > > >    static void print_dir_item_type(struct extent_buffer *eb,
> > > >                                    struct btrfs_dir_item *di)
> > > >    {
> > > > @@ -338,27 +340,27 @@ static void print_uuids(struct extent_buffer *eb)
> > > >    	printf("fs uuid %s\nchunk uuid %s\n", fs_uuid, chunk_uuid);
> > > >    }
> > > > -static void compress_type_to_str(u8 compress_type, char *ret)
> > > > +void btrfs_compress_type_to_str(u8 compress_type, char *ret)
> > > >    {
> > > >    	switch (compress_type) {
> > > >    	case BTRFS_COMPRESS_NONE:
> > > > -		strcpy(ret, "none");
> > > > +		strncpy(ret, "none", COMPRESS_STR_LEN);
> > > >    		break;
> > > >    	case BTRFS_COMPRESS_ZLIB:
> > > > -		strcpy(ret, "zlib");
> > > > +		strncpy(ret, "zlib", COMPRESS_STR_LEN);
> > > >    		break;
> > > >    	case BTRFS_COMPRESS_LZO:
> > > > -		strcpy(ret, "lzo");
> > > > +		strncpy(ret, "lzo", COMPRESS_STR_LEN);
> > > >    		break;
> > > >    	case BTRFS_COMPRESS_ZSTD:
> > > > -		strcpy(ret, "zstd");
> > > > +		strncpy(ret, "zstd", COMPRESS_STR_LEN);
> > > >    		break;
> > > >    	default:
> > > >    		sprintf(ret, "UNKNOWN.%d", compress_type);
> > > >    	}
> > > >    }
> > > > -static const char* file_extent_type_to_str(u8 type)
> > > > +const char* btrfs_file_extent_type_to_str(u8 type)
> > > >    {
> > > >    	switch (type) {
> > > >    	case BTRFS_FILE_EXTENT_INLINE: return "inline";
> > > > @@ -376,12 +378,12 @@ static void print_file_extent_item(struct extent_buffer *eb,
> > > >    	unsigned char extent_type = btrfs_file_extent_type(eb, fi);
> > > >    	char compress_str[16];
> > > > -	compress_type_to_str(btrfs_file_extent_compression(eb, fi),
> > > > +	btrfs_compress_type_to_str(btrfs_file_extent_compression(eb, fi),
> > > >    			     compress_str);
> > > >    	printf("\t\tgeneration %llu type %hhu (%s)\n",
> > > >    			btrfs_file_extent_generation(eb, fi),
> > > > -			extent_type, file_extent_type_to_str(extent_type));
> > > > +			extent_type, btrfs_file_extent_type_to_str(extent_type));
> > > >    	if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
> > > >    		printf("\t\tinline extent data size %u ram_bytes %llu compression %hhu (%s)\n",
> > > > diff --git a/kernel-shared/print-tree.h b/kernel-shared/print-tree.h
> > > > index 80fb6ef7..dbb2f183 100644
> > > > --- a/kernel-shared/print-tree.h
> > > > +++ b/kernel-shared/print-tree.h
> > > > @@ -43,4 +43,7 @@ void print_objectid(FILE *stream, u64 objectid, u8 type);
> > > >    void print_key_type(FILE *stream, u64 objectid, u8 type);
> > > >    void btrfs_print_superblock(struct btrfs_super_block *sb, int full);
> > > > +void btrfs_compress_type_to_str(u8 compress_type, char *ret);
> > > > +const char* btrfs_file_extent_type_to_str(u8 type);
> > > > +
> > > >    #endif
> > > > 
> > >