Re: [PATCH v7 07/10] fs: make do_linkat() take struct filename

[Dmitry Kadashev <dkadashev@gmail.com>]

On Wed, Jul 7, 2021 at 1:05 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This is the only one in the series that I still react fairly negatively at.
>
> I still just don't like how filename_lookup() used to be nice and easy
> to understand ("always eat the name"), and while those semantics
> remain, the new __filename_lookup() has those odd semantics of only
> eating it on failure.
>
> And there is exactly _one_ caller of that new __filename_lookup(), and it does
>
>         error = __filename_lookup(olddfd, old, how, &old_path, NULL);
>         if (error)
>                 goto out_putnew;
>
> and I don't even understand why you'd want to eat it on error, because
> if if *didn't* eat it on error, it would just do
>
>         error = __filename_lookup(olddfd, old, how, &old_path, NULL);
>         if (error)
>                 goto out_putnames;
>
> and it would be much easier to understand (and the "out_putnew" label
> would go away entirely)
>
> What am I missing? You had some reason for not eating the name
> unconditionally, but I look at this patch and I just don't see it.

__filename_lookup() does that "eat the name on error" for uniformity
with the __filename_create(), and the latter does that mostly because Al
suggested to do it that way:

https://lore.kernel.org/io-uring/20210201150042.GQ740243@zeniv-ca/

Granted, he did that back when this series was much smaller, only about
mkdirat, and in that case it looked like it makes things a tad simpler,
and even though I found the semantics a bit confusing, I've assumed that
I'm missing something and this is something the FS code does, so people
are used to it.

Anyway, I'll send v8 of this series with yet another preparation patch,
that will change filename_parenat() to return an error code instead of
struct filename *, and split it into two: filename_parenat() that always
eats the name, and __filename_parentat() that never eats the name. And
__filename_lookup() and __filename_create() will never eat the name as
well, so things are nice and uniform and easy to reason about.

And hopefully if Al does not like that approach he can weigh in.

-- 
Dmitry Kadashev