Re: [PATCH] ext3: also fix loop in do_split()
| From: | Andrew Morton <akpm-AT-linux-foundation.org> | |
| To: | roel kluin <roel.kluin-AT-gmail.com> | |
| Subject: | Re: [PATCH] ext3: also fix loop in do_split() | |
| Date: | Tue, 2 Dec 2008 12:07:51 -0800 | |
| Message-ID: | <20081202120751.8b0226ca.akpm@linux-foundation.org> | |
| Cc: | sct-AT-redhat.com, adilger-AT-sun.com, linux-ext4-AT-vger.kernel.org, linux-kernel-AT-vger.kernel.org | |
| Archive‑link: | Article |
On Sat, 29 Nov 2008 04:40:42 -0500
roel kluin <roel.kluin@gmail.com> wrote:
> unsigned i >= 0 is always true
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
> index 3e5edc9..8f5e15d 100644
> --- a/fs/ext3/namei.c
> +++ b/fs/ext3/namei.c
> @@ -1188,7 +1188,7 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode
*dir,
> /* Split the existing block in the middle, size-wise */
> size = 0;
> move = 0;
> - for (i = count-1; i >= 0; i--) {
> + for (i = count-1; i < count; i--) {
> /* is more than half of this entry in 2nd half of the block? */
> if (size + map[i].size/2 > blocksize/2)
> break;
A local variable called `i' should always have signed type. In fact,
it should have `int' type. Doing
unsigned i;
is an act of insane vandalism, punishable by spending five additional
years coding in fortran.
I suggest you fix this by giving `i' the type God intended, or by
making it unsigned and then renaming it to something which is not
intended to trick programmers and reviewers.
Sheesh.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Posted Dec 4, 2008 16:45 UTC (Thu)
by felixfix (subscriber, #242)
[Link] (1 responses)
Posted Dec 4, 2008 23:44 UTC (Thu)
by tialaramex (subscriber, #21167)
[Link]
n is the Natural numbers ℕ, or counting numbers, 1, 2, 3, 4, etc. If you're a programmer you count from zero (and languages mostly don't offer the strict set of natural number anyway) so I would be surprised to find -4 in a variable named 'n'.
i is in contrast the Integers ℤ (a variable named z might make me wonder if it the code was dealing with three, or more dimensions, in which case it might be a float... but otherwise it would certainly be an integer)
And by the same instinct I would expect an unfamiliar but obviously numerical variable 'r' to be a Rational number ℝ, which in C would mean a float or double.
[LWN seems to screw up some characters during the edit cycle, until someone fixes it imagine the appropriate character in place, or knock up some Javascript to fix it on the fly...)
Posted Dec 5, 2008 3:06 UTC (Fri)
by nlucas (guest, #33793)
[Link]
Posted Dec 11, 2008 8:45 UTC (Thu)
by ldo (guest, #40946)
[Link]
Assuming I correctly understood the intent of the original version, I would have written it like: See, no problems with overflow, i stays within the valid range for an unsigned at all times.
I find that 90% of my loops tend to have an exit in the middle.
Posted Dec 11, 2008 18:31 UTC (Thu)
by joib (subscriber, #8541)
[Link]
As an aside, Fortran does not have unsigned integer types.
As another aside, in modern Fortran 'implicit none' is frequently recommended, meaning that all variables must be declared.
Re: [PATCH] ext3: also fix loop in do_split()
Re: [PATCH] ext3: also fix loop in do_split()
Re: [PATCH] ext3: also fix loop in do_split()
To me 'i' means "index", not a shortcut for "int".
Nothing wrong with unsigned, properly used
for (i = count - 1;;)
{
... body of loop ...
if (i == 0)
break;
--i;
} /*for*/
Re: [PATCH] ext3: also fix loop in do_split()