| Message ID | 20170425045227.19713-1-lokeshvutla@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Tue, Apr 25, 2017 at 10:22:27AM +0530, Lokesh Vutla wrote: > In file ext4fs.c funtion ext4fs_read_file() compares an > unsigned expression with < 0 like below > > lbaint_t blknr; > blknr = read_allocated_block(&(node->inode), i); > if (blknr < 0) > return -1; > > blknr is of type ulong/uint64_t. read_allocated_block() returns > long int. So comparing blknr with < 0 will always be false. Instead > declare blknr as long int. > > Reported-by: Sunita Nadampalli <sunitan@ti.com> > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > fs/ext4/ext4fs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c > index 7187dcfb05..081509dbb4 100644 > --- a/fs/ext4/ext4fs.c > +++ b/fs/ext4/ext4fs.c > @@ -71,7 +71,7 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, > blockcnt = lldiv(((len + pos) + blocksize - 1), blocksize); > > for (i = lldiv(pos, blocksize); i < blockcnt; i++) { > - lbaint_t blknr; > + long int blknr; > int blockoff = pos - (blocksize * i); > int blockend = blocksize; > int skipfirst = 0; My only question is, did you catch that by inspection, clang, or a newer than gcc-6.3 warning? Also, fs/ext4/dev.c:63 is a similar problem, if you'd like to non-RFC a v2. Thanks!
On Tuesday 25 April 2017 11:34 PM, Tom Rini wrote: > On Tue, Apr 25, 2017 at 10:22:27AM +0530, Lokesh Vutla wrote: >> In file ext4fs.c funtion ext4fs_read_file() compares an >> unsigned expression with < 0 like below >> >> lbaint_t blknr; >> blknr = read_allocated_block(&(node->inode), i); >> if (blknr < 0) >> return -1; >> >> blknr is of type ulong/uint64_t. read_allocated_block() returns >> long int. So comparing blknr with < 0 will always be false. Instead >> declare blknr as long int. >> >> Reported-by: Sunita Nadampalli <sunitan@ti.com> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> fs/ext4/ext4fs.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c >> index 7187dcfb05..081509dbb4 100644 >> --- a/fs/ext4/ext4fs.c >> +++ b/fs/ext4/ext4fs.c >> @@ -71,7 +71,7 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, >> blockcnt = lldiv(((len + pos) + blocksize - 1), blocksize); >> >> for (i = lldiv(pos, blocksize); i < blockcnt; i++) { >> - lbaint_t blknr; >> + long int blknr; >> int blockoff = pos - (blocksize * i); >> int blockend = blocksize; >> int skipfirst = 0; > > My only question is, did you catch that by inspection, clang, or a newer This is reported by Sunita and not sure how she caught it. > than gcc-6.3 warning? Also, fs/ext4/dev.c:63 is a similar problem, if > you'd like to non-RFC a v2. Thanks! In this case do you want me to drop the check or change lbaint_t to long int and update the argument type cast where ever ext4fs_devread() is called? Thanks and regards, Lokesh
On Wed, Apr 26, 2017 at 08:56:49AM +0530, Lokesh Vutla wrote: > > > On Tuesday 25 April 2017 11:34 PM, Tom Rini wrote: > > On Tue, Apr 25, 2017 at 10:22:27AM +0530, Lokesh Vutla wrote: > >> In file ext4fs.c funtion ext4fs_read_file() compares an > >> unsigned expression with < 0 like below > >> > >> lbaint_t blknr; > >> blknr = read_allocated_block(&(node->inode), i); > >> if (blknr < 0) > >> return -1; > >> > >> blknr is of type ulong/uint64_t. read_allocated_block() returns > >> long int. So comparing blknr with < 0 will always be false. Instead > >> declare blknr as long int. > >> > >> Reported-by: Sunita Nadampalli <sunitan@ti.com> > >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > >> --- > >> fs/ext4/ext4fs.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c > >> index 7187dcfb05..081509dbb4 100644 > >> --- a/fs/ext4/ext4fs.c > >> +++ b/fs/ext4/ext4fs.c > >> @@ -71,7 +71,7 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, > >> blockcnt = lldiv(((len + pos) + blocksize - 1), blocksize); > >> > >> for (i = lldiv(pos, blocksize); i < blockcnt; i++) { > >> - lbaint_t blknr; > >> + long int blknr; > >> int blockoff = pos - (blocksize * i); > >> int blockend = blocksize; > >> int skipfirst = 0; > > > > My only question is, did you catch that by inspection, clang, or a newer > > This is reported by Sunita and not sure how she caught it. OK. Because if she's got clang used on a TI platform and linking and booting, I'd very much like to hear what else is required, or if nothing, what version of the tools she's using, so I can put that into my CI loop. I didn't have luck when I tried last, but it was a while ago. > > than gcc-6.3 warning? Also, fs/ext4/dev.c:63 is a similar problem, if > > you'd like to non-RFC a v2. Thanks! > > In this case do you want me to drop the check or change lbaint_t to long > int and update the argument type cast where ever ext4fs_devread() is called? Just drop it I think, thanks.
On Wednesday 26 April 2017 04:54 PM, Tom Rini wrote: > On Wed, Apr 26, 2017 at 08:56:49AM +0530, Lokesh Vutla wrote: >> >> >> On Tuesday 25 April 2017 11:34 PM, Tom Rini wrote: >>> On Tue, Apr 25, 2017 at 10:22:27AM +0530, Lokesh Vutla wrote: >>>> In file ext4fs.c funtion ext4fs_read_file() compares an >>>> unsigned expression with < 0 like below >>>> >>>> lbaint_t blknr; >>>> blknr = read_allocated_block(&(node->inode), i); >>>> if (blknr < 0) >>>> return -1; >>>> >>>> blknr is of type ulong/uint64_t. read_allocated_block() returns >>>> long int. So comparing blknr with < 0 will always be false. Instead >>>> declare blknr as long int. >>>> >>>> Reported-by: Sunita Nadampalli <sunitan@ti.com> >>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >>>> --- >>>> fs/ext4/ext4fs.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c >>>> index 7187dcfb05..081509dbb4 100644 >>>> --- a/fs/ext4/ext4fs.c >>>> +++ b/fs/ext4/ext4fs.c >>>> @@ -71,7 +71,7 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, >>>> blockcnt = lldiv(((len + pos) + blocksize - 1), blocksize); >>>> >>>> for (i = lldiv(pos, blocksize); i < blockcnt; i++) { >>>> - lbaint_t blknr; >>>> + long int blknr; >>>> int blockoff = pos - (blocksize * i); >>>> int blockend = blocksize; >>>> int skipfirst = 0; >>> >>> My only question is, did you catch that by inspection, clang, or a newer >> >> This is reported by Sunita and not sure how she caught it. > > OK. Because if she's got clang used on a TI platform and linking and > booting, I'd very much like to hear what else is required, or if > nothing, what version of the tools she's using, so I can put that into > my CI loop. I didn't have luck when I tried last, but it was a while > ago. Sure, Ill update you on this with more details. Thanks and regards, Lokesh > >>> than gcc-6.3 warning? Also, fs/ext4/dev.c:63 is a similar problem, if >>> you'd like to non-RFC a v2. Thanks! >> >> In this case do you want me to drop the check or change lbaint_t to long >> int and update the argument type cast where ever ext4fs_devread() is called? > > Just drop it I think, thanks. >
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 7187dcfb05..081509dbb4 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -71,7 +71,7 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, blockcnt = lldiv(((len + pos) + blocksize - 1), blocksize); for (i = lldiv(pos, blocksize); i < blockcnt; i++) { - lbaint_t blknr; + long int blknr; int blockoff = pos - (blocksize * i); int blockend = blocksize; int skipfirst = 0;
In file ext4fs.c funtion ext4fs_read_file() compares an unsigned expression with < 0 like below lbaint_t blknr; blknr = read_allocated_block(&(node->inode), i); if (blknr < 0) return -1; blknr is of type ulong/uint64_t. read_allocated_block() returns long int. So comparing blknr with < 0 will always be false. Instead declare blknr as long int. Reported-by: Sunita Nadampalli <sunitan@ti.com> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- fs/ext4/ext4fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)