- ext3_dx_find_entry() exit with out setting proper error pointer
- do_split() exit with out setting proper error pointer
it is realy painful because many callers contain folowing code:
de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
if (!(de))
return retval;
<<< WOW retval wasn't changed by do_split(), so caller failed
<<< but return SUCCESS :)
- Rearrange do_split() error path. Current error path is realy ugly, all
this up and down jump stuff doesn't make code easy to understand.
Signed-off-by: Monakhov Dmitriy <[email protected]>
---
fs/ext3/namei.c | 26 +++++++++++++++-----------
fs/ext4/namei.c | 26 +++++++++++++++-----------
2 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 49159f1..1a52586 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -969,6 +969,7 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
(block<<EXT3_BLOCK_SIZE_BITS(sb))
+((char *)de - bh->b_data))) {
brelse (bh);
+ *err = ERR_BAD_DX_DIR;
goto errout;
}
*res_dir = de;
@@ -1134,9 +1135,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
char *data1 = (*bh)->b_data, *data2;
unsigned split;
struct ext3_dir_entry_2 *de = NULL, *de2;
- int err;
+ int err = 0;
- bh2 = ext3_append (handle, dir, &newblock, error);
+ bh2 = ext3_append (handle, dir, &newblock, &err);
if (!(bh2)) {
brelse(*bh);
*bh = NULL;
@@ -1145,14 +1146,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
BUFFER_TRACE(*bh, "get_write_access");
err = ext3_journal_get_write_access(handle, *bh);
- if (err) {
- journal_error:
- brelse(*bh);
- brelse(bh2);
- *bh = NULL;
- ext3_std_error(dir->i_sb, err);
- goto errout;
- }
+ if (err)
+ goto journal_error;
+
BUFFER_TRACE(frame->bh, "get_write_access");
err = ext3_journal_get_write_access(handle, frame->bh);
if (err)
@@ -1195,8 +1191,16 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
goto journal_error;
brelse (bh2);
dxtrace(dx_show_index ("frame", frame->entries));
-errout:
return de;
+
+journal_error:
+ brelse(*bh);
+ brelse(bh2);
+ *bh = NULL;
+errout:
+ ext3_std_error(dir->i_sb, err);
+ *error = err;
+ return NULL;
}
#endif
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e7e1d79..682a3d7 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -967,6 +967,7 @@ static struct buffer_head * ext4_dx_find_entry(struct dentry *dentry,
(block<<EXT4_BLOCK_SIZE_BITS(sb))
+((char *)de - bh->b_data))) {
brelse (bh);
+ *err = ERR_BAD_DX_DIR;
goto errout;
}
*res_dir = de;
@@ -1132,9 +1133,9 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
char *data1 = (*bh)->b_data, *data2;
unsigned split;
struct ext4_dir_entry_2 *de = NULL, *de2;
- int err;
+ int err = 0;
- bh2 = ext4_append (handle, dir, &newblock, error);
+ bh2 = ext4_append (handle, dir, &newblock, &err);
if (!(bh2)) {
brelse(*bh);
*bh = NULL;
@@ -1143,14 +1144,9 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
BUFFER_TRACE(*bh, "get_write_access");
err = ext4_journal_get_write_access(handle, *bh);
- if (err) {
- journal_error:
- brelse(*bh);
- brelse(bh2);
- *bh = NULL;
- ext4_std_error(dir->i_sb, err);
- goto errout;
- }
+ if (err)
+ goto journal_error;
+
BUFFER_TRACE(frame->bh, "get_write_access");
err = ext4_journal_get_write_access(handle, frame->bh);
if (err)
@@ -1193,8 +1189,16 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
goto journal_error;
brelse (bh2);
dxtrace(dx_show_index ("frame", frame->entries));
-errout:
return de;
+
+journal_error:
+ brelse(*bh);
+ brelse(bh2);
+ *bh = NULL;
+errout:
+ ext4_std_error(dir->i_sb, err);
+ *error = err;
+ return NULL;
}
#endif
--
1.5.0.1
On Mar 04, 2007 17:18 +0300, Dmitriy Monakhov wrote:
> - ext3_dx_find_entry() exit with out setting proper error pointer
> - do_split() exit with out setting proper error pointer
> it is realy painful because many callers contain folowing code:
> de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
> if (!(de))
> return retval;
> <<< WOW retval wasn't changed by do_split(), so caller failed
> <<< but return SUCCESS :)
> - Rearrange do_split() error path. Current error path is realy ugly, all
> this up and down jump stuff doesn't make code easy to understand.
>
> Signed-off-by: Monakhov Dmitriy <[email protected]>
> ---
> fs/ext3/namei.c | 26 +++++++++++++++-----------
> fs/ext4/namei.c | 26 +++++++++++++++-----------
> 2 files changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
> index 49159f1..1a52586 100644
> --- a/fs/ext3/namei.c
> +++ b/fs/ext3/namei.c
> @@ -969,6 +969,7 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
> (block<<EXT3_BLOCK_SIZE_BITS(sb))
> +((char *)de - bh->b_data))) {
> brelse (bh);
> + *err = ERR_BAD_DX_DIR;
> goto errout;
> }
> *res_dir = de;
I have no objection to this change (by principle of least surprise) but
I don't know if it fixes a real problem. The one caller of this function
treats ERR_BAD_DX_DIR the same as bh == NULL.
> @@ -1134,9 +1135,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
> char *data1 = (*bh)->b_data, *data2;
> unsigned split;
> struct ext3_dir_entry_2 *de = NULL, *de2;
> - int err;
> + int err = 0;
>
> - bh2 = ext3_append (handle, dir, &newblock, error);
> + bh2 = ext3_append (handle, dir, &newblock, &err);
Why don't we just remove "err" entirely and use *error to avoid any risk
of setting err and not returning it in error? Also reduces stack usage
that tiny little bit.
In ext3_dx_add_entry() it appears like we should "goto journal_error"
to report an error after the failed call to do_split(), instead of just
"goto cleanup" so that we report an error in the filesystem. Not 100%
sure of this.
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.
Andreas Dilger <[email protected]> writes:
> On Mar 04, 2007 17:18 +0300, Dmitriy Monakhov wrote:
>> - ext3_dx_find_entry() exit with out setting proper error pointer
>> - do_split() exit with out setting proper error pointer
>> it is realy painful because many callers contain folowing code:
>> de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
>> if (!(de))
>> return retval;
>> <<< WOW retval wasn't changed by do_split(), so caller failed
>> <<< but return SUCCESS :)
>> - Rearrange do_split() error path. Current error path is realy ugly, all
>> this up and down jump stuff doesn't make code easy to understand.
>>
>> Signed-off-by: Monakhov Dmitriy <[email protected]>
>> ---
>> fs/ext3/namei.c | 26 +++++++++++++++-----------
>> fs/ext4/namei.c | 26 +++++++++++++++-----------
>> 2 files changed, 30 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
>> index 49159f1..1a52586 100644
>> --- a/fs/ext3/namei.c
>> +++ b/fs/ext3/namei.c
>> @@ -969,6 +969,7 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
>> (block<<EXT3_BLOCK_SIZE_BITS(sb))
>> +((char *)de - bh->b_data))) {
>> brelse (bh);
>> + *err = ERR_BAD_DX_DIR;
>> goto errout;
>> }
>> *res_dir = de;
>
> I have no objection to this change (by principle of least surprise) but
> I don't know if it fixes a real problem. The one caller of this function
> treats ERR_BAD_DX_DIR the same as bh == NULL.
Execly ERR_BAD_DX_DIR treats the same as bh == NULL and let's look at
callers code:
struct buffer_head * ext3_find_entry(......)
{
.......
bh = ext3_dx_find_entry(dentry, res_dir, &err);
/*
* On success, or if the error was file not found,
* return. Otherwise, fall back to doing a search the
* old fashioned way.
*/
if (bh || (err != ERR_BAD_DX_DIR))
<<<<< after ext3_dx_find_entry() has failed , but don't set err pointer
<<<<< we get (bh == NULL, err == 0) , and then just return NULL.
return bh;
.......
}
>
>> @@ -1134,9 +1135,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
>> char *data1 = (*bh)->b_data, *data2;
>> unsigned split;
>> struct ext3_dir_entry_2 *de = NULL, *de2;
>> - int err;
>> + int err = 0;
>>
>> - bh2 = ext3_append (handle, dir, &newblock, error);
>> + bh2 = ext3_append (handle, dir, &newblock, &err);
>
> Why don't we just remove "err" entirely and use *error to avoid any risk
> of setting err and not returning it in error? Also reduces stack usage
> that tiny little bit.
I've chosen this approuch becouse of:
1) this approuch was used in block allocation code.
2) this approuch prevent folowing errors:
*error = do_some_function(.....)
........ /* some code*/
if(error)
we do this checking as we do it thousands times before, but forget
what error it pointer here. And compiler can't warn us here because
it is absolutely legal operation. At least it is better to rename
error to errorp.
Btw: I've thought about adding assertations to error path witch may check
what errp pointer was properly initialized after error happends.
folowing code is draft only:
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -63,6 +63,7 @@ static struct buffer_head *ext3_append(handle_t *handle,
EXT3_I(inode)->i_disksize = inode->i_size;
ext3_journal_get_write_access(handle,bh);
}
+ assert(bh || *err);
return bh;
}
@@ -433,6 +434,7 @@ fail2:
frame--;
}
fail:
+ assert(*err != 0);
return NULL;
}
>
>
> In ext3_dx_add_entry() it appears like we should "goto journal_error"
> to report an error after the failed call to do_split(), instead of just
> "goto cleanup" so that we report an error in the filesystem. Not 100%
> sure of this.
do_split() already invoked ext3_std_error() on it's "journal_error" path.
>
> Cheers, Andreas
> --
> Andreas Dilger
> Principal Software Engineer
> Cluster File Systems, Inc.
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Dmitriy Monakhov <[email protected]> writes:
> - ext3_dx_find_entry() exit with out setting proper error pointer
> - do_split() exit with out setting proper error pointer
> it is realy painful because many callers contain folowing code:
> de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
> if (!(de))
> return retval;
> <<< WOW retval wasn't changed by do_split(), so caller failed
> <<< but return SUCCESS :)
> - Rearrange do_split() error path. Current error path is realy ugly, all
> this up and down jump stuff doesn't make code easy to understand.
Ohh my first patch change error message semantics in do_split(). Initially when
ext3_append() failed we just exit without printing error. In fact ext3_append()
may fail, it is legal and it's happens qite often (ENOSPC for example). This
cause annoying fake error message. So restore this semantic as it was before
patch.
Andrew please apply incremental patch what fix it:
Signed-off-by: Monakhov Dmitriy <[email protected]>
diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 1a52586..7edb617 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -1197,8 +1197,8 @@ journal_error:
brelse(*bh);
brelse(bh2);
*bh = NULL;
-errout:
ext3_std_error(dir->i_sb, err);
+errout:
*error = err;
return NULL;
}
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index f0a6c26..02a75db 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1195,8 +1195,8 @@ journal_error:
brelse(*bh);
brelse(bh2);
*bh = NULL;
-errout:
ext4_std_error(dir->i_sb, err);
+errout:
*error = err;
return NULL;
}