Re: [PATCH 1/2] btrfs-progs: convert: Prevent bit overflow for cctx->total_bytes

[David Sterba <dsterba@suse.cz>]

On Tue, Jul 21, 2020 at 06:29:31PM +0800, Qu Wenruo wrote:
> On 2020/7/21 下午5:58, David Sterba wrote:
> > On Tue, Jul 21, 2020 at 07:51:00AM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2020/7/21 上午12:09, David Sterba wrote:
> >>> On Mon, Jul 20, 2020 at 08:51:08PM +0800, Qu Wenruo wrote:
> >>>> --- a/convert/source-ext2.c
> >>>> +++ b/convert/source-ext2.c
> >>>> @@ -87,7 +87,8 @@ static int ext2_open_fs(struct btrfs_convert_context *cctx, const char *name)
> >>>>  	cctx->fs_data = ext2_fs;
> >>>>  	cctx->blocksize = ext2_fs->blocksize;
> >>>>  	cctx->block_count = ext2_fs->super->s_blocks_count;
> >>>> -	cctx->total_bytes = ext2_fs->blocksize * ext2_fs->super->s_blocks_count;
> >>>> +	cctx->total_bytes = (u64)ext2_fs->blocksize *
> >>>> +			    (u64)ext2_fs->super->s_blocks_count;
> >>>
> >>> Do you need to cast both? Once one of the types is wide enough for the
> >>> result, there should be no loss.
> >>>
> >> I just want to be extra safe.
> > 
> > Typecasts in code raise questions why are they needed, 'to be extra'
> > safe is not a good reason. One typecast in multiplication/shifts is a
> > common pattern to widen the result but two look more like lack of
> > understanding of the integer promotion rules.
> 
> My point here is, I don't want the reviewers or new contributors to
> bother about the promotion rules at all.

Ouch, I hope you don't mean that contributors should ignore the trickier
parts of C language. Especially reviewers _have_ to bother about all
sorts of subtle behaviour.

> They only need to know that using blocksize and blocks_count directly to
> do multiply would lead to overflow.
> 
> Other details like whether the multiply follows the highest factor or
> the left operator or the right operator, shouldn't be the point and we
> don't really need to bother.

... and introduce bugs?

> Thus casting both would definitely be right, without the need to refer
> to the complex rule book, thus save the reviewer several minutes.

The opposite, if you send me code that's not following known schemes or
idiomatic schemes I'll be highly suspicious and looking for the reasons
why it's that way and making sure it's correct costs way more time.