Messages in this thread | | | Date | Sun, 17 Apr 2005 23:00:15 +0200 (CEST) | From | Jesper Juhl <> | Subject | Re: [PATCH] fs/fcntl.c : don't test unsigned value for less than zero |
| |
On Fri, 15 Apr 2005, Matthew Wilcox wrote:
> On Fri, Apr 15, 2005 at 10:03:05PM +1000, Herbert Xu wrote: > > I suppose it could be smart and stay quiet about > > > > val < 0 || val > BOUND > > > > However, gcc is slow enough as it is without adding unnecessary > > smarts like this. > > It only warns with -W on, not with -Wall, so I see no compelling > reason to fix this.
Fixing the -W warning was not the main point. The main point was simply that the check makes no sense at all.
> I think the real problem here is that 'arg' > is declared 2 pages earlier in the function prototype (aka the > function-growth-hormone-imbalance syndrome). > > There's two good ways of fixing this, adding a f_setsig() function: > [...] > or add a function that checks a variable to see if it's a valid signal number: [...] > > Looks like futex.c, ptrace.c, signal.c, sys.c and almost every > architecture's ptrace code could easily make use of the latter, but not > the former. It also looks like we have a few off-by-one errors. For [...] > so I'd recommend the second solution.
Thank you for your feedback, that makes a lot of sense. That should get rid of the pointless tests, get rid of the -W warning and get rid of the off-by-one errors - sounds good to me. I'll create patches and send them along shortly.
> But be careful not to "fix up" > cases like: > > ./kernel/exit.c: if (sig < 1 || sig > _NSIG) > > where we really don't want to allow zero. > I'll watch out for those. I can either leave them alone or re-write as one of if (valid_signal(sig) && sig != 0) if (valid_signal(sig) && sig > 0) any preference? Personally I would probably go with the 'rewrite as if (valid_signal(sig) && sig > 0)' one to encourage use of the new valid_signal() function and make usage consistent.
-- Jesper Juhl
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |