[2/3] fs/ext2/inode: Fix a type cast error for fsdax

Commit Message

Huaisheng Ye July 1, 2018, 6:18 a.m. UTC

From: Huaisheng Ye <[e-mail]>

The type of offset within struct iomap is loff_t, which represents
file offset of mapping.

In ext2_iomap_begin, iomap->offset shall be given a type cast as
loff_t instead of u64.

Signed-off-by: Huaisheng Ye <[e-mail]>
---
 fs/ext2/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Al Viro July 1, 2018, 7:26 p.m. UTC | #1

On Sun, Jul 01, 2018 at 02:18:47PM +0800, Huaisheng Ye wrote:
> From: Huaisheng Ye <[e-mail]>
> 
> The type of offset within struct iomap is loff_t, which represents
> file offset of mapping.
> 
> In ext2_iomap_begin, iomap->offset shall be given a type cast as
> loff_t instead of u64.

Why is it an error?  loff_t is uniformly typedefed to long long.
In which case the second variant is different from the first one
*and* does not step into nasal demon territory?

> -	iomap->offset = (u64)first_block << blkbits;
> +	iomap->offset = (loff_t)first_block << blkbits;

Huaisheng Ye July 2, 2018, 6:23 a.m. UTC | #2

---- On Mon, 02 Jul 2018 03:26:00 +0800 Al Viro <[e-mail]> wrote ---- 
 > On Sun, Jul 01, 2018 at 02:18:47PM +0800, Huaisheng Ye wrote: 
 > > From: Huaisheng Ye <[e-mail]> 
 > >  
 > > The type of offset within struct iomap is loff_t, which represents 
 > > file offset of mapping. 
 > >  
 > > In ext2_iomap_begin, iomap->offset shall be given a type cast as 
 > > loff_t instead of u64. 
 >  
 > Why is it an error?  loff_t is uniformly typedefed to long long. 
 > In which case the second variant is different from the first one 
 > *and* does not step into nasal demon territory?

Sorry for my inaccuracy.
The type of iomap->offset is loff_t, is it better to cast first_block
to loff_t, then do the left shift operation?
 
 >  
 > > -    iomap->offset = (u64)first_block << blkbits; 
 > > +    iomap->offset = (loff_t)first_block << blkbits; 
 > 

---
Cheers,
Huaisheng

Jan Kara July 2, 2018, 7:55 a.m. UTC | #3

On Mon 02-07-18 14:23:42, Huaisheng Ye wrote:
>  ---- On Mon, 02 Jul 2018 03:26:00 +0800 Al Viro <[e-mail]> wrote ---- 
>  > On Sun, Jul 01, 2018 at 02:18:47PM +0800, Huaisheng Ye wrote: 
>  > > From: Huaisheng Ye <[e-mail]> 
>  > >  
>  > > The type of offset within struct iomap is loff_t, which represents 
>  > > file offset of mapping. 
>  > >  
>  > > In ext2_iomap_begin, iomap->offset shall be given a type cast as 
>  > > loff_t instead of u64. 
>  >  
>  > Why is it an error?  loff_t is uniformly typedefed to long long. 
>  > In which case the second variant is different from the first one 
>  > *and* does not step into nasal demon territory?
> 
> Sorry for my inaccuracy.
> The type of iomap->offset is loff_t, is it better to cast first_block
> to loff_t, then do the left shift operation?

I think what Al meant is that in the kernel loff_t is 64-bit anyway and
furthermore it is signed which means most people have to look up C standard
to remember how right shift is (un)defined if it would overflow ;). So this
does not really make the code any more readable. Rather on contrary...

								Honza

>  
>  >  
>  > > -    iomap->offset = (u64)first_block << blkbits; 
>  > > +    iomap->offset = (loff_t)first_block << blkbits; 
>  > 
> 
> ---
> Cheers,
> Huaisheng
> 
>

Patch

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 7163590..ca211bd 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -816,7 +816,7 @@  static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 
 	iomap->flags = 0;
 	iomap->bdev = inode->i_sb->s_bdev;
-	iomap->offset = (u64)first_block << blkbits;
+	iomap->offset = (loff_t)first_block << blkbits;
 	iomap->dax_dev = sbi->s_daxdev;
 
 	if (ret == 0) {