Messages in this thread | | | Date | Wed, 7 May 2008 08:08:22 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] Fix cpuset sched_relax_domain_level control file |
| |
On Wed, 7 May 2008 12:48:09 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> On Tue, May 06, 2008 at 06:41:39PM -0700, Paul Menage wrote: > > On Tue, May 6, 2008 at 6:38 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > > On Tue, 06 May 2008 18:08:17 -0700 Paul Menage <menage@google.com> wrote: > > > > > > > -static int update_relax_domain_level(struct cpuset *cs, char *buf) > > > > +static int update_relax_domain_level(struct cpuset *cs, s64 val) > > > > { > > > > - int val = simple_strtol(buf, NULL, 10); > > > > - > > > > - if (val < 0) > > > > + if ((int)val < 0) > > > > val = -1; > > > > > > > > > > Are you sure about the typecast here? If `val' has a value of say > > > 0x0000_ffff_ffff_ffff then I assume the casted value will be negative, only > > > it wasn't? > > > > It seems like the simplest approach - if it's outside the range of a > > positive int, set it to -1. > > That's very hard to understand for someone who looks at the code - and > being able to understand the code is much more important than the > number of characters in the source code. > > If you'd write something like > > if ((val < 0) || (val > INT_MAX)) > > instead it would be obvious for the reader what's happening here, and > that this was intended.
What he said.
Our poor reader now knows what was intended. But he still doesn't know _why_ it was intended.
| |