clean erofs_lookup()

PATCH In review Al Viro 3 participants 5 messages Oct 10, 2018 – Oct 16, 2018

Subsystems (2)

VFS Staging

lore

Share & tools

lore

Al Viro

author maintainer

2018-10-10 19:01 UTC #0 lore raw

PATCH 1/1 clean erofs_lookup()

d_splice_alias() does the right thing when given
ERR_PTR(-E...) for inode. No need for gotos, multiple returns, etc. in there.

Signed-off-by Al Viro <viro at zeniv.linux.org.uk>

---

drivers/staging/erofs/namei.c +7 ~~-12~~

index 546a47156101..8cf0617d4ea0 100644
@@ -223,18 +223,13 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	if (err == -ENOENT) {
 		/* negative dentry */
 		inode = NULL;
-		goto negative_out;
-	} else if (unlikely(err))
-		return ERR_PTR(err);
-
-	debugln("%s, %s (nid %llu) found, d_type %u", __func__,
-		dentry->d_name.name, nid, d_type);
-
-	inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
-	if (IS_ERR(inode))
-		return ERR_CAST(inode);
-
-negative_out:
+	} else if (unlikely(err)) {
+		inode = ERR_PTR(err);
+	} else {
+		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
+			dentry->d_name.name, nid, d_type);
+		inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
+	}
 	return d_splice_alias(inode, dentry);
 }

Gao Xiang

Reviewed-by on v1 1/1

2018-10-10 23:09 UTC #1 lore raw

Hi Alexander,

Quoted 3 lines · Al Viro

d_splice_alias() does the right thing when given
ERR_PTR(-E...) for inode. No need for gotos, multiple returns, etc. in there.

That is correct :) Thanks for your patch and attention.

Once in the first version we used "d_add" rather than "d_splice_alias" because I think no need to support disconnect dentries / export in the near future.

But We changed into "d_splice_alias" later, however it seems more redundant logics there...

Reviewed-by Gao Xiang <gaoxiang25 at huawei.com>

Thanks,
Gao Xiang

Quoted 34 lines

Signed-off-by Al Viro <viro at zeniv.linux.org.uk>

---

drivers/staging/erofs/namei.c +7 ~~-12~~

index 546a47156101..8cf0617d4ea0 100644
@@ -223,18 +223,13 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	if (err == -ENOENT) {
 		/* negative dentry */
 		inode = NULL;
-		goto negative_out;
-	} else if (unlikely(err))
-		return ERR_PTR(err);
-
-	debugln("%s, %s (nid %llu) found, d_type %u", __func__,
-		dentry->d_name.name, nid, d_type);
-
-	inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
-	if (IS_ERR(inode))
-		return ERR_CAST(inode);
-
-negative_out:
+	} else if (unlikely(err)) {
+		inode = ERR_PTR(err);
+	} else {
+		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
+			dentry->d_name.name, nid, d_type);
+		inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
+	}
 	return d_splice_alias(inode, dentry);
 }

Gao Xiang

2018-10-12 17:59 UTC #2 lore raw

PATCH RESEND 1/1 staging: erofs: clean erofs_lookup()

From: Al Viro <[email protected]>

d_splice_alias() does the right thing when given
ERR_PTR(-E...) for inode. No need for gotos, multiple returns, etc. in there.

Signed-off-by Al Viro <viro at zeniv.linux.org.uk>

Reviewed-by Gao Xiang <gaoxiang25 at huawei.com>

Signed-off-by Gao Xiang <gaoxiang25 at huawei.com>

---
Hi,

Frankly, I think it is a straight-forward cleanup by Al enough to be submitted to Linux 4.20 in constant to other pending fixes found in the process of EROFS productization these days, which I need more time to think over and fix formally to the community.

p.s. I have no idea whether this patch has been already queued up in Al's fs tree for 4.20... :'( +Cc Greg / the staging mailing list as well. Or please ignore this email...

Thanks,
Gao Xiang

 drivers/staging/erofs/namei.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

drivers/staging/erofs/namei.c +7 ~~-12~~

index 0039b76..5596c52 100644
@@ -223,18 +223,13 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	if (err == -ENOENT) {
 		/* negative dentry */
 		inode = NULL;
-		goto negative_out;
-	} else if (unlikely(err))
-		return ERR_PTR(err);
-
-	debugln("%s, %s (nid %llu) found, d_type %u", __func__,
-		dentry->d_name.name, nid, d_type);
-
-	inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
-	if (IS_ERR(inode))
-		return ERR_CAST(inode);
-
-negative_out:
+	} else if (unlikely(err)) {
+		inode = ERR_PTR(err);
+	} else {
+		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
+			dentry->d_name.name, nid, d_type);
+		inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
+	}
 	return d_splice_alias(inode, dentry);
 }

Signature · 1 line

2.7.4

Gao Xiang

2018-10-13 16:37 UTC #3 lore raw

Re: [PATCH RESEND] staging: erofs: clean erofs_lookup()

Hi,

Please ignore this patch, it has been queued by Al for 4.20 in
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=for-next&id=8300807f9e2dffb6552514763ad60e005c12eb94

Sorry for bothering...

Thanks,
Gao Xiang

Quoted 58 lines · Gao Xiang via Linux-erofs

From: Al Viro <viro at ZenIV.linux.org.uk>
d_splice_alias() does the right thing when given
ERR_PTR(-E...) for inode. No need for gotos, multiple returns, etc. in there.
Signed-off-by Al Viro <viro at zeniv.linux.org.uk>
Reviewed-by Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by Gao Xiang <gaoxiang25 at huawei.com>
---
Hi,
Frankly, I think it is a straight-forward cleanup by Al enough to be submitted to Linux 4.20 in constant to other pending fixes found in the process of EROFS productization these days, which I need more time to think over and fix formally to the community.
p.s. I have no idea whether this patch has been already queued up in Al's fs tree for 4.20... :'( +Cc Greg / the staging mailing list as well. Or please ignore this email...
Thanks,
Gao Xiang
 drivers/staging/erofs/namei.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)
drivers/staging/erofs/namei.c +7 ~~-12~~
index 0039b76..5596c52 100644
@@ -223,18 +223,13 @@ static struct dentry *erofs_lookup(struct inode *dir,
 	if (err == -ENOENT) {
 		/* negative dentry */
 		inode = NULL;
-		goto negative_out;
-	} else if (unlikely(err))
-		return ERR_PTR(err);
-
-	debugln("%s, %s (nid %llu) found, d_type %u", __func__,
-		dentry->d_name.name, nid, d_type);
-
-	inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
-	if (IS_ERR(inode))
-		return ERR_CAST(inode);
-
-negative_out:
+	} else if (unlikely(err)) {
+		inode = ERR_PTR(err);
+	} else {
+		debugln("%s, %s (nid %llu) found, d_type %u", __func__,
+			dentry->d_name.name, nid, d_type);
+		inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
+	}
 	return d_splice_alias(inode, dentry);
 }
 

Chao Yu

Reviewed-by on v1 1/1

2018-10-16 11:11 UTC #4 lore raw

In reply to Al Viro

Quoted 5 lines · Al Viro

d_splice_alias() does the right thing when given
ERR_PTR(-E...) for inode. No need for gotos, multiple returns, etc. in there.
Signed-off-by Al Viro <viro at zeniv.linux.org.uk>

Reviewed-by Chao Yu <yuchao0 at huawei.com>

Thanks,