From: Linus Torvalds <torvalds@linux-foundation.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-raid@vger.kernel.org,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Linux API <linux-api@vger.kernel.org>
Subject: Re: [PATCH 04/24] fs: move the putname from filename_create to the callers
Date: Mon, 20 Jul 2020 11:05:50 -0700 [thread overview]
Message-ID: <CAHk-=wimKMPiGP6n_HQUJ1rQ_6cT6hZH5rjQa_nfAgjB1mug+A@mail.gmail.com> (raw)
In-Reply-To: <20200720155902.181712-5-hch@lst.de>
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
next prev parent reply other threads:[~2020-07-20 18:06 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-20 15:58 add file system helpers that take kernel pointers for the init code Christoph Hellwig
2020-07-20 15:58 ` [PATCH 01/24] init: initialize ramdisk_execute_command at compile time Christoph Hellwig
2020-07-20 15:58 ` [PATCH 02/24] fs: add a do_kern_mount helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 03/24] fs: add a kern_umount helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 04/24] fs: move the putname from filename_create to the callers Christoph Hellwig
2020-07-20 18:05 ` Linus Torvalds [this message]
2020-07-20 15:58 ` [PATCH 05/24] fs: move the putname from filename_lookup " Christoph Hellwig
2020-07-20 18:11 ` Al Viro
2020-07-20 15:58 ` [PATCH 06/24] fs: add a kern_chdir helper Christoph Hellwig
2020-07-20 18:17 ` Al Viro
2020-07-20 15:58 ` [PATCH 07/24] fs: add a kern_chroot helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 08/24] fs: add a kern_access helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 09/24] fs: add a kern_chown helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 10/24] fs: move the uid16 (f)chown syscalls to fs/open.c Christoph Hellwig
2020-07-20 15:58 ` [PATCH 11/24] fs: add a kern_chmod helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 12/24] fs: add a kern_utimes helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 13/24] fs: add a kern_mkdir helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 14/24] fs: add a kern_mknod helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 15/24] fs: add a kern_link helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 16/24] fs: add a kern_symlink helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 17/24] fs: add a kern_unlink helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 18/24] fs: add a kern_rmdir helper Christoph Hellwig
2020-07-20 15:58 ` [PATCH 19/24] fs: remove vfs_statx_fd Christoph Hellwig
2020-07-20 15:58 ` [PATCH 20/24] fs: implement vfs_stat and vfs_lstat in terms of vfs_fstatat Christoph Hellwig
2020-07-20 15:58 ` [PATCH 21/24] fs: move vfs_fstatat out of line Christoph Hellwig
2020-07-20 15:59 ` [PATCH 22/24] fs: remove vfs_stat_set_lookup_flags Christoph Hellwig
2020-07-20 15:59 ` [PATCH 23/24] fs: remove KSTAT_QUERY_FLAGS Christoph Hellwig
2020-07-20 15:59 ` [PATCH 24/24] fs: add a kern_stat helper Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHk-=wimKMPiGP6n_HQUJ1rQ_6cT6hZH5rjQa_nfAgjB1mug+A@mail.gmail.com' \
--to=torvalds@linux-foundation.org \
--cc=gregkh@linuxfoundation.org \
--cc=hch@lst.de \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-raid@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).