[PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned.

[PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned.

[Jesper Juhl]

The 'arg' argument to the function do_fcntl in fs/fcntl.c is of type
'unsigned long', thus it can never be less than zero (all callers of
do_fcntl take unsigned arguments as well and pass on unsigned values),
thus the check for 'arg < 0' in the F_SETSIG case of the switch in
do_fcntl can never be true and thus does not need to be there in the first
place.  Patch below (against 2.6.1-rc1-mm2) removes this dead code.


--- linux-2.6.1-rc1-mm2-orig/fs/fcntl.c 2004-01-06 01:33:08.000000000 +0100
+++ linux-2.6.1-rc1-mm2/fs/fcntl.c      2004-01-08 02:44:45.000000000 +0100
@@ -331,9 +331,8 @@ static long do_fcntl(unsigned int fd, un
 			break;
 		case F_SETSIG:
 			/* arg == 0 restores default behaviour. */
-			if (arg < 0 || arg > _NSIG) {
+			if (arg > _NSIG)
 				break;
-			}
 			err = 0;
 			filp->f_owner.signum = arg;
 			break;


Patch is only compile tested, but as far as I can see it should be
correct.


Kind regards,

Jesper Juhl



PS. CC'ing Matthew Wilcox as he's listed as 'file locking' maintainer in
MAINTAINERS, and Linus Torvalds as he's listed as original author in the
file - sorry for the inconvenience, if any.

Re: [PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned.

[Linus Torvalds]

On Thu, 8 Jan 2004, Jesper Juhl wrote:
> 
> The 'arg' argument to the function do_fcntl in fs/fcntl.c is of type
> 'unsigned long', thus it can never be less than zero (all callers of
> do_fcntl take unsigned arguments as well and pass on unsigned values),

I'm not sure I like these kinds of patches.

I _like_ the code being readable. The fact that the compiler can optimize 
away one of the tests if the type is right is fine. It seems to be 
draconian to remove code that is correct and safe, especially when the 
code has no real downsides to it.

Do we have a compiler that needlessly complains again? 

Sometimes it is the _complaints_ that are bogus. 

		Linus

Re: [PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned.

[Jesper Juhl]

On Wed, 7 Jan 2004, Linus Torvalds wrote:
>
> On Thu, 8 Jan 2004, Jesper Juhl wrote:
> >
> > The 'arg' argument to the function do_fcntl in fs/fcntl.c is of type
> > 'unsigned long', thus it can never be less than zero (all callers of
> > do_fcntl take unsigned arguments as well and pass on unsigned values),
>
> I'm not sure I like these kinds of patches.
>

Ok, let me try and argue in favour of it, and if you think the arguments
are bogus then I won't be doing any more of this type of patches.


> I _like_ the code being readable.

I can't argue with that, but I don't think this patch actually decreases
readabillity. It's still perfectly clear what the remaining code does, and
if anybody is wondering if 'arg' could ever be <0 then a quick glance at
the type will answer that.

Would you like this sort of patch better if removing the code went
hand-in-hand with the addition of a one-line comment stating something
like  /* the test for arg < 0 is not done since arg is unsigned */ or ?


>  The fact that the compiler can optimize
> away one of the tests if the type is right i2Ds fine. It seems to be
> draconian to remove code that is correct and safe, especially when the
> code has no real downsides to it.

>From my point of view it's a matter of correctness. Testing an unsigned
value for <0 makes no sense, and doing things that make no sense is a bad
habbit in my oppinion. Yes, the code will be optimized away, so it doesn't
actually do any harm, and in this case it's a very small amount of code,
but that's not always so. A little while ago I posted a similar patch that
removes some dead code in (amongst others) ReiserFS, and in that case it's
a bit more code (not much, but a bit more) and there I think removing the
impossible code makes the code easier to read since a person trying to
find out what's going on does not have to spend time tracing the workings
of something that can never execute in any case.


>
> Do we have a compiler that needlessly complains again?
>

Gcc /will/ warn about the fact that the result of an unsigned comparison
with <0 is always false if the code in question is compiled with
"-W -Wall", with the standard compile options no such warning is given.


> Sometimes it is the _complaints_ that are bogus.
>

I'm not arguing against that. My mission here is not to silence any
warning just for the sake of silencing the warning, although I must admit
that I am trying to get rid of as many potential warnings as I can - It
would be nice to be able to compile the kernel with "-W -Wall" and not
have the output too cluttered, and some of the things that gcc will warn
about could potentially hide real bugs, so I believe it's a valid
exercise. But my real goal is mainly to find and fix potential problems,
squishing potential warnings is a secondary bennefit.


-- Jesper Juhl

Re: [PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned.

[Paul Jackson]

Jesper wrote:
> I can't argue with that, but I don't think this patch actually decreases
> readabillity. It's still perfectly clear what the remaining code does, and
> if anybody is wondering if 'arg' could ever be <0 then a quick glance at
> the type will answer that.

The basic thought likely to go through the head of someone first
thinking about this bit of logic is that arg has to be between 0 and
_NSIG to be valid.  It then requires a second thought to realize that
since arg is 0, you don't have to check explicitly for arg < 0.

Everytime we can avoid requiring 'a second thought', we (feable minded
humans) win.


> Would you like this sort of patch better if removing the code went
> hand-in-hand with the addition of a one-line comment stating something
> like  /* the test for arg < 0 is not done since arg is unsigned */ or ?

No - such a comment doesn't remove the 'second thought' in this case.
Rather, it emphasizes it.

It is the sort of comment that (1) I find myself writing often, and then
(2) later replacing with code that renders the comment irrelevant.

It's one of those little "reader beware" (caveat lector) comments that
often mark places where a little code refinement can remove a comment.
Whenever a comment that explains why something is or is not done or is
special cased can be replaced with code that just does (or doesn't do)
it or doesn't need a special case, with no loss of form, fit, function
or performance, that's likely a win-win.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned.

[Hans Reiser]

Code is like poetry --- bloat needs to come out of it, and a rigorous 
discipline of removing all unnecessary complexity from it needs to be 
made a habit so that the exact meaning  of it will be seen by all.

Boy was that pompous.;-) Oh well, I'd like the reiserfs portions of his 
patch to be accepted, and I'll forward them to you in a few days.  
Please consider accepting them.

Hans

Jesper Juhl wrote:

>On Wed, 7 Jan 2004, Linus Torvalds wrote:
>  
>
>
>> The fact that the compiler can optimize
>>away one of the tests if the type is right i2Ds fine. It seems to be
>>draconian to remove code that is correct and safe, especially when the
>>code has no real downsides to it.
>>    
>>


-- 
Hans

Re: [PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned.

[Matthew Wilcox]

On Wed, Jan 07, 2004 at 06:47:49PM -0800, Linus Torvalds wrote:
> 
> 
> On Thu, 8 Jan 2004, Jesper Juhl wrote:
> > 
> > The 'arg' argument to the function do_fcntl in fs/fcntl.c is of type
> > 'unsigned long', thus it can never be less than zero (all callers of
> > do_fcntl take unsigned arguments as well and pass on unsigned values),
> 
> I'm not sure I like these kinds of patches.
> 
> I _like_ the code being readable. The fact that the compiler can optimize 
> away one of the tests if the type is right is fine. It seems to be 
> draconian to remove code that is correct and safe, especially when the 
> code has no real downsides to it.
> 
> Do we have a compiler that needlessly complains again? 

gcc does indeed complain about it with -W.  But -W turns on so many other
warnings, this one's lost in the noise.  I would actually like to be
able to make the header files -W clean so that I can compile individual
files with -W to catch some of my worst mistakes.

But I don't think that making the entire build -W clean is worthwhile.
It'd make the code too ugly.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain