On Wed, Jul 7, 2021 at 5:28 AM Dmitry Kadashev [off-list ref] wrote:

Hence this preparation change splits filename_parentat() into two: one
that always consumes the name and another that never consumes the name.
This will allow to implement two filename_create() variants in the same
way, and is a consistent and hopefully easier to reason about approach.

I like it.

The patch itself is a bit hard to read, but the end result seems to make sense.

My main reaction is that this could have probably done a bit more
cleanup by avoiding some of the "goto exit1" kind of things.

Just as an example, now the rule is that "do_rmdir()" always does that

        putname(name);
        return error;

at the end, and I think this means that this whole function could be
split into a few trivial helper functions instead, and we'd have

   long do_rmdir(int dfd, struct filename *name)
  {
        int error;

        error = rmdir_helper(...)
        if (!retry_estale(error, lookup_flags)) {
                lookup_flags |= LOOKUP_REVAL;
                error = rmdir_helper(...);
        }
        putname(name);
        return error;
  }

which gets rid of both the "goto retry" and the "goto exit1".

With the meat of "do_rmdir()" done in that "rmdir_helper()" function.

I think the same is basically true of "do_unlinkat()" too.

But I wouldn't mind that cleanup as a separate patch. My point is that
I think this new rule for when the name is consumed is better and can
result in further cleanups.

(NOTE! This is from just reading the patch, I might have missed some case).

            Linus