fs/compat: remove redundant 'less than zero' check
Subsystems (2)
PATCH 1/1 fs/compat: remove redundant 'less than zero' check
Smatch complains about the check in compat.c. fs/compat.c:565 compat_rw_copy_check_uvector() warn:
unsigned 'nr_segs' is never less than zero.
I think, there is no reason to check if the value nr_segs is less than zero. So I removed it.
--- fs/compat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/compat.c +1 -1
@@ -562,7 +562,7 @@ ssize_t compat_rw_copy_check_uvector(int type, goto out; ret = -EINVAL ; -if (nr_segs >UIO_MAXIOV || nr_segs <0 ) +if (nr_segs >UIO_MAXIOV ) goto out;if (nr_segs > fast_segs) { ret = -ENOMEM ;
Signature · 1 line
2.1.0
Quoted 6 lines · Firo Yang
Smatch complains about the check in
compat.c.fs/compat.c:565compat_rw_copy_check_uvector()warn:
unsigned 'nr_segs' is never less than zero.I think, there is no reason to check if the value
nr_segsis less than zero. So I removed it.
It would be good to explain why you think this. What other statements in the code imply this property?
julia
Quoted 27 lines
Signed-off-by Firo Yang <[email protected]>--- fs/compat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/compat.c +1
-2@@ -562,7 +562,7 @@ ssize_t compat_rw_copy_check_uvector(int type, goto out; ret = -EINVAL ; -if (nr_segs >UIO_MAXIOV || nr_segs <0 ) +if (nr_segs >UIO_MAXIOV ) goto out;if (nr_segs > fast_segs) { ret = -ENOMEM ; --2.1.0
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): CAOXNuu2VKCWNSiePoSw4-G3TqxpXDE9fR75B75-fhQLa-SPcJg@mail.gmail.com
Quoted 2 lines · Firo Yang
It was because that1.
nr_segsstand for the number of 'segs' should be large or equal to 0, a positive number.
This is not very convincing. A negative number could be used to encode a failure of some previous operation, although normally that failure should have been checked for elsewhere.
Quoted 1 line
2. the type of
nr_segsis 'unsigned long', It imply a positive number.
OK, this seems like a reasonable explanation. Currently the test can never be true, so there is no point to have it. It would be good to note this in the commit message of your patch.
julia
Quoted 59 lines
3. the code if (
nr_segs>UIO_MAXIOV) is enough to keep the valuenr_segssafe.Regards
FiroOn Fri, Apr 24, 2015 at 6:10 PM, Julia Lawall <[email protected]> wrote:
On Fri, 24 Apr 2015, Firo Yang wrote: > Smatch complains about the check in compat.c. > fs/compat.c:565 compat_rw_copy_check_uvector() warn: > unsigned 'nr_segs' is never less than zero. > > I think, there is no reason to check if the value nr_segs > is less than zero. So I removed it. It would be good to explain why you think this. What other statements in the code imply this property? julia > > Signed-off-by: Firo Yang <[email protected]> > --- > fs/compat.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/compat.c b/fs/compat.c > index 6fd272d..beaf15b 100644 > --- a/fs/compat.c > +++ b/fs/compat.c > @@ -562,7 +562,7 @@ ssize_t compat_rw_copy_check_uvector(int type, > goto out; > > ret = -EINVAL; > - if (nr_segs > UIO_MAXIOV || nr_segs < 0) > + if (nr_segs > UIO_MAXIOV) > goto out; > if (nr_segs > fast_segs) { > ret = -ENOMEM; > -- > 2.1.0 >Quoted 2 lines
--
To unsubscribe from this list: send the line "unsubscribekernel-janitors" in
Quoted 3 lines
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
fs/compat.c ssize_t compat_rw_copy_check_uvector(int type, · line 565
Quoted 2 lines · Firo Yang
fs/compat.c +1
-1-if (nr_segs >UIO_MAXIOV || nr_segs <0 ) +if (nr_segs >UIO_MAXIOV )
Linus said at kernel summit that he thinks this kind of checks are ok.
Smatch already ignores checks like:
if (nr_segs < 0 || nr_segs > UIO_MAXIOV)
So making it ignore this one as well shouldn't be hard... I'll change Smatch to ignore this one as well.
regards,
dan carpenter
Quoted 7 lines · Dan Carpenter
fs/compat.c ssize_t compat_rw_copy_check_uvector(int type, · line 565
Quoted 2 lines · Firo Yang
fs/compat.c +1
-1-if (nr_segs >UIO_MAXIOV || nr_segs <0 ) +if (nr_segs >UIO_MAXIOV )Linus said at kernel summit that he thinks this kind of checks are ok.
Quoted 6 lines
Smatch already ignores checks like:
if (nr_segs < 0 || nr_segs > UIO_MAXIOV)So making it ignore this one as well shouldn't be hard... I'll change Smatch to ignore this one as well.
It is unsigned. What is the point of cluttering the code?
julia
Quoted 9 lines
regards,
dan carpenter--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · Julia Lawall
Quoted 7 lines · Dan Carpenter
fs/compat.c ssize_t compat_rw_copy_check_uvector(int type, · line 565
Quoted 2 lines · Firo Yang
fs/compat.c +1
-1-if (nr_segs >UIO_MAXIOV || nr_segs <0 ) +if (nr_segs >UIO_MAXIOV )Linus said at kernel summit that he thinks this kind of checks are ok.
Quoted 8 lines
Quoted 6 lines
Smatch already ignores checks like:
if (nr_segs < 0 || nr_segs > UIO_MAXIOV)So making it ignore this one as well shouldn't be hard... I'll change Smatch to ignore this one as well.
It is unsigned. What is the point of cluttering the code?
I guess I don't feel strongly either way. It was Linus who said something like "the intent is clear from the code" so he isn't a fan of these particular static checker fixes.
These are easy enough for me to filter out so they needn't cause a false positive.
They're more of a style issue than anything else. With coccinelle, it's easier to care about style issues because you can fix them automatically. In Smatch caring about style issues is a time suck.
regards,
dan carpenter