Skip to contentSkip to conversation

ext4: Fix the loop condition in ext4_mb_free_committed_blocks

PATCH Superseded Shen Feng 3 participants 4 messages Jun 11, 2008 – Jun 12, 2008 ext4 file system

lore
Share & tools
lore

Shen Feng

author
#0 lore raw

PATCH 1/1 ext4: Fix the loop condition in ext4_mb_free_committed_blocks

Since md is freed before the do-while checks it,
it's better to change it to while(1).

---
 fs/ext4/mballoc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/ext4/mballoc.c +1 -1
index 42553f6..063f820 100644
@@ -2572,7 +2572,7 @@ ext4_mb_free_committed_blocks(struct super_block *sb)
 		kfree(md);
 		ext4_mb_release_desc(&e4b);
 
-	} while (md);
+	} while (1);
 
 	mb_debug("freed %u blocks in %u structures\n", count, count2);
 }
Signature · 1 line
1.5.4.5
Reviewed-by on v1 1/1
#1 lore raw
Quoted 5 lines · Shen Feng

Since md is freed before the do-while checks it,
it's better to change it to while(1).

Quoted 23 lines
---
 fs/ext4/mballoc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/ext4/mballoc.c +1 -1
index 42553f6..063f820 100644
@@ -2572,7 +2572,7 @@ ext4_mb_free_committed_blocks(struct super_block *sb)
 		kfree(md);
 		ext4_mb_release_desc(&e4b);
 
-	} while (md);
+	} while (1);
 
 	mb_debug("freed %u blocks in %u structures\n", count, count2);
 }
Signature · 5 lines
1.5.4.5
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#2 lore raw
In reply to Shen Feng
Quoted 3 lines · Shen Feng

Since md is freed before the do-while checks it,
it's better to change it to while(1).

This actually isn't a bug, since there is no problem checking a pointer that has been freed; its only *dereferencing* a pointer which is bad. That being said, md is never NULL at the end of the loop, since in the middle of the loop is the only break condition:

	if (md == NULL)
		break;

So the patch saves a tiny amount of compiled code, but it isn't really a fix in any way.

That being said, if we're going to make this sort of change, my preference would be to use the more common C idiom:

	while (1) {
	      ...
	}

as opposed to

	do {
	      ...
	} while (1);

The former makes it quite clear that any exit from the loop is not going to be coming from loop construct itself, but from any embedded break statements inside the loop construct

Regards,

- Ted

author
#3 lore raw

PATCH v2 1/1 ext4: Fix the loop condition in ext4_mb_free_committed_blocks

Quoted 24 lines
Quoted 3 lines

Since md is freed before the do-while checks it,
it's better to change it to while(1).

This actually isn't a bug, since there is no problem checking a pointer that has been freed; its only *dereferencing* a pointer which is bad. That being said, md is never NULL at the end of the loop, since in the middle of the loop is the only break condition:

	if (md == NULL)
		break;

So the patch saves a tiny amount of compiled code, but it isn't really a fix in any way.

That being said, if we're going to make this sort of change, my preference would be to use the more common C idiom:

	while (1) {
	      ...
	}

as opposed to

Quoted 8 lines
	do {
	      ...
	} while (1);

The former makes it quite clear that any exit from the loop is not going to be coming from loop construct itself, but from any embedded break statements inside the loop construct

Yes. You are right. Revise the patch as you suggested.

---
 fs/ext4/mballoc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/ext4/mballoc.c +2 -2
index c9900aa..bd6cf22 100644
@@ -2521,7 +2521,7 @@ ext4_mb_free_committed_blocks(struct super_block *sb)
 		return;
 
 	/* there is committed blocks to be freed yet */
-	do {
+	while (1) {
 		/* get next array of blocks */
 		md = NULL;
 		spin_lock(&sbi->s_md_lock);
@@ -2561,7 +2561,7 @@ ext4_mb_free_committed_blocks(struct super_block *sb)
 		kfree(md);
 		ext4_mb_release_desc(&e4b);
 
-	} while (md);
+	}
 
 	mb_debug("freed %u blocks in %u structures\n", count, count2);
 }

-- 1.5.4.5