On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote:
> On Sun, 01 Jul 2007 03:36:22 -0400
> Mingming Cao <cmm@xxxxxxxxxx> wrote:
>
> > with the patch all headers are checked. the code should become
> > more resistant to on-disk corruptions. needless BUG_ON() have
> > been removed. please, review for inclusion.
> >
> > ...
>
> > @@ -269,6 +239,70 @@
> > return size;
> > }
> >
> > +static inline int
> > +ext4_ext_max_entries(struct inode *inode, int depth)
>
> Please remove the `inline'.
>
done
> `inline' is usually wrong. It is wrong here. If this function has a
> single callsite (it has) then the compiler will inline it. If the function
> later gains more callsites then the compiler will know (correctly) not to
> inline it.
>
> We hope. If we find the compiler still inlines a function as large as this
> one then we'd need to use noinline and complain at the gcc developers.
>
> > +{
> > + int max;
> > +
> > + if (depth == ext_depth(inode)) {
> > + if (depth == 0)
> > + max = ext4_ext_space_root(inode);
> > + else
> > + max = ext4_ext_space_root_idx(inode);
> > + } else {
> > + if (depth == 0)
> > + max = ext4_ext_space_block(inode);
> > + else
> > + max = ext4_ext_space_block_idx(inode);
> > + }
> > +
> > + return max;
> > +}
> > +
> > +static int __ext4_ext_check_header(const char *function, struct inode *inode,
> > + struct ext4_extent_header *eh,
> > + int depth)
> > +{
> > + const char *error_msg = NULL;
>
> Unneeded initialisation.
>
done
> > + int max = 0;
> > +
> > + if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
> > + error_msg = "invalid magic";
> > + goto corrupted;
> > + }
> > + if (unlikely(le16_to_cpu(eh->eh_depth) != depth)) {
> > + error_msg = "unexpected eh_depth";
> > + goto corrupted;
> > + }
> > + if (unlikely(eh->eh_max == 0)) {
> > + error_msg = "invalid eh_max";
> > + goto corrupted;
> > + }
> > + max = ext4_ext_max_entries(inode, depth);
> > + if (unlikely(le16_to_cpu(eh->eh_max) > max)) {
> > + error_msg = "too large eh_max";
> > + goto corrupted;
> > + }
> > + if (unlikely(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max))) {
> > + error_msg = "invalid eh_entries";
> > + goto corrupted;
> > + }
> > + return 0;
> > +
> > +corrupted:
> > + ext4_error(inode->i_sb, function,
> > + "bad header in inode #%lu: %s - magic %x, "
> > + "entries %u, max %u(%u), depth %u(%u)",
> > + inode->i_ino, error_msg, le16_to_cpu(eh->eh_magic),
> > + le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max),
> > + max, le16_to_cpu(eh->eh_depth), depth);
> > +
> > + return -EIO;
> > +}
> > +
> >
> > ...
> >
> > + i = depth = ext_depth(inode);
> >
>
> Mulitple assignments like this are somewhat unpopular from the coding-style
> POV.
>
okay.
> We have a local variable called `i' which is not used as a simple counter
> and which has no explanatory comment. This is plain bad. Please find a
> better name for this variable. Review the code for other such lack of
> clarity - I'm sure there's more.
>
"i" is is loop counter. I have moved the i= depth to before the while
loop.
>
> > - BUG_ON(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max));
> > - BUG_ON(eh->eh_magic != EXT4_EXT_MAGIC);
>
> Yeah, this patch improves things a lot.
>
> How well-tested is it? Have any of these errors actually been triggered?
>
> > path[i].p_hdr = ext_block_hdr(path[i].p_bh);
> > - if (ext4_ext_check_header(__FUNCTION__, inode,
> > - path[i].p_hdr)) {
> > - err = -EIO;
> > - goto out;
> > - }
> > }
> >
> > - BUG_ON(le16_to_cpu(path[i].p_hdr->eh_entries)
> > - > le16_to_cpu(path[i].p_hdr->eh_max));
> > - BUG_ON(path[i].p_hdr->eh_magic != EXT4_EXT_MAGIC);
> > -
> > if (!path[i].p_idx) {
> > /* this level hasn't been touched yet */
> > path[i].p_idx = EXT_LAST_INDEX(path[i].p_hdr);
> > @@ -1873,17 +1890,24 @@
> > i, EXT_FIRST_INDEX(path[i].p_hdr),
> > path[i].p_idx);
> > if (ext4_ext_more_to_rm(path + i)) {
> > + struct buffer_head *bh;
> > /* go to the next level */
> > ext_debug("move to level %d (block %llu)\n",
> > i + 1, idx_pblock(path[i].p_idx));
> > memset(path + i + 1, 0, sizeof(*path));
> > - path[i+1].p_bh =
> > - sb_bread(sb, idx_pblock(path[i].p_idx));
> > - if (!path[i+1].p_bh) {
> > + bh = sb_bread(sb, idx_pblock(path[i].p_idx));
> > + if (!bh) {
> > /* should we reset i_size? */
> > err = -EIO;
> > break;
> > }
> > + BUG_ON(i + 1 > depth);
>
> ouch. Couldn't we do WARN_ON then return -EIO here?
Done
>
> > + if (ext4_ext_check_header(inode, ext_block_hdr(bh),
> > + depth - i - 1)) {
> > + err = -EIO;
> > + break;
> > + }
> > + path[i+1].p_bh = bh;
>
> Really that should have been "i + 1". checkpatch misses this. It seems to
> be missing more stuff that it used to lately.
Done.
Hi Andrew,
Attached is incremental changes to address your comments.
Signed-off-by: Mingming Cao <cmm@xxxxxxxxxx>
---
fs/ext4/extents.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
Index: linux-2.6.22/fs/ext4/extents.c
===================================================================
--- linux-2.6.22.orig/fs/ext4/extents.c 2007-07-15 23:25:38.000000000 -0700
+++ linux-2.6.22/fs/ext4/extents.c 2007-07-15 23:44:03.000000000 -0700
@@ -239,7 +239,7 @@ static int ext4_ext_space_root_idx(struc
return size;
}
-static inline int
+static int
ext4_ext_max_entries(struct inode *inode, int depth)
{
int max;
@@ -263,7 +263,7 @@ static int __ext4_ext_check_header(const
struct ext4_extent_header *eh,
int depth)
{
- const char *error_msg = NULL;
+ const char *error_msg;
int max = 0;
if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
@@ -498,7 +498,7 @@ ext4_ext_find_extent(struct inode *inode
short int depth, i, ppos = 0, alloc = 0;
eh = ext_inode_hdr(inode);
- i = depth = ext_depth(inode);
+ depth = ext_depth(inode);
if (ext4_ext_check_header(inode, eh, depth))
return ERR_PTR(-EIO);
@@ -513,6 +513,7 @@ ext4_ext_find_extent(struct inode *inode
}
path[0].p_hdr = eh;
+ i = depth;
/* walk through the tree */
while (i) {
ext_debug("depth %d: num %d, max %d\n",
@@ -1973,13 +1974,16 @@ int ext4_ext_remove_space(struct inode *
err = -EIO;
break;
}
- BUG_ON(i + 1 > depth);
+ if (WARN_ON(i + 1 > depth)) {
+ err = -EIO;
+ break;
+ }
if (ext4_ext_check_header(inode, ext_block_hdr(bh),
depth - i - 1)) {
err = -EIO;
break;
}
- path[i+1].p_bh = bh;
+ path[i + 1].p_bh = bh;
/* save actual number of indexes since this
* number is changed at the next iteration */
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
