Skip to contentSkip to conversation

ext3: also fix loop in do_split()

PATCH In review roel kluin 4 participants 5 messages Nov 29, 2008 – Dec 5, 2008 ext4 file system

lore
Share & tools
lore

roel kluin

author
#0 lore raw

PATCH 1/1 ext3: also fix loop in do_split()

unsigned i >= 0 is always true

Signed-off-by Roel Kluin <[email protected]>

---

fs/ext3/namei.c +1 -1
index 3e5edc9..8f5e15d 100644
@@ -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;
#1 lore raw
Quoted 9 lines · On Sat, 29 Nov 2008 04:40:42 -0500 roel kluin <[email protected]> wrote:

unsigned i >= 0 is always true

Signed-off-by Roel Kluin <[email protected]>

---

fs/ext3/namei.c +0 -0
index 3e5edc9..8f5e15d 100644

fs/ext3/namei.c static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir, · line 1194

Quoted 8 lines
fs/ext3/namei.c +1 -1
@@ -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.

#2 lore raw
Quoted 5 lines · Andrew Morton

A local variable called i' should always have signed type. In fact, it should have int' type. Doing

	unsigned i;
Quoted 8 lines

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.

/me hangs head in shame, and points feebly but only halfheartedly at the other people who reviewed the change when it originally went in... I have no idea what I was thinking. Sorry. Please don't make me go back to Fortran.

-Eric

author
#3 lore raw

PATCH v4 1/1 ext3, ext4: make i signed in do_split()

In reply to Andrew Morton
Quoted 7 lines · Andrew Morton

Review

Quoted 4 lines · On Sat, 29 Nov 2008 04:40:42 -0500 roel kluin <[email protected]> wrote:

unsigned i >= 0 is always true

(quoted diff) +1 -1
-	for (i = count-1; i >= 0; i--) {
+	for (i = count-1; i < count; i--) {

			break;
Quoted 2 lines

A local variable called i' should always have signed type. In fact, it should have int' type.

You are right. although count is unsigned, dx_make_map() returns int, which is assigned to count, so this should fit in an int. (If you ack this, make sure to get this patch, and not one of the others.) --------------->8----------------8<--------------------
Make i signed, otherwise the loop will not end when it becomes negative.

Signed-off-by Roel Kluin <[email protected]>

---

fs/ext3/namei.c +2 -2
index 3e5edc9..4846327 100644
@@ -1156,9 +1156,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	u32 hash2;
 	struct dx_map_entry *map;
 	char *data1 = (*bh)->b_data, *data2;
-	unsigned split, move, size, i;
+	unsigned split, move, size;
 	struct ext3_dir_entry_2 *de = NULL, *de2;
-	int	err = 0;
+	int	err = 0, i;
 
 	bh2 = ext3_append (handle, dir, &newblock, &err);
 	if (!(bh2)) {
fs/ext4/namei.c +2 -2
index 63adcb7..bca90c1 100644
@@ -1166,9 +1166,9 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	u32 hash2;
 	struct dx_map_entry *map;
 	char *data1 = (*bh)->b_data, *data2;
-	unsigned split, move, size, i;
+	unsigned split, move, size;
 	struct ext4_dir_entry_2 *de = NULL, *de2;
-	int	err = 0;
+	int	err = 0, i;
 
 	bh2 = ext4_append (handle, dir, &newblock, &err);
 	if (!(bh2)) {
maintainer
#4 lore raw

Re: [PATCH v4] ext3, ext4: make i signed in do_split()

Quoted 10 lines · Roel Kluin
Quoted 7 lines · Andrew Morton

Review

Quoted 4 lines · On Sat, 29 Nov 2008 04:40:42 -0500 roel kluin <[email protected]> wrote:

unsigned i >= 0 is always true

(quoted diff) +1 -1
-	for (i = count-1; i >= 0; i--) {
+	for (i = count-1; i < count; i--) {

			break;
Quoted 6 lines
Quoted 2 lines

A local variable called i' should always have signed type. In fact, it should have int' type.

You are right. although count is unsigned, dx_make_map() returns int, which is assigned to count, so this should fit in an int. (If you ack this, make sure to get this patch, and not one of the others.)

ACK. I'll take care of getting this into the patch queue. I'll include both the ext3 and ext4 variants, since this one is simple enough that it's highly unlikely to conflict with anything Andrew might have in the -mm tree. This is also much more of a cleanup than anything else, so I don't think we need to push it until the merge window. (I *think* it might be possible for a 2k filesystems and *very* long filenames we might possibly be able to trigger a failure but it would be very hard to do, and it's been around for a very long time without anyone complaining about an actual real-life problem getting trigger.)

- Ted