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

[Qu Wenruo <wqu@suse.com>]

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.

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.

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

Thanks,
Qu