Re: [PATCH 05/11] VFS: add rename_lookup()

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Aug 13, 2025 at 06:04:32PM +1000, NeilBrown wrote:

> There is a git tree you could pull.....
> 
> My API effectively supports both lock_rename() users and
> lock_rename_child() users.  Maybe you want to preserve the two different
> APIs.  I'd rather avoid the code duplication.

What code duplication?  Seriously, how much of the logics is really shared?
Error checking?  So put that into a common helper...
 
> > This is too fucking ugly to live, IMO.  Too many things are mixed into it.
> > I will NAK that until I get a chance to see the users of all that stuff.
> > Sorry.
> > 
> 
> Can you say more about what you think it ugly?
> 
> Are you OK with combining the lookup and the locking in the one
> function?
> Are you OK with passing a 'struct rename_data' rather than a list of
> assorted args?
> Are you OK with deducing the target flags in this function, or do you
> want them explicitly passed in?
> Is it just that the function can use with lock_rename or
> lock_rename_child depending on context?

Put it that way: you are collapsing two (if not more) constructors
for the same object into one.  That makes describing (and proving,
and verifying the callers, etc.) considerably more painful, with very
little gain to be had.

You are not so much modifying rename_data as creating an object - "state
ready for rename".  The fact that you use the same chunk of memory
to encode the arguments of constructor is an implementation detail;
constraints on the contents of that chunk of memory are different both
from each other and from the resulting object.

In effect, you have a weird union of several types here and the fact
that C type system is pretty weak does not help.  Having separate
constructors at least documents which rules apply; conflating them is
asking for trouble.

It's the same problem as with flags arguments, really.  It makes proofs
harder.  "I'm here" is a lot easier to deal with than "such and such
correlations hold between the values of such and such variables".

If we have several constructors (and they can easily share common helpers
- no problem with that), we can always come back and decide to fold them
into one; splitting is a lot harder, exactly because such flags, etc.,
do not stay local.  I've done both kinds of transformations and in the
"split" direction it ended up with bloody painful tree-wide analysis -
more often than not with buggy corner cases caught in process.

Let's not go there; if, in the end of process, we look at the result and
see that unifying some of them makes sense - good, it won't be hard to do.
But making it as "flexible" as possible as the first step pretty much
locks you into a lot of decisions that are better done when the final
state is seen - and possibly had been massaged for a while.