Phil Marek writes:
> I think that at least the errorcode in mkdir is wrong - in
> fs/ext2/namei.c:ext2_mkdir the errorcode gets rewritten to EIO -
> even if it was already set!! (to ENOSPACE or anything else)
> So I suggest something like
>
> fs/ext2/namei.c:437
> + err=0;
> inode = ext2_new_inode (dir, S_IFDIR, &err);
> fs/ext2/namei.c:440
> - return -EIO;
> + return err || -EIO;
Not needed, as ext2_new_inode() always sets &err to some sane value.
This is easily seen by looking in the ext2_new_inode() code.
Linux-2.2.14 had it right here - should be simply "return err", but
I think Ted changed this as part of an unrelated fix. Comments, Ted?
I believe I submitted a micro-patch to fix this several months ago.
> fs/ext2/namei.c:451
> - return -EIO;
> + return err || -EIO;
It is better to fix the cause (i.e. the function) rather than the symptom
(i.e. the return code). Again, this should simply be "return err", since
ext2_bread always sets err to some sane value. I traced this through:
ext2_bread() calls ext2_getblk to set *err or sets *err = -EIO;
ext2_getblk() calls ext2_get_block to set *err;
ext2_get_block() returns 0, -EIO, or {inode,block}_getblk _may_ set err;
The inode_getblk and block_getblk functions themselves do not always set *err,
although err = 0 before they are called from ext2_get_block(), so there is
_always_ a return value set from ext2_get_block().
The ext2_get_block() function may call block_getblk() several times without
checking the return value, so it _appears_ it would overwrite the last error
value. However, one (non-obvious) thing to note is that bh = NULL on error
return* from {inode,block}_getblk(), and calls to block_getblk() are a no-op
if bh == NULL.
(*) There was one (probably very rare) error case where block_getblk()
might return non-NULL: after bforget(result) jumps back to repeat: and
then we get an error with ext2_alloc_block() - we return a bad result.
I also fixed this in the attached patch.
I also found that ext2_alloc_block() did not set a return code in the
(#undef'd) PREALLOC path, and inode_getblk() set a return value instead
of using the error from ext2_alloc_block(). I also verified that
ext2_alloc_block() itself always sets an error code.
The below patch is against 2.3.99-pre5, but I don't think this portion of
the code has changed in the meantime. IMHO, the patch is "obviously correct"
and should be applied to the kernel, although it has not recieved any real
testing (as it deals entirely with error cases). It may also be worthwhile
to check if these changes are needed in 2.2.X.
Cheers, Andreas
--- cut here ---
diff -ru linux-2.3.99p5/fs/ext2/balloc.c linux-2.3.99p5-ext2base/fs/ext2/balloc.c
--- linux-2.3.99p5/fs/ext2/balloc.c Sun Mar 26 21:34:49 2000
+++ linux-2.3.99p5-ext2base/fs/ext2/balloc.c Mon May 29 10:50:17 2000
@@ -473,11 +473,8 @@
if (i >= sb->u.ext2_sb.s_groups_count)
i = 0;
gdp = ext2_get_group_desc (sb, i, &bh2);
- if (!gdp) {
- *err = -EIO;
- unlock_super (sb);
- return 0;
- }
+ if (!gdp)
+ goto io_error;
if (le16_to_cpu(gdp->bg_free_blocks_count) > 0)
break;
}
diff -ru linux-2.3.99p5/fs/ext2/inode.c linux-2.3.99p5-ext2base/fs/ext2/inode.c
--- linux-2.3.99p5/fs/ext2/inode.c Sun Mar 26 21:34:49 2000
+++ linux-2.3.99p5-ext2base/fs/ext2/inode.c Mon May 29 10:36:34 2000
@@ -117,7 +117,7 @@
inode->u.ext2_i.i_prealloc_count--;
ext2_debug ("preallocation hit (%lu/%lu).\n",
++alloc_hits, ++alloc_attempts);
-
+ *err = 0;
} else {
ext2_discard_prealloc (inode);
ext2_debug ("preallocation miss (%lu/%lu).\n",
@@ -200,6 +200,7 @@
return ret;
}
+/* must return NULL on error */
static struct buffer_head * inode_getblk (struct inode * inode, int nr,
int new_block, int * err, int metadata, long *phys, int *new)
{
@@ -232,7 +233,6 @@
limit >>= EXT2_BLOCK_SIZE_BITS(inode->i_sb);
if (new_block >= limit) {
send_sig(SIGXFSZ, current, 0);
- *err = -EFBIG;
return NULL;
}
}
@@ -260,7 +260,6 @@
tmp = ext2_alloc_block (inode, goal, err);
if (!tmp) {
- *err = -ENOSPC;
return NULL;
}
if (metadata) {
@@ -311,7 +310,7 @@
* can fail due to: - not present
* - out of space
*
- * NULL return in the data case is mandatory.
+ * NULL return in the data case, or on error, is mandatory.
*/
static struct buffer_head * block_getblk (struct inode * inode,
struct buffer_head * bh, int nr,
@@ -384,6 +383,7 @@
if (*p) {
ext2_free_blocks (inode, tmp, 1);
bforget (result);
+ result = NULL;
goto repeat;
}
} else {
@@ -479,17 +479,18 @@
ptr = iblock;
/*
- * ok, these macros clean the logic up a bit and make
- * it much more readable:
+ * Ok, these macros clean the logic up a bit and make
+ * it much more readable. One thing to note is that bh == NULL
+ * if there is an error, and block_getblk is a no-op in that case.
*/
#define GET_INODE_DATABLOCK(x) \
inode_getblk(inode, x, iblock, &err, 0, &phys, &new)
#define GET_INODE_PTR(x) \
inode_getblk(inode, x, iblock, &err, 1, NULL, NULL)
#define GET_INDIRECT_DATABLOCK(x) \
- block_getblk (inode, bh, x, iblock, &err, 0, &phys, &new);
+ block_getblk (inode, bh, x, iblock, &err, 0, &phys, &new)
#define GET_INDIRECT_PTR(x) \
- block_getblk (inode, bh, x, iblock, &err, 1, NULL, NULL);
+ block_getblk (inode, bh, x, iblock, &err, 1, NULL, NULL)
if (ptr < direct_blocks) {
bh = GET_INODE_DATABLOCK(ptr);
@@ -547,13 +548,11 @@
struct buffer_head * ext2_getblk(struct inode * inode, long block, int create, int * err)
{
struct buffer_head dummy;
- int error;
dummy.b_state = 0;
dummy.b_blocknr = -1000;
- error = ext2_get_block(inode, block, &dummy, create);
- *err = error;
- if (!error && buffer_mapped(&dummy)) {
+ *err = ext2_get_block(inode, block, &dummy, create);
+ if (!*err && buffer_mapped(&dummy)) {
struct buffer_head *bh;
bh = getblk(dummy.b_dev, dummy.b_blocknr, inode->i_sb->s_blocksize);
if (buffer_new(&dummy)) {
diff -ru linux-2.3.99p5/fs/ext2/namei.c linux-2.3.99p5-ext2base/fs/ext2/namei.c
--- linux-2.3.99p5/fs/ext2/namei.c Thu Apr 27 22:42:20 2000
+++ linux-2.3.99p5-ext2base/fs/ext2/namei.c Mon May 29 09:30:51 2000
@@ -437,7 +437,7 @@
inode = ext2_new_inode (dir, S_IFDIR, &err);
if (!inode)
- return -EIO;
+ return err;
inode->i_op = &ext2_dir_inode_operations;
inode->i_fop = &ext2_dir_operations;
@@ -448,7 +448,7 @@
inode->i_nlink--; /* is this nlink == 0? */
mark_inode_dirty(inode);
iput (inode);
- return -EIO;
+ return err;
}
de = (struct ext2_dir_entry_2 *) dir_block->b_data;
de->inode = cpu_to_le32(inode->i_ino);
--- cut here ---
--
Andreas Dilger \ "If a man ate a pound of pasta and a pound of antipasto,
\ would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/ -- Dogbert
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed May 31 2000 - 21:00:21 EST