diff mbox series

[v2,2/2] ext2: ext2_find_entry() return -ENOENT if no entry found

Message ID 20200603063514.3904811-2-yi.zhang@huawei.com
State Not Applicable
Headers show
Series [v2,1/2] ext2: propagate errors up to ext2_find_entry()'s callers | expand

Commit Message

Zhang Yi June 3, 2020, 6:35 a.m. UTC 
Almost all callers of ext2_find_entry() transform NULL return value to
-ENOENT, so just let ext2_find_entry() retuen -ENOENT instead of NULL
if no valid entry found, and also switch to check the return value of
ext2_inode_by_name() in ext2_lookup() and ext2_get_parent().

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/dir.c   | 11 ++++++++---
 fs/ext2/namei.c | 27 ++++++++-------------------
 2 files changed, 16 insertions(+), 22 deletions(-)

Comments

Jan Kara June 5, 2020, 3:11 p.m. UTC | #1 
On Wed 03-06-20 14:35:14, zhangyi (F) wrote:
> Almost all callers of ext2_find_entry() transform NULL return value to
> -ENOENT, so just let ext2_find_entry() retuen -ENOENT instead of NULL
> if no valid entry found, and also switch to check the return value of
> ext2_inode_by_name() in ext2_lookup() and ext2_get_parent().
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Thanks for the patch. Just one small nit below.

> @@ -419,11 +419,16 @@ int ext2_inode_by_name(struct inode *dir, const struct qstr *child, ino_t *ino)
>  	struct page *page;
>  	
>  	de = ext2_find_entry(dir, child, &page);
> -	if (IS_ERR_OR_NULL(de))
> +	if (IS_ERR(de))
>  		return PTR_ERR(de);
>  
> -	*ino = le32_to_cpu(de->inode);
>  	ext2_put_page(page);
> +	if (!de->inode) {

ext2_find_entry() will not ever return de with de->inode == 0 because
ext2_match() never returns true for such entries. So I'd just remove this
condition...

								Honza
Zhang Yi June 8, 2020, 1:20 a.m. UTC | #2 
On 2020/6/5 23:11, Jan Kara wrote:
> On Wed 03-06-20 14:35:14, zhangyi (F) wrote:
>> Almost all callers of ext2_find_entry() transform NULL return value to
>> -ENOENT, so just let ext2_find_entry() retuen -ENOENT instead of NULL
>> if no valid entry found, and also switch to check the return value of
>> ext2_inode_by_name() in ext2_lookup() and ext2_get_parent().
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> Suggested-by: Jan Kara <jack@suse.cz>
> 
> Thanks for the patch. Just one small nit below.
> 
>> @@ -419,11 +419,16 @@ int ext2_inode_by_name(struct inode *dir, const struct qstr *child, ino_t *ino)
>>  	struct page *page;
>>  	
>>  	de = ext2_find_entry(dir, child, &page);
>> -	if (IS_ERR_OR_NULL(de))
>> +	if (IS_ERR(de))
>>  		return PTR_ERR(de);
>>  
>> -	*ino = le32_to_cpu(de->inode);
>>  	ext2_put_page(page);
>> +	if (!de->inode) {
> 
> ext2_find_entry() will not ever return de with de->inode == 0 because
> ext2_match() never returns true for such entries. So I'd just remove this
> condition...
> 
Indeed, I missed this point, will do.

Thanks,
Yi.
diff mbox series

Patch

diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 95e4f0bd55a3..96c40fd7550e 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -393,7 +393,7 @@  struct ext2_dir_entry_2 *ext2_find_entry (struct inode *dir,
 		}
 	} while (n != start);
 out:
-	return NULL;
+	return ERR_PTR(-ENOENT);
 
 found:
 	*res_page = page;
@@ -419,11 +419,16 @@  int ext2_inode_by_name(struct inode *dir, const struct qstr *child, ino_t *ino)
 	struct page *page;
 	
 	de = ext2_find_entry(dir, child, &page);
-	if (IS_ERR_OR_NULL(de))
+	if (IS_ERR(de))
 		return PTR_ERR(de);
 
-	*ino = le32_to_cpu(de->inode);
 	ext2_put_page(page);
+	if (!de->inode) {
+		ext2_error(dir->i_sb, __func__, "bad inode number: %u",
+			   le32_to_cpu(de->inode));
+		return -EFSCORRUPTED;
+	}
+	*ino = le32_to_cpu(de->inode);
 	return 0;
 }
 
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 4b38e558d477..9601d469c5b1 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -56,17 +56,18 @@  static inline int ext2_add_nondir(struct dentry *dentry, struct inode *inode)
 static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, unsigned int flags)
 {
 	struct inode * inode;
-	ino_t ino = 0;
+	ino_t ino;
 	int res;
 	
 	if (dentry->d_name.len > EXT2_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
 	res = ext2_inode_by_name(dir, &dentry->d_name, &ino);
-	if (res)
-		return ERR_PTR(res);
-	inode = NULL;
-	if (ino) {
+	if (res) {
+		if (res != -ENOENT)
+			return ERR_PTR(res);
+		inode = NULL;
+	} else {
 		inode = ext2_iget(dir->i_sb, ino);
 		if (inode == ERR_PTR(-ESTALE)) {
 			ext2_error(dir->i_sb, __func__,
@@ -81,14 +82,13 @@  static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, uns
 struct dentry *ext2_get_parent(struct dentry *child)
 {
 	struct qstr dotdot = QSTR_INIT("..", 2);
-	ino_t ino = 0;
+	ino_t ino;
 	int res;
 
 	res = ext2_inode_by_name(d_inode(child), &dotdot, &ino);
 	if (res)
 		return ERR_PTR(res);
-	if (!ino)
-		return ERR_PTR(-ENOENT);
+
 	return d_obtain_alias(ext2_iget(child->d_sb, ino));
 } 
 
@@ -289,10 +289,6 @@  static int ext2_unlink(struct inode * dir, struct dentry *dentry)
 		err = PTR_ERR(de);
 		goto out;
 	}
-	if (!de) {
-		err = -ENOENT;
-		goto out;
-	}
 
 	err = ext2_delete_entry (de, page);
 	if (err)
@@ -349,10 +345,6 @@  static int ext2_rename (struct inode * old_dir, struct dentry * old_dentry,
 		err = PTR_ERR(old_de);
 		goto out;
 	}
-	if (!old_de) {
-		err = -ENOENT;
-		goto out;
-	}
 
 	if (S_ISDIR(old_inode->i_mode)) {
 		err = -EIO;
@@ -369,14 +361,11 @@  static int ext2_rename (struct inode * old_dir, struct dentry * old_dentry,
 		if (dir_de && !ext2_empty_dir (new_inode))
 			goto out_dir;
 
-		err = -ENOENT;
 		new_de = ext2_find_entry(new_dir, &new_dentry->d_name, &new_page);
 		if (IS_ERR(new_de)) {
 			err = PTR_ERR(new_de);
 			goto out_dir;
 		}
-		if (!new_de)
-			goto out_dir;
 		ext2_set_link(new_dir, new_de, new_page, old_inode, 1);
 		new_inode->i_ctime = current_time(new_inode);
 		if (dir_de)