Re: [PATCH v5 1/3] btrfs: add helper function describe_block_group()

[Anand Jain <anand.jain@xxxxxxxxxx>]

Thanks for the review.. more below.

On 11/20/2018 01:02 AM, David Sterba wrote:
> On Wed, Nov 14, 2018 at 09:17:10PM +0800, Anand Jain wrote:
> > Improve on describe_relocation() add a common helper function to describe
> > the block groups.
> >
> > Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> > Reviewed-by: David Sterba <dsterba@xxxxxxxx>
> > ---
> > v4.1->v5: Initialize buf[128] to null.
> > v4->v4.1: Use strcpy(buf, "|NONE"); as in the original
> > v3->v4: Just pass full flag name in the define DESCRIBE_FLAG(flag,..),
> > 	 so that it can be used at couple of more places.
> > 	Rename describe_block_groups() to btrfs_describe_block_groups().
> > 	Drop useless return u32.
> > v3:	Born.
> >
> >   fs/btrfs/relocation.c | 30 +++---------------------------
> >   fs/btrfs/volumes.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   fs/btrfs/volumes.h    |  1 +
> >   3 files changed, 47 insertions(+), 27 deletions(-)
> >
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 924116f654a1..0373b3cc1d36 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -4184,37 +4184,13 @@ static struct reloc_control *alloc_reloc_control(void)
> >   static void describe_relocation(struct btrfs_fs_info *fs_info,
> >   				struct btrfs_block_group_cache *block_group)
> >   {
> > -	char buf[128];		/* prefixed by a '|' that'll be dropped */
> > -	u64 flags = block_group->flags;
> > +	char buf[128] = {'\0'};
> >   
> > -	/* Shouldn't happen */
> > -	if (!flags) {
> > -		strcpy(buf, "|NONE");
> > -	} else {
> > -		char *bp = buf;
> > -
> > -#define DESCRIBE_FLAG(f, d) \
> > -		if (flags & BTRFS_BLOCK_GROUP_##f) { \
> > -			bp += snprintf(bp, buf - bp + sizeof(buf), "|%s", d); \
> > -			flags &= ~BTRFS_BLOCK_GROUP_##f; \
> > -		}
> > -		DESCRIBE_FLAG(DATA,     "data");
> > -		DESCRIBE_FLAG(SYSTEM,   "system");
> > -		DESCRIBE_FLAG(METADATA, "metadata");
> > -		DESCRIBE_FLAG(RAID0,    "raid0");
> > -		DESCRIBE_FLAG(RAID1,    "raid1");
> > -		DESCRIBE_FLAG(DUP,      "dup");
> > -		DESCRIBE_FLAG(RAID10,   "raid10");
> > -		DESCRIBE_FLAG(RAID5,    "raid5");
> > -		DESCRIBE_FLAG(RAID6,    "raid6");
> > -		if (flags)
> > -			snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags);
> > -#undef DESCRIBE_FLAG
> > -	}
> > +	btrfs_describe_block_groups(block_group->flags, buf, sizeof(buf));
> >   
> >   	btrfs_info(fs_info,
> >   		   "relocating block group %llu flags %s",
> > -		   block_group->key.objectid, buf + 1);
> > +		   block_group->key.objectid, buf);
> >   }
> >   
> >   /*
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index f435d397019e..12b3d0625c6a 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -123,6 +123,49 @@ const char *get_raid_name(enum btrfs_raid_types type)
> >   	return btrfs_raid_array[type].raid_name;
> >   }
> >   
> > +void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
> > +{
> > +	int i;
> > +	int ret;
> > +	char *bp = buf;
> > +	u64 flags = bg_flags;
> > +	u32 size_bp = size_buf;
> > +
> > +	if (!flags) {
> > +		strcpy(bp, "NONE");
> > +		return;
> > +	}
> > +
> > +#define DESCRIBE_FLAG(f, d) \
>
> Macro arguments should be in ( ) in the body

 Fixed in v6.

> > +	do { \
> > +		if (flags & f) { \
> > +			ret = snprintf(bp, size_bp, "%s|", d); \
> > +			if (ret < 0 || ret >= size_bp) \
> > +				return; \
>
> Ok, this seems to be safe. snprintf will not write more than size_bp and
> this will be caught if it overflows.
>
> The return is obscured by the macro, I'd rather make it more visible
> that there is a potential change in the code flow. The proposed change:
>
> 				goto out_overflow;
> 				...
>
> and define out_overflow at the end of function. Same for the other
> helper macros.

 makes sense. Thanks for explaining. Fixed in v6.

> Also please align the backslashes of the macro a few tabs to the right.

 Yep done in v6.

Thanks, Anand


> > +			size_bp -= ret; \
> > +			bp += ret; \
> > +			flags &= ~f; \
> > +		} \
> > +	} while (0)