| Message ID | 20200720155902.181712-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/24] init: initialize ramdisk_execute_command at compile time | expand |
On Mon, Jul 20, 2020 at 8:59 AM Christoph Hellwig <hch@lst.de> wrote: > > This allows reusing the struct filename for retries, and will also allow > pushing the getname up the stack for a few places to allower for better > handling of kernel space filenames. I find this _very_ confusing. Now the rule is that filename_create() does the putname() if it fails, but not if it succeeds. That's just all kinds of messed up. It was already slightly confusing how "getname()" was paired with "putname()", and how you didn't need to check for errors, but at least it was easy to explain: "filename_create() will check errors and use the name we got". That slightly confusing calling convention made the code much more compact, and nobody involved needed to do error checks on the name etc. Now that "slightly confusing" convention has gone from "slightly" to "outright", and the whole advantage of the interface has completely gone away, because now you not only need to do the putname() in the caller, you need to do it _conditionally_. So please don't do this. The other patches also all make it *really* hard to follow when putname() is done - because depending on the context, you have to do it when returning an error, or when an error was not returned. I really think this is a huge mistake. Don't do it this way. NAK NAK NAK. Please instead of making this all completely messy and completely impossible to follow the rule about exactly who does "putname()" and under what conditions, just leave the slight duplication in place. Duplicating simple helper routines is *good*. Complex and hard-to-understand and non-intuitive rules are *bad*. You're adding badness. Linus
diff --git a/fs/namei.c b/fs/namei.c index 72d4219c93acb7..6ebe400c9736d2 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3478,7 +3478,6 @@ static struct dentry *filename_create(int dfd, struct filename *name, error = err2; goto fail; } - putname(name); return dentry; fail: dput(dentry); @@ -3496,8 +3495,12 @@ static struct dentry *filename_create(int dfd, struct filename *name, struct dentry *kern_path_create(int dfd, const char *pathname, struct path *path, unsigned int lookup_flags) { - return filename_create(dfd, getname_kernel(pathname), - path, lookup_flags); + struct filename *name = getname_kernel(pathname); + struct dentry *dentry = filename_create(dfd, name, path, lookup_flags); + + if (!IS_ERR(dentry)) + putname(name); + return dentry; } EXPORT_SYMBOL(kern_path_create); @@ -3513,7 +3516,12 @@ EXPORT_SYMBOL(done_path_create); inline struct dentry *user_path_create(int dfd, const char __user *pathname, struct path *path, unsigned int lookup_flags) { - return filename_create(dfd, getname(pathname), path, lookup_flags); + struct filename *name = getname(pathname); + struct dentry *dentry = filename_create(dfd, name, path, lookup_flags); + + if (!IS_ERR(dentry)) + putname(name); + return dentry; } EXPORT_SYMBOL(user_path_create);
This allows reusing the struct filename for retries, and will also allow pushing the getname up the stack for a few places to allower for better handling of kernel space filenames. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/namei.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)